Skip to content

refactor(desktop): close R2 M5 at the AppShell root - #5952

Merged
Astro-Han merged 7 commits into
apache:mainfrom
chihumyum:refactor/m5-root-closure
Oct 3, 2026
Merged

Astro-Han merged 7 commits into
apache:mainfrom
chihumyum:refactor/m5-root-closure

Conversation

@chihumyum

@chihumyum chihumyum commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

After #5934–#5937, npm run check:renderer-architecture -- --report still listed two retained-root rows scheduled for M5. M5's fourth item, "remove migrated legacy exceptions, public raw-state exports and redundant props/helpers", and its fifth, "document every retained root projection/lifecycle with its actual consumer, owner and allowed capability", were also open. This PR closes all three, in five commits:

Commit What changes Who loses what
61cad0bcc OnboardingConnectionSeed, a render-null watch beside the onboarding authority, seeds the default Host's connection projection from each accepted snapshot, or asks it to refresh when onboarding cannot be read. AppShellContent loses its last useEffect and the M5 row for it.
e485fa592 useAppShellProjectContext stops returning the default Host's project list, the Local Host's projects and the raw selected id. It also drops the Local Host project subscription (projects.getLocalSnapshot, subscribeLocalChanges), which nothing has read since #5937 moved project mutations to Task Entry. Its row moves from "M5" to "application lifecycle" (see below). use-project-context.ts loses 2 bridge paths and 1 effect.
eae997042 Props nobody reads are removed (details below). A type-level test pins the trimmed contracts. AppShell stops computing 3 chat-model values and importing ProviderLogo.
3681d31d3 AppShellDetailPanel (stateless, React-only) moves to shell/detail-panel.tsx, the target the ledger's shell-root-and-frame entry already named. Two ownership labels are corrected. The AppShell family loses a legacy file.
d084b62d0 The retained-root rows that stay at the root now name their actual consumers, owners and capabilities. —
03c30db74 The "Transitional feature exports outside Conversation" table now records OverlaysConsumer and ManualDiagnosticReportConsumer as retained rather than "M5". The overlay layer composes the legacy Settings surface and the palette command list, which a feature cannot import; the manual report is a cross-region command. Documentation only. —

The props removed in eae997042:

  • Seven ChatView props. ChatView declares activeConnectionLabel, activeModel, activeModelLabel, activeProviderType, renderProviderMark, modelChoices and onModelChange but never reads them. They are removed from its props. The transcript region passed all but activeModel; the WorkHub conversation passed activeModel.
  • uiLocaleOverride on AppShellContent. Only AppShell's LocaleProvider reads it.
  • SessionNavigationPorts.activateSession. No rail action calls it.
  • The WorkHub rail entry object. Its active repeated workHubActive and its label was a constant. The rail now takes onOpenWorkHub and builds the entry itself.

Why the project context stays at the root

useAppShellProjectContext reads the owner Session's project (path, Git state, current project, capabilities) or, with no Session, the default Host's. The root composes that into:

  • the titlebar;
  • Workbar's project id and aliases;
  • Module Hub's and the command palette's client-path access;
  • composer mentions;
  • the lifecycle handlers' default-Host refresh.

It writes nothing; #5937 moved the mutations and open-folder commands to Task Entry. Every reader is a root-composition input that depends on the owner Session, which AppShellContent derives. That makes it the same class as the three per-Host useShellConnections projections, which #5934's table already records as "application lifecycle". The row now says so, instead of scheduling a move that would only rename the projection. Consolidating it with Task Entry's own per-Host project reads would be a separate change.

Inventory (main 229e1b466 → this branch)

