test(desktop): sink the slash-command submit rule to its routing function

[Astro-Han]

Summary

dispatches /side instead of steering it into a running turn (slash-command-menu.spec.ts:211) timed out on #4748's CI, on a branch that changes nothing it touches — locator.click: Timeout 30000ms exceeded.

Unlike the specs #4741 removed, this one is not asserting geometry. It guards a single rule:

A slash command submitted while a Turn is running is dispatched, not queued as follow-up text.

That rule lived as an inline ternary at the call site:

const followUpAtSubmit = !slashCommand
  ? resolveFollowUpModeAtSubmit({ requestedMode, hasActiveTurn: hasActiveTurnAtSubmit({ ... }) })
  : undefined;

Nothing short of a full Electron run could reach it — while the two functions it wrapped already live in follow-up-submit-routing.ts and already have main/__tests__/follow-up-submit-routing.test.ts. The seam was there; the rule was just outside it.

This moves the condition into resolveFollowUpModeAtSubmit, where the rest of the submit routing is decided, and asserts it in that module's existing test file. A browser, a fake backend and a real running Turn become three lines.

The parameter takes the parsed command rather than a boolean derived from it, and it is required rather than optional: there is exactly one call site, and a caller that forgets it should not compile. slashCommand passes as shorthand, which leaves app-shell.tsx three tokens lighter than the ternary it replaces — #4581 turned the renderer ledger into a one-way ratchet mid-review, and a isSlashCommand: slashCommand !== null spelling was one token heavier than what it removed.

The E2E test goes with the move, and takes two assertions with it.

/side being offered mid-turn is still covered by shows only slash commands executable in the current session state. /compact being withheld mid-turn is not. That guard is !(streaming && id === 'compact') (app-shell.tsx:1331), and it only hides the command while a Turn streams — but the surviving spec waits for Fake backend received: seed session before opening the menu, so streaming is false there and its toHaveCount(4) counts /compact as present. The rule now has no automated coverage in any tier. This PR neither broke it nor caught it; it belongs to the same tier-three move, per #4727.

The deleted spec also ended by asserting .maka-quote-workbar-panel reached count 1 — the side chat actually opened. The unit test pins the routing decision, which is that outcome's precondition, not the outcome. The early return at if (sessionId && followUpAtSubmit) is the only thing that could have prevented dispatch, so the moved assertion covers where the rule can break, but the residue is real.

This is the first case of the tier-three move discussed in #4727, and the cheapest possible one: the target module and its test file already existed.

Refs #4727, #4741

Verification

• node --test dist/main/__tests__/follow-up-submit-routing.test.js — 6 passed, including the new dispatches a slash command mid-turn instead of steering it into the Turn.
• Fails without the change: with if (input.isSlashCommand) return undefined; removed, the new test fails and the rest still pass.
• playwright test --list — 87 tests in 35 files, down from 88.
• node scripts/check-renderer-architecture.mjs --base fde5fb07 — passes against the rebase target. The ledger records app-shell.tsx three tokens lighter (15620 → 15617).
• npm run build, npm run format, npm run lint — clean.
• npm run typecheck -w @maka/desktop reports three pre-existing errors in packages/ui/src (Astryx ChatLayoutProps / MarkdownProps / SideNavItemProps) that are present without this change and do not involve either file it touches. CI is the authority here.

AI use

Select exactly one:

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

Tool(s) and scope: Claude Code located the rule behind the flaking E2E test, moved it onto the existing routing seam, and wrote the assertion.

Checklist

• Tests cover the change and fail without it
• 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 d5bc0fa9bf218a2aec76148a63bd0d5ed6e5bd4a. No P0 or P1. One P2, and it is in the description rather than the code.

I ran the move rather than reading it, since the claim under test is "the rule is still pinned after it moved."

Verified

