fix(desktop): keep session workspace action identities fixed by Astro-Han · Pull Request #4110 · apache/maka

Astro-Han · 2026-08-28T18:52:31Z

Summary

Switching a session held the renderer at 34% CPU with peaks near 77%, and roughly 58% of that was React render and commit work. The cause is one function identity.

setActiveId was a function declaration in the useAppShellSessionWorkspace body, so it changed identity on every AppShell render. As activateSession it invalidated openSession's useCallback, then the Session navigation controller's commands, then the host's rowActions and onSelectSession, then renderSessionRow — which defeated SessionNavRow's memo on every commit. A single switch re-rendered all 32 sidebar rows about twenty times, and because every Astryx button removes and re-adds its inline anchor-name per render, that switch also produced roughly 2,500 style writes and the style recalculation they force.

Every dependency these actions close over is a ref box, a React state setter, or a method of the once-created session-UI controller, so they are constant by construction rather than by discipline. createSessionWorkspaceActions moves them out of the render body and the hook instantiates it once; refreshSessions and seedSessions get the same treatment. They are deliberately not routed through useStableActions, whose facade exists for factories whose closures do capture changing deps — paying for that indirection here would buy nothing.

Smaller pieces ride along, each its own commit:

@maka/ui's formatAbsoluteTimestamp was a second copy of the Intl options @maka/core/relative-time already owned, and built a formatter per call — about 1,300 per switch, since the sidebar reads one per row for the tooltip and one for the accessible name. Core's cache could not have absorbed them either: getRelativeFormat and getAbsoluteFormat shared one cachedLocale and cleared each other on a miss, so alternating readings rebuilt a formatter every call. Core now caches each with its own locale and exports the function, and the three UI call sites import it from there. Profiling attributed ~33 ms per switch to this, and an A/B swapping in a memoising Intl.DateTimeFormat moved renderer JS by less than the run-to-run spread — a duplicate-authority removal, not a measurable win.
Two declarations the move left without an owner: the local MessageListUpdater copies in app-shell-{chat,turn,revision}-actions, and the alias re-export of formatAbsoluteTimestamp in chat-display-helpers.
A contract spec that budgets the rail's DOM writes for one switch, described under Review focus.

Refs #4109

Behaviour differences worth naming

Neither is reachable from a current call site; both are recorded because they widen what a future one could do.

The moved formatAbsoluteTimestamp drops a typeof Intl === 'undefined' fallback to toISOString(). Core's module has never had that guard and formatRelativeTimestamp's absolute branch already called Intl unconditionally, so it only protected a runtime that would fail a line later.
Core's signature defaults locale to 'zh', where the UI copy required it. All four call sites pass it explicitly; the default matches the module's three sibling formatters.

Verification

All measurements are same-instance A/B: the two identities were made runtime-switchable and alternated inside one running dev app, six switches each. Cross-instance comparison is not usable here — restarting the app moves these numbers by more than the effect.

per session switch	unstable	stable
Row renders	459	85
`memo` hits	0	432
DOM mutations	3,438	1,992
Renderer JS	521 ms	380 ms
Renderer CPU under a repeated-switch loop	34% (peak 77%)	24% (peak 55%)

Renderer JS fell in 6 of 6 paired runs. Idle CPU was 0.1% before and after; this workload is entirely interaction-driven.

Ran locally:

session-workspace-action-identity, session-navigation-controller, relative-time — pass
session-rail-render-contract (Playwright) — pass, three consecutive runs
tsc -p apps/desktop/tsconfig.renderer.json --noEmit, @maka/core and @maka/ui builds, npm run format — clean

Not run: the full repository suite, and the end-to-end check on a dev app built from this branch — a dev app from another checkout holds the shared profile lock. The Playwright spec covers the same observable on a clean fixture, so the gap is the manual pass, not the assertion.

Falsifiability was checked for both new tests by reverting the fix in the built renderer bundle. The identity test fails with setActiveId changed identity between renders; the contract spec fails on rows-touched, 12 of 12 rows written where 2 is the budget. Restoring the mutual cache invalidation in core fails the formatter test.

Review focus

The contract spec is the piece worth arguing about. It asserts an outcome rather than an identity, because this rail has had several independent regressions of the same shape and each was invisible to the others. An identity assertion pins one mechanism in one hook; the next plain function declaration upstream passes every existing check, since the dependency arrays stay correct and useExhaustiveDependencies has nothing to flag.

What carries the contract is which rows were written, not how many writes there were. A switch touches the leaving row and the arriving row and nothing else, at any rail length. A total budget cannot say that: 3 * rows is 1.5 whole-rail renders, so a regression that re-renders the rail exactly once — the likeliest one, since renderSessionRow depends on rowActions, sessionMeta and three Sets — would have passed. The spec also counts row remounts, because React sets attributes before insertion and an attribute-only observer reads a whole rail remounting as cheaper than a re-render, and asserts the counter fired at all, because its sensitivity comes from an Astryx ref callback that upstream could reasonably memoise.

It also covers ground this PR does not fix. A switch still produces about twenty commits, and why is not yet attributed; it is not animation-driven, since emulating prefers-reduced-motion cuts requestAnimationFrame callbacks from 188 to 13 while the render count is unchanged. That cascade multiplies whatever the rail costs per render, and anything that raises it shows up in this budget. Tracked in #4109 along with the structural direction — letting the rail subscribe through the store seam app-shell-session-ui-state.ts already establishes, instead of receiving props through five layers.

AI use

Select exactly one:

No generative tool made a substantive contribution
Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code. It drove the CDP profiling and DOM probes that isolated the cause and produced every number above, then wrote the change, the tests, and this description. Three Claude Code agents then reviewed the branch adversarially — runtime correctness, formatter equivalence, and test validity — each tasked with refuting the PR's claims rather than confirming them; they found no correctness defect, and every finding they did raise about the tests is addressed in the last three commits. That review is AI output and does not substitute for human review. The human contributor reviewed the diff, the commit messages, and the measurement method. Generated-by trailers are on all six commits.

Checklist

Tests cover the change and fail without it
Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

Yes — described under Summary above
No

`setActiveId` and its siblings were function declarations in the `useAppShellSessionWorkspace` body, so every AppShell render handed consumers new identities. `activateSession` alone invalidated `openSession`'s `useCallback`, then the Session navigation controller's `commands`, then the host's `rowActions` and `onSelectSession`, then `renderSessionRow` — which defeated `SessionNavRow`'s `memo` on every commit. One session switch re-rendered all 32 sidebar rows about twenty times, and each Astryx button rewrote its inline `anchor-name` per render, so a switch also produced roughly 2,500 style writes. Every dependency these actions close over is a ref box, a React state setter, or a method of the once-created session-UI controller, so they are constant by construction rather than by discipline. `createSessionWorkspaceActions` moves them out of the render body and the hook instantiates it once; `refreshSessions` and `seedSessions` get the same treatment. This is why they are not routed through `useStableActions`, whose facade exists for factories whose closures do capture changing deps. Measured by alternating the two identities inside one running instance, six switches each: row renders 459 to 85, DOM mutations 3,438 to 1,992, renderer JS 521 ms to 380 ms, and renderer CPU under a repeated-switch loop 34% to 24% with peaks falling from 77% to 55%. Two imports in the session-list hook gain their `.js` extension so the workspace module tree loads under Node, which the new identity contract test needs. Generated-by: Claude Code

`@maka/ui`'s `formatAbsoluteTimestamp` was a second copy of the `Intl` options `@maka/core/relative-time` already owned, and it built a formatter on every call — the session sidebar reads one per row for the tooltip and one for the row's accessible name, so a single session switch constructed roughly 1,300 of them. Core's own cache could not have absorbed that either: `getRelativeFormat` and `getAbsoluteFormat` shared one `cachedLocale` and cleared each other on a miss, so alternating readings of the same timestamp rebuilt a formatter every call. Core now caches each formatter with its own locale and exports `formatAbsoluteTimestamp`; the UI copy is re-exported rather than reimplemented, so the tooltip and the accessible name cannot drift. Profiling attributed about 33 ms per session switch to the constructions. An A/B inside one running instance, swapping a memoising `Intl.DateTimeFormat` in and out six times each, moved renderer JS by less than the run-to-run spread — this is a duplicate-authority removal, not a measurable win. Generated-by: Claude Code

