test(desktop): sink the /compact menu rule to a tested predicate - #4762
Conversation
The Desktop composer withholds `/compact` while a Turn streams. #4741 deleted the only case that covered it, and the surviving slash-command spec opens the menu after the Turn ends, where `streaming` is false — so nothing catches a regression today. The rule lived inside the array filter that builds the menu, reachable only by rendering AppShell. It moves to `desktop-slash-command.ts`, next to the parser that owns the same four commands, and `node:test` asserts both halves: an idle Session is offered `/compact`, a streaming one is offered everything else and nothing less. Dropping the guard fails that test. The catalog query stays in AppShell on purpose: the debt ratchet counts dependencies per file, and moving `@maka/core/slash-command-catalog` into the smaller module would book a new dependency there rather than retire one. For the same reason the unused `enqueueInteraction` import goes — the predicate needs an import specifier, and AppShell had a dead one to spend. Refs #4727 #4752 Generated-by: Claude Code
jackwener
left a comment
There was a problem hiding this comment.
Approving at adb9f976f. No findings. This closes the coverage gap left by #4752, and the test is stronger than the assertion it replaces.
I ran it rather than reading it, including two mutations and one check of the test's own scaffolding.
The predicate is the same rule
Character for character against the inline version it replaces:
old: (session === 'none' || Boolean(activeId)) && !(streaming && id === 'compact')
new: (session === 'none' || state.hasSession) && !(state.streaming && id === 'compact')
with the call site passing hasSession: Boolean(activeId) and streaming: turnActive || activeStreamingLive — the same two values the old closure read directly. No behavior change.
Both halves are pinned, independently
- Dropping
&& !(state.streaming && id === 'compact')fails onlywithholds /compact while the Turn streams, and nothing else. - Dropping
(session === 'none' || state.hasSession)fails onlyoffers only the commands that need no Session before one exists.
Two tests, two halves, no overlap. Neither mutation is caught by the other's test, which is what makes them a partition rather than a pair of overlapping smoke checks.
The line that keeps the first test from being a tautology
assert.ok(idle.includes('compact'), ...) looks redundant next to the deepEqual beneath it. It is not, and this is worth stating because it is the difference between a test that pins a rule and one that merely can fail.
Made /compact unavailable in every state — the case where the command disappears from the catalog entirely:
| Result | |
|---|---|
with the assert.ok line |
fails: an idle Session can compact its context |
| with that line deleted | 2 passed, 0 failed |
Without the precondition, offered({streaming: true}) and idle.filter(id => id !== 'compact') are both just idle, and the deepEqual compares a list to itself. The guard is the only thing standing between this test and a green tautology.
The same instinct shows up in the second test, which spells out ['graph', 'swarm'] instead of deriving it from the catalog. Deriving it would keep passing when a new session: 'none' command appears; spelling it out forces someone to decide. The comment says exactly that, and it is the right call.
Also correct
Removing the enqueueInteraction import is right — app-shell.tsx has no remaining reference to it, and the only live use, in quote-companion-core.ts, imports it itself. The nonTriviaTokens drop from 15617 to 15602 in renderer-architecture.json is consistent with the import removal plus the inlined filter moving out.
Context
#4752 stated that the /compact mid-turn assertion, deleted along with its E2E spec, remained covered by shows only slash commands executable in the current session state. It did not: that spec waits for the Turn to finish before opening the menu, so streaming is false when it counts commands. This PR gives the rule a home where it is actually asserted, and at a tier where it will not flake.
Still marked draft, with label and test pending on this head. My local runs used Node type stripping against source, so CI remains the authority on the built output and the architecture snapshot.
简体中文
在 adb9f976f 上批准。没有发现问题。 这补上了 #4752 留下的覆盖缺口,而且这个测试比它所替代的那条断言更强。
我把它跑了一遍而不是读了一遍,包括两次变异,外加一次对测试自身脚手架的检验。
谓词就是原来那条规则
与它替换掉的 inline 版本逐字对照:
旧: (session === 'none' || Boolean(activeId)) && !(streaming && id === 'compact')
新: (session === 'none' || state.hasSession) && !(state.streaming && id === 'compact')
调用点传入 hasSession: Boolean(activeId) 和 streaming: turnActive || activeStreamingLive——正是旧闭包直接读的那两个值。无行为变化。
两半各自被钉住,互不重叠
- 去掉
&& !(state.streaming && id === 'compact'),只有withholds /compact while the Turn streams, and nothing else失败。 - 去掉
(session === 'none' || state.hasSession),只有offers only the commands that need no Session before one exists失败。
两个测试、两半条件、没有重叠。任一变异都不会被另一个测试捕获,这正是「一个划分」而非「两个互相重叠的冒烟检查」的区别。
那一行让第一个测试不至于变成同义反复的断言
assert.ok(idle.includes('compact'), ...) 挨着下面的 deepEqual,看上去像是多余的。它不是,而且这一点值得写出来,因为它正是「钉住规则的测试」与「仅仅能失败的测试」之间的分界。
我把 /compact 在所有状态下都变为不可用——也就是「该命令从目录里整个消失」的情形:
| 结果 | |
|---|---|
保留 assert.ok 那一行 |
失败:an idle Session can compact its context |
| 删掉那一行 | 2 passed,0 failed |
没有这个前置条件,offered({streaming: true}) 和 idle.filter(id => id !== 'compact') 就都只是 idle,而 deepEqual 是在把一个列表和它自己比较。 那行守卫是这个测试与一次「绿色同义反复」之间唯一的屏障。
同样的直觉也体现在第二个测试里:它把 ['graph', 'swarm'] 显式写出,而不是从目录推导。推导的话,新增一个 session: 'none' 命令时它会继续通过;显式写出则会逼人做决定。 注释说的正是这件事,而这个取舍是对的。
其余也都正确
删掉 enqueueInteraction 这个 import 是对的——app-shell.tsx 里已无任何引用,而唯一还在使用它的 quote-companion-core.ts 自己有 import。renderer-architecture.json 里 nonTriviaTokens 从 15617 降到 15602,与「删除 import + 内联过滤器外移」相符。
背景
#4752 曾表示:随 E2E spec 一起被删除的 /compact mid-turn 断言,仍由 shows only slash commands executable in the current session state 覆盖。实际并非如此:那个 spec 要等 Turn 结束后才打开菜单,所以它数命令时 streaming 是 false。这个 PR 给了那条规则一个真正会被断言到的位置,而且是在一个不会 flake 的层级上。
仍标记为 draft,label 与 test 在这个 head 上均为 pending。我的本地运行用的是 Node 类型剥离直接跑源码,构建产物与架构快照仍以 CI 为准。
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.
Summary
The Desktop composer withholds
/compactwhile a Turn streams — compacting the context the running Turn is still reading from is not something the menu should offer. #4741 deleted the only case that covered that rule, and the surviving slash-command spec opens the menu after the Turn ends, wherestreamingis false. So the rule has no coverage in any tier right now; #4752 said as much and left it as the next tier-three move.The rule lived inside the array filter that builds the menu, reachable only by rendering AppShell. It moves to
desktop-slash-command.ts, next to the parser that owns the same four commands, andnode:testasserts both halves: an idle Session is offered/compact, a streaming one is offered everything else and nothing less. This is a move, not a rewrite — behavior is unchanged.Two things worth naming:
@maka/core/slash-command-cataloginto the smaller module reads better, but the debt ratchet counts dependencies per file, so that books a new dependency indesktop-slash-command.tsinstead of retiring one.enqueueInteractionwas imported from@maka/uiand never used, so it pays for it.Refs #4727 #4752
Verification
npm run test:dist --workspace @maka/desktop— 2115 pass, 0 fail&& !(state.streaming && id === 'compact')from the predicate turns the new test rednpm run typecheck --workspace @maka/desktop,npm run format,npm run lint— cleannode scripts/check-renderer-architecture.mjs --base 3f4ac8c9dd— passes;app-shell.tsxnonTriviaTokens15617 → 15602AI use
Select exactly one:
Tool(s) and scope: Claude Code — located the uncovered rule, moved the predicate, wrote the test, ran the checks above.
Checklist
Does this PR entail a change in behavior?