The condition is semantically unchanged. The call site went from !slashCommand to isSlashCommand: slashCommand !== null, which are only equivalent if slashCommand is never undefined or another falsy non-null value. parseDesktopSlashCommand is declared DesktopSlashCommand | null, and every path in its body returns explicitly — { kind: 'compact' }, the three parser results, or null. There is no implicit undefined exit, so the two spellings agree on the whole value range. The "no behavior change" box is correct.

The new test pins the rule. Six pass as written. Removing if (input.isSlashCommand) return undefined; gives five passes and exactly one failure — dispatches a slash command mid-turn instead of steering it into the Turn — with the rest still green, matching the description exactly.

The seam is the right one. if (sessionId && followUpAtSubmit) returns early, so that branch is the only place a slash command could be swallowed before reaching its kind handler. Pinning resolveFollowUpModeAtSubmit pins the decision at the point where the rule actually lives, which is why three lines can replace an Electron run here. Requiring isSlashCommand rather than defaulting it is also right: one call site, and a forgotten argument should be a compile error, not a silent queue.

P2 — one deleted assertion is not covered by the test named as its replacement

The description says the removed spec's other assertions — the menu offering /side but not /compact mid-turn — remain covered by shows only slash commands executable in the current session state. The /compact half does not survive.

The filter hides that command only while a Turn is streaming:

&& !(streaming && id === 'compact')

The deleted spec checked exactly that state: it held a Turn open with FAKE_HOLD_OPEN_PROMPT, asserted the 停止 button was visible, and then asserted /compact had count 0. The surviving spec submits seed session and waits for Fake backend received: seed session before opening the menu — the Turn has finished by then, streaming is false, and the assertion it makes is toHaveCount(4), which counts /compact as present rather than absent.

So the rule "/compact is not offered while a Turn is running" now has no automated coverage at all, in either tier. It is a small guard, and this PR did not break it — but the description tells a future maintainer there is a net under it, and there is not. Either adjust that sentence, or add the mid-turn case to the surviving spec, which already has the fixture for it.

The /side half of the claim is fine: it appears among the four commands after seeding.

One thing the move does give up, and why it is acceptable

The deleted spec ended by asserting .maka-quote-workbar-panel reached count 1 — the side chat actually opened. The unit test asserts the routing decision returns undefined, which is the precondition for that, not the outcome. That gap is real but narrow: the early return above is the only thing that could have prevented dispatch, so the moved assertion covers the part where the rule can break. Noting it because the tier-three trade in #4727 is easier to evaluate when the residue is stated rather than implied.

Process

Still marked draft, and test is pending on this head — this approval covers the change, but the draft flag blocks merging, and the merge decision is a human's regardless. My local runs used Node's type stripping against the source module; they do not substitute for the built-dist run or the Playwright suite in CI.

简体中文

在 d5bc0fa9bf218a2aec76148a63bd0d5ed6e5bd4a 上批准。没有 P0/P1。 有一条 P2,而且它在描述里,不在代码里。

我把这次搬移跑了一遍而不是读了一遍——因为待检验的命题正是「规则搬走之后是否仍被钉住」。

已验证

条件在语义上没有变化。 调用点从 !slashCommand 变成了 isSlashCommand: slashCommand !== null,这两者只有在 slashCommand 永远不是 undefined(以及其它 falsy 非 null 值)时才等价。 parseDesktopSlashCommand 的声明是 DesktopSlashCommand | null,而它函数体里每一条路径都显式返回——{ kind: 'compact' }、三个解析器结果、或 null,不存在隐式的 undefined 出口,所以两种写法在整个值域上一致。「无行为变化」这一勾是对的。

新测试确实钉住了这条规则。 原样 6 个通过。移除 if (input.isSlashCommand) return undefined; 后是 5 通过、恰好 1 失败——正是 dispatches a slash command mid-turn instead of steering it into the Turn,其余仍绿,与描述完全吻合。