The rail's cost has had several independent causes — `setActiveId` changing identity on every AppShell render, `Intl` formatters rebuilt per row, catalog refreshes replacing unchanged row objects — and each was invisible to the others. Asserting identities pins one mechanism in one hook; the next plain function declaration upstream passes every existing check, because the dependency arrays stay correct. So the assertion is on the outcome: switching a session may write at most three inline styles per rail row. Inline `style` is the dominant term, since every Astryx button removes and re-adds its `anchor-name` per render, and it needs no React internals to observe — a `MutationObserver` over the rail is the whole probe. Measured on the new twelve-row fixture: 4 writes when the rail behaves, the leaving and the arriving row at two each, stable across runs; 336 with `setActiveId` restored to a per-render identity. The budget of 36 sits an order of magnitude clear of both. This also covers the unattributed commit cascade in apache#4109: whatever raises the number of commits a switch produces shows up here. Generated-by: Claude Code

The budget was a total, scaled by row count, and one-sided. Each of those let a real regression through. A total of `3 * rows` is 1.5 whole-rail renders, so a change that renders the rail exactly once more than it should stayed under it — and that is the likeliest regression, because `renderSessionRow` depends on `rowActions`, `sessionMeta` and three Sets, any of which becoming a fresh object per render defeats `SessionNavRow`'s memo for every row at once. The identity test could not see it either: it reads the workspace hook's return value, not what AppShell assembles from it. Attributing each write to its row removes the hole and the row-count coupling together — a switch touches the leaving row and the arriving row, whatever the rail's length — and it asserts the fix's own missing middle, that memo holding means untouched rows do no DOM work. Counting remounts closes the other side: React sets attributes before insertion, so an attribute-only observer reads a whole rail unmounting and remounting as CHEAPER than a re-render. `styleWrites > 0` is the counter's liveness check. Every write counted comes from an Astryx ref callback with no `useCallback` around it; if that is ever memoised upstream, healthy and regressed readings both collapse to zero and a one-sided budget passes forever. The two fixed `waitForTimeout` calls were the only thing keeping a slow machine out of the measurement window, with `retries: 0` behind them. Polling until the counter is quiet for ~300ms states the actual precondition, and runs faster: 2.2s against 3.6s. The identity test now derives its keys from the hook's return value. The hand-kept list covered 11 of the 23 functions it returns and would have kept covering 11 as more were added. Verified by reverting the fix in the built renderer bundle: the run fails on rows-touched, and passes three times in a row with the fix in place. Generated-by: Claude Code

Extracting the workspace actions gave this type an owner and an export. Leaving the three local copies in place would have made the PR that merged one duplicate authority create another. Generated-by: Claude Code

Once the implementation moved to `@maka/core/relative-time`, the export left behind in `chat-display-helpers` held nothing — it was a second name for the same function, and `relative-time.tsx` reached the one module through both names at once. Drift is prevented by there being a single implementation, not by which file the callers name. `formatAbsoluteTimestamp` is not in the package's public exports, so this moves three imports and removes a concept without changing a contract. Generated-by: Claude Code

jackwener

I found no P0–P3 issues on exact head 35c0ffdfcfa92ebb54c41d480644d41efec94a2e.

setActiveId was a function declaration inside useAppShellSessionWorkspace, so it changed identity on every AppShell render. AppShell passes it as activateSession into the Session navigation controller, which rebuilds commands, then rowActions / onSelectSession, then renderSessionRow, which defeats SessionNavRow's memo. That is a reachable session-switch path.

