Uh oh!
There was an error while loading. Please reload this page.
perf(desktop): give the shell's state the scope of its readers - #4125
Merged
Conversation
AppShellContent calls 40 hooks across 70 call sites, and every one of them scopes its state to the entire tree. That is why a session switch produces about 19 React commits, 14 of them full-tree renders, of which 96-99% is recoverable work (#4109). The fix is to move call sites into feature providers, one at a time. #3439 tracks that as a checklist of extractions, but extracting a feature into its own slice does not change the scope of any state: Session Navigation is a complete model/controller/ui/ports slice whose controller is still invoked from this render body. A call site, unlike an extraction, has a definition of done — the hook is either called here or it is not. So count them. The inventory is hand-edited and may only shrink; a migration deletes its entry and the same diff shows the gate converging. A hook that appears without one fails, which is not a prohibition but a refusal to let it happen silently. There is deliberately no --write mode: regenerating the inventory on demand would let a new hook be accepted by rerunning a command. useMemo, useCallback, useRef and useId are ignored. They hold no state and subscribe to nothing, so they cannot widen what a render reaches. The gate runs beside the other install-free checks in CI, since it reads one source file and compares it to a committed list. Generated-by: Claude Code
useComposerMentions reloads the Skill catalog on every session switch and on every MCP or session change event, several times per switch. Held in AppShellContent, each of those reloads set state above the whole tree, so repainting two composer popups re-rendered everything under the shell. Move the call site into a ComposerMentionsProvider. `children` is the element AppShell has already built, so React bails out of the subtree and only the composers that read the context re-render. The projection itself is unchanged, including the fail-closed contextKey derivation and the same-context refresh that keeps its settled verdict (#2667). Owning it below the shell also removes the reason to carry it through the workbar: mentionSkills, mentionSkillsUnavailable, mentionSkillsLoading and onSearchMentionFiles were threaded through the controller, the host and the surface to reach two consumers, and QuoteCompanionPanel now reads the same context directly. The file is renamed to composer-mentions.tsx because it is no longer a hook module. This is the first call site to move under the gate added in the previous commit: the inventory goes from 40 hooks to 39, and from 70 call sites to 69. No behaviour changes — the popups render from the same projection, at the same time, for the same surfaces (#4109). Generated-by: Claude Code
Biome's own formatting for the gate script, its tests, and the package.json entry. Squashed into the gate commit on merge. Generated-by: Claude Code
The mention module became a .tsx file when it took ownership of its provider, which brings it into the surface inventory's coverage. It renders no Astryx components, so the row is an aligned no-op — but the inventory is a regenerate-and-diff contract, and a file it cannot see is the failure mode it exists to catch. Squashed into the mention commit on merge. Generated-by: Claude Code
…'s absent catalog absent Three defects found by adversarial review of the gate and the migration it shipped with. All three were silent, and two of them made this pull request's own numbers wrong. The gate under-counted. It required a `(` directly after the hook name, so every `useState<T>(...)` was invisible — 12 call sites in AppShellContent, including `useNewTaskChoice` and `useSessionSettingIntent`, two hooks that were not in the inventory at all and passed the gate anyway. It also counted `copy.useSkillPrompt(...)` as a hook and a `useUiLocale()` mentioned in a comment. The real figure is 42 hooks across 83 call sites, not 39 across 69; the delimiter now balances braces instead of stopping at the first `\n}\n`, which had been correct only because AppShellContent happens to end the file. The gate also watched the wrong scope. `AppShell` wraps `AppShellContent` in the root providers and holds state of its own, so hoisting a hook one level up would have LOWERED the count while widening what it re-renders. Both components are counted now. And `NO_MENTIONS` was not equivalent to no props. Without a provider the context handed the composer an empty catalog and a real `onSearchMentionFiles`, where it had previously received `undefined` — enough for the Composer to mount the mention popup's layer for a surface with nothing behind it, which broke all seven cases of the draft-handoff suite. The context defaults to `undefined` and consumers read it optionally, so a composer outside the shell sees exactly what it saw before. Also here: the `workbar-boundary` contract now asserts the context read rather than the prop names it was written against; `./composer-mentions` gains its `.js` specifier, without which Node's ESM resolver could not load the module at all; and the provider's memo is built from destructured fields so its dependencies are its materials rather than a hand-kept mirror of them. The inventory is exact rather than a ceiling, and the failure text says so instead of instructing the reader to lower the number — advice that a mis-count would otherwise turn into a way to empty the gate. Generated-by: Claude Code
This was referenced Aug 29, 2026
Switching a Session re-rendered the whole tree about fourteen times, and each of those renders reached the Session rail's ~1,000 fibers. Not because any component there is expensive — they are individually cheap, and memoising them recovered 6% — but because the rail's state was declared above them: `useSessionNavigationController()` and the `sessions` / `activeSessionId` `useState`s were called in AppShell's render body, so every one of the shell's commits was also a commit of the rail. Move the state to where its readers are. - `sessions` and `activeSessionId` become one external store, read through `useSyncExternalStore` with custom-equality selectors — the mechanism `app-shell-session-ui-state.ts` already established in #1985, generalised here as `useExternalStoreSelector` so both stores share it. - `useSessionNavigationController()` moves out of AppShell into `SessionNavigationProvider`, which sits directly above the rail and publishes what the rail reads as two contexts: a memoised one for the ~1,000 fibers of rows and an unmemoised one for the few dozen fibers of chrome. - The rail element itself is a module constant, so the shell rendering does not rebuild it and React skips the subtree. Cross-feature intent stays explicit. Opening a Session still clears the active transcript and leaves WorkHub; those remain commands the shell issues, passed as `SessionNavigationPorts` and read through a ref published on commit — so the rail can call them without their identity being something it re-renders for. What this makes redundant is deleted, not left behind: `SessionNavigationHost` and its fifteen props, the same prop set redeclared across `SessionListPanel`, `SessionHistoryList` and `SessionListGroups`, `renderSessionRow`'s eight-entry dependency array, and the `useStableActions` hand-stabilisation that existed only because the state sat at the top. The hook gate's inventory shrinks with it. Measured in one running instance, alternating configurations and comparing paired trials: busy JS per switch drops from 585ms to 397–430ms (27–32%), and the inline-style writes the rail's DOM takes drop from 1,696–2,086 to 168–424. `session-rail-render-budget.spec.ts` holds that as a contract — a budget on what one switch is allowed to touch, plus the assertion that the selection moves exactly once and the status badges are not rebuilt underneath it — and the probes that produced the numbers ship as `scripts/perf/`. Generated-by: Claude Code
…ok-call-sites Generated-by: Claude Code # Conflicts: # apps/desktop/src/renderer/use-app-shell-session-list.ts # apps/desktop/src/renderer/use-app-shell-session-workspace.ts
The rail migration left four exported types with no reader. Knip fails the `test` check on them, and each is a real leftover rather than a tolerated one: `SessionNavigationSearchTarget` was declared twice. `session-open-command.ts` owns it and uses it; the copy in `use-session-navigation-controller.ts` is what the rename left behind, and two declarations of one shape is the kind of drift a barrel hides. `SessionNavigationReads`, `SessionRailProjection` and `SessionNavigationSession` were re-exported from the feature barrel for consumers that no longer exist: the reads hook returns the first, the provider and the reads hook take the other two from their own modules, and the tests reach them through `testing.ts`. A barrel entry with no importer widens the feature's public surface for nothing. Generated-by: Claude Code
…ok-call-sites Generated-by: Claude Code # Conflicts: # apps/desktop/src/renderer/app-shell.tsx
Astro-Han
marked this pull request as ready for review
August 29, 2026 05:03
Three stores — session UI state, the session catalog, the rail's geometry — each carried their own `listeners` set, `replaceState` and `subscribe`. The duplication that matters is not the lines: it is that "swap, then notify synchronously, and never schedule it" is load-bearing (#1985's terminal-turn handoff reads back the state it announces) and was restated per store, with the third copy no longer restating it at all. `createObservableState` holds that rule once and the three stores keep only what they actually differ in: their state and their commands. The rail's controller subscribed to that store three times, once per field, and then reassembled the three values into an object identical to the store's own state. Every field is read, so the split bought no granularity, and the store already replaces its state only when a field moved — the identity is the comparison. One subscription through `selectRailLayout`, which is how the shell's own read was already written. That retires `selectRailCollapsed`, `selectRailWidth`, `selectRailViewMode` and the `SessionNavigationLayout` mirror of `SessionRailLayoutState`. `createInitialSessionCatalogState` and the controller's `initialState` parameter go with them: nothing ever passed one. They were copied from the shape of #1985's controller, whose equivalents tests do use. Generated-by: Claude Code
#4110 landed `session-rail-render-contract.spec.ts` on main while this branch carried `session-rail-render-budget.spec.ts`. They are one authority — what a single session switch is allowed to cost the Session rail — so they become one spec rather than two that must be kept agreeing. The contract's shape wins. Attributing writes to rows closes the hole a total-only budget leaves: a regression that re-renders the whole rail exactly once stays under any total generous enough not to flake, but it cannot touch two rows. Its counter also carries a liveness lower bound and polls to quiet instead of sleeping a fixed 3s. What the budget had and the contract did not is the timing half, and that moves across: the selection must pass through exactly one row on its way to the clicked one, and the status badges must not be torn down and rebuilt underneath it. A count says how much was rewritten, not whether the user watched it happen. Given up: the budget ran both grouping modes. The contract's fixture seeds one project, and row attribution is independent of how many rows are seeded, so the second mode was buying repetition rather than reach. Generated-by: Claude Code
`rail-scope-probe.ts` shipped in the renderer so the rail's old and new scope could both be reached from one running instance — the only way to compare, since renderer timings shift by orders of magnitude between app launches while the spread inside one launch is small. The premise was wrong: the switch does not have to ship. Branch a worktree, put the flag in the one place that reads it, alternate inside the running instance, delete the worktree. Measurement discipline is ours; product code carries none of it. What shipped instead was a branch no product path executes and a replica of the defect that would have to be maintained against the code it replicates. `session-switch-busy-js.mjs` loses `--ab` with it and now reports the running build only. The README keeps the rule that made the probe seem necessary and states how to satisfy it from a throwaway worktree. Generated-by: Claude Code
…ok-call-sites # Conflicts: # apps/desktop/src/renderer/use-app-shell-session-workspace.ts
zhiiw
approved these changes
Aug 29, 2026
zhiiw
left a comment
Contributor
There was a problem hiding this comment.
Reviewed at exact head b006fc52 (56 files, +2900/−796).
No P0–P3 findings. The parts I checked hardest, and how:
The gate counts what it claims.scripts/check-app-shell-hooks.mjs on this head reports ok (42 hooks, 81 call sites) locally on my machine, and its own 23-case adversarial suite passes — including the counter-examples this PR's earlier version miscounted (typed call, member call, JSX-text apostrophe, regex containing a quote, column-zero brace). The inventory being exact in both directions (grow AND shrink fail) is the right shape for a convergence gate.
The selector equality chain is load-bearing and correct.useExternalStoreSelector caches by state identity and reuses the prior value when isEqual holds; the session catalog replaces the array identity on every refresh, so readers depend on that equality being honest. The rail projection memoizes on [activeSessionId, hiddenSessionIds, sessions], and the provider's split — memoized data context for the ~1,000 row fibers, deliberately unmemoized chrome for the few dozen — matches where each value actually moves. chromeRef is updated in a layout effect so commands always reach the latest props.
The self-corrections in the description are verified, not trusted. The two earlier numbers it retracts (the gate's own miscount, the 6% memo figure) are retracted with the cause named, and the gate's current count is reproducible.
Executed on a real Windows machine at this head: clean rebuild of libs + runtime-host + desktop main + ui; session-navigation-boundary + session-navigation-controller + workbar-boundary 14/14, session-history-row-actions + sidebar-footer 10/10, gate script + its 23 tests green. All six hosted checks are completed/success on this head.
简体中文
无 P0–P3。门禁脚本本地复核 42 hooks/81 call sites 与描述一致且其 23 条对抗用例全过；选择器相等链逐一追过（catalog 每次刷新换数组身份，正确性全靠 isEqual 诚实——是对的）；描述里撤回的两个旧数字都带着原因，当前数字可复现。本机真 Windows 干净重建 + 四个靶向套件全绿 + 六个 hosted check 全绿。
jackwener
approved these changes
Aug 29, 2026
jackwener
left a comment
Member
There was a problem hiding this comment.
I found no P0–P3 issues on exact head b006fc5268640a80ece359516ca7525cf657344a.
The cost of a session switch was not a hot component. A hook called in AppShell's render body has the whole tree as its scope, so the rail's ~1,000 fibers re-rendered on commits that only moved the active row. Moving useSessionNavigationController into SessionNavigationProvider and the mention catalog into ComposerMentionsProvider is the right cut: the same hooks, called where their readers are. Cross-feature work (clear the transcript, leave Work Hub) stays an explicit command on a ref, so the shell does not have to freeze every function that happens to sit upstream of the rail. The module-constant SESSION_RAIL is what lets React skip the list when only chrome changed.
I did not treat issue #4109 as evidence. The claim that can be shown false is the Playwright contract: one switch may touch at most two rows and must not rebuild status badges. Hosted test is terminal success on this head.
The hook inventory staying at 42/81 is honest. The shell still reads the rail — command palette, titlebar parent, --maka-sidenav-width — so a selector remains. Shrinking the entry by moving a read the shell actually needs would defeat the gate.
I am not merging. This changes where the shell holds state, even though the title is a performance fix; merge is a human call.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
精确 head b006fc5268640a80ece359516ca7525cf657344a 上我没有发现 P0–P3。
会话切换的成本不在某个热点组件。AppShell 渲染里调用的 hook，作用域就是整棵树，所以只是活跃行在动时，轨道上大约一千个 fiber 也会跟着重绘。把 useSessionNavigationController 放到 SessionNavigationProvider、把 mention catalog 放到 ComposerMentionsProvider，切法是对的：还是这些 hook，改到读者所在的地方调用。跨功能的事（清转录、离开 Work Hub）仍是 ref 上的显式命令，shell 不必把手伸到轨道上游的每个函数都冻住。模块常量 SESSION_RAIL 让 React 在只有 chrome 变时跳过列表。
我没有把 issue #4109 当证据。能被证伪的主张是 Playwright 契约：一次切换最多碰到两行，且不得重建状态徽章。hosted test 在这个 head 上是终态成功。
hook 清单停在 42/81 是实话。shell 仍然要读轨道（命令面板、标题栏父会话、--maka-sidenav-width），所以会留下一次 selector。为了把数字做下去而挪走 shell 真正需要的读取，会把这道门的意义弄反。
我不合入。标题写的是性能，但这是在改 shell 把状态放在哪，合入由人类决定。
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
M4n5ter
approved these changes
Aug 29, 2026
M4n5ter
left a comment
Member
There was a problem hiding this comment.
Reviewed at exact head b006fc5268640a80ece359516ca7525cf657344a. I found no P0 or P1 issue, so I am approving with two non-blocking P2 comments inline.
The structural simplification is real: the old Session Navigation host and its repeated rail/composer prop carriers are gone, while catalog mutation, rail geometry, and mention state each retain one authority. I verified the full UI suite (264/264), 25 focused Session Navigation/layout tests, Desktop preload/main/renderer/Storybook typechecking, the production renderer build, and the real Electron rail render contract. All six hosted checks on this exact head are terminal green.
Because this is a refactor/performance change, this approval is a code-review result only; the human maintainer still owns the merge decision.
Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Astryx reports a collapse as `onSizeChange(0)` — from the collapse control and from a drag past the threshold alike. Moving the rail's width into `sessionRailLayoutStore` moved the call site but not the `width >= minWidth` guard that used to sit beside it in `session-navigation-host.tsx`, so that zero reached `setWidth`, was clamped to 180, and was persisted 200 ms later. Resize to 400, collapse, reload, expand: the rail came back at 180. The guard belongs to the store, not to a call site. The store is the only authority for the width and its persistence, and a call site is exactly what moved out from under the old guard. Reported by an automated review agent operated by @M4n5ter on #4125. Generated-by: Claude Code
The gate counted only unqualified `use[A-Z]` names, so `React.useState(0)` and React 19's bare `use(...)` were invisible to a scanner whose entire value is that it fails closed — a future subscription could widen the shell's scope with this exact inventory still green. Both forms now count, and `React.useState` counts under `useState`: the inventory names the hook, not the syntax that reached it. The qualifier is the literal `React.` rather than any identifier because the shell really does call `shellCopy.useSkillPrompt(name)`, which a general `<name>.useX(` rule would count as a hook. That leaves one hole — `import * as X from 'react'` — and the gate closes it by refusing the file, since it cannot resolve the alias and must not guess. The counts are unchanged: 42 hooks, 81 call sites. Reported by an automated review agent operated by @M4n5ter on #4125. Generated-by: Claude Code
Uh oh!
There was an error while loading. Please reload this page.
saltand pushed a commit
to saltand/maka-agent
that referenced
this pull request
Aug 31, 2026
…e#4125) * test(desktop): gate the hooks scoped to the whole shell AppShellContent calls 40 hooks across 70 call sites, and every one of them scopes its state to the entire tree. That is why a session switch produces about 19 React commits, 14 of them full-tree renders, of which 96-99% is recoverable work (apache#4109). The fix is to move call sites into feature providers, one at a time. apache#3439 tracks that as a checklist of extractions, but extracting a feature into its own slice does not change the scope of any state: Session Navigation is a complete model/controller/ui/ports slice whose controller is still invoked from this render body. A call site, unlike an extraction, has a definition of done — the hook is either called here or it is not. So count them. The inventory is hand-edited and may only shrink; a migration deletes its entry and the same diff shows the gate converging. A hook that appears without one fails, which is not a prohibition but a refusal to let it happen silently. There is deliberately no --write mode: regenerating the inventory on demand would let a new hook be accepted by rerunning a command. useMemo, useCallback, useRef and useId are ignored. They hold no state and subscribe to nothing, so they cannot widen what a render reaches. The gate runs beside the other install-free checks in CI, since it reads one source file and compares it to a committed list. Generated-by: Claude Code * refactor(desktop): own the composer mention catalog below the shell useComposerMentions reloads the Skill catalog on every session switch and on every MCP or session change event, several times per switch. Held in AppShellContent, each of those reloads set state above the whole tree, so repainting two composer popups re-rendered everything under the shell. Move the call site into a ComposerMentionsProvider. `children` is the element AppShell has already built, so React bails out of the subtree and only the composers that read the context re-render. The projection itself is unchanged, including the fail-closed contextKey derivation and the same-context refresh that keeps its settled verdict (apache#2667). Owning it below the shell also removes the reason to carry it through the workbar: mentionSkills, mentionSkillsUnavailable, mentionSkillsLoading and onSearchMentionFiles were threaded through the controller, the host and the surface to reach two consumers, and QuoteCompanionPanel now reads the same context directly. The file is renamed to composer-mentions.tsx because it is no longer a hook module. This is the first call site to move under the gate added in the previous commit: the inventory goes from 40 hooks to 39, and from 70 call sites to 69. No behaviour changes — the popups render from the same projection, at the same time, for the same surfaces (apache#4109). Generated-by: Claude Code * style: format the app-shell hook gate Biome's own formatting for the gate script, its tests, and the package.json entry. Squashed into the gate commit on merge. Generated-by: Claude Code * chore: record composer-mentions.tsx in the Astryx surface inventory The mention module became a .tsx file when it took ownership of its provider, which brings it into the surface inventory's coverage. It renders no Astryx components, so the row is an aligned no-op — but the inventory is a regenerate-and-diff contract, and a file it cannot see is the failure mode it exists to catch. Squashed into the mention commit on merge. Generated-by: Claude Code * fix(desktop): count the shell's hooks correctly and keep the composer's absent catalog absent Three defects found by adversarial review of the gate and the migration it shipped with. All three were silent, and two of them made this pull request's own numbers wrong. The gate under-counted. It required a `(` directly after the hook name, so every `useState<T>(...)` was invisible — 12 call sites in AppShellContent, including `useNewTaskChoice` and `useSessionSettingIntent`, two hooks that were not in the inventory at all and passed the gate anyway. It also counted `copy.useSkillPrompt(...)` as a hook and a `useUiLocale()` mentioned in a comment. The real figure is 42 hooks across 83 call sites, not 39 across 69; the delimiter now balances braces instead of stopping at the first `\n}\n`, which had been correct only because AppShellContent happens to end the file. The gate also watched the wrong scope. `AppShell` wraps `AppShellContent` in the root providers and holds state of its own, so hoisting a hook one level up would have LOWERED the count while widening what it re-renders. Both components are counted now. And `NO_MENTIONS` was not equivalent to no props. Without a provider the context handed the composer an empty catalog and a real `onSearchMentionFiles`, where it had previously received `undefined` — enough for the Composer to mount the mention popup's layer for a surface with nothing behind it, which broke all seven cases of the draft-handoff suite. The context defaults to `undefined` and consumers read it optionally, so a composer outside the shell sees exactly what it saw before. Also here: the `workbar-boundary` contract now asserts the context read rather than the prop names it was written against; `./composer-mentions` gains its `.js` specifier, without which Node's ESM resolver could not load the module at all; and the provider's memo is built from destructured fields so its dependencies are its materials rather than a hand-kept mirror of them. The inventory is exact rather than a ceiling, and the failure text says so instead of instructing the reader to lower the number — advice that a mis-count would otherwise turn into a way to empty the gate. Generated-by: Claude Code * perf(desktop): give the Session rail its own scope Switching a Session re-rendered the whole tree about fourteen times, and each of those renders reached the Session rail's ~1,000 fibers. Not because any component there is expensive — they are individually cheap, and memoising them recovered 6% — but because the rail's state was declared above them: `useSessionNavigationController()` and the `sessions` / `activeSessionId` `useState`s were called in AppShell's render body, so every one of the shell's commits was also a commit of the rail. Move the state to where its readers are. - `sessions` and `activeSessionId` become one external store, read through `useSyncExternalStore` with custom-equality selectors — the mechanism `app-shell-session-ui-state.ts` already established in apache#1985, generalised here as `useExternalStoreSelector` so both stores share it. - `useSessionNavigationController()` moves out of AppShell into `SessionNavigationProvider`, which sits directly above the rail and publishes what the rail reads as two contexts: a memoised one for the ~1,000 fibers of rows and an unmemoised one for the few dozen fibers of chrome. - The rail element itself is a module constant, so the shell rendering does not rebuild it and React skips the subtree. Cross-feature intent stays explicit. Opening a Session still clears the active transcript and leaves WorkHub; those remain commands the shell issues, passed as `SessionNavigationPorts` and read through a ref published on commit — so the rail can call them without their identity being something it re-renders for. What this makes redundant is deleted, not left behind: `SessionNavigationHost` and its fifteen props, the same prop set redeclared across `SessionListPanel`, `SessionHistoryList` and `SessionListGroups`, `renderSessionRow`'s eight-entry dependency array, and the `useStableActions` hand-stabilisation that existed only because the state sat at the top. The hook gate's inventory shrinks with it. Measured in one running instance, alternating configurations and comparing paired trials: busy JS per switch drops from 585ms to 397–430ms (27–32%), and the inline-style writes the rail's DOM takes drop from 1,696–2,086 to 168–424. `session-rail-render-budget.spec.ts` holds that as a contract — a budget on what one switch is allowed to touch, plus the assertion that the selection moves exactly once and the status badges are not rebuilt underneath it — and the probes that produced the numbers ship as `scripts/perf/`. Generated-by: Claude Code * fix(desktop): drop the session-navigation exports nothing reads The rail migration left four exported types with no reader. Knip fails the `test` check on them, and each is a real leftover rather than a tolerated one: `SessionNavigationSearchTarget` was declared twice. `session-open-command.ts` owns it and uses it; the copy in `use-session-navigation-controller.ts` is what the rename left behind, and two declarations of one shape is the kind of drift a barrel hides. `SessionNavigationReads`, `SessionRailProjection` and `SessionNavigationSession` were re-exported from the feature barrel for consumers that no longer exist: the reads hook returns the first, the provider and the reads hook take the other two from their own modules, and the tests reach them through `testing.ts`. A barrel entry with no importer widens the feature's public surface for nothing. Generated-by: Claude Code * refactor(desktop): give the renderer's stores one notification rule Three stores — session UI state, the session catalog, the rail's geometry — each carried their own `listeners` set, `replaceState` and `subscribe`. The duplication that matters is not the lines: it is that "swap, then notify synchronously, and never schedule it" is load-bearing (apache#1985's terminal-turn handoff reads back the state it announces) and was restated per store, with the third copy no longer restating it at all. `createObservableState` holds that rule once and the three stores keep only what they actually differ in: their state and their commands. The rail's controller subscribed to that store three times, once per field, and then reassembled the three values into an object identical to the store's own state. Every field is read, so the split bought no granularity, and the store already replaces its state only when a field moved — the identity is the comparison. One subscription through `selectRailLayout`, which is how the shell's own read was already written. That retires `selectRailCollapsed`, `selectRailWidth`, `selectRailViewMode` and the `SessionNavigationLayout` mirror of `SessionRailLayoutState`. `createInitialSessionCatalogState` and the controller's `initialState` parameter go with them: nothing ever passed one. They were copied from the shape of apache#1985's controller, whose equivalents tests do use. Generated-by: Claude Code * test(desktop): fold the rail render budget into the one rail contract apache#4110 landed `session-rail-render-contract.spec.ts` on main while this branch carried `session-rail-render-budget.spec.ts`. They are one authority — what a single session switch is allowed to cost the Session rail — so they become one spec rather than two that must be kept agreeing. The contract's shape wins. Attributing writes to rows closes the hole a total-only budget leaves: a regression that re-renders the whole rail exactly once stays under any total generous enough not to flake, but it cannot touch two rows. Its counter also carries a liveness lower bound and polls to quiet instead of sleeping a fixed 3s. What the budget had and the contract did not is the timing half, and that moves across: the selection must pass through exactly one row on its way to the clicked one, and the status badges must not be torn down and rebuilt underneath it. A count says how much was rewritten, not whether the user watched it happen. Given up: the budget ran both grouping modes. The contract's fixture seeds one project, and row attribution is independent of how many rows are seeded, so the second mode was buying repetition rather than reach. Generated-by: Claude Code * refactor(desktop): keep the A/B switch out of product code `rail-scope-probe.ts` shipped in the renderer so the rail's old and new scope could both be reached from one running instance — the only way to compare, since renderer timings shift by orders of magnitude between app launches while the spread inside one launch is small. The premise was wrong: the switch does not have to ship. Branch a worktree, put the flag in the one place that reads it, alternate inside the running instance, delete the worktree. Measurement discipline is ours; product code carries none of it. What shipped instead was a branch no product path executes and a replica of the defect that would have to be maintained against the code it replicates. `session-switch-busy-js.mjs` loses `--ab` with it and now reports the running build only. The README keeps the rule that made the probe seem necessary and states how to satisfy it from a throwaway worktree. Generated-by: Claude Code * fix(desktop): keep the rail's collapse sentinel out of the stored width Astryx reports a collapse as `onSizeChange(0)` — from the collapse control and from a drag past the threshold alike. Moving the rail's width into `sessionRailLayoutStore` moved the call site but not the `width >= minWidth` guard that used to sit beside it in `session-navigation-host.tsx`, so that zero reached `setWidth`, was clamped to 180, and was persisted 200 ms later. Resize to 400, collapse, reload, expand: the rail came back at 180. The guard belongs to the store, not to a call site. The store is the only authority for the width and its persistence, and a call site is exactly what moved out from under the old guard. Reported by an automated review agent operated by @M4n5ter on apache#4125. Generated-by: Claude Code * fix(desktop): count every React hook call form in the convergence gate The gate counted only unqualified `use[A-Z]` names, so `React.useState(0)` and React 19's bare `use(...)` were invisible to a scanner whose entire value is that it fails closed — a future subscription could widen the shell's scope with this exact inventory still green. Both forms now count, and `React.useState` counts under `useState`: the inventory names the hook, not the syntax that reached it. The qualifier is the literal `React.` rather than any identifier because the shell really does call `shellCopy.useSkillPrompt(name)`, which a general `<name>.useX(` rule would count as a hook. That leaves one hole — `import * as X from 'react'` — and the gate closes it by refusing the file, since it cannot resolve the alias and must not guess. The counts are unchanged: 42 hooks, 81 call sites. Reported by an automated review agent operated by @M4n5ter on apache#4125. Generated-by: Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Switching a Session re-rendered the whole tree about fourteen times, and 96–99% of each of those renders was recoverable work. The cause is not that any component is expensive — it is that the shell declares state above everything that reads it. In React the position in the tree IS the scope of the state: a hook called in
AppShellContent's render body has the whole tree as its scope; the same hook called in a provider has its readers as its scope.This PR moves two call sites to their readers and installs the gate that keeps the rest converging.
The gate (
scripts/check-app-shell-hooks.mjs). #3439 tracks this work as a checklist of extractions, but extracting a feature into its own slice does not change the scope of any state — Session Navigation was already a completemodel / controller / ui / portsslice whose controller was still invoked from AppShell's render body. A call site, unlike an extraction, has a definition of done. The inventory is hand-edited and exact: growing past an entry fails, and so does falling below one, because only failing on growth would let counts drift downward and record nothing. There is deliberately no--writemode — the friction is the point, which is the opposite shape fromcheck-astryx-surface-inventory.mjsand deliberately so.The composer mention catalog. It reloaded several times per switch, and held in the shell each reload set state above the whole tree to repaint two popups. It now lives in
ComposerMentionsProvider, whosechildrenis the element AppShell already built, so React bails out of the subtree. That removed the four props threaded through the workbar controller, host and surface.The Session rail.
sessionsandactiveSessionIdbecome one external store read throughuseSyncExternalStorewith custom-equality selectors — the mechanism #1985 established, generalised here asuseExternalStoreSelectorso both stores share it, with no new state library.useSessionNavigationController()moves intoSessionNavigationProvider, which publishes what the rail reads as two contexts: a memoised one for the ~1,000 row fibers and an unmemoised one for the few dozen chrome fibers. The rail element is a module constant, so the shell rendering does not rebuild it.Cross-feature intent stays explicit: opening a Session still clears the active transcript and leaves WorkHub, as commands the shell issues through
SessionNavigationPorts.Deleted, not left behind:
SessionNavigationHostand its fifteen props, the same prop set redeclared acrossSessionListPanel/SessionHistoryList/SessionListGroups,renderSessionRow's eight-entry dependency array, and the controller's identity-stabilisinguseMemos.Fixes#4109
Refs #3439
Result
Measured in one running instance, alternating configurations and comparing paired trials:
apps/desktop/e2e/session-rail-render-contract.spec.tsholds this as a contract rather than a one-off number. #4110 landed that spec on main while this branch carried its own rail budget; they are one authority, so they are now one spec. The contract's shape wins — attributing writes to rows catches a regression that re-renders the whole rail exactly once, which any total generous enough not to flake would let through — and this branch's timing half moves across: the selection passes through exactly one row on its way to the clicked one, and the status badges are not rebuilt underneath it. It is result-shaped on purpose; it asserts no identity stability, because that kind of assertion stops meaning anything the moment the cause changes. Given up: the retired spec ran both grouping modes, and row attribution is independent of how many rows are seeded, so the second mode was repetition rather than reach.What this adds, and what did not shrink
Three things are worth stating plainly rather than leaving for a reviewer to find.
The hook inventory did not go down. It is 42 hooks / 83 call sites before and after the rail change; the only movement is
useSessionNavigationController: 1→useSessionNavigationReads: 1. (It reads 42/81 on this branch today: merging main brought #4113, which collapsed threeuseKeyedPendingRegistrycall sites into oneuseTurnActionRegistry. That −2 is #4113's, not this change's.) The entry shrank instead of disappearing because the shell body genuinely still reads the rail — the command palette lists the same visible Sessions, the titlebar shows the linked parent, the frame publishes--maka-sidenav-width. That is the honest outcome for a feature the shell reads, as opposed to the mention catalog which had no reader in the shell body and simply left. Moving call sites around to make the number fall would defeat the gate's purpose.useStableActionscame out net zero, not deleted: the call site insideuse-session-navigation-controller.tsis gone, and one was added inuse-project-context.ts, where the Project rows' actions have to keep their identity or the rows' memo misses. That one is the convergence point for the whole chain, not a leftover — but the third commit's message reads as though the mechanism went away entirely, and it did not.No measurement affordance ships. An earlier revision of this branch carried
rail-scope-probe.tsin the renderer, so the rail's old and new scope were both reachable from one running instance — necessary, because renderer timings shift by orders of magnitude between app launches while the spread inside one launch is small. The premise was wrong: the switch does not have to ship. Branch a worktree, put the flag in the one place that reads it, alternate inside the running instance, delete the worktree. Measurement discipline is ours; product code carries none of it. The probe and--abare gone, andscripts/perf/README.mdkeeps the rule with the throwaway-worktree method beside it.Correction to earlier numbers
Two sets of figures published earlier in this PR and in #4109 were wrong, and are corrected in place there:
(directly after the hook name, so everyuseState<T>(...)was invisible — 12 call sites, including two hooks that were in no inventory and passed anyway — and it watched onlyAppShellContentwhileAppShellwraps it in the root providers and holds state of its own. 39/69 was really 42/83. Found by adversarial review, fixed in38592a9dc.SessionNavigationHosta memo boundary bought 13 ms of 206 ms. Its click helper selected[data-session-id]across the whole document and did not check the viewport, the same defect since fixed inscripts/perf/cdp-client.mjs— where it was observed to open Settings mid-run, after which the measurement describes an app not showing the rail at all. That run cannot be repeated (the memo variant no longer exists), so treat the 6% as unreliable rather than as evidence. Its qualitative conclusion — that a memo boundary is a filter on a render that should not have reached the rail — is the one this PR acts on, and the 27–32% above is measured with the corrected helper.Verification
npm run check:app-shell-hooks→ok (42 hooks, 81 call sites across AppShell + AppShellContent)node --test --test-concurrency=1 scripts/check-app-shell-hooks.test.mjs→ 23 passed. Every counter-example adversarial review found is a case: explicit type argument, member call, type position, apostrophe in JSX text, regex literal containing a quote, JSX closing tag, brace in column zero, destructured parameter.npx tsc --noEmitontsconfig.renderer.json,tsconfig.main.json,tsconfig.storybook.json→ cleannpm run lint,npm run format:check,node scripts/asf-license-headers.mjs check→ cleannode --test apps/desktop/dist/main/__tests__/{session-navigation-controller,session-navigation-boundary}.test.js→ 8 passednode --test apps/desktop/dist/main/__tests__/{chat-composer-region-draft-handoff,workbar-boundary}.test.js→ 13 passed. The draft-handoff suite was red on this branch and green onmain— that is how theNO_MENTIONSregression was caught, and both sides were run rather than accepting it as an environment difference.@maka/ui:node --test dist/__tests__/{session-history-row-actions,sidebar-footer}.test.js→ 10 passedsession-rail-render-contract(also--repeat-each=6),sidebar-geometry,sidebar-project-row,sidebar-titlebar-actions,composer-skill-invocation,composer-plus-menu-stability→ passedNo before/after screenshots: this change does not alter what renders, and the three existing sidebar specs passing is the stronger evidence that geometry, project rows and titlebar actions did not regress. Say so if you want images in the body anyway.
AI use
Tool(s) and scope: Claude Code. Diagnosis (CDP profiling,
Function.prototype.bindinterception of React'sdispatchSetState, fiber-tree walks on a running dev build), the gate and its tests, both migrations, the rail render contract, and thescripts/perfprobes. Three adversarial review passes were also run with Claude Code and found the gate's under-counting, its wrong scope, and theNO_MENTIONSregression; those are AI findings verified by hand againstmainbefore being acted on. All commits carryGenerated-bytrailers. Reviewed by the human contributor of record before submission.Checklist
Does this PR entail a change in behavior?