接缝选对了。 if (sessionId && followUpAtSubmit) 是早返回,所以那个分支是 slash command 在抵达它的 kind 处理之前唯一可能被吞掉的地方。钉住 resolveFollowUpModeAtSubmit 就是把断言放在这条规则真正所在的位置,这也正是三行能替代一次 Electron 运行的原因。 把 isSlashCommand 设为必填而非可选同样是对的:只有一个调用点,漏传应当是编译错误而不是静默排队。

P2:被删掉的一条断言,并没有被那个被点名接替它的测试覆盖

描述说,被删 spec 的其余断言——mid-turn 时菜单提供 /side 但不提供 /compact——仍由 shows only slash commands executable in the current session state 覆盖。其中 /compact 那一半没有活下来。

过滤器只在 Turn 正在流式运行时隐藏该命令:

&& !(streaming && id === 'compact')

被删的 spec 检查的正是那个状态:它用 FAKE_HOLD_OPEN_PROMPT 把一个 Turn 挂住,断言「停止」按钮可见,然后断言 /compact 的 count 为 0。而存活的那个 spec 提交 seed session 后要等到 Fake backend received: seed session 出现才打开菜单——那时 Turn 已经结束,streaming 为 false,而它做的断言是 toHaveCount(4),这个数字把 /compact 算作存在而不是缺席。

所以「Turn 运行期间不提供 /compact」这条规则,现在在任何一层都没有自动化覆盖了。这是个很小的护栏,而且本 PR 并没有破坏它——但描述会让后来的维护者以为它下面有网,而实际上没有。 要么改掉那句话,要么把 mid-turn 这个情形补进存活的那个 spec——它已经有现成的 fixture。

声称里 /side 的那一半没问题:seed 之后它确实在那四个命令中。

这次搬移确实放弃了一样东西,以及为什么可以接受

被删的 spec 最后断言 .maka-quote-workbar-panel 变为 count 1——侧聊真的打开了。而单元测试断言的是路由决策返回 undefined,那是它的前提,不是结果。这个缺口真实存在但很窄:上面那个早返回是唯一可能阻止 dispatch 的东西,所以被搬移的断言覆盖的正是这条规则可能被破坏的那一段。之所以写出来,是因为 #4727 里那个第三层搬移的取舍,在残差被明说而不是被默认时更容易评估。

流程

仍标记为 draft,且 test 在这个 head 上仍 pending——本批准针对这次改动,但 draft 标记会挡住合并,而且合并与否本来就由人决定。我的本地运行用的是 Node 的类型剥离直接跑源码模块,不能替代 CI 里基于构建产物的运行和 Playwright 套件。

---

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.

[Astro-Han]

P2 confirmed and fixed in the description — the /compact claim was wrong, and I checked all three parts of your reading before changing it: the filter at app-shell.tsx:1331 is !(streaming && id === 'compact'), the surviving spec waits for Fake backend received: seed session before opening the menu, and its toHaveCount(4) does count /compact as present. So "Turn running ⇒ no /compact" has no coverage in any tier.

I took the first of your two remedies and not the second, deliberately.

The description now says the guard is uncovered rather than implying a net, and says why: this PR neither broke it nor caught it. Adding the mid-turn case back into the surviving spec would move the suite in the opposite direction from the one this PR argues for — the point is not that this particular test flaked, it is that a rule expressible as a pure predicate was only reachable through Electron. !(streaming && id === 'compact') is exactly that shape. Putting it back into Playwright would buy coverage at the price of the argument, and it would be the second guard of this kind sitting in a suite we are trying to empty of them.

What it should get instead is the same treatment the submit rule just got: the command-visibility predicate lifted out of the app-shell array filter and asserted directly. That is a separate change with its own extraction, not something to fold into this one, and it belongs with the tier-three work in #4727.

On the residue you noted at the end — the deleted spec asserting the side chat panel actually opened, versus the unit test asserting the precondition — agreed, and thank you for stating it rather than letting it pass. It is now in the description too, with your reasoning for why the gap is narrow.

One correction to the process note: the branch has since been rebased onto #4581 and the architecture ledger regenerated, so your approval at d5bc0fa predates the current head.