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.
Explain unconfirmed delivery and the messages waiting behind it without exposing Host terminology. Use labelled icon actions with tooltips and one body text size across queue content and status. Keep the existing Host admission, reconciliation identity and cancellation authority. Extend the existing controller, composer and recovery checks and reuse the queue story for unknown and failed delivery states. No new delivery state or scheduler is introduced. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Generated-by: Devin
Delivery explanations should not compete with the queued message body. Move them into the existing action tooltips and remove the redundant predecessor-status projection and second-line styling. Keep accessible recovery actions and the original reconciliation/cancellation boundary. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Generated-by: Devin
A message still waiting for its receipt is not admitted to the Host queue, so placing it under the "next turn" group told users it would send when nothing guaranteed that. Split local deliveries into their own section below the queue: a status icon carries the tone (pending, unconfirmed, failed) and keeps the explanation in a tooltip, while the recovery action stays an icon button. Host-admitted rows keep their group headers, grips and edit/promote/delete actions unchanged. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Generated-by: Devin
Queued steering, follow-ups and local in-flight sends are the same thing to the user — a message that has not landed yet — so the plate now renders one flat list instead of grouped sections and an outbox strip. Every row is a single line plus icon buttons: send-now, edit and delete for admitted entries; edit (cancel and restore the draft) and delete for unsent ones; a warning-tinted check-delivery for the unconfirmed state, which is the one case where "send now" would risk double execution. Group headers and persistent status text are gone; the send-shortcuts hint lives on a hover-revealed help icon. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Generated-by: Devin
The floating shortcut hint sat over the first row's actions, so the row intercepted hover and the tooltip could never open. Reserve the corner with a uniform right gutter on row actions and raise the icon above the row background. Also refresh the stale tooltip copy assertion in the WorkHub e2e. Generated-by: Devin
The composer staging area is for follow-ups only. Steering the Host has queued but not yet consumed now appears as a transcript bubble with edit and delete actions — edit retracts the entry and restores its text to the draft, delete retracts it outright. Entries that leave the Host queue without an admission event retire their bubble, and the local outbox accepting a send no longer removes the queue-owned copy. Generated-by: Devin
The hidden affordance was the only place teaching Shift+Enter, and discoverability there is not worth a permanent per-row gutter plus a floating button that had to win pointer hit-testing against row actions. Enter-follow-up then steer-if-needed stays the safe default path, and the action is reversible, so the surface no longer advertises it. Generated-by: Devin
A queue-owned steering bubble used to be stored into the transient map on queue_update and reconciled back out by three per-surface copies of departure bookkeeping, plus an isQueuedSteering gate so the local outbox's accept race could not delete it. The stored copy was only ever a cache of what the Host snapshot already says. Now the snapshot is the single source: queue_update stores entries and retires the listed local intents, and each surface derives steering bubbles at the render boundary through withQueuedSteeringTransients. A silent withdrawal, a consume, or a reseed-proven cancellation all retire the bubble for free; a late local copy with the same id is filtered by the same derivation instead of needing a guard. Side Chat's reseed reconciliation reads queue state between React flushes, so its messageQueue writes go through a ref and the state copy only feeds rendering. AppShell/WorkHub read the snapshot directly at render. Generated-by: Devin
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 是普通操作下用户可见的内容丢失,且与门禁无关 —— 门禁那条挡住合并,但即使门禁不存在,那两条仍然值得先修。
|
The queued-steering projection, its retract/draft-restore path, and the plate's four entry actions all lived in app-shell.tsx, which the renderer architecture ratchet measures: the change added a hook call, four window.maka.sessions bridge paths, and ~300 tokens of new root debt. useAppShellSessionUiState now owns the whole surface via useSessionMessageQueue: it selects the active Session's Host queue snapshot, derives the steering transcript bubbles, and runs the entry actions through the extended ConversationServices.sessions port. Locale, toast, and the composer handle come from the feature's own contexts, so the shell just destructures display.queueSurface — its bridge count, hook count, and token debt all drop below the base ledger. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1b152b5 to
d395f59
Compare
Summary
The composer plate used to mix 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.Now the plate is one flat list of follow-ups only. Queued steering renders in the transcript as a pending bubble derived straight from the Host queue snapshot at the render boundary, with edit/delete actions on the bubble footer. Local sends keep plain rows; the only special case is an unconfirmed delivery, whose single action checks whether it arrived instead of offering a resend that could duplicate execution.
Refs #2262.
What changed
withQueuedSteeringTransientsderives transcript bubbles from the queue snapshot in all three surfaces (main chat, WorkHub, Side Chat); the bubble appears, updates and disappears with the snapshot alone.projectQueuedTransientMessages, theisQueuedSteering/getMessageQueueseam, three copies of departed-entry bookkeeping,queuedSteeringRef, the shortcut-help Tooltip and its copy keys.Verification
tsc --noEmiton all four desktop tsconfigs;@maka/ui, desktop main and renderer builds pass.e2e/workhub-layout.spec.ts3/3 ande2e/session-local-recovery.spec.tsupdated for the cancel affordance and passing.QueuedSteeringInTranscriptrenders the bubble through the real derivation; the plate story keeps a steering entry in its fixture to prove it is filtered out.Screenshots
Same
PendingPlatestory, same viewport — grouped steering + follow-up rows become a flat follow-up list:Queued steering is a new transcript surface, so there is no same-story before:
AI use
Tool(s) and scope: Devin — implementation, tests, stories, E2E updates and screenshot evidence.
Checklist
Does this PR entail a change in behavior?