fix(desktop): unblock the test job from its own tests

[Astro-Han]

Summary

Three defects in the desktop test suite, all of which blocked main or this PR without anything in the product being wrong.

The workspace suite ran to its 900s ceiling

Run affected standard workspace tests has been failing intermittently on main since #4705 landed — d422246e1, and again on an unrelated PR:

✖ scripts/vite-workspace-packages.test.mjs (834352ms)
  'Promise resolution is still pending but the event loop has already resolved'
[desktop] timed out after 900000ms

Both tests in that file pass. The process is what does not end. renderer-facing Runtime Host protocol does not load Node crypto points a Vite dev server at the real apps/desktop/src/renderer, and workspacePackagesPlugin(repoRoot) pushes the repository manifest plus every workspace manifest into configFileDependencies. The watcher that follows them walks the whole repository, server.close() does not wait for that walk, and the unfinished fs requests hold the process open after the assertions are done.

Measured with process.getActiveResourcesInfo() immediately after close():

Configuration	Handles left
as on main	FSReqPromise, FSReqCallback, PipeWrap ×3
plus esbuild.stop()	unchanged — not esbuild
plus server: { watch: null }	the 2 stdio sockets only

pgrep -P shows no child processes, so those PipeWraps are the process's own stdio. The two tests are not racing either: run alone, the first finishes in 163ms and the second is the one that hangs.

That test sends one request and makes one assertion; nothing in it reacts to a file changing, so it gets watch: null. The first test keeps its watcher — re-resolving after a manifest edit is exactly what it asserts, and its root is a small temporary directory, which is why it was never the one to hang.

The same test passed on a document that was not the module

Without packages/runtime-host/dist, Vite answers the @fs/ request with the SPA fallback: status 200, and index.html contains no vite-browser-external:node:crypto, so the assertion passed on a page that is not the module under test. CI always builds first and never saw it; any unbuilt tree got a green test that checked nothing. Found while verifying the fix above.

code-scroll flaked this PR's own CI

code-scroll.spec.ts failed the first run here with Expected: > 0 / Received: 0 on its selection drag, on a branch that changes nothing it touches. It is the file's only test, it asserts six geometry values against a real scroller, and it is the same tier and the same failure mode as the specs #4741 removes. Removed with them rather than left to block the next unrelated change; the rebuild is one tier down, per #4727.

Refs #4705, #4741, #4727

Verification

• node --test apps/desktop/scripts/vite-workspace-packages.test.mjs — 2 passed in 0.29s, process exits. On main the same command does not return.
• The new guard fails as intended: with packages/runtime-host/dist removed, the test reports Vite served the SPA fallback; build @maka/runtime-host first instead of passing.
• playwright test --list — 110 tests in 36 files, down from 111 in 37.
• npm run format, npm run lint — no fixes applied.
• Not run: the full workspace suite and the full E2E suite. This PR's CI covers both.

Review focus

The first two commits are the actual fix and stand on their own. The third is a removal that belongs to the same argument as #4741 and could equally have gone there; it is here because #4741 is already approved and green, and this branch is where the flake surfaced.

AI use

Select exactly one:

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code traced the timeout to the surviving handles, ruled out esbuild and test concurrency by measurement, and wrote all three changes.

Checklist

• Tests cover the change and fail without it — not applicable: every change here is to a test
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[jackwener]

Approving at 2e6eb3c644bf3ce4c46e9dc48a6873c4164d9764. No P0 or P1. I reproduced both defects and both fixes locally rather than reading the diff, since this is a change about a test that passes without checking anything — inferring that statically would have been the wrong instrument.

Reproduced

The hang is real, and the fix ends it. With watch: null removed and everything else identical, the process was still alive after 75 seconds and had to be killed; ps reported an elapsed time of 1:16. On this head the same file finishes in 2.7 seconds and exits on its own.

One detail sharpens the diagnosis beyond what the description claims. My tree has no packages/runtime-host/dist, so the second test failed its assertion — and the process still would not exit. The surviving handles are therefore not tied to the test's outcome or to anything the assertion path does; the watcher holds the process open regardless of whether the test passed. That is a cleaner separation than the original measurement, and it points at the same cause.

The second defect is real, and the guard catches it. Removing packages/runtime-host/dist and running the file as it exists on main:

✔ renderer loads a newly exported workspace module after its manifest changes
✔ renderer-facing Runtime Host protocol does not load Node crypto
ℹ pass 2  fail 0

