refactor(desktop): move Goal controller ownership below AppShell by chihumyum · Pull Request #4316 · apache/maka

chihumyum · 2026-08-31T06:05:50Z

Summary

Move production Goal controller ownership out of AppShell and into GoalProvider.

Publish separate composer, indicator, and dialog projections at their actual reader boundaries.
Tie the projection transport to the authoritative Composer / ChatView prop contracts and perform explicit, compile-checked handoff at the two authorized Desktop leaves.
Keep WorkHub and Workbar isolated: their Composer / ChatView instances do not consume the Session Goal projections.
Remove Goal controller/model reads and Goal prop plumbing from AppShell; keep useGoalController out of the production barrel.
Prevent ownership return with unique-controller, unique-production-consumer, forbidden-hook, leaf-Omit, AppShell Hook-budget, and renderer-architecture guards.
Rebase onto 920d7142d and regenerate the Astryx 0.5.2 surface inventory while resolving the documentation conflict.

This intentionally changes one transient UI state: while the selected Session is not yet owner-backed by the hydrated catalog, the unusable Set Goal... entry stays hidden instead of briefly appearing and doing nothing. Goal business behavior otherwise remains in use-goal-controller.ts unchanged. Stabilizing the producer of reportError is left to #4315, where the duplication is visible.

Performance evidence

Measured on final head 3b47b2c35 against 920d7142d in a 12-Session fixture. Each workload ran legacy/scoped ownership as alternating pairs inside the same Electron process; root-mode changes and resynchronization stayed outside the measurement window. The controller-only workload used 6 pairs and the complete goal.pause workload used 12. Renderer profiling sampled at 100 microseconds; the Fiber probe was disabled during each busy-JS window. Values below are conventional medians.

Workload	Legacy owner	Scoped owner	Change	Paired result
Controller-only token update: rendered fibers	629	49	-92.2%	6/6 improved
Controller-only token update: renderer busy JS	5.016 ms	1.061 ms	-78.9%	6/6 improved
Controller-only token update: commits	1	1	unchanged	6/6 tied
Complete `goal.pause`: renderer busy JS	24.879 ms	23.637 ms	-5.0%	9/12 improved
Complete `goal.pause`: summed rendered fibers	3,053.5	3,063	+0.3%	6/12 improved
Complete `goal.pause`: commits	6.5	7	+7.7%	2/12 improved, 4 tied

The controller-only result is strong and stable: both ranges are disjoint (629-629 vs 49-49 fibers; 4.199-5.633 vs 0.769-1.363 ms), and every pair improved.

The complete pause workload covers preload -> IPC -> Host -> broadcast -> get -> controller -> React. Its busy-JS ranges overlap (21.848-29.949 vs 20.729-26.598 ms), while commits and fibers did not improve. The performance claim is therefore limited to controller-owned renderer work. The complete-path result is directional only and does not establish an end-to-end pause speedup; renderer busy JS also excludes Electron main-process CPU and I/O.

The temporary dual-owner switch, probes, performance spec, results, and worktrees were removed after measurement and are not part of this PR.

Verification

npm run rebuild - passed, including the production renderer build.
npm --workspace @maka/desktop run test:dist - 1,845/1,845 passed.
npm exec -w @maka/desktop -- playwright test --config e2e/playwright.config.ts e2e/goal-dialog-budget.spec.ts --workers=1 --reporter=line - 1/1 passed.
npm run typecheck - passed.
npm run lint and npm run format:check - passed (3,055 linted / 1,833 formatted files).
npx knip --workspace @maka/desktop --no-progress and npx knip --workspace packages/ui --no-progress - passed.
npm run check:app-shell-hooks - passed at 42 Hooks / 80 call sites.
npm run check:renderer-architecture -- --base 920d7142d001e81868bcd50a28a5bb3898415e3b - 61/61 fixtures passed.
npm run astryx:surface-inventory - passed at 235 files / 1 exclusion.
NODE_NO_WARNINGS=1 npm --workspace @maka/storage run test:dist - 1,077 passed / 8 skipped.
git range-diff, git diff --check upstream/main...HEAD, git merge-base --is-ancestor upstream/main HEAD, and git merge-tree --write-tree --messages upstream/main HEAD - passed.

