You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
refactor(desktop): make AppShell a renderer composition root #3439
The previous AppShell governance work improved file-level separation, but
AppShell remains the default owner of renderer state, environment IO,
subscriptions, feature lifecycles, and prop assembly.
After the Workbar extraction, app-shell.tsx is still 3,230 lines with
105 imports and 21 direct window.maka references. The wider app-shell* / use-app-shell* family contains 30 files and 8,942 lines.
This is not a line-count target. The problem is ownership and dependency
direction.
Desired boundary
AppShell may own:
root providers and error boundaries;
top-level location and frame composition;
composition of feature public APIs;
explicit cross-feature intent wiring.
AppShell must not own:
feature reducers or persistence;
feature-specific subscriptions or environment IO;
transcript, submission, Goal, Workbar, or dynamic-resource lifecycles;
tool-specific view-model construction.
Proposed direction
Use the Workbar model / controller / ui / ports / desktop adapter / testing boundary as the reference for the remaining renderer slices.
No product, visual, IPC, storage-schema, copy, or shortcut changes.
Composer mount semantics, bootstrap behavior, streaming isolation,
Session-switch races, and Workbar resource lifecycles must remain compatible.
Non-goals
No new global state library or service locator
No IPC redesign
No packages/ui extraction
No mechanical movement of every large renderer file into features/
If anyone would like to take one of the remaining items, please comment with the scope you’re interested in so we can coordinate and avoid overlapping work.
Scope: move Goal state and subscriptions, arm/pause/resume/clear lifecycle, dialog ownership, and Goal view-model assembly behind a feature boundary with a Desktop adapter, test fakes, and boundary coverage. AppShell will only provide the active Session context and consume the feature host/commands/selectors surface.
This will preserve the current product behavior, IPC contracts, copy, shortcuts, and Session-switch semantics. I’ll keep the broader shell-region/guardrail checklist item separate unless this change genuinely completes it.
Goals has now landed in #3531, and I’ve updated the checklist.
I’m taking the Module Hub ownership slice next.
Scope: move the Extensions/Automations hub selection and header composition, plus Skills and Scheduled Tasks state, refresh, and mutation lifecycles, behind a feature boundary with narrow Desktop service adapters, test fakes, and boundary coverage. AppShell should pass top-level navigation and explicit cross-feature intents and render the feature host, rather than own module data or bridge calls.
I’ll preserve the current routing, layout, copy, IPC/storage contracts, error feedback, refresh fencing, and active-surface semantics. MCP and Daily Review already have page-level ownership, so I’ll keep their internal behavior separate and only compose those pages through the hub boundary unless a small shared seam is required.
I’m intentionally not taking Session catalog authority because #3619 is already active in that area.
Draft PR #3649 now implements the Module Hub ownership slice.
The feature boundary owns Skills, Scheduled Tasks, keep-awake, hub routing/header composition, and the previously Shell-owned Daily Review bridge/actions. The Daily Review page internals remain page-owned. MCP remains the explicit page-owned direct-bridge exception and is only composed by ModuleHubHost.
AppShell now provides top-level navigation and explicit cross-feature intents, renders the public host, and consumes the narrow selectors needed by Session rail/composer wiring. Local verification is green at exact head 0764fd5: 1368 Desktop tests, 178 Storybook renders, 4 related Playwright tests, lint/format/build/typecheck, Desktop/UI Knip, Astryx inventory, diff check, and a clean merge-tree against current upstream/main.
An AI red/blue pass reproduced and fixed several lifecycle races, followed by a fresh exact-head review with no surviving in-scope P0–P3 findings. This does not replace independent human approval.
I’m leaving the Module Hub checklist item unchecked until the PR is merged.
While attributing renderer CPU on a session switch (#4109) I ran into something that bears on how the boxes here are defined, so leaving the evidence rather than an opinion.
app-shell.tsx declares 16 useState of its own. The 536 hooks on the AppShellContent fiber — 108 useState/useReducer, 9 useSyncExternalStore, 110 effects — come from the 37 custom hooks it calls. The feature separation this issue asks for has largely happened at the file level; what has not moved is where those hooks are called.
Session Navigation is the clearest case. It is already a complete model / controller / ui / ports slice, and useSessionNavigationController() is still invoked from AppShell's render body (app-shell.tsx:1672). In React the position in the tree is the scope of the state, so the extraction moved the code without changing what a state change re-renders: a session switch still produces ~19 commits, ~14 of them full-tree, and the rail is re-rendered by every one.
Two measurements, both alternating configurations inside one running instance:
Stabilising every identity into SessionNavigationHost and giving it a memo boundary eliminates ~72% of the rail's renders and buys 13 ms of 206 ms (~6%, on wins 15/18).
96–99% of each full-tree render is recoverable by perfect memoisation; only 13–56 fibers per commit have genuinely changed props.
So the leaf is not where the cost is, and neither is any one file's size.
If it is useful, the checklist items could be restated as call sites rather than extractions — "useX is no longer called in AppShellContent" has a definition of done that "extract X" does not, and it can be held by an allow-list lint rule that converges to zero. #4109 is the first slice measured this way and can serve as the template. Entirely your call; I did not want to change the framing of your issue from the outside.
(Investigation and measurements produced with Claude Code, on a running dev build.)
Correction. The paragraph above originally said 7 and 38. Those numbers came from a counter that required a ( directly after the hook name, so it skipped every useState<T>(...). Adversarial review of #4125 caught it; the corrected figures are above, and #4109 has been updated too. The argument is unchanged — if anything the gap between what the file declares and what it carries is what the correction narrows.
@Astro-Han Thanks — this is useful evidence, and I agree that the definition of done should include state scope and hook call sites, not only file extraction. A slice is not complete if its controller or state hook is still called from AppShellContent.
I opened #4088 as the root foundation for this: one central renderer architecture checker inventories AppShell hook calls together with root imports, bridge/environment access, and feature boundaries, and treats the existing values as monotonic debt. I would prefer one central gate rather than a second long-lived hook-specific gate.
For #4125, please keep owning the Session Navigation and performance slice; I will not duplicate it. My proposed integration order is:
Keep the runtime measurements and render-budget test.
Place the Session catalog/provider authority in the declared renderer zones.
Record any remaining AppShell read as explicit transitional debt, keep it from growing, and use call-site/state-scope removal as the completion criterion.
One clarification: #4088 itself should not claim a runtime speedup. It prevents future root growth and makes the migrations enforceable. The measured performance improvement comes from moving state and subscriptions to the providers or regions that read them, as your session-switch data demonstrates.
Following up on the R1 ratchet from #4088: I ran into this while implementing the pricing-override UI (#2015 / PR #4164).
Settings → Usage is still legacy-zone and frozen by the closure ratchet, and #2015 requires the pricing editor to live as a Usage tab (not a new top-level Settings page). Adding that net-new tab needs a +1 dependency on the frozen usage-settings-page.tsx, with no existing code to remove as an offset — so it can't pass the ratchet, and there's no features/usage slice to attach to yet. Usage/Settings wasn't one of the slices tracked here.
Since I'm already in this surface for the pricing work, I'm happy to take on extracting the Usage settings surface into a features/usage slice (following the model / controller / ui / ports / desktop adapter / testing boundary), which would also unblock the pricing UI. Opened #4425 to track it. Flagging here since it's a slice the R1 foundation didn't cover.
Following up on the R1 ratchet from #4088: I ran into this while implementing the pricing-override UI (#2015 / PR #4164).
Settings → Usage is still legacy-zone and frozen by the closure ratchet, and #2015 requires the pricing editor to live as a Usage tab (not a new top-level Settings page). Adding that net-new tab needs a +1 dependency on the frozen usage-settings-page.tsx, with no existing code to remove as an offset — so it can't pass the ratchet, and there's no features/usage slice to attach to yet. Usage/Settings wasn't one of the slices tracked here.
Since I'm already in this surface for the pricing work, I'm happy to take on extracting the Usage settings surface into a features/usage slice (following the model / controller / ui / ports / desktop adapter / testing boundary), which would also unblock the pricing UI. Opened #4425 to track it. Flagging here since it's a slice the R1 foundation didn't cover.
Thanks — this is exactly the kind of unowned legacy growth the R1 ratchet was intended to surface. #4425 looks like the right scoped follow-up; I would prefer that over weakening the ledger or reopening #3439.
Before implementation, could we keep the Usage ownership migration separable from the Pricing product decision? The current #4164 head still implements the full built-in ∪ overrides table, while the earlier direction on #2218 preferred an overrides-only surface with a catalog-backed picker. The ownership extraction should preserve the existing Usage behavior first, reuse the shared Desktop feature-services composition seam, and define completion by legacy Usage hooks/Bridge/dependency closure actually decreasing—not only by delegating through a new wrapper.
One small factual note: the current architecture failure is broader than a single dependency edge; it reports two new dependencies on usage-settings-page.tsx and six new files entering the legacy AppShell closure.
Successor tracking issue: #4582. It restates the remaining items by call site (hook no longer called in AppShell/AppShellContent, bridge paths gone from app-shell.tsx, controllerOwners registered) and adds the Usage, App Update, and shared-infrastructure items that this issue did not cover. Open PRs #4315, #4491, #4498, and #4440 should reference #4582 going forward.
Activity
chihumyum commentedon Aug 22, 2026
I’m currently working on this issue
If anyone would like to take one of the remaining items, please comment with the scope you’re interested in so we can coordinate and avoid overlapping work.
Contributions are very welcome.
chihumyum commentedon Aug 22, 2026
I’m taking the Goals slice next.
Scope: move Goal state and subscriptions, arm/pause/resume/clear lifecycle, dialog ownership, and Goal view-model assembly behind a feature boundary with a Desktop adapter, test fakes, and boundary coverage. AppShell will only provide the active Session context and consume the feature host/commands/selectors surface.
This will preserve the current product behavior, IPC contracts, copy, shortcuts, and Session-switch semantics. I’ll keep the broader shell-region/guardrail checklist item separate unless this change genuinely completes it.
chore: refresh Astryx surface inventory
refactor(desktop): extract goals as a feature slice
chore: refresh Astryx surface inventory
refactor(desktop): extract goals as a feature slice
chore: refresh Astryx surface inventory
refactor(desktop): extract goals as a feature slice
chore: refresh Astryx surface inventory
refactor(desktop): extract goals as a feature slice (#3531)
chihumyum commentedon Aug 23, 2026
Goals has now landed in #3531, and I’ve updated the checklist.
I’m taking the Module Hub ownership slice next.
Scope: move the Extensions/Automations hub selection and header composition, plus Skills and Scheduled Tasks state, refresh, and mutation lifecycles, behind a feature boundary with narrow Desktop service adapters, test fakes, and boundary coverage. AppShell should pass top-level navigation and explicit cross-feature intents and render the feature host, rather than own module data or bridge calls.
I’ll preserve the current routing, layout, copy, IPC/storage contracts, error feedback, refresh fencing, and active-surface semantics. MCP and Daily Review already have page-level ownership, so I’ll keep their internal behavior separate and only compose those pages through the hub boundary unless a small shared seam is required.
I’m intentionally not taking Session catalog authority because #3619 is already active in that area.
chihumyum commentedon Aug 23, 2026
Draft PR #3649 now implements the Module Hub ownership slice.
The feature boundary owns Skills, Scheduled Tasks, keep-awake, hub routing/header composition, and the previously Shell-owned Daily Review bridge/actions. The Daily Review page internals remain page-owned. MCP remains the explicit page-owned direct-bridge exception and is only composed by ModuleHubHost.
AppShell now provides top-level navigation and explicit cross-feature intents, renders the public host, and consumes the narrow selectors needed by Session rail/composer wiring. Local verification is green at exact head 0764fd5: 1368 Desktop tests, 178 Storybook renders, 4 related Playwright tests, lint/format/build/typecheck, Desktop/UI Knip, Astryx inventory, diff check, and a clean merge-tree against current upstream/main.
An AI red/blue pass reproduced and fixed several lifecycle races, followed by a fresh exact-head review with no surviving in-scope P0–P3 findings. This does not replace independent human approval.
I’m leaving the Module Hub checklist item unchecked until the PR is merged.
7 remaining items
Astro-Han commentedon Aug 29, 2026
While attributing renderer CPU on a session switch (#4109) I ran into something that bears on how the boxes here are defined, so leaving the evidence rather than an opinion.
app-shell.tsxdeclares 16useStateof its own. The 536 hooks on theAppShellContentfiber — 108useState/useReducer, 9useSyncExternalStore, 110 effects — come from the 37 custom hooks it calls. The feature separation this issue asks for has largely happened at the file level; what has not moved is where those hooks are called.Session Navigation is the clearest case. It is already a complete
model / controller / ui / portsslice, anduseSessionNavigationController()is still invoked from AppShell's render body (app-shell.tsx:1672). In React the position in the tree is the scope of the state, so the extraction moved the code without changing what a state change re-renders: a session switch still produces ~19 commits, ~14 of them full-tree, and the rail is re-rendered by every one.Two measurements, both alternating configurations inside one running instance:
SessionNavigationHostand giving it amemoboundary eliminates ~72% of the rail's renders and buys 13 ms of 206 ms (~6%, on wins 15/18).So the leaf is not where the cost is, and neither is any one file's size.
If it is useful, the checklist items could be restated as call sites rather than extractions — "
useXis no longer called inAppShellContent" has a definition of done that "extract X" does not, and it can be held by an allow-list lint rule that converges to zero. #4109 is the first slice measured this way and can serve as the template. Entirely your call; I did not want to change the framing of your issue from the outside.(Investigation and measurements produced with Claude Code, on a running dev build.)
Correction. The paragraph above originally said 7 and 38. Those numbers came from a counter that required a
(directly after the hook name, so it skipped everyuseState<T>(...). Adversarial review of #4125 caught it; the corrected figures are above, and #4109 has been updated too. The argument is unchanged — if anything the gap between what the file declares and what it carries is what the correction narrows.chihumyum commentedon Aug 29, 2026
@Astro-Han Thanks — this is useful evidence, and I agree that the definition of done should include state scope and hook call sites, not only file extraction. A slice is not complete if its controller or state hook is still called from
AppShellContent.I opened #4088 as the root foundation for this: one central renderer architecture checker inventories AppShell hook calls together with root imports, bridge/environment access, and feature boundaries, and treats the existing values as monotonic debt. I would prefer one central gate rather than a second long-lived hook-specific gate.
For #4125, please keep owning the Session Navigation and performance slice; I will not duplicate it. My proposed integration order is:
One clarification: #4088 itself should not claim a runtime speedup. It prevents future root growth and makes the migrations enforceable. The measured performance improvement comes from moving state and subscriptions to the providers or regions that read them, as your session-switch data demonstrates.
perf(desktop): give the shell's state the scope of its readers (#4125)
liuxiaocs7 commentedon Sep 1, 2026
Following up on the R1 ratchet from #4088: I ran into this while implementing the pricing-override UI (#2015 / PR #4164).
Settings → Usageis still legacy-zone and frozen by the closure ratchet, and #2015 requires the pricing editor to live as a Usage tab (not a new top-level Settings page). Adding that net-new tab needs a +1 dependency on the frozenusage-settings-page.tsx, with no existing code to remove as an offset — so it can't pass the ratchet, and there's nofeatures/usageslice to attach to yet. Usage/Settings wasn't one of the slices tracked here.Since I'm already in this surface for the pricing work, I'm happy to take on extracting the Usage settings surface into a
features/usageslice (following themodel / controller / ui / ports / desktop adapter / testingboundary), which would also unblock the pricing UI. Opened #4425 to track it. Flagging here since it's a slice the R1 foundation didn't cover.chihumyum commentedon Sep 1, 2026
Thanks — this is exactly the kind of unowned legacy growth the R1 ratchet was intended to surface. #4425 looks like the right scoped follow-up; I would prefer that over weakening the ledger or reopening #3439.
Before implementation, could we keep the Usage ownership migration separable from the Pricing product decision? The current #4164 head still implements the full built-in ∪ overrides table, while the earlier direction on #2218 preferred an overrides-only surface with a catalog-backed picker. The ownership extraction should preserve the existing Usage behavior first, reuse the shared Desktop feature-services composition seam, and define completion by legacy Usage hooks/Bridge/dependency closure actually decreasing—not only by delegating through a new wrapper.
One small factual note: the current architecture failure is broader than a single dependency edge; it reports two new dependencies on usage-settings-page.tsx and six new files entering the legacy AppShell closure.
refactor(desktop): move Settings → Usage into a features/usage slice
chihumyum commentedon Sep 2, 2026
Successor tracking issue: #4582. It restates the remaining items by call site (hook no longer called in
AppShell/AppShellContent, bridge paths gone fromapp-shell.tsx,controllerOwnersregistered) and adds the Usage, App Update, and shared-infrastructure items that this issue did not cover. Open PRs #4315, #4491, #4498, and #4440 should reference #4582 going forward.refactor(desktop): move Settings → Usage into a renderer feature slice (
refactor(desktop): move Settings → Usage into a renderer feature slice (
refactor(desktop): make TaskEntryRoot the registered Task Entry owner (…
refactor(desktop): move App Update controller below AppShell (#4498)