The factory only closes over ref boxes, React state setters, and methods of the once-created session-UI controller, so one instance for the renderer's lifetime is the right cut. refreshSessions / seedSessions get the same treatment. The identity test reads every function the hook returns rather than a hand-kept list; the rail contract asserts which rows were written, so a single whole-rail re-render cannot hide. I did not treat issue #4109 as evidence.

The formatter change matches the same path: relative and absolute readings no longer share one locale slot, and the UI copy of formatAbsoluteTimestamp is gone. Call sites still pass locale explicitly.

I am not merging. Hosted test was still queued when I posted. This review does not claim CI is green.

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 35c0ffdfcfa92ebb54c41d480644d41efec94a2e 上我没有发现 P0–P3。

setActiveId 写在 useAppShellSessionWorkspace 函数体里，每次 AppShell 渲染都会换新身份，再传到导航控制器的 activateSession，一路打掉 SessionNavRow 的 memo。这是会话切换会走到的路径。

工厂只闭合 ref、React setter，以及一次性创建的 session-UI 控制器方法，所以渲染器生命周期内只建一次是对的。refreshSessions / seedSessions 同样处理。身份测试读 hook 返回的全部函数；栏合同断言被写到的行，整栏重渲染一次也藏不住。我没有把 issue #4109 当证据。

相对和绝对时间格式化不再共用一个 locale 槽；UI 里那份 formatAbsoluteTimestamp 已删。调用点仍显式传 locale。

我不合入。发这条时 hosted test 还在排队，这次审查不表示 CI 已绿。

本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。

M4n5ter

I found no P0-P3 issues on exact head 35c0ffdfcfa92ebb54c41d480644d41efec94a2e.

The once-created workspace actions read changing state through refs and retain the previous selection, message, transient-projection, reload-intent, retry, stop, and catalog behavior. They do not freeze mutable session state in closures. The extraction is therefore a real lifetime boundary, not only a file move. The timestamp cleanup also removes the UI's duplicate absolute formatter and gives relative and absolute formatting independent locale caches.

I verified this with the complete Desktop main suite (1,650 tests), the complete UI suite (264 tests), the Core timestamp suite, the workspace identity test, and three consecutive real-Electron rail-contract runs. Two mutations independently restored the unstable action identity and the formatter-cache thrash; each made its targeted regression test fail. The current-main merge tree is clean and preserves every reviewed changed-file blob.

Hosted windows_recovery is green. Hosted test is still in progress, so this approval does not claim that CI is complete. I am not merging this pull request.

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
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。

zhiiw

Reviewed at exact head 35c0ffdf (15 files, +680/-220).

The fix is sound by construction: everything the once-created action factory captures is a ref box, a React state setter, or a method of the once-created session-UI controller — I traced the one non-obvious capture (sessionUi.clearSessionUiState) to its controller to confirm. The moved bodies match the previous hook-body implementations; the only behavioral deltas are the two the description names, both unreachable from current call sites.

The rail contract spec budgets the outcome (rows touched ≤ 2, zero remounts, bounded style writes) rather than the mechanism, and its liveness assertion (styleWrites > 0) keeps the budget from passing vacuously if the upstream write source is ever memoised away.

Verified locally: clean rebuild at this head, session-workspace-action-identity and relative-time suites green. Checks at this head: test and windows_recovery both completed/success.

#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