Measure (#4582 "Completion measure", --report) Before After
AppShell-family bridge references (only the E2E fixture remains) 1 1
AppShell-family action factories (only the E2E fixture remains) 1 1
Hook-gate entries / call sites 25 / 30 24 / 29
Rows without a retained-root row 0 0
Rows scheduled for M5 2 0
Rows scheduled for M3 (slice A) 8 8
AppShell-family legacy files 11 10
app-shell.tsx lines / nonTriviaTokens 1,760 / 8,017 1,732 / 7,860
Legacy renderer files 175 174
Closure bridge references (not an R2 measure) 158 156

Not in this PR

  • Slice A's M3 work: the eight M3 rows, useAppShellSessionUiReads and its one-line use-app-shell-session-ui-reads.ts re-export, the Composer region props, and useShellChatModel. Their table rows are left for A to retire rather than edited here.
  • ChatView's onNew. It is also never read, but it is required and passed by about 20 tests and stories. Removing it would be churn outside the root.
  • Catalog props. Several features (SessionNavigationProvider, SessionTurnRequestInboxProvider, SessionSettingsProvider, the command list) still receive the Session catalog as a prop although SessionCatalogContext is above them. That is explicit injection rather than dead threading, so I left it.

Refs #4582

Review focus

  • Table edits. Only the rows that stay at the root were rewritten, from an audit of app-shell.tsx against each hook. Main corrections:
    • the memory indicator is the transcript's session-context chip, not the titlebar;
    • the rail reads feed the transcript's revision navigation and the titlebar, not the command palette;
    • useSystemUiLocale, useAppShellNavRefSync, useStableActions and useToast live in legacy files or @maka/ui;
    • useShellAppearance writes no settings;
    • ConversationLifecycle, not the transcript, bumps the pet counter.
  • Ownership. The ledger only accepts a fixed set of targetZone values, so the effects entry keeps split-by-capability under its new name, root-lifecycle-effects. No rule changed.
  • Overlap with slice A. A is editing the same README table, the ledger and app-shell.tsx. Whichever lands second merges main and regenerates.

Verification

Run locally on Node 24.19.0 at d084b62d0 (on 229e1b466):

  • npm --workspace @maka/desktop run clean:main && … build:test && … test:dist: 3,208 / 3,208 pass. New or changed tests:
    • onboarding-authority.test.ts: the seed maps a snapshot once, skips an unchanged one, and refreshes on a failed read.
    • use-project-context.test.ts: the Desktop stub no longer offers the Local Host snapshot, so a leftover call would throw.
    • app-shell-retired-props.test.ts: type-level. Re-adding any of the seven ChatView props, the rail's workHubEntry or the port's activateSession fails the build; checked by re-adding modelChoices → TS2322.
    • session-navigation-controller.test.ts: the WorkHub entry test now goes through onOpenWorkHub.
  • npm --workspace @maka/ui run clean && … build && … test:dist: 699 / 699 pass.
  • Electron: npm --workspace @maka/desktop run build:with-deps && npx playwright test --config e2e/playwright.config.ts: 33 / 34. The failure is session-workbar.spec.ts:179, the post-reload toBeVisible at line 224. That spec is flaky on main itself: 4 of 8 runs failed at 255ae23ae and 2 of 8 at 8ad836ce1 (measured for refactor(desktop): retire AppShell's direct Desktop bridge (R2 M5) #5936).
  • npm run check:renderer-architecture -- --base 229e1b4665a06a77ac6afb93ce47b3eaa78e0103 --strict-base: pass.
  • npm run check:app-shell-hooks: 24 / 29.
  • npm run typecheck (preload, main, renderer, storybook), npm run lint, npm run format:check, npx knip --workspace apps/desktop, npx knip --workspace packages/ui: pass.
  • npm run windows:inventory (current), npm run astryx:surface-inventory (regenerated for the moved panel; ok), npm run check:asf-headers: pass.

Each commit was typechecked and architecture-checked on its own, with @maka/ui rebuilt per commit.

AI use

Select exactly one:

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

Tool(s) and scope: Claude Opus 5.5 in Claude Code wrote the implementation, tests and PR text. Two read-only Claude Code subagents audited the retained-root rows and AppShell's props; their findings were verified against the code before use. The author reviewed the change and decided to submit it. Every commit carries Generated-by: Claude Opus 5.5.

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 (the only runtime difference is one fewer Local Host project subscription, whose result was never read)

…g authority

AppShellContent kept one useEffect, marked for removal in M5, that seeded
the default Host's connection projection from the onboarding snapshot,
or refreshed it when onboarding could not be read.
OnboardingConnectionSeed, a render-null watch in the onboarding
authority's contract, now does that from the authority itself, with the
shell supplying only the projection's seed and refresh commands.

AppShellContent loses its last useEffect (gate 25/30 → 24/29) and the
retained-root row for it.

Refs apache#4582

Generated-by: Claude Opus 5.5
useAppShellProjectContext still returned the default Host's project list,
the Local Host's projects and the raw selected project id, and kept a
Local Host project subscription (projects.getLocalSnapshot and
subscribeLocalChanges) for them, although no reader remained after
project mutations moved to Task Entry (apache#5937). Drop those outputs and the
subscription.

The hook stays at the root: it is the per-Host project projection the
shell composes into the titlebar, Workbar, Module Hub, command palette
and mentions, in the same class as the per-Host connection projections.
Its retained-root row now records that as an application lifecycle with
its actual consumers instead of an M5 removal.

Refs apache#4582

Generated-by: Claude Opus 5.5
After the R2 merges, AppShell still threaded several values nothing read:

- seven ChatView props (activeConnectionLabel, activeModel,
  activeModelLabel, activeProviderType, renderProviderMark, modelChoices,
  onModelChange) that ChatView declares but never reads. The transcript
  region passed all but activeModel, and the WorkHub conversation passed
  activeModel. They are removed from ChatView's props, with the
  ProviderLogo import and the chat-model values AppShell computed only
  for them.
- uiLocaleOverride on AppShellContent; only AppShell's LocaleProvider
  reads it.
- SessionNavigationPorts.activateSession, which no rail action calls.
- the WorkHub rail entry object: its `active` repeated workHubActive and
  its label was a constant. The rail now takes onOpenWorkHub and builds
  the entry itself.

A type-level test pins the trimmed contracts.

Refs apache#4582

Generated-by: Claude Opus 5.5
…ect ownership labels

AppShellDetailPanel is a stateless frame with no hooks and no
dependencies beyond React, which is what the shell zone holds and what
the ledger's shell-root-and-frame entry targets. It moves to
shell/detail-panel.tsx, so the AppShell family loses one legacy file.

Two ownership entries no longer described their files:
- mixed-bootstrap-and-conversation-effects is now root-lifecycle-effects,
  since app-shell-effects.ts holds only root lifecycle hooks.
- app-shell-copy.ts, which is command-palette error copy used by
  app-shell-command-actions.ts, moves to commands-and-overlays.

Refs apache#4582

Generated-by: Claude Opus 5.5
…owners

The retained-root table is the residual inventory R2 closes on, so every
row should match the code. An audit of the rows that stay at the root
found wrong consumers (the memory indicator is the transcript's
session-context chip, not the titlebar; the rail reads go to the
transcript and titlebar, not the command palette), wrong owners
(useSystemUiLocale, useAppShellNavRefSync, useStableActions and useToast
live in legacy files or @maka/ui) and stale capabilities
(useShellAppearance writes no settings; ConversationLifecycle, not the
transcript, bumps the pet counter). Those rows now name what the code
does.

The M3 rows are left to the M3 owner, which retires them.

Refs apache#4582

Generated-by: Claude Opus 5.5
…ined

The "Transitional feature exports outside Conversation" table scheduled
OverlaysConsumer and ManualDiagnosticReportConsumer for M5, with the
legacy command actions. Neither can leave the root within R2.
app-shell-overlays.tsx composes the legacy Settings surface, which it
imports lazily, and the palette command list that apache#4997 deliberately
kept as a shell injection point; feature zones may not import legacy
code. The manual report is the diagnostics owner's command handed to the
shell-built palette options. Both rows now record why they stay.

Refs apache#4582

Generated-by: Claude Opus 5.5

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 03c30db74e757b434f1e78b960624c38b6d45b26 (21 files, +158/−168).

No P0–P3 findings.

Shape: this is a shrink, which is what closing an R2 slice at the AppShell root should look like. Of the 21 files, 17 are production and they net −75 (+79/−154). The two largest movements are both reductions — app-shell.tsx +7/−35 and use-project-context.ts +5/−33 — workhub-root.tsx and session-navigation/ports.ts each lose a line, and app-shell-detail-panel.tsx moves to shell/detail-panel.tsx as a pure rename (0/0), which is literally the "close it at the root" shape. The one file that grows meaningfully is onboarding-authority.ts (+29/−1), the authority taking over what the root was doing — the same pattern the earlier slices in this series used.

Both ledgers only tighten, which is the criterion this series is held to: renderer-architecture.json +10/−28 (net −18) and scripts/check-app-shell-hooks.mjs 0/−1. Nothing is loosened.

Gate on this head: test is not finished (null when I looked), so I make no CI claim. Note that the previous head of this PR (d084b62d) had already passed test and label; this revision is the drifted one I declined to review earlier and which has now been re-frozen, so it needs its own verdict. mergeable is true and the base is current main.

What I could not judge

  • The internal correctness of what moved into onboarding-authority.ts and out of the two shrinking files; this pass verified the shape, the ledger direction and the run's state.
  • No Electron run.

I did not approve, request changes, or merge.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

…osure

# Conflicts:
#	apps/desktop/renderer-architecture.json

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review (Claude lineage). This was not written by a human, and it does not approve the PR. Please verify before acting.

Incremental review of 3c9bf72e against 03c30db7. The only new commit is a merge of main at f7633c3d (#5826). There are no new code commits.

Verdict: no P0–P3.

  • Ledger conflict. I compared the merge tree with git merge-tree's automatic result. The only manual resolution is in renderer-architecture.json: the app-shell.tsx metrics, set to 57 import specifiers and 7852 tokens. CI's strict-base check accepts the regenerated values.
  • app-shell.tsx. Main's two changes merged in automatically: executorSessionPending and Conversation.createExecutorSessionActivator(...). Typecheck passes in CI.

CI and status: test passes on 3c9bf72e. The PR is mergeable and is blocked only on review.

#5954 at 1e8cc72b: the two still conflict only in renderer-architecture.json, renderer/README.md and scripts/check-app-shell-hooks.mjs. app-shell.tsx merges automatically. No order is required. Whichever PR lands second should merge main again, keep both sides' rows, and regenerate the ledger.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at @Astro-Han's explicit request: no blocking findings in our automated reviews of this head, and CI is green.

@Astro-Han
Astro-Han merged commit 1c507e8 into apache:main Oct 3, 2026
1 check passed
chihumyum added a commit to chihumyum/maka that referenced this pull request Oct 3, 2026
…composer-read-side

apache#5952 (R2 M5 closure) and this branch both cut the AppShell hook gate
and rewrote retained-root rows:
- Hook gate: both removals apply. `useEffect` (apache#5952) and
  `useAppShellTurnPresentation` (this branch) are gone: 19 hooks / 24
  call sites, matching the merged tree.
- Retained-root table: apache#5952's rewritten rows are the base. This
  branch's removed hooks drop out, and its three cross-region rows
  replace apache#5952's M3 rows. Those rows no longer list the transcript's
  model labels and model picker, which apache#5952 removed from ChatView. The
  Session-workspace row names the Composer edits instead of the ref,
  and the bootstrap row drops the pending-ref cleanup this branch
  retired. No row is scheduled for removal.
- Exports table: apache#5952's two "stays" rows (OverlaysConsumer,
  ManualDiagnosticReportConsumer) and this branch's three are kept.
- Ledger: main's copy minus this branch's retired legacy paths and
  emptied ownership entry, regenerated.

Generated-by: Claude Opus 5.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants