fix(desktop): keep session workspace action identities fixed - #4110
Draft
Astro-Han wants to merge 3 commits into
Draft
fix(desktop): keep session workspace action identities fixed#4110Astro-Han wants to merge 3 commits into
Astro-Han wants to merge 3 commits into
Conversation
`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
6 tasks
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 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.
setActiveIdwas a function declaration in theuseAppShellSessionWorkspacebody, so it changed identity on every AppShell render. AsactivateSessionit invalidatedopenSession'suseCallback, then the Session navigation controller'scommands, then the host'srowActionsandonSelectSession, thenrenderSessionRow— which defeatedSessionNavRow'smemoon every commit. A single switch re-rendered all 32 sidebar rows about twenty times, and because every Astryx button removes and re-adds its inlineanchor-nameper 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.
createSessionWorkspaceActionsmoves them out of the render body and the hook instantiates it once;refreshSessionsandseedSessionsget the same treatment. They are deliberately not routed throughuseStableActions, whose facade exists for factories whose closures do capture changing deps — paying for that indirection here would buy nothing.Two smaller pieces ride along, each its own commit:
@maka/ui'sformatAbsoluteTimestampwas a second copy of theIntloptions@maka/core/relative-timealready 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:getRelativeFormatandgetAbsoluteFormatshared onecachedLocaleand 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; the UI copy is re-exported rather than reimplemented. Profiling attributed ~33 ms per switch to this, and an A/B swapping in a memoisingIntl.DateTimeFormatmoved renderer JS by less than the run-to-run spread — a duplicate-authority removal, not a measurable win.Refs #4109
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.
memohitsRenderer 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— passsession-rail-render-contract(Playwright) — passtsc -p apps/desktop/tsconfig.renderer.json --noEmit,biome checkon the touched files — cleanNot 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. Reverting
??=to=in the workspace hook fails the identity test withsetActiveId changed identity between rendersand the contract spec withExpected: <= 36 / Received: 336. 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 — at most three inline style writes per rail row for one switch — 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
useExhaustiveDependencieshas nothing to flag.The budget is measured, not guessed. On the new twelve-row fixture the rail writes 4 styles when it behaves — the leaving row and the arriving row, two each, stable across runs — against 336 with the defect restored. 36 sits an order of magnitude clear of both.
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-motioncutsrequestAnimationFramecallbacks 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 seamapp-shell-session-ui-state.tsalready establishes, instead of receiving props through five layers.AI use
Select exactly one:
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. The human contributor reviewed the diff, the commit messages, and the measurement method.
Generated-bytrailers are on all three commits.Checklist
Does this PR entail a change in behavior?