Screenshots are not applicable; the only visible change removes a transient unusable menu entry during catalog hydration.

AI use

Select exactly one:

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

Tool(s) and scope: Codex - renderer architecture, implementation, tests, performance measurement, review handling, conflict resolution, and documentation.

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

chihumyum · 2026-09-01T05:39:08Z

@Astro-Han Hi, can you check this out? Thanks a lot

Astro-Han

Thanks — this lands cleanly, and the performance section is the most disciplined I've read here in a while. It reports that the complete goal.pause path did not improve (overlapping busy-JS ranges, commits and fibers slightly worse) and explicitly limits the claim to controller-owned render scope. That honesty is what makes the controller-only result — 629 → 63 fibers, 6.6ms → 1.6ms busy JS, disjoint ranges, 6/6 paired — actually persuasive.

The design is right too. Three separate contexts rather than one is exactly what earns the reduction: a composer-only change must not wake the indicator. And passing children through untouched so React bails on the unrelated frame is the correct way to scope this without widening Goal state back to the root.

Reviewed at head 25f20ec6. Approving. One P2 I'd like fixed here, two P3s that don't need to be.

P2 — the cloneElement injection gives up a compile-time contract, and this PR is defining a pattern that will be copied.

GoalComposerTargetProps and GoalIndicatorTargetProps declare every injected prop optional, so ReactElement<GoalComposerTargetProps> is satisfied by any element — the compiler cannot tell that the child is the one that actually reads these props. onSetGoal originates in packages/ui/src/composer.tsx:449 and reaches the child through ChatComposerRegionProps extends Omit<ComponentProps<typeof Composer>, …>, so a rename or removal on that cross-package contract would silently turn the injection into a dead prop with no type error. Before this change, onSetGoal={…} at the JSX site was checked.

Today the hole is covered by goal-dialog-budget.spec.ts — the 设定 Goal… menu item renders only when props.onSetGoal is truthy (composer.tsx:1916), so a broken injection fails a real end-to-end path. That is genuinely good coverage and it's why this isn't blocking. But a compile error is cheaper than an Electron run, and #4315 is already reproducing this provider-plus-boundary shape; every further controller that moves below AppShell will copy it. Cheapest fix: require the injected keys in the target-props types, or type the child as ReactElement<ComponentProps<typeof ChatComposerRegion>>.

P3-1 — the composer's enablement gate changed from activeId to ownerActiveId. Old: activeId && localInteractionAvailable. New: canOpenDialog && activeSessionId, where activeSessionId is ownerActiveId = activeCatalogSession && !sharedSessionActive ? activeId : undefined (app-shell.tsx:367). When activeId is set but the session isn't in the catalog yet, the old code rendered an enabled 设定 Goal… whose openDialog returns early on an empty activeSessionId (use-goal-controller.ts:216); the new gate hides the entry instead. The new behavior is better — a dead menu item is worse than none — but the PR says "No intended user-visible behavior change". No code change needed; I'll say so in the squash message unless it was unintended, in which case restoring activeId is the fix.

P3-2 — the provider re-stabilizes reportError because the producer is unstable.showSessionError is a plain function declaration inside AppShellContent (app-shell.tsx:2620), so it gets a fresh identity every render, which is why GoalProvider needs the reportErrorRef + useLayoutEffect + useCallback trio. That's correct as written, but the workaround sits on the wrong side of the boundary, and #4315's provider repeats the same stabilization. If showSessionError's dependencies allow a useCallback, doing it once would let both providers take the prop directly. Out of scope here — worth settling on #4315, where the duplication becomes visible.

AI use: reviewed with Claude Code (diff reading, tracing the gate and prop-injection paths through AppShell, the goals controller, and the shared Composer contract). I verified ownerActiveId's definition, openDialog's early return, showSessionError's declaration form, and the E2E path that exercises the composer injection myself against the head SHA.

