apache / apache/maka

refactor(desktop): make AppShell a composition root, R2 (migrate by call site)

Open
#4,582 7 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
5.4k
Forks
502
Avg merge
1d 2h
Merged PRs (30d)
715

Description

## Why a new issue

#3439 was closed when the R1 foundation (#4088, corrected by #4249) merged, with 9 of its 12 checklist items still open. Four controller migrations (#4315, #4316, #4491, #4498) and the Usage slice (#4425 / #4440) currently reference a closed issue. This issue is the successor and carries the remaining work.

The definition of done changes, following the evidence Astro-Han posted on #3439 and #4109: extracting a feature into `features//` does not change the scope of any state. A slice is **done** when

1. none of its hooks is called in the render body of `AppShell` or `AppShellContent` (the entry disappears from `scripts/check-app-shell-hooks.mjs`, or shrinks to a single equality-selected read that the shell genuinely needs);
2. its `window.maka.*` paths are gone from `app-shell.tsx` (the `bridgePaths` entry in `renderer-architecture.json` shrinks);
3. it is registered as a `controllerOwners` entry once #4315 lands;
4. the same PR deletes the prop threading and helpers the move makes redundant.

Not a fifth condition, but a convention worth following where it is cheap (Astro-Han, below): a migrated slice is testable without Electron — its `ports.ts` names the capabilities and `testing.ts` ships fakes — so when a slice moves, move the E2E coverage that only exists because the behaviour lived in the shell into the slice's own tests in the same PR. `goals` (#4316) is the finished example; `features/` has 12 slices and no test files yet. This also orders the list: **WorkHub router** unlocks `workhub-layout` and `workhub-reconstruction`, and **Conversation host** covers `streaming-remount` (where #4664 sits). Layering background: #4727.

## Baseline (upstream/main `6c8e749d3`, 2026-09-03)

| | |
|---|---|
| `app-shell.tsx` | 3,324 lines, 105 imports |
| hooks in `AppShell` + `AppShellContent` render bodies | 42 hooks / 78 call sites (`check:app-shell-hooks`) |
| `window.maka.*` paths in `app-shell.tsx` | 32 distinct / 33 references (ledger `bridgePaths`) |
| feature slices | 9 (`connection-settings`, `goals`, `module-hub`, `runtime-host-management`, `session-collaboration`, `session-navigation`, `session-settings`, `task-entry`, `workbar`) |

For reference, `app-shell.tsx` had 23 bridge references when #3439 opened; the five extraction PRs did not stop new product work (#3427, #3497, #3635, #3764, #3798) from landing bridge calls in the shell. The R1 ratchet has held the count since 2026-08-30.

### Progress

| | 2026-09-03 (`6c8e749d3`) | 2026-09-05 (`cbeb1a93c`) |
|---|---|---|
| `app-shell.tsx` | 3,324 lines, 105 imports | 3,293 lines, 101 imports |
| hooks in `AppShell` + `AppShellContent` | 42 hooks / 78 call sites | 41 hooks / 77 call sites |
| `window.maka.*` paths in `app-shell.tsx` | 32 distinct / 33 references | 32 distinct / 33 references |
| registered `controllerOwners` | none | `useModuleHubController`, `useTaskEntryController`, `useAppUpdateController` |
| feature slices | 9 | 12 (`usage` from #4440; `conversation` and `network-proxy` landed from other work) |

Merged so far under this issue: #4440, #4590, #4315, #4491, #4498, #4905. Merged 2026-09-08: #4905 (the two P3s from #4498's re-review, as `994a80d79`). Open: #4997 (Commands / overlays), #4584 (unreviewed).

## Checklist

Each item names the hooks and bridge paths that must leave `AppShell`/`AppShellContent`. Claim an item by commenting; one slice per PR.

### Controllers already extracted, call site still in the shell

- [x] **Module Hub** — `useModuleHubController` → #4315 (merged 2026-09-04; first `controllerOwners` registration)
- [x] **Goals** — `useGoalController` → #4316 (merged 2026-09-01)
- [x] **Task Entry** — `useTaskEntryController` → #4491 (merged 2026-09-06 as `227e9d50e`; `TaskEntryRoot` owns the controller and is registered with the guard)
- [ ] **Workbar** — `useWorkbarController`; `WorkbarHost` still receives `model` from the shell → claimed by @theSatvik, #4692 (open)
- [ ] **Session Navigation residue** — `useSessionNavigationReads` (a selector read; keep only if the shell body still needs it after Conversation/Session controls move)

### Shell-owned state that has no slice yet

- [x] **App Update** — inline `useEffect`/`useState`; bridge `app.installUpdate`, `app.retryUpdateDownload`, `app.subscribeUpdateStatus`, `app.updateStatus` → #4498 (merged 2026-09-06 as `00cada01c`; `AppUpdateProvider` registered with the guard, services context on the shared factory; About page re-merged onto #4857 so the row reads `aboutUpdateRow` through the feature projection; the About row's missing-provider test and comment trim landed in #4905 on 2026-09-08)
- [x] **Usage settings** — legacy `settings/usage-settings-page.tsx` → #4440 (merged 2026-09-03; extraction only, the pricing editor stays with #2015)
- [ ] **Conversation host / transcript runtime** — `useAppShellTurnPresentation`, `useShellLiveTurn`, `useActiveSessionEvents`, `useTurnActionRegistry`, `useActiveExecutionBoundary`, `useSessionEventHealthPolling`, `useShellRunUpdates`, `useShellResume`, `useShellMemoryPill`, `useShellChatModel`; bridge `transcripts` (2), `sessions.listActiveInteractions`, `sessions.subscribeActiveInteractions`, `sessions.listTurnLandmarks`, `sessions.compact`, `notifications.runEnded`
- [ ] **Task submission / composer** — `useTaskSubmissionReadiness`, `useComposerAttachments`, `useAppShellComposerQuotes`, `useNewTaskChoice`; bridge `attachments`, `attachments.readBytes`, `sessions.updateQueueEntry`, `sessions.retractQueueEntry`, `sessions.promoteQueueEntry`, `sessions.reorderQueueEntries`
- [ ] **Session controls** — `useSessionSettingIntent`, `useSessionCollaborationDialog`, `usePlanModeState`, `useSettingsModal`; bridge `sessions.getPlanState`, `sessions.abandonPlanProposal`, `sessions.setCollaborationMode`
- [ ] **Session catalog / workspace authority** — `useAppShellSessionWorkspace`, `useAppShellSessionUiReads`, `useAppShellProjectContext`, `useAppShellNavRefSync`; bridge `sessions` (whole namespace handed down)
- [ ] **WorkHub router** — bridge `workHub.resolveCoordinationSession`, `workHub.record`, `workHub.candidates`, `workHub.act`, `settings.getClient`, `settings.subscribeClientChanged`, `runtimeHostProfiles.subscribeChanges`, `connections.subscribeEvents` (coordinate with #3492)
- [ ] **App lifecycle / bootstrap** — `useAppShellBootstrapSubscriptions`, `useAppShellHostEffects`, `useAppShellPersistenceEffects`, `useOnboardingSnapshot`, `useShellConnections` (3), `useShellAppearance`, `useSystemUiLocale`; bridge `diagnostics.copyReport`, `diagnostics.copyPreviousMainProcessInterruption`, `diagnostics.takePreviousMainProcessInterruption`, `onboarding.setMilestone`
- [ ] **Commands / overlays** — `useCommandPalette`, `useKeyboardHelp`, `useShellSearch`, `useSettingsModal` → #4997 (open; `OverlaysRoot` registered with the guard, gate inventory 40 → 36 hooks). `useToast` stays until its 45 shell call sites move with their slices
- [ ] **Raw shell state** — `useState` (15 + 2), `useEffect` (14), `useLayoutEffect` (2), `useStableActions` (6) converge to the region/provider that reads them; this item closes last

### Shared infrastructure (do before the next three slices)

- [x] **Collapse the per-slice join cost** (Astro-Han, #3439). Provide one `createServicesContext(name)` outside the renderer zones (checker treats `packages/ui` and `src/shared` as external) and reduce Desktop adapters to `{ ns: bridge.ns }` or `{ ...bridge.ns, }`. Measured on `6c8e749d3` with `tsc -p tsconfig.renderer.json`: 81 of 119 adapter forwarders are identity. Replacing identity forwarders with `...bridge.` and keeping the real adaptation verbatim produced zero new type errors for `goals`, `module-hub` (`skills`, `scheduledTasks`), `session-navigation`, `task-entry`, and `workbar` (`terminal`, `todo`, `browser`, `attachments`); together that is about 80 lines removed across 5 adapters. The blocks that mix identity forwarders with multi-line adaptation (`workbar` `review`/`artifacts`/`inspector`/`sideChat`, `module-hub` `dailyReview`, `runtime-host-management` `peerMesh`) need a hand pass; `connection-settings`, `session-collaboration`, and `session-settings` do real adaptation and stay as they are. Bridge namespaces are plain objects (no `this` in preload), so passing them whole is runtime-safe. **Done in #4590 (merged 2026-09-03):** `createServicesContext` lives in `application/contracts`, every slice context is four lines, and adapters hand a bridge namespace through where the port is a structural subset (33 identity forwarders removed). Blocks that rename, guard, or translate stay hand-written: the adapter tests drive Proxy-based bridge recorders with no own keys, so a spread would copy nothing.
- [ ] **One gate for AppShell hooks** (needs @Astro-Han's agreement — the hook gate's no-`--write` friction is deliberate). Proposal: the ledger gains a component-scoped `hookCalls` for `AppShell`/`AppShellContent` with exact-match semantics, `controllerOwners` (#4315) is the provenance authority, and `scripts/check-app-shell-hooks.mjs` retires. Until then both gates run in CI.
- [ ] **Ratchet hardening** (follow-ups to #4249): fail instead of warn when the base worktree cannot be materialized under `CI`; run the base commit's checker against the head tree as a cross-check so a PR cannot lower both sides by weakening a rule; add the two-commit git-fixture test #4249 deferred. → #4584 (open at `13ac30d42`; reviewed by @Astro-Han on 2026-09-08, the P2 withdrawn as outside the stated scope, the README P3 taken). Follow-up not in #4584: the cross-check protects measurement only. A comparator-only change (relax `validateMonotonicDebt` on the head, add one legacy file, regenerate) still passes `--strict-base`; running the comparison under the base module's own entry point would close it, and @Astro-Han's exact-head git fixture reproduces the false pass.

## Invariants (unchanged from #3439)

No product, visual, IPC, storage-schema, copy, or shortcut changes. Composer mount semantics, bootstrap behavior, streaming isolation, Session-switch races, and Workbar resource lifecycles stay compatible. Performance evidence, where given, is measured in one running Electron instance with alternating configurations and paired trials, and the claim is bounded to what was measured.

## Non-goals

No new global state library or service locator; no IPC redesign; no `packages/ui` extraction; no mechanical moves into `features/` that leave the hook call site in the shell.

Contributor guide

Open the contributing guide

Research direction

Start with app-shell.tsx and the checklist item you claim; inspect the named hooks and bridge paths, then read scripts/check-app-shell-hooks.mjs and renderer-architecture.json. Run the shell-hook checker and relevant tests before changing one slice. Done means the slice no longer calls hooks or window.maka paths from AppShell, is registered in controllerOwners, and has redundant prop threading and helpers removed.

Written by the indexing model from the issue text.

Assessment

Tech stack
electron, typescript
Domain
desktop, developer-experience, frontend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.