perf(desktop): stop republishing catalog bookkeeping to the shell - #5550
Conversation
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-grok-reviewer. Occam and gate lane at exact head 4d4bd2b2b08ddea1b9f507d8c47fa34ca05c50e9. I am not NO-GO. COMMENT only, no approve. I did not read existing pull-request comments before sealing.
Draft, MERGEABLE. test FAILURE is a fact, not green. GitHub base feb9cf22f; merge-base with live main 1c217a5c5 is that same SHA — 0 behind.
The feature-import ban is blocking a missing public export, not a lazy test
useAppShellSessionUiState is already on features/conversation/index.ts. The test does not import the hook. It imports shellSessionRowEqual (app-shell-session-ui-state.test.ts:41) — which is the named invariant of this pull request (rail-only keys versus value-compared keys) — and that symbol is on neither index.ts nor testing.ts.
Same-feature production code such as composer-mentions-provider.tsx may deep-import. A file under src/main/__tests__ may not. The gate text is exact: tests must use index, testing, or stories.
So the gate is turning a should-do into a must: export shellSessionRowEqual — from index.ts if it is a public contract (fail-closed extra render is a shell policy), or from testing.ts if it is test-only. Regenerating the ledger cannot invent that export.
Same class as #5546 and #5494: the ratchet is pointing at a real missing seam, not at a prettier count. I would still want that export if the gate vanished, and I would not NO-GO the product — shellSessionRowEqual being fail-closed (unknown field ⇒ republish) is the right direction.
app-shell.tsx nonTriviaTokens 12972 → 12974
The only AppShell diff is passing sessionCatalogController into renderComposerMentionsProvider (app-shell.tsx:2186). That is +2 tokens of whole-tree debt bought for "mentions read the catalog rather than sessions.list()."
The ratchet is doing its job here: AppShell must not grow. The catalog already lives in the workspace/conversation boundary, and mentions can take it there instead. Smallest wiring is not the same as the right room.
Does "no basis, therefore NO-GO" fire?
No. This is a 13-file follow-on to #5532's publish-iff-changed work, scoped to rail bookkeeping, with the out-of-scope streaming and idle floors declared. The two uncleared gate lines are placement problems — a missing export and an AppShell argument — not missing product basis.
Performance
The claimed A/B (−98% fibers on flag toggle) is unverified. I did not measure it. Author-provided numbers are not evidence.
未验证
I did not run --write followed by --strict-base locally; I am taking the two remaining gate lines as reported and independently confirming they match the source — the deep import of shellSessionRowEqual, and the extra AppShell argument. I did not run Knip, Storybook smoke or e2e.
简体中文
不 NO-GO。 绑 4d4bd2b2b;draft、MERGEABLE;test FAILURE 是事实不是绿;与 live main 的 merge-base 即 PR base,落后 0。
那条 feature 导入禁令挡的是「缺一个公开导出」,不是测试偷懒。 useAppShellSessionUiState 本来就在 features/conversation/index.ts 上;测试导入的不是这个 hook,而是 shellSessionRowEqual(app-shell-session-ui-state.test.ts:41)—— 那正是本单点名的不变量(仅 rail 用的键 vs 需要按值比较的键),而它既不在 index.ts 也不在 testing.ts。同 feature 的生产代码(如 composer-mentions-provider.tsx)可以深度导入,src/main/__tests__ 下的文件不行 —— 门禁原文就是「测试必须走 index / testing / stories」。
所以门禁是把一件本该做的事变成硬要求:把 shellSessionRowEqual 导出 —— 属于公开契约就放 index.ts(fail-closed 多渲染一次是 shell 策略),只给测试用就放 testing.ts。重新生成台账变不出这个导出。 与 #5546、#5494 同类:棘轮指向的是一条真实缺失的接缝,不是把计数弄好看。门禁不存在我也想要这个导出;但我不对产品 NO-GO —— shellSessionRowEqual 采用 fail-closed(未知字段 ⇒ 重新发布)方向是对的。
app-shell.tsx 的 12972→12974:AppShell 唯一的改动是把 sessionCatalogController 传进 renderComposerMentionsProvider(:2186)—— 为「mentions 读 catalog 而不是 sessions.list()」付出 +2 token 的全树债务。棘轮在这里是对的:AppShell 不许长大;catalog 本来就在 workspace/conversation 边界里,mentions 可以在那儿拿。最小接线 ≠ 正确的房间。
「没依据 ⇒ NO-GO」不触发:这是 #5532「变了才发布」的 13 文件后续,范围限定在 rail 记账,越界的流式/空闲下限都已声明;两条未清的门禁行是位置问题(缺导出、AppShell 多一个参数),不是缺产品依据。
性能:作者声称的 −98% fibers 未验证,我没测。作者给的数字不是证据。
未验证:我没有在本地跑 --write 再 --strict-base,而是采信已报告的两条门禁行,并独立确认它们与源码相符(shellSessionRowEqual 的深度导入、AppShell 多出的参数);未跑 Knip / Storybook smoke / 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-grok (same GitHub owner as the coordinating review agent, different model). Review of exact head 4d4bd2b2b08ddea1b9f507d8c47fa34ca05c50e9.
I did not read existing pull-request comments. I am not approving. Draft. Required test is completed/failure. I am not calling CI green. The author's fiber counts (73→20 commits, 22k→372 fibers) are unverified — I did not measure.
Frame
Merge-base feb9cf22f (same as the PR base). Behind 1 (#5531). MERGEABLE.
[P3] the tests pin the equality helper, not "the shell stops republishing bookkeeping"
shellSessionRowEqual skips activityAt / hasUnread / isFlagged / lastMessagePreview / localCreatedAt / revision / statusUpdatedAt / subagentRuntime, and valuesEquals everything else. The two new tests are good as far as they go:
holds identity across rail-only bookkeeping— changing only those keys returnstruerepublishes when a rendered field moves—status/name/permissionMode/lastMessageAtreturnfalse
But they call that function on two literals. They do not mount useExternalStoreSelector with it, so they would not fail if the hook ignored the equality argument altogether. The predicate is pinned; the wiring that makes the predicate matter is not.
Also unpinned:
setExecutionskipping publication when the projected value is equal but the object identity is new (production usesvaluesEqual; the tests only callsetExecutioninside live-turn cases and never assert a skip)- mentions no longer issuing a
sessions.list()IPC persessions:changed—composer-mentions.test.tsonly rewires the catalog and deletes the mockedlist/subscribeChanges; it asserts nothing about IPC counts - the browser panel subscribing only while visible
Fail-open is untested too: a row field added later and not listed in NON_RENDERED_ROW_KEYS will cause a republish, and nothing catches that direction.
So what is pinned is "these particular keys are currently treated as rail-only" — not "the shell no longer re-renders on bookkeeping."
[P3] app-shell.tsx nonTriviaTokens 12972 → 12974
The shell now passes sessionCatalogController into renderComposerMentionsProvider. That is the mentions IPC removal, paid for with two extra tokens in the very file the ratchet watches. Fewer publishes, larger shell inventory — coherent, but it is gate debt.
The new test imports features/conversation/controller/use-app-shell-session-ui-state.js directly, which matches the architecture error that survives regenerating the ledger (feature imports must use index). I did not re-run the checker; this matches from the diff.
Verification bounds
Walked: shellSessionRowEqual and its tests, setExecution's valuesEqual, the mentions provider, the one-line app-shell.tsx change, and the composer-mentions.test.ts rewiring. Did not run desktop tests, --write, Knip, Storybook or e2e. Did not read other reviews.
简体中文
无 P0–P2,2×P3。 绑 4d4bd2b2b;merge-base = PR base feb9cf22f,落后 1(#5531);draft、MERGEABLE;test failure,不写成绿;作者的 fiber 数字(73→20 commits、22k→372 fibers)未验证。
[P3] 测试钉的是那个相等谓词,不是「shell 不再因记账而重新发布」。 shellSessionRowEqual 跳过 activityAt/hasUnread/isFlagged/lastMessagePreview/localCreatedAt/revision/statusUpdatedAt/subagentRuntime,其余走 valuesEqual。两条新测试本身没问题(只改记账键 → true;status/name/permissionMode/lastMessageAt 变 → false)。但它们是拿两个字面量去调这个函数 —— 没有把它挂到 useExternalStoreSelector 上,所以即使那个 hook 完全忽略这个相等参数,测试也不会失败。谓词被钉住了,让谓词起作用的接线没有。
同样没钉的:setExecution 在投影值相等、对象是新身份时跳过发布(生产用 valuesEqual,测试只在 live-turn 里调过 setExecution,从未断言跳过);mentions 不再对每次 sessions:changed 发 sessions.list() IPC(composer-mentions.test.ts 只是改了接线、删掉 mock 的 list/subscribeChanges,没有断言 IPC 次数);browser panel 仅在可见时订阅。fail-open 方向也没测:将来新增的行字段若未列入 NON_RENDERED_ROW_KEYS,会导致重新发布,没有任何用例能抓到。
⇒ 被钉住的是「目前这些键被当作 rail-only」,不是「shell 不再因记账重渲」。
[P3] app-shell.tsx 12972→12974:shell 现在把 sessionCatalogController 传进 renderComposerMentionsProvider —— 这正是移除 mentions IPC 的代价,而它恰好计入棘轮盯着的那个文件。发布变少、shell 清单变大:说得通,但那是门禁债。
新测试直接 import features/conversation/controller/use-app-shell-session-ui-state.js,与重新生成台账后仍然存在的那条架构错误(feature imports must use index)吻合。我没有重跑 checker,这是从 diff 对上的。
验证边界:走过 shellSessionRowEqual 及其测试、setExecution 的 valuesEqual、mentions provider、app-shell.tsx 的一行改动、composer-mentions.test.ts 的接线改动。未跑 desktop 测试、未跑 --write、未跑 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. Seat: kabi-grok.
zhiiw
left a comment
There was a problem hiding this comment.
Independent review (blind — no existing comments read). Conclusions bind to 4d4bd2b2b08ddea1b9f507d8c47fa34ca05c50e9 (Draft; test is red — see below).
The headline claim is falsifiable, and I falsified-tested it independently: it holds. The claim "stop republishing catalog bookkeeping to the shell" is measurable at the selector layer, so I measured it (linkedom + react-dom/client, the repo's own test stack, no Electron window):
- This head: 1,000 commits moving only rail-only bookkeeping fields (
isFlagged/activityAt/revision, fresh objects each time) → 0 re-renders of ashellSessionRowEqual-gatedselectSessionByIdsubscriber. A real change (name) at a fresh revision re-renders. (The probe's first draft fed the rename at a stale revision and was correctly rejected by the anti-regression guard — that guard works too.) - The old shape (same selector, no equality gate): every commit re-renders — 1,001 renders for the same stream.
That is the mechanism-level evidence for the claim. The author's Electron A/B numbers (16,801 → 372 rendered fibers per 8 toggles, etc.) remain the author's; I did not run a visible-window harness (this machine has an active user), so those are marked unverified here.
The code reads clean. shellSessionRowEqual fails in the safe direction (an unlisted field republishes); the rail-only exclusion list is the right shape with the right comment. Mentions reading the catalog instead of sessions.list()-per-event is a straight win with the surface shrink (list/subscribeChanges out of ConversationServices). The browser panel's subscribe-while-visible + reseed-on-return keeps session-switch reset correct (stateSessionRef). The artifact pane's identity-stable poll is the same pattern.
The red test lane is real and not one command away — I reproduced it locally: check-renderer-architecture.mjs fails with two entries, and only one is staleness-shaped:
- (inline) the new test imports
shellSessionRowEqualfrom the feature's controller file directly, which the import-surface rule forbids — regenerate cannot fix that one; app-shell.tsxnonTriviaTokens 12972 → 12974 — the ratchet moved +2; either shave the two tokens or justify the bump in the ledger.
Verified locally (Windows, Node 24.18.1): the five touched suites — 26/26; the architecture-gate failure reproduces as above.
Not verified: the author's Electron A/B figures; e2e; the Knip/smoke lanes that sit behind the red gate.
COMMENT per convention; the gate failure is the blocker.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
Row-level catalog patches still republished the whole active row to the shell on every event: flag/unflag, unread and preview bookkeeping all committed a fresh row object, and the observation channel re-sent a fresh execution projection per frame. Each republish re-rendered the chat surface — measured at ~2.7k rendered fibers per flag toggle, mostly astryx controls rewriting DOM attributes. - Gate the shell's whole-row reads with a field-aware equality that skips rail-only bookkeeping (activity, unread, flag, preview, revision, timestamps, subagent runtime) and compares every other field by value; fields added later republish until proven rail-only, so the failure direction is an extra render rather than a stale value. - Compare execution projections by value before publishing: the channel resends an equivalent object on unrelated metadata events. - Feed composer mentions from the renderer catalog instead of a full sessions.list() IPC per sessions:changed; drop the now-unused list and subscribeChanges ports from the conversation service surface. - Subscribe the browser panel only while visible and reseed via getState on the way back; keep artifact poll results identity-stable when the list is unchanged. Measured (Electron + react commit probe, 8 flag toggles on the active row): 73 commits / ~22k rendered fibers -> 20 commits / 372 fibers; the row diff stays isFlagged+revision and remaining renders are the rail's own flag/selection update. Generated-by: Devin
…nt the catalog above services - Export shellSessionRowEqual through the conversation feature's testing surface; the test no longer deep-imports the controller file. - Give the session catalog a React context mounted once in composition (createDesktopFeatureServices owns the instance): providers read it via useSessionCatalogController instead of a prop drilled through app-shell, keeping app-shell.tsx byte-identical to the ratchet baseline. - Pin the wiring, not just the predicate: a mounted selectSessionById subscriber survives a rail-only patch and re-renders on a name change; setExecution holds publication for a fresh-but-equal projection; a field outside the rail-only list fails closed. - The slash-menu story drives its session-update refresh through a catalog commitPatch (thinkingLevel bump) instead of a subscribed 'updated' event. Generated-by: Devin
4d4bd2b to
ac760b3
Compare
|
Review findings resolved at ac760b3 (branch rebased onto 173aed9). Thanks @zhiiw for the exact repro and the React 18 probe — that mechanism-level evidence directly informed the fix. Architecture gate (both lines)
Test pinning (the "predicate, not wiring" gap) Added, all in
Deferred with reason: a mounted BrowserPanel hidden-gating test needs a DOM/React harness beyond what this suite provides; behavior is covered by the Electron/CDP measurement harness ( Perf figures: figures are author-run on a real Electron build; the instrumentation scripts are not in this diff. The strict A/B (clean base Verification: |
…entity test useAppShellSessionWorkspace reads the catalog from SessionCatalogContext now, so the probe needs the provider the composition mounts in production. Generated-by: Devin
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-grok (same GitHub owner as the coordinating review agent, different model). Recheck of exact head edf12ca5318371fc44292d41af831d12313867b4. Comments on 4d4bd2b2b do not apply.
I did not read existing pull-request comments. I am not approving. Draft. Required test is completed/success. I did not rebuild dist locally.
git merge-tree --write-tree against current origin/main succeeded. Behind 1 (1c18700a7). Merge-base 173aed934.
The hook-ignore P3 is closed — not extra literals
keeps a catalog row subscriber mounted through rail-only patches mounts useExternalStoreSelector(..., shellSessionRowEqual), patches flag/unread/preview/revision, and requires renders === 1. A name patch requires renders === 2. If the hook dropped the equality argument, the first patch would fail the test. Import is features/conversation/testing.js, not the controller file.
Token P3 is closed by moving wiring, not raising the ledger
app-shell.tsx is again renderComposerMentionsProvider(composerMentionsSurface) — the extra catalog argument is gone from that file.
Also now pinned: setExecution skips a fresh-but-equal projection; an unknown row field fail-opens. Still unpinned: mentions sessions.list() call count; browser panel subscribe-while-visible.
简体中文
席位:kabi-grok(与协调评审 agent 同 GitHub owner,不同模型)。复核绑定 head edf12ca5318371fc44292d41af831d12313867b4;针对 4d4bd2b2b 的评论不再适用。
我没有读本 PR 已有评论。不批准。 仍是 draft。必需的 test 为 completed/success。我没有在本地 rm -rf dist 重建后重跑。
git merge-tree --write-tree 对当前 origin/main 成功。落后 1(1c18700a7)。merge-base 173aed934。
上轮「谓词没挂上 hook」这条 P3 —— 真关掉了,不是再塞几个字面量。 keeps a catalog row subscriber mounted through rail-only patches 真的挂了 useExternalStoreSelector(..., shellSessionRowEqual),然后 patch flag/unread/preview/revision 并要求 renders === 1,再 patch name 要求 renders === 2。如果 hook 忽略第四个相等性参数,第一次 patch 就会红。 这正是上轮缺的那一环。shellSessionRowEqual 是从 features/conversation/testing.js 进测试的,不再直接 import controller 文件。
token 那条 P3 —— 是靠挪接线关掉的,不是把账本调大。 app-shell.tsx 回到了 renderComposerMentionsProvider(composerMentionsSurface),多出来的 catalog 参数从这一行消失了。门禁转绿的原因是接线挪走,不是把债务上限抬高。
这轮顺手补钉的:setExecution 对「新对象但同值」的 projection 不再重复发布(0 次 notify);rail-only 列表不认识的字段走 fail-open。
仍未钉住(不阻塞上面两条):mentions 的 sessions.list() 调用次数;browser panel 仅在可见时订阅。
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.
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-opus-dev (same GitHub owner and same model lineage as the coordinating review agent — see the independence note at the end). Behaviour-regression lane, exact head edf12ca5318371fc44292d41af831d12313867b4.
No P0–P2 in this lane. 2×P3. I am not approving.
Gate facts
git merge-tree --write-tree against current origin/main exits 0 and writes tree 4675335c6201424bb60468f2022ffd75f13701f8 — no conflict, established locally rather than from GitHub's mergeable field. Behind main by 1, ahead by 3. I rebuilt after rm -rf dist and confirmed the artifacts come from this head: shellSessionRowEqual and SessionCatalogContext both appear in apps/desktop/dist/..., and neither symbol exists on any other head.
The mount-order change is safe on the production path
useSessionCatalogController moves from building its own useRef to reading useContext, and throws when the Provider is missing. The failure I went looking for was a mount path with no Provider turning a silent self-construction into a hard crash. It isn't reachable:
createDesktopFeatureServices()is called at module scope (main.tsx:36), not per render, so the controller cannot churn.SessionCatalogContext.Providerwraps<App>atmain.tsx:71, above every consumer.- There is exactly one
createRootin the repository (main.tsx:70) — no second render entry can miss the Provider. - Only two consumers exist:
composer-mentions-provider.tsx:129anduse-app-shell-session-workspace.ts:44. Both sit under the Provider.
Correcting something that reads like a risk but isn't: the lifetime did not change. The old useRef version was also one-per-renderer, because the root mounts once and AppShellContent does not remount across workspace switches. This is prop-drilling becoming context — wiring, not lifetime — so the stale-catalog-across-workspaces problem I expected to find does not exist.
The real risk surface: what shellSessionRowEqual skips
It declines to compare eight fields — activityAt, hasUnread, isFlagged, lastMessagePreview, localCreatedAt, revision, statusUpdatedAt, subagentRuntime — and its comment asserts that nothing renders them from beneath a whole-row read. I checked that assertion field by field, and today it holds:
- Scanning the renderer for reads of those eight fields on
activeCatalogSession/activeHostSession/requestedCatalogSession/requestedHostSessionand the aliasesactiveSession/activeDesktopSession: zero. - That includes spreads —
app-shell.tsx:674does...activeSessionintoactiveSessionForModelControls, whose consumers read onlyllmConnectionId/llmConnectionSlug— and the whole-row hand-offs intousePlanModeState,useWorkbarController,useSessionEventHealthPollingandsharedSessionDialog.openSession. None read the eight. Thesession-collaborationgrep exited 1 with no matches, so I ran a same-shape positive control that returned 132 hits to confirm the command was not simply malformed. - Dependency arrays hang off scalars (
activeSessionId,.status,.llmConnectionId), never a whole-row object, so suppressing a re-render cannot shift effect timing. - The rail, palette and mentions do render several of these fields, but through their own subscriptions and comparators — mentions'
conversationSessionListsEqualcompareslastMessagePreviewexplicitly — so they never pass through this gate. The design is self-consistent.
[P3] The comment at composer-mentions-provider.tsx:136-138 contradicts the code
It reads "a flag/rename/activity bump republishes nothing here." Flag and activity are true: isFlagged and activityAt are both on the skip list. Rename is not. name is absent from NON_RENDERED_ROW_KEYS, so a rename changes row identity, skillRelevantRow changes, the effect that lists it as a dependency re-runs, and the skill list is refetched.
The direction is harmless — an extra refresh, never a stale one — but this comment is the specification the next person will read before editing that list, and one of its three claims is false.
[P3] The skip-list invariant is protected in only one direction
Adding a field defaults to compare-and-republish, which is the safe default, and republishes for a field the rail-only list does not know about pins it.
Nothing guards the other direction. If someone later reads lastMessagePreview, isFlagged or subagentRuntime from a gated row, they get a silently stale value and no test fails. The promise is pinned; the path that would violate it is not. These field names also look entirely renderable — the rail and palette already render them — so this is an easy thing to walk into. Worth a guard: either narrow the gated row at the type level to a view without those eight keys, or add a test asserting that a gated row is never read for them.
Tests run (all from this head's artifacts)
app-shell-session-ui-state 17/17 (including the four new shellSessionRowEqual cases), session-workspace-action-identity 2, message-queue-ui-state 4, chat-composer-region-draft-handoff 7, app-shell-pending-attachments 5, app-shell-attachment-preflight-send 2, composer-mentions 4, session-reference-composer 3, conversation-services-adapter 1. fail = 0.
What I did not verify — please do not read this lane as covered
I did not run real Electron interaction. The P2s this lane has historically produced came from controlled IPC timing in a real window. What I did is static tracing plus unit tests, which cannot reach that class of finding. The accurate statement is that this regression lane is still absent this round; I covered part of the gap, and "I found nothing" is not "the lane passed."
I also did not verify the author's whole-window −86% DOM-mutation claim, did not re-check the other seats' prior findings, and my eight-field scan is static: it covers property access, spreads, Object.keys/values/entries and computed access, but not a path that exposes a whole row indirectly through serialization or deep copy.
Independence note: I share both an owner and a model lineage with the coordinating review agent, so this review is not independent corroboration of anything it concluded. The only different-owner line this round is a separate seat.
简体中文
席位:kabi-opus-dev(与协调评审 agent 同 owner 且同模型谱系 —— 见末尾独立性声明)。行为回归格,绑定 head edf12ca5318371fc44292d41af831d12313867b4。本格未发现 P0–P2,2×P3。不批准。
门禁事实:git merge-tree --write-tree 对当前 origin/main exit 0,写出 tree 4675335c62…,无冲突(本地实测,不依赖 GitHub 的 mergeable 字段)。落后 main 1、领先 3。rm -rf dist 后重建,产物确认来自本 head(shellSessionRowEqual 与 SessionCatalogContext 均出现在 apps/desktop/dist/…,这两个符号只在本 head 存在)。
挂载顺序变更在生产路径上是安全的。 useSessionCatalogController 从 useRef 自建改为 useContext,且缺 Provider 时直接 throw。我要找的失败是「某条挂载路径没有 Provider ⇒ 从静默自建变成硬崩」—— 不可达:createDesktopFeatureServices() 在 main.tsx:36 模块作用域调用而非每次 render;SessionCatalogContext.Provider 在 main.tsx:71 包住 <App>,位于所有消费者之上;全仓只有一个 createRoot(main.tsx:70);消费者仅两处(composer-mentions-provider.tsx:129、use-app-shell-session-workspace.ts:44),都在 Provider 之下。
纠正一个看起来像风险、实际不是的点:生命周期没有变。 旧的 useRef 版本同样是「每个 renderer 一个」,因为 root 只挂载一次、AppShellContent 不随工作区切换重挂。这是 prop 钻透改成 context 的接线改动,不是 lifetime 改动,所以我原本预期的「跨工作区残留旧目录」并不存在。
真正的风险面:shellSessionRowEqual 跳过了什么。 它对八个字段不比较(activityAt/hasUnread/isFlagged/lastMessagePreview/localCreatedAt/revision/statusUpdatedAt/subagentRuntime),注释断言「没有东西在整行读之下渲染它们」。这个断言我逐条查了,今天成立:对 activeCatalogSession/activeHostSession/requestedCatalogSession/requestedHostSession 及别名 activeSession/activeDesktopSession,全渲染层扫描这八个字段的读取 → 0 处;含展开(app-shell.tsx:674 的 ...activeSession → activeSessionForModelControls,消费者只读 llmConnectionId/Slug)以及整行传下去的 usePlanModeState/useWorkbarController/useSessionEventHealthPolling/sharedSessionDialog.openSession,均未读这八个字段(session-collaboration 目录 grep exit=1 确无匹配,并跑了同形正对照得 132 命中,确认不是命令写错);依赖数组全部挂在标量上,没有以整行对象做 effect key,所以抑制重渲染不会改 effect 时序;rail/palette/mentions 确实渲染其中几个字段,但走各自独立的订阅与比较器(mentions 的 conversationSessionListsEqual 明确比较 lastMessagePreview),不经过这道门 —— 设计自洽。
[P3] composer-mentions-provider.tsx:136-138 注释与代码不符。 注释写「a flag/rename/activity bump republishes nothing here」。flag 与 activity 属实(isFlagged、activityAt 都在跳过清单里);rename 不属实 —— name 不在 NON_RENDERED_ROW_KEYS 中,所以改名会改变行身份 → skillRelevantRow 变化 → 把它列入依赖数组的那个 effect 重跑 → 技能列表重新拉取。方向无害(多刷一次,不会变旧),但这句注释是下一个人编辑这张清单前要读的规格,三项里有一项是假的。
[P3] 跳过清单的不变量只受单向保护。 新增字段默认走「比较 → 重发」,是安全的默认,且有 republishes for a field the rail-only list does not know about 钉住。反方向没有任何保护:将来有人从被门控的行上读 lastMessagePreview/isFlagged/subagentRuntime,会拿到静默过期的值,而没有任何测试会失败。承诺被钉住了,会违反它的那条路没有。 而且这几个字段名看上去完全「可渲染」(rail 和 palette 本来就在渲染),下一个人踩中不奇怪。建议加守卫:要么在类型层把门控行收窄成不含这八个键的视图,要么加一条断言「门控行不得被读取这些键」的测试。
跑过的测试(产物均来自本 head):app-shell-session-ui-state 17/17(含新增的 shellSessionRowEqual 四条)、session-workspace-action-identity 2、message-queue-ui-state 4、chat-composer-region-draft-handoff 7、app-shell-pending-attachments 5、app-shell-attachment-preflight-send 2、composer-mentions 4、session-reference-composer 3、conversation-services-adapter 1。fail = 0。
我没有验的(重要,别把这格当已覆盖):我没有跑真实 Electron 交互。 这条线历来产出 P2 靠的是真实窗口里的受控 IPC 时序;我做的是静态追踪加单元测试,覆盖不了那一类发现。准确的说法是:这条回归线这轮仍然缺席,我只补上了一部分,「我没发现问题」不等于「这条线过了」。 另外:作者声称的整窗口 DOM 变更 −86% 我没验;其他席位上轮的发现是否真被关掉我没复核;我对八个字段的扫描是静态的,覆盖属性访问、展开、Object.keys/values/entries 与计算属性访问,但通过序列化/深拷贝间接暴露整行的路径不在范围内。
独立性声明:我与协调评审 agent 同 owner、同模型谱系,本结论不构成对其任何结论的独立佐证;本轮唯一不同 owner 的独立线是另一席位。
Coordinator note (kabi-opus): I independently reproduced the first P3 before publishing rather than relaying it. NON_RENDERED_ROW_KEYS at use-app-shell-session-ui-state.ts:41-50 contains the eight fields listed and does not contain name; skillRelevantRow at composer-mentions-provider.tsx:139 is built with shellSessionRowEqual and appears in the dependency array of the effect that calls refresh(). The comment's "rename" claim is false as written. The second P3 is a statement about absent coverage, which I have not tried to falsify.
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-dev; published by the coordinating seat.
jackwener
left a comment
There was a problem hiding this comment.
I reviewed edf12ca5318371fc44292d41af831d12313867b4.
No P0–P2.
The two issues I had on the previous head are gone:
shellSessionRowEqualis exported from the conversation feature's testing entry, and the test imports that entry instead of the controller file.- AppShell no longer threads the catalog into the mentions provider. The catalog is created once with the desktop services and provided above them. Production callers sit under that provider.
I did not measure the claimed whole-window render reduction.
简体中文
我审查了 edf12ca5318371fc44292d41af831d12313867b4。没有 P0–P2。
上一版里我提的两点已经不在:行相等函数从 conversation 的 testing 入口导出,测试不再深入 controller;AppShell 不再把 catalog 传进 mentions,catalog 在 services 之上挂好,生产路径上的调用都在 Provider 下面。
整窗口渲染下降的数字我没有测。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Summary
Row-level catalog patches (Refs #5532) still republished the whole active row to the shell on every event. Flag/unflag, unread markers, preview and activity bookkeeping all commit a fresh row object, and the observation channel resends a fresh execution projection per frame — each republish re-rendered the chat surface, mostly astryx controls rewriting DOM attributes.
activeCatalogSession,requestedCatalogSession, the mentions skill-reload row) withshellSessionRowEqual: rail-only bookkeeping fields (activityAt,hasUnread,isFlagged,lastMessagePreview,localCreatedAt,revision,statusUpdatedAt,subagentRuntime) no longer republish; every other field compares by value. A field added later republishes until proven rail-only — the failure direction is an extra render, not a stale value.sessions.list()IPC persessions:changed; drop the now-unusedlist/subscribeChangesports from the conversation service surface.getStateon the way back; keep artifact poll results identity-stable when the list is unchanged.Out of scope on purpose: transcript rendering while the shell is obscured (settings covering) still commits ~400 DOM mutations/s — that is an observable-behavior change (catch-up on return) and will come as a separate PR. The ~100 mut/s idle floor is astryx controls rewriting
styleper render — upstream amplification, also untouched.A/B (same machine, same specs, back-to-back:
main@ feb9cf2 vs this branch)React commit probe + MutationObserver/CDP in real Electron, driving real
sessions.setFlaggedIPC:isFlagged+revision, both rail-onlysessions.list()→ setState + skill reload)Remaining per-toggle work is the rail's own flag/selection render plus astryx per-render
styleamplification — the same amplification that makes up the idle floor.Verification
npm run format,npm run lintclean;build:main,build:rendererpass.node --teston the five affected suites: 26/26 pass, including newshellSessionRowEqualcases (ignored-field patches keep identity; compared-field changes republish).main@ feb9cf2 with identical harnesses, not an intermediate branch state.The two measurement specs are local harnesses and intentionally not part of this diff.
AI use
Tool(s) and scope: Devin — implementation, unit tests, measurement harnesses, and CDP attribution.
Checklist
Does this PR entail a change in behavior?