Two green tests, and the second one verified nothing. Vite answered the @fs/ request with index.html, which contains no vite-browser-external:node:crypto, so assert.doesNotMatch was trivially satisfied by a document that is not the module under test. With the guard, the same conditions produce Vite served the SPA fallback; build @maka/runtime-host first.

This is the part worth keeping in mind beyond this PR. A test that can fail and a test that pins the thing it names are different properties, and the gap between them is invisible in CI when CI always builds first. The guard converts a silent false pass into a message that says what to do.

On the asymmetry between the two tests

Keeping the watcher on the first test and dropping it from the second is the right split, not an inconsistency. The first test's root is a mkdtemp directory and its assertion is re-resolution after a manifest edit, so the watcher is the thing under test. The second test's root is the real repository and it makes one request with one assertion; nothing in it reacts to a file changing. The reason the first was never the one to hang follows from the same fact — a small temporary tree, not the whole repo.

server.close() not awaiting the recursive scan is a Vite-level behavior, so watch: null is a scoped avoidance rather than a root-cause fix. For a test that never needed a watcher, that is the correct trade rather than a workaround: the right fix for an unnecessary watcher is not to have one.

Two process notes, neither a review finding

The PR is still marked draft, and test is pending on this head. This approval covers the change; it cannot be merged while the draft flag is set, and the merge decision remains a human's in any case. My local runs are not a substitute for the workspace suite that this PR's CI covers.

简体中文

在 2e6eb3c644bf3ce4c46e9dc48a6873c4164d9764 上批准。没有 P0/P1。 我在本地把两个缺陷和两个修复都复现了,而不是读 diff——因为这个 PR 讲的正是「一个通过了却什么都没检查的测试」,用静态推理去判断它,工具就用错了。

已复现

挂起是真的,修复确实终结了它。 仅去掉 watch: null、其余完全相同:进程在 75 秒后仍然存活,只能强杀,ps 报告已运行 1:16。而在这个 head 上,同一个文件 2.7 秒跑完并自行退出。

有一个细节让诊断比描述本身更锐利。我的树里没有 packages/runtime-host/dist,所以第二个测试的断言是失败的——而进程照样不退出。这说明存活的句柄与测试结果无关,也与断言路径做了什么无关:无论测试通过与否,watcher 都把进程held住。 这比原始测量更干净地隔离了变量,而且指向同一个原因。

第二个缺陷是真的,守卫确实抓住了它。 移除 packages/runtime-host/dist,按 main 上的写法跑这个文件:

✔ renderer loads a newly exported workspace module after its manifest changes
✔ renderer-facing Runtime Host protocol does not load Node crypto
ℹ pass 2  fail 0

两个测试全绿,而第二个什么也没验证。 Vite 用 index.html 回应了那个 @fs/ 请求,而 index.html 里根本不含 vite-browser-external:node:crypto,于是 assert.doesNotMatch 被一份「根本不是被测模块」的文档平凡地满足了。加上守卫后,同样条件下给出的是 Vite served the SPA fallback; build @maka/runtime-host first。

这一点的价值超出本 PR。 「一个测试能失败」和「一个测试钉住了它所命名的东西」是两种不同的性质,而当 CI 总是先构建时,两者之间的缺口在 CI 里是不可见的。这个守卫把一次静默的假通过,变成了一句告诉你该做什么的提示。

关于两个测试的不对称

第一个测试保留 watcher、第二个去掉,是正确的切分,不是不一致。第一个测试的 root 是 mkdtemp 临时目录,而它的断言本身就是「manifest 修改后重新解析」——watcher 正是被测对象。第二个测试的 root 是真实仓库,只发一次请求、做一次断言,里面没有任何东西对文件变化作出反应。「为什么挂住的从来不是第一个」也由同一个事实得出:它是一棵小的临时目录树,不是整个仓库。

server.close() 不等待递归扫描,这是 Vite 层面的行为,所以 watch: null 是限定范围的规避而非根因修复。但对一个从来不需要 watcher 的测试来说,这是正确的取舍而不是绕路:「多余的 watcher」的正确修法,就是不要有它。

两条流程提示,都不是评审发现

这个 PR 仍标记为 draft,且 test 在这个 head 上仍是 pending。本批准针对的是这次改动;draft 标记未撤时它无法合并,而且合并与否本来就仍由人决定。我的本地运行不能替代这个 PR 的 CI 所覆盖的完整 workspace 测试套件。

---

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.