fix(ui): render queued steering in the transcript and fold follow-ups into the staging drawer - #5546
Conversation
8d4bf02 to
1b152b5
Compare
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-opus-review-orchestrator. Review of exact head 1b152b5b7aafb077fef1d1205be4d0ca3f1266a9. Draft, MERGEABLE, test FAILURE. COMMENT only, no approve.
Verdict
No P0–P3 from me. The CI failure is a stale ledger, but it hides most of the verification — so I ran three of the masked steps locally and report what is still unrun.
The CI failure is the regenerate class, and I checked that specifically
Check renderer architecture reports four lines, and all four are counter diffs: app-shell.tsx nonTriviaTokens 13003→13237, app-shell-session-events.ts 2687→2677, the window.maka.sessions.retractQueueEntry bridge path 1→2, and one added dependency ./application/contracts/transient-message-projection.js. Grepping the failure output for forbidden / not allowed / must not returns zero hits.
That distinction matters because it is not always true. On #5494 the same check mixed two prohibitions in among the counters, and those can never be cleared by regenerating. Here they can.
What that failure is hiding
It runs early, so these never executed: Typecheck, the workspace unit tests, Knip, Storybook smoke, Desktop e2e. That is the third pull request today with this shape, and on #5532 it hid a Desktop e2e failure of 23 of 37 specs across an entire review round.
This branch declares the same @astryxdesign/core my checkout has installed, so a local run is valid here. I filled what I could:
| step | result |
|---|---|
@maka/ui tests |
554/554 pass |
apps/desktop typecheck |
clean — all four tsconfigs (preload, main, renderer, storybook) |
apps/desktop main suite |
2774/2774 pass |
| Knip · Storybook smoke · Desktop e2e | still unrun by anyone |
Both builds were done after deleting the output directory, and I confirmed the artifacts came from this head before trusting a result.
One caveat on that desktop number, because it first came back red. The suite initially reported 2775 tests with 1 failure — composer-git-branch.test.js. That file does not exist in source at this head; #5542 deleted it earlier today, and build:test compiles without pruning outputs whose sources are gone, so a stale .js dated two days ago was still being collected. Removing that one orphan gives 2774/2774. The failure was my working copy, not this branch — recording it so the number is reproducible rather than mysterious.
What I verified in the change itself
The new messageQueue ref/state pair holds its stated invariant. The comment says reseed reconciliation reads the queue between React flushes so every writer goes through the ref. setMessageQueue appears exactly twice in the file — the useState declaration and inside applyMessageQueue — so there is no bypassing writer to leave the ref stale.
The not_admitted handling merged today survived the rebase. use-quote-companion.ts:699 still retires on cancelled || not_admitted. Worth checking explicitly because this branch touches that same function heavily and a rebase is exactly where such a fix gets dropped.
The direction is sound and its invariant is pinned. Making queued steering a projection of the Host queue snapshot — "the bubble appears, updates and disappears with queue alone" — removes a dual-source rendering path rather than adding one. queued steering derives a transcript bubble that lives and dies with the snapshot asserts the bubble's content and actions, that edit hands text back to the composer, that delete retracts without a draft, and that emptying entries removes the bubble.
未验证
- Knip, Storybook smoke and Desktop e2e, as above. Desktop e2e is the gap that mattered on #5532 today.
- No browser and no Electron; I did not exercise the queue plate or the transcript bubble in a running app, only through tests and source.
- I did not review the 27-file change file by file — I went after the invariants the commits claim, the rebase-survival of adjacent merged work, and the masked verification.
- The draft state and the red check are both facts at lock; I am not calling this green.
简体中文
结论:我这边无 P0–P3。 CI 的红是台账过期那一类,但它挡住了大部分验证,所以我在本地补跑了其中三步,并说明还有哪些没人跑过。
这次确实是「重新生成」那一类,而且我专门核过:Check renderer architecture 报的四条全是计数差(token 13003→13237、2687→2677、retractQueueEntry 1→2、新增一条依赖),grep forbidden/not allowed 零命中。这个区分不是理所当然的 —— #5494 上同一个检查里就混着两条禁令,那种重新生成永远清不掉;这次可以。
它挡住了什么:Typecheck、各 workspace 单测、Knip、Storybook smoke、Desktop e2e 全没执行。这是今天第三个这种形状的 PR,而 #5532 上它曾把「37 条挂 23 条」的 Desktop e2e 崩溃藏了整整一轮评审。
这条分支声明的 @astryxdesign/core 与我本机一致,本地跑有效,于是我补了:@maka/ui 554/554 通过;apps/desktop typecheck 四个 tsconfig 全过;desktop 主进程套件 2774/2774 通过。两次构建都先删了输出目录,并在采信结果前确认产物来自本 head。Knip / Storybook smoke / Desktop e2e 仍然没人跑过。
关于 desktop 那个数字有一处交代:它第一次跑出来是 2775 条挂 1 条(composer-git-branch.test.js)。该文件在本 head 源码里并不存在(今天合并的 #5542 删掉了它),而 build:test 只编译、不清理源码已消失的旧产物,所以一份两天前的 .js 仍被收集。删掉这一个孤儿后是 2774/2774。那条失败属于我的工作副本,不是这条分支 —— 写在这里是为了让这个数字可复现。
我在改动本身上验了三件事:①新的 messageQueue ref+state 的不变量成立(setMessageQueue 全文只出现两次:useState 声明与 applyMessageQueue 内,无旁路写入);②今天合并的 not_admitted 处理在 rebase 后仍在(use-quote-companion.ts:699)—— 本单大改同一个函数,rebase 正是这类修复最容易丢的地方;③方向成立且不变量有测试钉住 —— 把 queued steering 变成 Host 队列快照的派生投影,是去掉一条双源渲染路径而不是新增,lives and dies with the snapshot 那条测试连「entries 清空后 bubble 消失」都断言了。
未验证:Knip、Storybook smoke、Desktop e2e(最后一条正是今天 #5532 栽的地方);无浏览器无 Electron,没有在运行中的应用里实际操作队列面板与 transcript bubble;27 个文件我没有逐个审,而是奔着各提交自己声称的不变量、相邻已合并工作的 rebase 存活、以及被挡住的验证去的;draft 与红勾都是锁定时的事实,我没有把它写成绿。
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.
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-terra-review (same GitHub owner as the coordinating review agent, different model). Independent cross-review at exact fresh head 1b152b5b7aafb077fef1d1205be4d0ca3f1266a9. NO-GO — 1×P2 + 1×P3, no P0/P1. Draft, still DIRTY. I did not read the pull request's review comments or other reviewers' findings before sealing.
P2 — this diff leaves the mandatory renderer-architecture gate red, and regenerating the ledger will not clear it
Introduced at apps/desktop/src/renderer/app-shell.tsx:69,697-712,1790-1803; the unchanged expected root-debt entry is apps/desktop/renderer-architecture.json:713-….
A clean npm --workspace @maka/desktop run check:architecture passes all 112 checker fixtures, then rejects the production ledger: the new ./application/contracts/transient-message-projection.js dependency is absent, window.maka.sessions.retractQueueEntry now occurs twice rather than once, and nonTriviaTokens changed in both app-shell.tsx and app-shell-session-events.ts.
Reachability: root npm test runs this gate before the workspace typechecks and test suites, so every normal required run stops before those downstream checks. This is not a flaky test and not a checker-fixture failure.
Why editing the ledger is not sufficient: check-renderer-architecture.mjs:3282-3290 compares the head's root debt against a materialized base tree and rejects an increase. Updating the expected counts satisfies the ledger comparison but still fails the monotonicity rule.
Minimal repair: move the new queue-transcript projection and retract ownership out of the legacy AppShell into an existing appropriate conversation boundary — and avoid the second bridge path — or otherwise restore the current AppShell debt level. Then regenerate, check the ledger, and run the gate.
P3 — apps/desktop/src/renderer/styles/composer.css:647 adds a blank line at end of file
git diff --check reports it. Remove the blank line. Introduced by this diff.
What did pass
#2262 is a human-authored, closed product issue specifying an explicit next-turn queue and current-turn presentation; the implementation maps queued steering from the Host snapshot in main chat, WorkHub and Side Chat. In a clean checkout I ran npm ci --ignore-scripts, applied the repository dependency patches, and saw @maka/ui 554/554 pass, a clean npm run build:test pass, and 105 focused desktop queue / Side-Chat / WorkHub tests pass.
I did not run Knip, Storybook smoke or Desktop E2E, because the required architecture gate is red. The exact head was fresh-fetched again immediately before this report and remains unchanged.
No GitHub write, approval, merge or rebase from this seat.
简体中文
NO-GO,1×P2 + 1×P3,无 P0/P1。 绑最新 head 1b152b5b7;draft、DIRTY;封板前未读本 PR 的评审评论与其他评审者的结论。
P2 —— 本次改动让必需的 renderer-architecture 门禁保持红色,而且「重新生成台账」清不掉它。 干净环境下 check:architecture 先通过全部 112 个检查器夹具,随后拒绝生产台账:新增依赖 ./application/contracts/transient-message-projection.js 未登记、window.maka.sessions.retractQueueEntry 由一次变两次、app-shell.tsx 与 app-shell-session-events.ts 的 nonTriviaTokens 变化。
可达性:根 npm test 在各 workspace 的 typecheck 与测试之前跑这道门禁,因此每次正常的必需运行都会停在它之前。这不是抖动,也不是夹具失败。
为什么改台账不够:check-renderer-architecture.mjs:3282-3290 拿 head 的 root debt 与物化的 base 树比较,只要增加就拒绝;改掉期望计数能过台账比对,仍会卡在单调性规则上。
最小修法:把新的队列-transcript 投影与 retract 归属从 legacy AppShell 移到合适的既有会话边界(并避免那第二条 bridge 路径),或以其它方式把 AppShell 的 debt 降回当前水平;然后重新生成台账并跑门禁。
P3 —— styles/composer.css:647 在文件末尾多了一个空行,git diff --check 会报。删掉即可。
通过的部分:#2262 是人类撰写、已关闭的产品 issue,明确规定了「下一轮队列」与「当前轮呈现」;实现把 queued steering 从 Host 快照映射到主聊天、WorkHub 与 Side Chat。干净检出下 npm ci --ignore-scripts + 应用仓库依赖补丁后:@maka/ui 554/554 通过、npm run build:test 干净通过、105 项定向的 desktop 队列 / Side-Chat / WorkHub 测试通过。
因为必需的架构门禁是红的,我没有跑 Knip、Storybook smoke 与 Desktop E2E。 报告前再次拉取,exact head 未变。本席未做任何 GitHub 写入、批准、合并或 rebase。
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.
|
Seat: kabi-opus-review-orchestrator. Correction to my own review above, same head I was wrong that regenerating the ledger would clear this gateIn my review I classified the I did not take that on trust. I reproduced it at this head: He is right and I was wrong. Once the ledger comparison is satisfied, the monotonicity rule against the base begins reporting — so the first violation class was hiding the second inside the same script. My working tree is back to the committed ledger; nothing was left modified. This matters beyond bookkeeping: I told the author more than once that this was one command away, and I said I would approve once the ledger was regenerated and CI went green. That plan was built on a wrong reading. The actual repair is the one in his P2 — reduce the AppShell debt rather than re-record it. What my earlier check did and did not establishGrepping the failure output for Unchanged from my reviewNo P0–P3 from me on the change itself, and the local runs stand: Knip, Storybook smoke and Desktop e2e remain unrun by anyone, for the reason he gives: the gate is red before them. 简体中文更正我上面那份评审:我说「重新生成台账即可」是错的。 我当时把 我没有直接采信,而是在本 head 上复现了(命令与输出见上): 他对,我错。 台账比对被满足之后,针对 base 的单调性规则才开始报 —— 前一类违规在同一个脚本里挡住了后一类。我的工作树已还原到提交版台账,未留下任何修改。 这不只是记账问题:我不止一次告诉作者「一条命令的事」,并说过「台账重生成、CI 转绿我就批」——那个计划建立在错误的判读上。真正的修法是他 P2 里那条:把 AppShell 的 debt 降下来,而不是重新记录它。 我先前那次检查确立了什么、没确立什么:grep 评审中未变的部分:改动本身我仍无 P0–P3;本地结果仍然成立(
|
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-grok-reviewer. Placement lane at exact head 1b152b5b7aafb077fef1d1205be4d0ca3f1266a9. I am not NO-GO on the product. I agree the AppShell wiring should move, and that the move is real placement rather than a prettier count. I am not taking over another reviewer's gate finding. COMMENT only, no approve. I did not read existing pull-request comments before sealing.
Draft. Live mergeable is CONFLICTING — I am binding the SHA, not the mergeability. test FAILURE is a fact, not green. Against GitHub base 205a06efb (which is the merge-base with live main), 2 commits behind.
1. Does #2262 need AppShell?
No. Issue #2262 is closed, and its text is a claim rather than evidence — but what it asks for is a Host-authoritative next-turn queue plus current-turn presentation. The projection that delivers that is already a session-scoped contract: withQueuedSteeringTransients in application/contracts/transient-message-projection.ts. WorkHub (use-workhub-controller.ts:672) and Side Chat (use-quote-companion.ts:1677) already call it at the session surface. The product does not need whole-tree AppShell state.
Main chat still lives in AppShellContent, so this diff wires a useMemo and retractQueueEntry there (app-shell.tsx:697-713, :1790). That is "where main chat currently sits," not "this belongs above the whole tree." The gate is turning a should-do — put it on the session boundary — into a must.
2. After the move, is the new place more correct?
Yes, and this is the same class as the renderer-architecture.json −51 I assessed on #5532: moving or deleting a node is the right outcome when the node was in the wrong scope. Three pieces of evidence that the new placement is real rather than cosmetic:
- The derivation is already a contract, and two of the three surfaces already consume it outside AppShell.
- Retract is a Host Session operation (
sessions.retractQueueEntry). WorkHub goes throughservices.retractQueueEntry; AppShell adds a secondwindow.maka.sessions.retractQueueEntrypath (:1784,:1792). That dual bridge is the substance behind "avoid a second bridge path" — not a counting trick. restoreLocalMessageDraftis composer-scoped (composerRefplusactiveIdRef). It can travel with the conversation feature; it does not need shell render-body lifetime.
3. If the gate vanished, would "no basis, therefore NO-GO" fire?
Not on the product. The plate carries follow-ups only; queued steering becomes transcript bubbles derived from the Host snapshot. That stands without the ratchet. I would still want the AppShell wiring moved, but I would not NO-GO this pull request solely because the wiring sits in the wrong room.
The gate NO-GO — that the check is red and regenerating the ledger cannot clear it — is the correct gate outcome, and I am not contradicting it. My point is narrower and worth stating on its own:
Do not confuse "the gate forbids new AppShell debt" with "the feature has no basis."
P3 — trailing blank line
composer.css ends }\n\n. Independently confirmed; git diff --check would flag it. I will not inflate it past P3.
未验证
I did not run the UI suite, the desktop typecheck, Knip, Storybook or e2e. Numbers claimed by another reviewer are not my evidence. I did not re-run the architecture checker locally.
简体中文
我不对产品给 NO-GO。 我同意 AppShell 那段接线应该挪走,而且挪走是真的放对位置,不是把计数弄好看。我不接管别的评审席的门禁 finding。draft;live mergeable 为 CONFLICTING(我绑的是 SHA,不是可合并性);test FAILURE 是事实不是绿;相对 base 205a06efb 落后 2。
一、#2262 需要进 AppShell 吗?不需要。 它要的是 Host 权威的下一轮队列 + 当前轮呈现,而实现它的投影本来就是 session 作用域的合约(withQueuedSteeringTransients),WorkHub(:672)与 Side Chat(:1677)已经在会话边界调用它。主聊天目前恰好住在 AppShellContent 里,所以本单把 useMemo 与 retractQueueEntry 接在了那儿 —— 那是「主聊天现在在哪」,不是「这东西该在整棵树之上」。门禁是把一件本该做的事变成了硬要求。
二、挪走之后是不是更对?是。 与我在 #5532 判 renderer-architecture.json −51 同类:节点本来就在错误的作用域时,挪/删就是应然。三条证据:①投影已经是合约,三个界面里有两个已在 AppShell 之外消费它;②retract 是 Host Session 操作,WorkHub 走 services.retractQueueEntry,而 AppShell 另开了第二条 window.maka.sessions.retractQueueEntry(:1784、:1792)—— 这条双 bridge 才是「避免第二条 bridge 路径」的实质,不是计数把戏;③restoreLocalMessageDraft 是 composer 作用域的,可以跟着会话特性走,不需要 shell 渲染体的生命周期。
三、门禁若不存在,「没依据 ⇒ NO-GO」会触发吗?对产品不会。 面板只放后续消息、queued steering 变成由 Host 快照派生的 transcript 气泡 —— 这在没有棘轮的情况下也站得住。我仍然希望那段接线挪走,但不会仅因为接线放错房间就 NO-GO 整个 PR。
门禁那条 NO-GO(检查红、且重新生成台账清不掉)是正确的门禁结论,我不与之矛盾。我要说的是更窄的一句:不要把「门禁禁止新增 AppShell debt」误读成「这个功能没有依据」。
未验证:我没跑 UI 套件、desktop typecheck、Knip、Storybook 或 e2e;别的评审席声称的数字不构成我的证据;我也没有在本地重跑架构检查器。
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.
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-sol. Draft round-trip lane at exact head 1b152b5b7aafb077fef1d1205be4d0ca3f1266a9. 2×P2 — I do not recommend merging this head. Scoped review, not approval. COMMENT only.
Both findings are about editing a queued steering: editing restores only text and loses the original attachments and quotes, and switching Sessions while the retraction is in flight withdraws the entry without restoring its text to the originating draft.
P2 — restore attachments and references along with the edited text
apps/desktop/src/renderer/application/contracts/transient-message-projection.ts:90
For a queued steering carrying an attachment or a quote, Edit now retracts the original entry and restores only the string. WorkHub, Side Chat and main chat all ultimately call only setText / appendText, so the attachment or quote that was displayed on the bubble is absent from the restored draft and from its next send.
The previous in-place queue update retained the rest of entry.content. Please transfer the complete queued content into the draft, or keep in-place editing. A successful text-only restore is not a lossless edit.
To be precise about the blast radius: the attachment files are not deleted. What is lost is their association with the message the user was editing.
P2 — save recovered text to the originating draft after navigation
apps/desktop/src/renderer/app-shell.tsx:614
Click Edit on Session A's queued steering, switch to Session B before retractQueueEntry resolves, then let the request succeed. The entry has been withdrawn, but the active-Session guard discards the restore — leaving A without its queued text.
I reproduced the ordering with these production callback expressions; the same-session control restores correctly. Save the recovered content under A's draft key even when A is inactive, and only focus the composer when it is still showing A.
Controls that behave correctly
A failed retraction does not restore the draft, and both hooks surface the error. Delete does not restore a draft. Existing text is preserved in the successful same-Session control. So the fence is not simply absent — these two orderings fall outside it.
Why the suite did not catch either
The complete Desktop build and 101 related tests passed. Neither path is covered: the tests exercise text-only queued entries, and none composes a retraction with a Session switch before it resolves.
未验证
I checked mounted production WorkHub and Side Chat hooks and the main-chat callback expressions with controlled queue snapshots, mutation responses and draft sinks. I did not run full Electron or Host flows, Storybook smoke, Knip, or Desktop e2e, and I did not repeat another reviewer's architecture-gate investigation. Hosted test remains failed.
简体中文
2×P2,不建议合入本 head。 两条都关于「编辑一条已排队的 steering」。
P2 ①(transient-message-projection.ts:90)—— 编辑只恢复文本,丢掉附件/引用。 带附件或引用的 queued steering,Edit 现在会撤回整条,却只把那串文本交回;WorkHub、Side Chat 与主对话最终都只调 setText/appendText,于是气泡上原本显示的附件/引用既不在恢复的草稿里,也不会随下一次发送回去。此前的原地队列更新会保留 entry.content 的其余部分。请把完整的排队内容交回草稿,或保留原地编辑 —— 只恢复文本的成功,不是无损的编辑。 说清影响边界:附件文件本身没有被删除,丢的是它与用户正在编辑的那条消息的关联。
P2 ②(app-shell.tsx:614)—— 导航之后要把恢复的文本存回原来那条会话的草稿。 在会话 A 上点编辑,在 retractQueueEntry 返回之前切到 B,然后让请求成功:条目已经被撤回,而 activeId 守卫把恢复丢弃了 —— A 那边既没有队列条目,也没有那段文本。 我用这些生产回调表达式复现了该顺序,同会话对照能正确恢复。即使 A 不是当前会话,也应把恢复内容存到 A 的草稿键下,只有在界面仍停留在 A 时才聚焦 composer。
行为正确的对照:撤回失败时不恢复草稿,且两个 hook 都会显示错误;删除不恢复草稿;同会话成功对照中既有文本被保留。所以围栏并非不存在,是这两种顺序落在它外面。
为什么套件没抓到:完整 Desktop 构建与 101 项相关测试通过,但两条路径都没被覆盖 —— 测试用的是纯文本队列条目,也没有任何用例把「撤回」与「在它返回前切换会话」组合起来。
未验证:我核的是挂载的生产 WorkHub / Side Chat hook 与主对话回调表达式(受控的队列快照、变更响应与草稿槽)。未跑完整 Electron/Host 流程、Storybook smoke、Knip 或 Desktop e2e,也未重复其他席位对架构门禁的调查。hosted test 仍失败。
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.
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-grok (same GitHub owner as the coordinating review agent, different model). Review of exact head 1b152b5b7aafb077fef1d1205be4d0ca3f1266a9. I did not read existing comments. 2×P3. Not approving.
Frame. Merge-base 205a06efb (same as the PR base). Ahead 9, behind 3 (#5070, #5073, #5532). Draft, CONFLICTING/DIRTY. test is completed/failure, failing at Check renderer architecture; Typecheck, Knip, e2e and workspace tests were skipped after it. Not green. I did not reproduce the architecture gate and I do not endorse another reviewer's finding on it.
Nine commits vs their messages
Each commit does the thing in its subject, with two later steps undoing earlier ones:
ea12cdfce(help icon reachable by pointer) is removed bye261d1a67(drop the help icon).1b152b5b7is the inventory leftover.ee55624daputs queued steering in the transcript by storing a bubble;94f47ed5ckeeps the transcript placement but stops storing and derives from the Host snapshot.
The head is coherent. The history is not a straight line.
[P3] the snapshot test pins helper de-duplication, not plate + transcript
withQueuedSteeringTransients filters stored transients by queue-owned ids — the comment calls this "a message never renders twice." The test asserts ['message-1', 'message-steer'] and that empty entries leaves no bubble. That is the merge rule, including "the queue copy replaces the local copy."
It is not a render of ChatView plus the plate. The plate already keeps next_turn only, and nothing in the suite mounts both. So the stated invariant and the tested one are not the same statement.
[P3] Side Chat messageQueue ref + state has no test
applyMessageQueue writes the ref and then setMessageQueue. Statically that is the only setMessageQueue call, and reseed reads the ref between flushes. No test fails if a future writer updates state only.
Verification bounds
Walked: the nine commit stats and subjects, the plate filter, withQueuedSteeringTransients and its test, applyMessageQueue, and the failing CI step. Did not run the UI or desktop suites. Did not read other reviews.
简体中文
2×P3,不批准。 merge-base 205a06efb(与 PR base 一致),领先 9、落后 3(#5070、#5073、#5532);draft、CONFLICTING/DIRTY;test failure,失败步是 Check renderer architecture,其后 Typecheck/Knip/e2e/workspace 测试被跳过;我没有复现架构门禁,也不为别的评审席那条背书。
九个提交名实相符,但其中两步推翻了前面:ea12cdfce(让帮助图标可点)被 e261d1a67(删掉帮助图标)撤销,1b152b5b7 是清单收尾;ee55624da 用「存一个气泡」的方式把 queued steering 放进 transcript,而 94f47ed5c 保留了 transcript 位置但不再存储,改为从 Host 快照派生。head 上的最终态是自洽的,历史不是一条直线。
[P3] 那条快照测试钉的是 helper 的去重,不是「面板 + transcript」。 withQueuedSteeringTransients 按队列拥有的 id 过滤已存 transient,注释称之为「一条消息不会渲染两次」;而测试断言的是 ['message-1','message-steer'] 以及 entries 清空后没有气泡 —— 那是合并规则(含「队列副本替换本地副本」),不是把 ChatView 和面板一起渲染。面板代码确实已经只留 next_turn,但套件里没有任何用例把两者一起挂载。所以「声称的不变量」与「被测试的不变量」不是同一句话。
[P3] Side Chat 的 messageQueue ref+state 没有测试守着。 applyMessageQueue 先写 ref 再 setMessageQueue,静态看它是唯一的 setMessageQueue 调用点,reseed 在两次 flush 之间读 ref。但将来若有写入方只更新 state,没有任何测试会失败。
验证边界:走过九个提交的 stat 与标题、面板过滤、withQueuedSteeringTransients 及其测试、applyMessageQueue、CI 失败步。未跑 UI/desktop 套件,未读其他评审。
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. Seat: kabi-grok.
|
Seat: kabi-opus-review-orchestrator. Second correction to my own review on this head, and the round synthesis. I overstated what the snapshot test pinsIn my review I wrote that the "never renders twice" invariant is pinned by @kabi-grok's lane shows that is not what the test asserts. It pins So the stated invariant and the tested one are different statements, and I presented them as the same. That is the second thing I got wrong on this pull request today — the first being the architecture gate, corrected above. Round summary — five review lines at this head
Counted honestly: 3×P2 and 2×P3 that are distinct. The trailing-newline P3 was confirmed independently by two seats and is one finding, not two. @kabi-grok explicitly did not endorse the gate P2, and @kabi-grok-reviewer explicitly did not take it over — so the gate finding rests on @kabi-terra-review plus my reproduction, not on four agreeing voices. On independence: four of these five seats are different models under one GitHub owner. That is different blind spots, not independent judgment. The only different-owner seat was unavailable for this round. I am not presenting five lines as five independent confirmations. What I would fix first, on the evidence rather than on seat count: @kabi-sol's two P2s are user-visible content loss on an ordinary action, and they are independent of the gate. The gate P2 blocks the merge, but those two would still be worth fixing if the gate did not exist. 简体中文我在本单第二次更正自己:我高估了那条快照测试钉住的东西。 我的评审里说「一条消息不会渲染两次」这个不变量已被测试钉住,并以此支持「方向对且有守卫」。@kabi-grok 指出测试断言的其实是 本轮五条线的结果见上表。诚实计数:3×P2 + 2×P3(互不重复)。 CSS 末尾空行那条由两席各自独立确认,是同一条,不是两条;@kabi-grok 明确不为门禁那条 P2 背书,@kabi-grok-reviewer 明确不接管它 —— 所以门禁那条依据的是 @kabi-terra-review 加上我的复现,不是四个声音一致。 独立性:五席里有四席是同一 GitHub owner 下的不同模型 —— 那是不同盲区,不是独立判断;唯一不同 owner 的席位本轮不可用。我不会把五条线说成五次独立确认。 若按证据而非席位数排先后:@kabi-sol 那两条 P2 是普通操作下用户可见的内容丢失,且与门禁无关 —— 门禁那条挡住合并,但即使门禁不存在,那两条仍然值得先修。
|
1b152b5 to
d395f59
Compare
f4599e6 to
bc05a06
Compare
|
All findings from the kabi-sol P2 — edit restores only text (attachments/quotes lost). Fixed in kabi-sol P2 — navigation race discards the restore. Fixed in kabi-terra P2 — architecture gate cannot be cleared by regenerating. Fixed in composer.css trailing blank line (kabi-terra, kabi-grok-reviewer — one finding). Removed in kabi-grok P3 — the plate+transcript invariant is not pinned together. Addressed in kabi-grok P3 — Side Chat kabi-opus corrections. Both acknowledged — the strict-base reproduction was correct and is exactly what Verification at |
9d9f2da to
eb54a75
Compare
|
Adversarial review round (fresh eyes on Fixed — two P2s, one P3, all in the "positive evidence only" family:
Simplification audit outcome ( Also hardened a latent footgun the audit flagged: the queued-steering Edit action is now offered only when the caller can actually restore the draft ( Declined with reasons: the three-way queue-controller triplication stays — the surfaces genuinely differ in lifecycle (per-session map vs. one persistent session vs. fork-reseed tombstones) and this PR already unified the shared projection helpers; Verified: runtime-host 27/27, desktop 172/172 (observer 61 incl. updated seed expectations), UI 6/6, CLI 230/230, storybook typecheck, renderer architecture check (ledger + strict base), Biome format/lint, and |
0d2850b to
cb37dd5
Compare
4d1058f to
b7b5f07
Compare
jackwener
left a comment
There was a problem hiding this comment.
[kabi-opus-dev] Taking over finding ① from kabi-terra-review (not in this round), plus the implementation-wide removal and consistency check. Bound to 55cd6757ca3b98b73a3dfc0fd6dae00bd656f46f; re-checked against GitHub immediately before posting, unmoved. Draft; CI test pending, so I claim nothing from CI.
Conclusion: ① and its P3 are both fixed, and ① is fixed the right way rather than papered over. No P0–P3 from my lane.
① renderer-architecture gate — fixed, and I checked the distinction that mattered
I read terra's original review rather than the dispatch summary, because their finding contained the load-bearing detail: check-renderer-architecture.mjs:3282-3290 compares the head's root debt against a materialized base tree and rejects an increase, so regenerating the ledger would satisfy the comparison and still fail monotonicity. That makes "the gate is green" insufficient on its own — the question is why it is green.
Evidence that the debt moved rather than the numbers:
retractQueueEntrynow occurs zero times inapp-shell.tsx(terra's 1→2 item). Ownership sits in the services/ports layer:create-workbar-services.ts:203-204,create-workhub-services.ts:115,features/workhub/ports.ts:68,features/workbar/ports.ts:247.transient-message-projectionlives underapplication/contracts/and is registered in the ledger; its importers aresession-workspace-actions.tsand the three feature controllers, not the legacy shell.- Clean-environment run (
npm cithennpm --workspace @maka/desktop run check:architecture): 112/112 checker fixtures pass, thenRenderer architecture check passed., exit 0.
That is the "minimal repair" terra prescribed — move the projection and retract ownership out of the legacy AppShell — not a ledger edit.
P3 (trailing blank line, composer.css:647): fixed. git diff --check against the base is clean.
Removal completeness — complete
projectQueuedTransientMessages: 0 hits. isQueuedSteering: 0 hits. Both gone repo-wide, no residual references.
withQueuedSteeringTransients across the three surfaces — consistent
One definition (application/contracts/transient-message-projection.ts:54), exactly three call sites, one per surface, all passing the same option shape { locale, retract, restoreDraft }:
| surface | call site | retract binding | draft key |
|---|---|---|---|
| main chat | use-session-message-queue.ts:110 |
runAction(…, sessionId) — explicitly pinned |
sessionId |
| WorkHub | use-workhub-controller.ts:738 |
deleteQueuedEntry → mutateQueue((target) => …) |
sessionId |
| Side Chat | use-quote-companion.ts:1746 |
deleteQueuedEntry |
companion.id |
The differences are surface-appropriate rather than drift: each wraps retract in its own session-resolving helper, and Side Chat keys the draft to the companion because that is its session identity. The guards match their surface too (sessionId for the first two, companion for Side Chat).
Boundary: the main chat's explicit pinning (// Bubble actions stay bound to the Session that rendered them) sits in the same area as finding ③. I am not adjudicating ③ — that is another reviewer's lane and they are re-running the original repro. I am reporting structure only.
What I verified (this round, this head)
- Head matches; base
027d6afba, 59 files. - Clean
npm ci(exit 0), thencheck:architectureas quoted above — the full output is 112 fixtures plus the production ledger check. git diff --checkclean against the base.- Residual-reference greps for the two removed symbols and for
retractQueueEntryinapp-shell.tsx. npm run build:testexit 0;transient-message-projection8 pass / 0 fail.
What I did NOT check
- No running app, no browser. Source, the gate, and node tests only.
- Findings ② and ③ are not mine and I did not judge them, including whether full content restore now works and whether the A→B retract race is closed.
- I did not run Knip, Storybook smoke, Desktop E2E, or the full suite; no Windows.
- I did not ablate anything this round.
Automated review seat. Not an independent human review, and not independent of the other seats posting from this account.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the 59-file diff at 55cd6757ca3b98b73a3dfc0fd6dae00bd656f46f. I found no substantiated P0–P3 issue in the paths inspected; this is not merge approval.
The queue snapshot now owns admitted steering/follow-up presentation. Main chat retires matching local placeholders on queue_update (app-shell-session-events.ts:303–325); the same snapshot-to-bubble projection is used by main chat, WorkHub, and Side Chat (transient-message-projection.ts:48–98). I traced edit through Host retraction and the keyed draft/context restorers (use-session-message-queue.ts:78–125, app-shell.tsx:675–680, workhub-composer.tsx:59–100, quote-companion-panel.tsx:169–180). I also checked the unconfirmed-send gates and reseed resolution paths in WorkHub and Side Chat, the Desktop-only empty-queue seed, and the changed regression tests and stories. I did not find a demonstrated duplicate-execution or cross-Session draft write in these flows. The queue/draft abstractions appear tied to the three actual surfaces rather than an unrelated refactor.
Limits: this was a static review; I did not run the local suites or a real Desktop/Electron interaction (this checkout has Node 18 and no installed dependencies). The current-head test check is still in progress, so its result is not part of this conclusion. git diff --check and a merge-tree against fetched origin/main are clean. No database/schema migration is introduced. A browser focus/visibility check after a Side Chat tab switch, and a real reconnect/session-switch run, remain unverified.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
jackwener
left a comment
There was a problem hiding this comment.
[kabi-sol] Two P2 issues remain in the edit/restore path at 55cd6757ca3b98b73a3dfc0fd6dae00bd656f46f.
Earlier findings:
- Full-content restoration: partially fixed. The original mounted WorkHub/Side Chat probes now receive structured text, attachments and quotes rather than only text. However, their resend consumers do not handle the newly restored
retainedattachments: WorkHub rejects the resend, while Side Chat omits the attachment from its submission. The inline comment identifies both consumers. - Navigation during retraction: partially fixed. A normal A→B switch now restores into A, leaving B untouched. Concurrent edits for A and B also append into their respective drafts. But navigating to a shared Session unmounts the ordinary composer, so successful retraction still has no text draft sink. Returning to A leaves the retracted text absent.
Evidence run this round:
- Fresh dependency installation and patching, Desktop workspace-dependency and main builds succeeded; 131 focused tests passed (122 queue/transient/WorkHub/Side Chat/draft-handoff tests plus nine staged-content tests).
- Re-ran the original production WorkHub/Side Chat hook probes with controlled queue/retract services. Successful edit returns structured content; failed retraction and Delete return no draft.
- Mounted the production queue hook with the production draft hook and a controlled text port: ordinary navigation, two Session edits in flight, and rejected-retraction control.
- Mounted the production queue hook with actual ChatComposerRegion/Composer in LinkeDOM. Keeping the ordinary composer mounted preserves
existing A\n\nqueued instruction; replacing it while retraction waits (the shared-Session branch used by AppShell) makes its ref null and returning to A yields empty text. The harness models that conditional branch; it is not the entire AppShell. - Mounted the production WorkHubComposer logic and staging hook, substituting only the child Composer presentation to capture its callback. Restoring a valid attachment creates a retained staging item; invoking the actual resend callback calls
prepareAttachments(A, []), reports an upload-reference error, and never callsonSend. A report-local control that consumes the retained reference directly sends the original attachment successfully. - Extracted Side Chat's current Composer
onSendcallback verbatim and connected it to the mounted production companion hook with a retained staging item. Resend reaches the controlledsubmitFollowUpservice with{}for structured content, without the attachment. This observes the service boundary, not a live Host send.
No full Electron/real Host transport, browser layout, Windows, architecture-gate, full-suite, or complete cross-surface isolation certification. CI test was still in progress at the final head check. Production sources were not changed; control changes live only in the evidence directory. No approval.
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.
zhiiw
left a comment
There was a problem hiding this comment.
Bound to 55cd6757ca3b98b73a3dfc0fd6dae00bd656f46f (re-checked against GitHub immediately before posting, unmoved). Draft; CI test green on this head. Real Windows machine, Node 24.18.1. My lane: real-machine suites plus ablation of the four named rules.
No new P0–P3 of my own; no inline comments. The four rules are all pinned, and each pin bites exactly its own test.
Suites on this head (Windows, all green — nothing to attribute)
- packages/ui:
chat-turn-steering-order,composer-message-queue,composer-send-toggle,composer-shortcuts— 56/56. - packages/runtime-host:
session-projector— 29/29. - apps/desktop:
message-queue-ui-state,transient-message-projection,chat-view-optimistic-render,app-shell-busy-race-settlement,app-shell-first-send-cleanup,composer-directories,composer-mentions,quote-companion-retry,runtime-host-session-observer,session-reference-composer,session-workspace-action-identity,workhub-send-visibility— 231/231.
Ablations (each restored, rebuilt, re-verified green)
- Edit restores full content — removed
attachments/directoryReferences/quotesfrom the edit action'srestoreDraft(transient-message-projection.ts) → exactlyediting a queued steering restores content under the owning Session even after navigationgoes red (14/15 otherwise). - Keyed draft store — made
restoreDraftwrite to the active Session instead of the bound one (use-session-message-queue.ts) → the same test goes red (it pins both halves: content and destination). - Unconfirmed delivery cannot resend — gave
state === 'unknown'a second (remove) delivery action (session-local-messages.tsx) → exactlylocal delivery recovery cannot republish accepted Host queue rowsgoes red; the surviving surface offers onlyCheck delivery. - Steering and follow-ups never cross — widened the transcript filter to admit
next_turnentries (withQueuedSteeringTransients) → exactlyqueued steering derives a transcript bubble that lives and dies with the snapshotgoes red. The drawer side holds the complement (projectComposerMessageQueuefiltersnext_turnand dedups against the whole queue), so the partition is enforced from both ends.
The retract-then-restore ordering is right at the seam I read: edit's restoreDraft only runs after retract resolves, so a failed edit restores nothing; the error toast fires inside runAction and the projection's .catch(ignore) only suppresses the rejection, not the report.
Corroboration of kabi-sol's P2① (consumer-side resend gap) — by code reading
His probe says WorkHub rejects the resend of a restored retained attachment. The mechanism is visible in the production send path: workhub-composer.tsx:112-121 resolves every staged item through uploaded.current (an in-memory cache of this composer's uploads) and falls to services.prepareAttachments(sessionId, toComposerIngestItems([item])) on a miss. A restored retained reference is not in that cache and has no branch of its own — it is re-ingested as if it were a fresh local file, which is where the upload-reference failure comes from, and the send aborts with attachmentUploadFailed. The restore side (draftRestore → staged.restoreAttachments) does its half; the send side has no retained-reference half. My four-rule ablations above pass because their pins live on the main-chat path; this consumer gap is outside them, which is consistent with his "why the suite didn't catch it" account.
Read, no finding
withQueuedSteeringTransientsderives bubbles purely from the snapshot (placement === 'current_turn', Host order preserved) and collapses any lingering local copy of an admitted entry — thequeue_updatesnapshot/retire tests cover the lifecycle.restoreDirectoriesre-derives the same${draftKey}:${hostId}owner key the picker staged under, so a retracted directory reference lands back in its own bucket.- The composer handle's
setDraft/appendDraftwrite the keyed store unconditionally but only focus when the active key matches — that is the correct half of the navigation rule (write always, focus only when showing). - CI note: hosted
testis green on this exact head.
Not checked
Storybook smoke locally, e2e, full repo suite, Electron/runtime flows, the other seats' P2 re-checks (architecture gate, shared-session draft sink) — those are their lanes.
UTC 2026-09-27 09:45.
3631497 to
4eb034d
Compare
jackwener
left a comment
There was a problem hiding this comment.
[kabi-opus-dev] Re-review of the Guest composer refactor (b495ccc60, now b4f493fd1 after the rebase), the architecture gate, removal completeness, and three-surface consistency. Automated review (Claude). Same owner as the coordinating seat, so this is not independent corroboration.
Bound to 4eb034d4916e953d582db9dfda4408cf3632ab6d. I did the analysis on 36314974d. The head then moved to the rebased 4eb034d49. git range-diff shows all three new commits replayed unchanged (=), so I reran every check listed below on 4eb034d49.
Conclusion: no P0–P2. 1×P3 (inline): the AppShell wiring that actually fixes the old composer-swap bug is not pinned by any test.
Where the Guest composer moved
- Before: in
app-shell.tsx,sharedSessionActive ? <SessionTurnRequestComposer/> : <ChatComposerRegion/>. That swap unmounted the Composer, and it was the root cause of the old ③ text loss. - After: AppShell always renders one
ChatComposerRegion, wrapped inSessionCollaboration.GuestTurnRequests. For an owned Session the wrapper projectsundefined. For a shared one it projects{ composer: { onSend → requestTurn, pendingMessages → staging-drawer rows } }, or{ notice }when the Guest has no Turn access. Inchat-composer-region.tsx,guestComposerPropspasses only the model-display props plusguest.composer. Owner interactions, quotes, mentions, directories, attachments and goal controls do not reach a Guest's Composer. Drafts stay in the Composer's own per-key store (draftKey = activeId), so the module-level draft/attempt maps and the draft-token scheme are gone rather than moved.
Removal completeness
Deleted: the component (451 lines), its test (328), 12 CSS selectors, 3 copy keys × 3 locales, and the index.ts/testing.ts exports. A repo-wide grep for the component name, its file path, every removed CSS class, the 3 removed copy keys, and the replaced toComposerIngestItems/retainedAttachmentRefs finds 0 hits outside dist. As a positive control, the grep finds GuestTurnRequests and toSubmittedAttachments where expected. Every surviving session-collaboration copy key still has a reader, except shareDescription, which was already unused at base and is not this PR's.
Architecture gate — green for the right reason
The npm script check:architecture runs without --base, so locally it does not run the base-tree monotonicity comparison. CI passes --base "$BASE_SHA" --strict-base, so I ran it that way:
4eb034d49vs its merge-base82247633f(current main): "Renderer architecture check passed against 8224763…", exit 0; fixture suite 112/112.app-shell.tsxnonTriviaTokens: 12385 on main → 12151 at this head. The refactor itself adds +18 (the extra render-prop layer), which is well inside the net reduction.useRefin app-shell 13 → 12.- Before the rebase,
36314974dpassed against its old base027d6afbabut failed against then-current main:app-shell.tsx: new or increased hookCalls debt useWorkbarController, because main's #4692 moved Workbar below AppShell. The rebase resolved this properly, sinceuseWorkbarController(appears 0 times inapp-shell.tsxnow. The ledger was not just regenerated.
Three surfaces
withQueuedSteeringTransients still has exactly three call sites (main chat use-session-message-queue.ts, WorkHub use-workhub-controller.ts, Side Chat use-quote-companion.ts). 39fa0483b makes attachment submission more uniform: main chat, WorkHub and Side Chat now all go through one toSubmittedAttachments, which splits retained vs. new items in one place. The Guest's staging-drawer rows are Turn requests, not queued steering, so they correctly bypass that helper.
Verified (on 4eb034d49, output kept)
Clean npm ci (lockfile identical to 36314974d); strict-base gate as above; git merge-tree --write-tree HEAD origin/main exit 0; npm run build:test exit 0; guest-turn-requests, workhub-composer-restore, quote-companion-retry, goals-boundary, and session-turn-request-inbox-model 90/90; residue greps above.
Not verified
- Local typecheck failed with 7 errors in the untouched
src/preload/runtime-host-session-catalog.ts, even after a fullnpm run build. CI's ownTypecheckstep passed on36314974d, so I treat this as my environment. I am not claiming a local typecheck pass. - I did not run the app, Electron, or a browser.
- I did not check the Guest write, permission, or draft-isolation behaviour, or whether the model picker is really read-only for a Guest. That is @kabi-sol's lane.
- No Knip, Storybook smoke, E2E, or full test suite. No Windows run.
jackwener
left a comment
There was a problem hiding this comment.
[kabi-sol] The two previous P2 repros pass, but I found one further P2 in the Guest draft flow. This recheck ran at the requested 36314974dfa25b883fd6124c66edf593d1558a16.
- Restored attachments now survive WorkHub resend as their original Host references, without another upload. The original Side Chat resend probe now reaches
submitFollowUpwithretainedAttachmentsinstead of an empty structured-content object. - Switching from an owned Session to a shared Session no longer removes the normal Composer. Re-running the deferred-retraction probe with the new GuestTurnRequests wrapper retains the composer handle and restores the queued text into the original Session's draft.
- New P2: send a Guest request, write the next unsent draft while the response is pending, then navigate away before success. The shared Composer's completion clears the originating draft unconditionally when its key is inactive, deleting the newer text. Staying in that Session preserves it. The inline comment identifies the actual clear operation and its precondition.
This round: fresh dependency installation/patching and Desktop dependency/main builds succeeded; 98 focused tests passed. Those include Guest request routing, hidden composer without Turn access, owner controls excluded from Guest mode, lost-response reconciliation, retry-ID reuse, draft handoff and attachment restoration. I also ran the original controlled attachment/queue probes and an additional mounted GuestTurnRequests + actual ChatComposerRegion/Composer probe. The latter submits the real form, changes draft text through the composer handle, and defers the request service response. The rendered textbox remains editable during that response wait. A report-local control adding a stored-text equality check before the inactive-key clear preserves the new draft.
No full Electron, real Host authorization/transport, Windows, architecture gate or complete permission/isolation certification. The WorkHub probe substitutes the child Composer presentation to capture its real callback; Side Chat uses the verbatim panel callback with its production hook. The Guest/navigation probes mount actual shared Composer components in LinkeDOM.
The live branch had already rebased to 4eb034d4916e953d582db9dfda4408cf3632ab6d by publication. Tests above remain bound to the requested pre-rebase head; I separately compared the Guest controller/wrapper, ChatComposerRegion, Composer and draft hook against that live head and found their contents unchanged. This is not a test or CI certification of the rebased branch. No approval.
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.
jackwener
left a comment
There was a problem hiding this comment.
[kabi-opus-dev] Re-review of b4dc3d441 (removal completeness), 39e153033 (my earlier P3), and the architecture gate. Automated review (Claude). @kabi-sol has the Guest draft P2, the retract state machine, and send-clear consistency. Same owner as the coordinating seat, so this is not independent corroboration.
Bound to 39e15303376b243e0468fcf6d808f3947fbcfd14; head re-checked before publishing. It fast-forwards from 4eb034d49, which I reviewed last round. CI has not reported on this head yet, so I cite no CI result.
Conclusion: no P0–P2. My earlier P3 is partly closed, and 1×P3 remains (inline). One observation, not graded, on the Host operation.
① b4dc3d441 removal completeness
Repo-wide git grep, head vs previous head 4eb034d49:
| symbol | before | now |
|---|---|---|
updateQueueEntry (client, IPC handler, preload, bridge contract, 3 ports, 2 service adapters) |
36 | 0 |
IPC channel sessions:updateQueueEntry |
4 | 0 |
CSS .maka-composer-queue-edit |
9 | 0 |
copy saveQueuedEntry / cancelQueuedEntryEdit (3 locales) |
10 / 6 | 0 / 0 |
- No caller still needs the removed path.
@maka/uitypecheck exit 0. I ran desktoptsconfig.main,.rendererand.storybookindividually: all exit 0 with 0 errors..preloadshows the same 7 errors I see on every head, all in the untouchedsrc/preload/runtime-host-session-catalog.tsand none in the changedpreload.ts; last round CI's Typecheck passed with them present locally. The removedrequiredText/requiredSequencehelpers and thequeueRevisionfield on the plate projection have no remaining readers, or typecheck would fail.queueRevisionstill has 161 hits because promote, retract and reorder keep their CAS. - Knip, as CI runs it (
--workspace apps/desktop,--workspace packages/ui): both exit 0. The planted-export positive control I ran on #5761 used the same workspace config. - Observation, no grade: the Host side of this operation stays, and after this PR nothing in the repo calls it. Its live references are all Host-internal:
queue.entry.updateinprotocol/message.ts:263,operations.ts:315,message-coordinator.ts:415, epoch note 46, and theQueueEntryUpdateInputdecoder. Its own tests are the only callers, andgit grepoutsidepackages/runtime-hostfinds no client. Leaving it here is reasonable, since removing a protocol operation is an epoch change and belongs in its own PR. It is now a server capability with no in-repo client, so it may deserve a follow-up issue so it doesn't linger.
② Is my previous P3 closed by 39e153033? Partly.
The new test reads the real app-shell.tsx from source (readFileSync on ../../../src/renderer/app-shell.tsx, which resolves correctly from dist/main/__tests__). That is the source-boundary option I suggested, and it is not a second hand-written mirror. Ablation: each mutation was applied to app-shell.tsx, the test rerun, and the file restored (tree clean, test passes again):
| mutation | remounts Composer? | new test |
|---|---|---|
A. key={shared ? … : …} on <GuestTurnRequests> (my original ablation) |
yes | fails ✅ |
B. same key on the parent <TaskEntry.TaskEntryWorkspacePickerConsumer> |
yes | passes ❌ |
C. {sharedSessionActive ? <div/> : (<TaskEntry…>…)} around the slot |
yes | passes ❌ |
D. {!sharedSessionActive && (<TaskEntry…>…)} around the slot |
yes | passes ❌ |
C is the original bug's shape: a ternary above the slot. Details inline.
③ Architecture gate, run the way CI runs it
--base 82247633f --strict-base(merge-base): passed.--base 592d2d4dc --strict-base(current main): passed. Fixtures 112/112.- Ledger realism: regenerating with
--writeat head gives a file byte-identical to the committedrenderer-architecture.json(restored afterwards).app-shell.tsxnonTriviaTokens12171 → 12164. git merge-tree --write-tree HEAD origin/main: one conflict, in the generateddocs/astryx-surface-file-inventory.md. It needs regenerating on rebase and is not hand-mergeable text.
Tests run
All 13 test files touched since 4eb034d49: desktop 156/156, @maka/ui 15/15.
Not verified
I did not re-run the Guest draft probe or check the retract state machine's content, ownership or cross-surface clear consistency. Those are @kabi-sol's. I did not run the app, E2E or the full suite, and there is no CI result yet.
|
|
||
| // The harness below mirrors AppShell's wiring; this pins AppShell to it, since | ||
| // app-shell.tsx is not in the node test build. | ||
| test('AppShell mounts the one ChatComposerRegion inside GuestTurnRequests with no owner/Guest remount', () => { |
There was a problem hiding this comment.
[kabi-opus-dev] P3 — this pins the slot's own attributes, not that the slot is always mounted. The original bug's shape still passes.
Reachability: ① normal user path (owned Session → shared Session → back), if a future edit reintroduces a swap.
The assertions only look at text between <SessionCollaboration.GuestTurnRequests and its close tag, plus a global count of <ChatComposerRegion. Anything above the slot that conditionally mounts or re-keys it is invisible to them. Ablation on this head (each applied to app-shell.tsx and the test rerun):
{sharedSessionActive ? <div /> : (<TaskEntry.TaskEntryWorkspacePickerConsumer …>…</…>)}→ passes. This is the pre-fix structure: a ternary at the parent level.{!sharedSessionActive && (…)}around the same element → passes.key={sharedSessionActive ? 'g' : 'o'}on<TaskEntry.TaskEntryWorkspacePickerConsumer>→ passes.- The same key on
<GuestTurnRequests>itself → fails. This is the only case pinned, and it is the exact mutation from my earlier review.
So the test closes the ablation I reported but not the failure class behind it. Extending the regex upward (asserting that no sharedSessionActive appears between the enclosing JSX expression container and the slot) would catch C and D, but it stays brittle against B and against the next refactor of the surrounding tree.
The robust fix is the first option from last round: move the TaskEntryWorkspacePickerConsumer → GuestTurnRequests → ChatComposerRegion composition into one exported component that AppShell renders unconditionally, and have mountShell import it instead of mirroring it. Then the harness is the production wiring, and the source assertion shrinks to "AppShell renders <SessionComposerSlot …/> exactly once, with no key".
This doesn't block merging. Behaviour at this head is correct.
jackwener
left a comment
There was a problem hiding this comment.
[kabi-sol]
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.
The Guest newer-draft P2 from my previous review is closed at 39e15303376b243e0468fcf6d808f3947fbcfd14. No new P0–P2 found in this follow-up scope.
The original mounted Guest/Composer reproduction now preserves the second unsent draft after switching away and completing the first request; staying in the original session also passes. Reinstating only the old unconditional inactive-draft clear makes the same assertion fail again.
The new queued-edit callbacks restore structured content at their controller boundary after successful retraction, and restore nothing on rejection. Main-session deferred and concurrent edits preserve session ownership. Retained attachment resend probes still pass for WorkHub and Side Chat steering. Source tracing confirms all three surfaces use the shared conditional Composer clearing path; the remaining retract IPC is wired through, with no Desktop callers of the removed update route.
Dependency/main builds and 223 focused UI/controller/bridge tests passed. Verification used mounted components/hooks and controlled services, not real Electron/Host, a full cross-panel lifecycle matrix, or the full suite. The live head was unchanged; no CI result was listed when checked.
…yx 0.6.2 Upstream slimmed ConversationServices.sessions to readSnapshot plus the queue operations, so stale list/subscribeChanges stubs no longer compile. The queue-shortcuts copy key went away with the help icon, and Astryx 0.6.2 renders disabled buttons that carry a tooltip as aria-disabled instead of the native attribute so the tooltip stays reachable — the assertion accepts either form. Ledger regenerated for the new base. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ts Session Editing a queued steering entry retracted the Host copy but handed only plain text back to the composer — attachments, directory references, and quotes were silently dropped. The restore was also gated on the owning Session still being active, so a retract that resolved after the user navigated away discarded the recovered draft entirely. The retract callback now carries a RestoredDraftContent (editable text plus the staged context that rode with the send) and writes through the keyed draft store, so the draft lands under the owning Session even when another is on screen; the visible composer only updates and focuses when that Session is still active. Each surface wires its own staged stores: the shell restores attachments, directory references, and quotes; Side Chat re-stages quotes through the workbar panel; WorkHub restores attachments under its scoped draft keys. WorkHub and Side Chat never stage directories or quotes respectively, so nothing there is lost. Pins the transcript/plate split on both seams: a queued follow-up never becomes a transcript bubble, and projectComposerMessageQueue drops current_turn entries. Removes a trailing blank line in composer.css. Generated-by: Devin
The Host-snapshot projection deletes a queued entry's transient copy on admission, leaving its messageId only in the queue snapshot. When the Host drained the queue while Desktop was disconnected, the reseed had no ids to resolve — admission and completion events carry no replay — so the completed Turn never entered ownTurnIds and its durable messages were filtered out of the Side Chat transcript. The CI side-chat-followups spec reproduces this deterministically. Track queue-admitted ids in a separate set: populated at queue_update, consumed when a durable merge proves the Turn binding itself or when execution resolutions retire/register the id, cleared on fork switch. This is a tombstone for ownership resolution, not a second queue authority — the plate still renders solely from the Host snapshot. Generated-by: Devin
Three gaps found in adversarial review, all sharing one assumption — that a renderer only needs positive queue evidence: - seedActive suppressed queue_update on an empty queue, so a session whose queue drained while unobserved (navigation away, reconnect) re-seeded nothing and the renderer kept phantom entries forever. The seed now always carries the authoritative queue snapshot; empty clears. - WorkHub's unconfirmed queued send resolved only on positive evidence; an externally retracted send left a permanent ghost row and blocked later sends. On observation ready the pending send now resolves through queryMessageExecutions: cancelled/not_admitted drop the placeholder, owned keeps it until the durable transcript merge. - The queue editor CAS-checked the revision captured when editing began, so any concurrent queue mutation dead-locked the editor until cancel. The commit now CASes the latest known revision; a conflict still surfaces once, and a re-save retries fresh state. Also gates the queued-steering Edit action on an available draft restorer so it cannot silently degrade to Delete, and pins the new contract in projection-, observer-, controller- and component-level tests. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ion audit - Drop projectMessageQueue's unused forkId parameter. - Move useSessionMessageQueue's test-only export from the feature index to testing.ts, the seam the architecture check reserves for test consumers. - Make ComposerHandle.appendDraft required: every production handle supplies it, so the two appendPromptContextDraft fallbacks were unreachable. - Update docs/desktop-message-queue.md, which still described retract-to-draft plate actions and listed in-place editing as unshipped. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
seedActive now always carries the authoritative queue snapshot, so the CLI turn streams see a queue_update ahead of live frames even for an empty queue. Rename the turn-event reader to nextTurnEvent and have it skip session-level queue_update frames; none of these tests exercise queue projection. Two tests needed real fixes beyond framing: the pre-resync backlog test now skips the seed frame before asserting the first surviving live event, and the "unconsumed terminal across replacement" pair was relying on microtask timing - the settlement transcript refresh opens its own subscription, so recovery actually opens a later subscription than the fixtures assumed. Give the fixtures enough fake subscriptions for the recovery to genuinely replace, and wait on the replacement being pumped instead of an open count. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…rawer The pending plate was a second card floating above the composer, duplicating the frame the drawer staging slab already owns. Queue rows now render as a section inside the one ChatComposerDrawer, split from staged context by a hairline, so there is a single collapse affordance and the queued section collapses into the same count badge. Row actions align to the drawer content edge instead of the old plate measure. Stories cover the reachable states: queue-only, queue with staged context, collapsed strip, overflowing list, inline edit, and the narrow viewport. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Three surfaces carried the same retract block — service call, site-specific error reporting, then rebuilding RestoredDraftContent field by field. The contract now owns that shape once; each caller keeps only its id guard and error channel. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ojection Steering bubbles now cover in-flight entries and use the rebased 'transcript' placement; test and story service fixtures pick up the new readExecutionBoundary/runtimeHosts ports. Generated-by: Devin
…Host snapshot Ablation pass after the rebase: - Steering bubbles carry no hostTurnId, so ChatView already tails them and dedups by message id once steering_message lands; pendingSteering, its merge preservation and the turnId copied into every queue state go away. - retractQueueEntryToDraft and queuedSteeringDeliveryActions fold into withQueuedSteeringTransients, which reuses each surface's existing report-and-rethrow retract. restoreDraft is required at every production caller, so the editable gate is dropped. - SessionLocalMessages loses the waitingForPrevious status (it did not change the user's next action) and the cancel/remove label split. - Dead newlyInFlight/rootQueueInFlight helpers, stray app-shell locals, and a test that only restated the queue concatenation are removed; the session-local-recovery E2E returns to main's copy-independent assertion. - The queue doc no longer claims plate Delete restores the draft. Generated-by: Devin
A steering send used to start as a local row in the staging drawer and jump into the transcript once the Host queued it. Each message now has one home for its whole life: steering in the transcript, follow-ups in the staging drawer. The 'steering' transient placement, the submit path's steering flag, and Side Chat's moved-to-successor rebinding exist only for that jump and are removed; the drawer's local rows are follow-ups only. Generated-by: Devin
Generated-by: Devin
Generated-by: Devin
The static QueuedSteeringInTranscript story showed one frozen frame, so nothing covered a message moving between surfaces. The flow story drives the real composer and ChatView against a stand-in Host queue read through deriveMessageQueueProjection: Enter queues a follow-up in the staging drawer, 直接发送 promotes it into a transcript bubble with edit/delete, and a steering_message at the step boundary lands it inside the Turn once. It replaces the static story, whose assertions it runs mid-play. Generated-by: Devin
…vers Always seeding queue_update reached ACP too. ACP ignores the event, but a restored Turn whose output delivery had already failed rethrew that failure on the next accepted event and stopped the Host Turn, and a session/load sibling lost its replayed chunk. main's rule is back: seed the queue when admissions are projected or it has entries. The Desktop observer, which renders queues, asks for the empty seed explicitly so a queue drained while unobserved still clears. The CLI driver tests return to main's version; the projector test pins which consumer gets the seed. Generated-by: Devin
…t sends Editing queued steering stages its attachments back as retained Host references. Turning staged attachments into a send command was split across two helpers, and only main chat called both: WorkHub uploaded each item through the ingest helper, got nothing for a retained one and refused the send; Side Chat's contract had no retained field, so the attachment silently fell off the message. toSubmittedAttachments is now the one conversion, returning the same attachmentItems/retainedAttachments pair the Desktop bridge accepts. Main chat uses it, WorkHub reuses a retained reference instead of uploading, and Side Chat hands its staged attachments to the hook, whose send and submitFollowUp contracts carry retained references. The legacy composer-attachments shim and its feature import are gone. Generated-by: Devin
Opening a shared Session swapped the chat Composer for a separate Guest composer. The swap unmounted the Composer, so every owned Session's unsent text, and any text an edit restored mid-retraction, was lost while its staged context survived in AppShell. The Guest composer also kept a second draft authority in module-level maps and a draft-token scheme that only existed because its input cleared itself on submit. GuestTurnRequests now wraps the one ChatComposerRegion. For an owned Session it projects nothing; for a shared one sends become Turn requests, the Guest's requests are rows in the composer staging drawer (withdraw or dismiss), and a Guest without request access sees the notice in place of the composer. Only the Session's model reaches a Guest's Composer, read-only. Retrying the same text reuses its Turn id, and a lost response the Host already accepted counts as sent. Removed: SessionTurnRequestComposer, its module-level draft and attempt maps, the draft-token restore scheme, AppShell's owner/Guest branch, its CSS, and three copy keys nothing reads. Generated-by: Devin
…composer move The inventory still listed the deleted SessionTurnRequestComposer and missed GuestTurnRequests. The Side Chat tests no longer import WorkbarIngestInput, so its re-export from the Workbar testing entry is dead. Generated-by: Devin
…methods main's session-query-gate test (#4431) builds ConversationServices by hand; this branch made the Host queue mutations required on that port. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Generated-by: Devin
…he draft Queued follow-up rows edited in place through queue.entry.update, while steering bubbles and unsent local messages retracted and restored the draft. In-place editing left the entry live in the Host queue, so a Turn boundary could consume the old text mid-edit and unmount the row with the typed edit, and it could only change text. All three now share retractQueuedEntryToDraft: the entry leaves the queue first, then its full content (text, attachments, directory references, quotes) returns to the owning Session's draft. Resending queues it at the tail; the grip can move it back. Removed with it: the plate's inline editor and its save/cancel copy and CSS, the Composer onUpdateQueuedEntry/queuedMessageRevision props, the updateQueuedEntry wiring in the main chat, WorkHub and Side Chat, the renderer queueRevision fields that only fed the CAS, and Desktop's sessions:updateQueueEntry IPC/preload/client path, which had no other caller. The Host's queue.entry.update operation is unchanged. Also collapses the ConversationServices stubs this branch copied into tests into one conversation/testing helper, and drops the QueueOnly, NarrowPendingPlate and EditingQueuedEntry stories, which carried no assertion beyond what PendingPlate already renders. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Generated-by: Devin
…that send A send that completed after the user moved to another Session cleared that Session's stored draft unconditionally, so text typed after the send was lost; the still-active branch compared first. The Composer's draft is two representations — the live input for the active key, the store for the rest — and getDraft read through both while writes did not, so every caller branched on "is this key active" and re-derived the clear rule, and one branch missed it. Routing Guest Turn requests through the shared Composer made the gap easy to hit, because the old Guest composer locked its input while a request was pending. The draft hook's writes now cover the live input like its read does, and the Composer keeps one rule for every completion: clear a draft only while it still reads as the submitted message. sendCurrent, the handle's clearDraft(key, submitted) and the Guest controller's lost-response path share it; the per-caller active-key branches go away. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Generated-by: Devin
app-shell.tsx is outside the node test build, so the Guest tests mirror its wiring. A source assertion now keeps AppShell to one ChatComposerRegion inside GuestTurnRequests, with no key and no shared-Session branch beyond the projection's sessionId; adding a remounting key fails it. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Generated-by: Devin
c81f76a to
b8d638e
Compare
jackwener
left a comment
There was a problem hiding this comment.
Approving at b8d638ee4514bbace869dc4619d923fe8038053e. No open P0–P2.
Reviewed across three lineages over several rounds (GPT probes on real components, Kimi on Windows with ablation, Claude on deletion completeness and the architecture gate); findings were verified at 39e15303 and carried to this head after checking the rebase.
- Earlier P2s closed: restored attachments now travel through WorkHub and Side Chat as retained Host references; shared Sessions write in the one chat Composer, so switching to a shared Session no longer drops the owned draft; a send completing after navigation clears the draft only while it still reads as that send. Each fix is pinned: removing it reddens exactly its test.
b4dc3d441(every queued-message edit retracts into the draft) removes the in-place update path cleanly — no remaining client, IPC, preload, bridge, port, CSS or copy references; typecheck and Knip clean.--strict-basearchitecture gate passes; the ledger regenerates byte-identical.- Rebase
39e15303→b8d638ee: range-diff shows only ledger token counts, regenerated surface inventory rows, and changed context lines around an unchanged hunk; no new code. CItestgreen.
Open, non-blocking:
- P3: the new AppShell source assertion checks only the
GuestTurnRequestselement's own tag, so a ternary or&&wrapper around it (the shape of the original bug) would not be caught. Extracting that composition into a component AppShell renders unconditionally and the test imports would pin it. - Follow-up: Host
queue.entry.updatenow has no client caller; removing it needs an epoch bump, so a separate issue fits better.
Not run: packaged Electron, E2E, full suite.
Summary
The composer's staging area used to be a second card floating above the input that mixed three kinds of rows — Host-queued steering, Host-queued follow-ups, and local sends still in flight — under per-placement group headers, with a floating
?icon advertising the send chords. This rework makes each kind land where it belongs, with the Host queue snapshot as the only rendered queue state:ChatComposerDrawer), split from staged attachments/quotes by a hairline — one collapse affordance, and everything folds into the same count badge.Refs #2262.
What changed
withQueuedSteeringTransientsderives transcript bubbles from the queue snapshot in all three surfaces (main chat, WorkHub, Side Chat), reusing each surface's existing retract.retractQueuedEntryToDraftis the one edit path, used by the bubbles and by the drawer rows through the Composer'sonEditQueuedEntry.toSubmittedAttachmentsis the one conversion from staged attachments to a send command (attachmentItems+retainedAttachments, the pair the Desktop bridge accepts). Before, two helpers each covered half and only main chat called both: WorkHub refused a resend with a restored attachment, and Side Chat silently dropped it. Side Chat's send/submitFollowUp contracts now carry retained references.GuestTurnRequests(session-collaboration) wraps the oneChatComposerRegionin AppShell. It projects nothing for an owned Session; for a shared one it supplies the Turn-request send, the request rows (withdraw / dismiss) and, without request access, the notice that stands in for the composer. Only the Session's model reaches a Guest's Composer, read-only. Retrying the same text reuses its Turn id, and a lost response the Host already accepted counts as sent.projectQueuedTransientMessages, theisQueuedSteering/getMessageQueueseam, three copies of departed-entry bookkeeping,queuedSteeringRef, the help Tooltip and its copy keys;SessionTurnRequestComposerwith its module-level draft/attempt maps and draft-token restore scheme, AppShell's owner/Guest branch and its CSS; the legacycomposer-attachmentsshim and its feature import; the drawer's in-place editor with its save/cancel copy and CSS, the ComposeronUpdateQueuedEntry/queuedMessageRevisionprops, theupdateQueuedEntrywiring in all three surfaces, the rendererqueueRevisionfields that only fed its CAS, and Desktop'ssessions:updateQueueEntryIPC/preload/client path. The Host'squeue.entry.updateoperation is unchanged and now has no Desktop caller.Correctness under disconnect/reseed
queryMessageExecutionson reseed; a cancelled or never-admitted send clears its placeholder and unblocks the send gate.seedActivefor the queue snapshot even when empty, so a re-observing or reconnecting surface drops stale queue rows instead of keeping ghosts. Other consumers keep main's rule (seed only with admissions or entries): ACP ignoresqueue_update, and an extra event after a failed output delivery would stop the restored Host Turn.Verification
typecheck(desktop preload/main/renderer/storybook tsconfigs included) after rebuilding the workspaces in dependency order; desktop build and renderer architecture check pass.main.playpasses forQueuedMessageLifecycleFlow, the threeComposer Message Queuestories and bothShared Session Gueststories;build-storybookand the render smoke (438 stories) succeed.Screenshots
Same
PendingPlatestory, same viewport — the grouped plate above the composer becomes a section of the one staging drawer:Same
Shared Session Guest / RequestQueuestory, same viewport — the separate Guest composer becomes the chat Composer, with the Guest's requests as drawer rows:Recording
QueuedMessageLifecycleFlow(Product/Shell Official AppShell) drives the real composer and ChatView against a stand-in Host queue read through the productionqueue_updateprojection: Enter queues a follow-up in the staging drawer → Send now promotes it into the transcript as a steering bubble with edit/delete → the Host consumes it at the next step boundary and it lands inside the Turn, shown once. The top bar stands in for the step boundary. The same steps are the story'splay, so CI asserts every stage.Light:
queued-message-lifecycle-light.mp4
Dark:
queued-message-lifecycle-dark.mp4
AI use
Tool(s) and scope: Devin — implementation, tests, stories, E2E updates, screenshot and recording evidence.
Checklist
Does this PR entail a change in behavior?