Move production Goal controller ownership into GoalProvider and expose reader-local projections for the composer, indicator, and dialog host. Keep AppShell free of Goal controller and model ownership, route composer and indicator state through separate UI-owned contexts consumed only by the authorized Desktop leaves, bind them at the authoritative Composer and ChatView props, and lock the boundary with architecture and render-scope coverage. The Set Goal entry now waits for an owner-backed active session instead of briefly exposing an unusable action before catalog hydration. Generated-by: Codex

chihumyum · 2026-09-01T09:15:43Z

Addressed the review feedback in 3b47b2c35:

P2 fixed: removed the structurally loose cloneElement injection. The two required projection transport shapes now derive from the authoritative Composer / ChatView prop contracts, and the authorized Desktop leaves hand the values to the real JSX props explicitly. Production-consumer uniqueness, hook absence, leaf Omits, and AppShell exclusion are guarded so WorkHub/Workbar cannot inherit the Session Goal later.
P3-1 documented as intentional: the unusable Set Goal... entry remains hidden until the selected Session is owner-backed after catalog hydration. The commit message, Summary, screenshot note, and behavior checkbox now say so.
P3-2 left to refactor(desktop): move Module Hub controller below AppShell #4315: producer-side reportError stabilization is unchanged here.

The branch is rebased onto 920d7142d; the Astryx inventory conflict was regenerated against 0.5.2 and passes at 235 files / 1 exclusion.

Final-head performance remeasurement preserves the bounded claim: controller-only rendered fibers 629 -> 49 (-92.2%) and renderer busy JS 5.016 -> 1.061 ms (-78.9%), both 6/6 paired wins with disjoint ranges. Full goal.pause busy JS was directional only (24.879 -> 23.637 ms, -5.0%, 9/12, overlapping ranges), while commits/fibers did not improve, so this is not presented as an end-to-end speedup.

Local final-head verification: rebuild/typecheck/lint/format/Knip passed; Desktop 1,845/1,845; storage 1,077 passed / 8 skipped; Goal E2E 1/1; renderer architecture 61/61; AppShell guard 42 Hooks / 80 call sites; clean merge-tree. Hosted CI is rerunning on the new head.

Astro-Han

Re-verified on head 3b47b2c35. The cloneElement injection is gone and both projection shapes derive from the Composer / ChatView prop contracts, so the P2 is closed.

…che#4316) AppShell owned the Goal controller, so every Goal state change re-rendered the shell and, through it, the whole chat surface. The controller's own token update re-rendered 629 fibers to repaint an indicator. Move production ownership into `GoalProvider` and publish the composer, indicator, and dialog projections separately, each at the boundary that actually reads it. The two authorized Desktop leaves take their projection from a context consumer and hand it to the real `Composer` / `ChatView` props, so the render scope stops at the subtree instead of the shell. The transport shapes are indexed off those components' own prop contracts, which makes the handoff compile-checked rather than structural, and the leaf `Omit`s remove the props from what the parent may pass at all. WorkHub and Workbar mount their own `Composer` / `ChatView` without the Session Goal projections and are unaffected. Goal business behavior stays in `use-goal-controller.ts`. AppShell loses its Goal controller and model reads along with the Goal prop plumbing, and `useGoalController` stays out of the production barrel. Unique-controller, unique-production-consumer, forbidden-hook, leaf-`Omit`, AppShell Hook-budget, and renderer-architecture guards keep ownership from returning. Controller-owned renderer work improves decisively: rendered fibers 629 -> 49 (-92.2%) and renderer busy JS 5.016 -> 1.061 ms (-78.9%), every pair improved with disjoint ranges. The complete `goal.pause` path is directional only (-5.0% busy JS, overlapping ranges, commits and fibers flat), so this is not an end-to-end pause speedup. One transient UI state changes: while the selected Session is not yet owner-backed by the hydrated catalog, the unusable "Set Goal..." entry stays hidden instead of briefly appearing and doing nothing. Stabilizing the producer of `reportError` is left to apache#4315, where the duplication is visible. Generated-by: Codex

chihumyum · 2026-08-31T06:05:50Z