…ty (#4113) A simplification audit of the session-workspace slice, following the perf fix in #4110. Four concepts leave the renderer; nothing is added that the code did not already imply. **One authority for "an action is in flight."** Message retry, stop, permission mode and session model each had two representations — a `Set` ref the duplicate guard read, and a `Record<string, boolean>` in the session UI store the disabled mask rendered. Nothing read across the pair; they were kept aligned by hand at every claim, every `finally`, and two teardown paths that agreed only because one called the other in the right order. State replacement in the controller is synchronous, so a claim is visible to the next `getState()` in the same task and the guard can read the map it already writes. `createPendingClaim` puts both halves behind one compare-and-set. Gone with it: `addPendingSessionAction` / `clearPendingSessionAction` and the optional-setter parameter both call sites always passed, three private copies of `omitSessionKey`, the four `set*BySession` setters those copies fed, and one of the two teardown paths. **The pending registry collapses onto the turn footer.** `useKeyedPendingRegistry` was generic over `trackState` and `autoClearMs` because it served three instances; two moved to the store above, and both options had only ever had one consumer. `useTurnActionRegistry` says that directly. Its `clearAll()` had no caller — the unmount cleanup walked `timersRef` and `keysRef` itself, reaching around the method that exists to do it — so `timersRef` no longer needs to be public. **The store is reached through its controller.** `useAppShellSessionUiState` returned the controller plus a member-by-member copy, `useAppShellSessionWorkspace` copied that copy, and AppShell destructured the result; adding a map meant editing three lists nothing kept in agreement. The hook now returns the controller, named at 27 call sites. **The delegating facade gets one home and a contract test.** `stable-actions.ts` was split from its hook on the stated grounds that staying React-free made it testable from `node:test`. No such test existed and `createDelegatingActions` had one consumer. The claim is honoured the other way round: the facade moves in with `useStableActions`, becomes private, and the contract is asserted through the hook, where React's commit semantics are part of what is promised. Behaviour change: `setPermissionMode` now claims before its bypass confirmation rather than after, so a second click cannot open a second dialog. The control reads as pending while the user decides, which is what is true; a cancelled confirmation releases the claim. Refs #4109 Generated-by: Claude Code

@M4n5ter

* 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 (#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 * 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 (#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 * 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 #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 (#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 * test(desktop): fold the rail render budget into the one rail contract #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 #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 #4125. Generated-by: Claude Code

…4110) `useAppShellSessionWorkspace` handed back a fresh identity for every action function on every render. `setActiveId` fed `activateSession`, which fed `openSession`'s `useCallback`, which fed the navigation controller's `commands`, the host's `rowActions`/`onSelectSession`, and finally `renderSessionRow` — so `SessionNavRow`'s `memo` was defeated on every commit and one session switch rerendered the whole Session rail. Astryx's buttons rewrite `anchor-name` through an un-memoized inline ref callback (React calls it with `null`, then with the element), so each rerendered row also cost two inline-style writes per button. Lazy ref initialization pins the identities, and two contracts hold the fix: - The identity unit test now derives the action keys from the hook's own return value instead of a hand-written list that covered 11 of 23 functions, with a guard against the hook's shape collapsing into a vacuously passing loop. - The rail e2e attributes DOM writes per row rather than budgeting a total that scaled with row count: at most two rows touched, zero remounts, a liveness lower bound so a detached observer cannot pass, and polling to quiescence instead of a fixed wait. Both tests were verified to fail against the reverted fix. Along the way `formatAbsoluteTimestamp` loses its second copy — `@maka/core` is now its only authority, with the alias re-export removed — and the three renderer action modules share the session workspace's `MessageListUpdater` instead of each declaring the type again. Behaviour differences: the `typeof Intl === 'undefined'` fallback to `toISOString()` is gone, and the surviving formatter defaults `locale` to `'zh'` where the removed copy required it — all four call sites pass it explicitly. Not verified: a live re-check in a dev app built from this branch; the shared dev-profile lock was held elsewhere. Generated-by: Claude Code

…ty (apache#4113) A simplification audit of the session-workspace slice, following the perf fix in apache#4110. Four concepts leave the renderer; nothing is added that the code did not already imply. **One authority for "an action is in flight."** Message retry, stop, permission mode and session model each had two representations — a `Set` ref the duplicate guard read, and a `Record<string, boolean>` in the session UI store the disabled mask rendered. Nothing read across the pair; they were kept aligned by hand at every claim, every `finally`, and two teardown paths that agreed only because one called the other in the right order. State replacement in the controller is synchronous, so a claim is visible to the next `getState()` in the same task and the guard can read the map it already writes. `createPendingClaim` puts both halves behind one compare-and-set. Gone with it: `addPendingSessionAction` / `clearPendingSessionAction` and the optional-setter parameter both call sites always passed, three private copies of `omitSessionKey`, the four `set*BySession` setters those copies fed, and one of the two teardown paths. **The pending registry collapses onto the turn footer.** `useKeyedPendingRegistry` was generic over `trackState` and `autoClearMs` because it served three instances; two moved to the store above, and both options had only ever had one consumer. `useTurnActionRegistry` says that directly. Its `clearAll()` had no caller — the unmount cleanup walked `timersRef` and `keysRef` itself, reaching around the method that exists to do it — so `timersRef` no longer needs to be public. **The store is reached through its controller.** `useAppShellSessionUiState` returned the controller plus a member-by-member copy, `useAppShellSessionWorkspace` copied that copy, and AppShell destructured the result; adding a map meant editing three lists nothing kept in agreement. The hook now returns the controller, named at 27 call sites. **The delegating facade gets one home and a contract test.** `stable-actions.ts` was split from its hook on the stated grounds that staying React-free made it testable from `node:test`. No such test existed and `createDelegatingActions` had one consumer. The claim is honoured the other way round: the facade moves in with `useStableActions`, becomes private, and the contract is asserted through the hook, where React's commit semantics are part of what is promised. Behaviour change: `setPermissionMode` now claims before its bypass confirmation rather than after, so a second click cannot open a second dialog. The control reads as pending while the user decides, which is what is true; a cancelled confirmation releases the claim. Refs apache#4109 Generated-by: Claude Code

@M4n5ter

…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

github-actionsBot added the effort/L Under 1000 readable lines label Aug 28, 2026

Astro-Han mentioned this pull request Aug 28, 2026
refactor(desktop): make the session UI store the only pending authority #4113
Merged
6 tasks

Astro-Han marked this pull request as ready for review August 29, 2026 03:02

Astro-Han added 6 commits August 29, 2026 11:07

refactor(desktop): reuse the session workspace's MessageListUpdater
1ba5aa3
Extracting the workspace actions gave this type an owner and an export. Leaving the three local copies in place would have made the PR that merged one duplicate authority create another. Generated-by: Claude Code

Astro-Han force-pushed the fix/renderer-stabilize-session-workspace-actions branch from 205973b to 35c0ffdCompare August 29, 2026 03:08

jackwener approved these changes Aug 29, 2026
View reviewed changes

M4n5ter approved these changes Aug 29, 2026
View reviewed changes

zhiiw approved these changes Aug 29, 2026
View reviewed changes

Astro-Han merged commit 17aae7c into apache:mainAug 29, 2026
2 checks passed

Astro-Han mentioned this pull request Aug 29, 2026
perf(desktop): give the shell's state the scope of its readers #4125
Merged
6 tasks

me2seeks mentioned this pull request Sep 4, 2026
fix(desktop): npm run dev stuck on preload skeleton — protocol barrel pulls node:crypto into the renderer #4706
Closed

Astro-Han mentioned this pull request Sep 5, 2026
tracking(perf): bound Desktop and Runtime Host work as history grows #4677
Open
11 tasks

Navigation Menu

Uh oh!

fix(desktop): keep session workspace action identities fixed - #4110

fix(desktop): keep session workspace action identities fixed#4110
Astro-Han merged 6 commits into
apache:mainfrom
Astro-Han:fix/renderer-stabilize-session-workspace-actions

Astro-Han commented Aug 28, 2026•
edited
Loading

Uh oh!

jackwener left a comment

Uh oh!

M4n5ter left a comment

Uh oh!

zhiiw left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Astro-Han commented Aug 28, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Behaviour differences worth naming

Verification

Review focus

AI use

Checklist

Uh oh!

jackwener left a comment

Choose a reason for hiding this comment

Uh oh!

M4n5ter left a comment

Choose a reason for hiding this comment

Uh oh!

zhiiw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Astro-Han commented Aug 28, 2026•
edited
Loading