Skip to content

refactor(desktop): move App Update controller below AppShell - #4498

Open
chihumyum wants to merge 2 commits into
apache:mainfrom
chihumyum:refactor/app-update-feature-scope
Open

refactor(desktop): move App Update controller below AppShell#4498
chihumyum wants to merge 2 commits into
apache:mainfrom
chihumyum:refactor/app-update-feature-scope

Conversation

@chihumyum

@chihumyum chihumyum commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Move the renderer-owned App Update lifecycle out of AppShell into a feature slice, with AppUpdateProvider as the sole production owner of useAppUpdateController.

  • compose update capabilities through a typed Desktop adapter and feature service port; AppShell now only mounts the Provider
  • project update state independently to About and Sidebar Footer, so progress events no longer invalidate navigation or the shell
  • keep the shared update contract type-only while preserving subscription-before-snapshot ordering, stale-result fences, and single-flight check/install behavior
  • add AST-backed boundary tests that pin the controller, Provider, adapter, and leaf readers, while rejecting raw or computed bridge access, internal imports, aliases, namespaces, re-exports, export-all, and dynamic imports

No user-visible copy, update protocol, or install behavior is intended to change.

Refs #4582 (supersedes #3439)

Performance evidence

Measured in Electron 43.4.1 with a production React bundle, the same renderer PID, and alternating legacy/scoped runs:

Controlled downloading-status workload AppShell owner Feature owner Change
React commits per push 1 1 unchanged
Rendered fibers per push 512 1 -99.8%
Pure renderer/controller busy JS 0.1329 ms/push 0.0068 ms/push -94.9%
Full IPC -> preload -> adapter -> Provider busy JS 0.2111 ms/push 0.0696 ms/push -67.0%

The structural run used 8 rounds x 40 pushes, the pure renderer run 9 alternating rounds x 1,000 pushes, and the full propagation run 7 alternating rounds x 200 pushes. The full path used a real BrowserWindow, Electron IPC, the production preload bundle, Desktop adapter, and App Update Provider; only the main status producer was a fixture.

These measurements cover controlled renderer update propagation. They do not claim faster updater downloads or installs, lower main-process/network/disk cost, total wall/FPS improvement, or an exact production AppShell fiber count.

Verification

  • npm run rebuild, npm run typecheck, npm run lint, and npm run format:check
  • App Update focused suites: 22/22
  • Desktop and UI full compiled test suites
  • renderer architecture fixtures: 61/61; AppShell hook gate: 42 hooks / 77 call sites
  • Astryx inventory: 239 files / 1 declared exclusion
  • Knip for apps/desktop and packages/ui, ASF headers, stale-dist, and git diff --check
  • the default full-workspace sweep's only local non-green signal was Node's experimental SQLite warning from a managed Storage child; with NODE_NO_WARNINGS=1, Storage passed 1,077 tests with 8 skipped
  • node scripts/desktop-real-window-smoke.mjs --programmatic-only --diagnostic-wait-ms 20000 passed against the built renderer: visible 1280x840 movable/resizable window, renderer ready, appFrame mounted, Search focused, dock visible, and no ErrorBoundary
  • manual real-window inspection after closing Search showed sidebar, tasks, conversation, and composer rendering normally without a crash or ErrorBoundary
  • built Settings Storybook play clicked "检查更新", observed "已是最新版本。", and reported no page or console errors

The real-window smoke used a source-built Electron app, not a packaged or signed installer; packaged updater download/install remains outside this PR's evidence.

Review focus

Please review the ownership guard and the two independent projections together: the feature should retain the only update subscription and bridge capability, while About and Sidebar Footer receive only the state needed at their render boundaries.

AI use

Select exactly one:

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

Tool(s) and scope: OpenAI Codex contributed substantive architecture design, implementation, tests, performance measurement, and verification. The human contributor reviewed the work and chose to submit it.

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

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 1, 2026
@chihumyum
chihumyum force-pushed the refactor/app-update-feature-scope branch from 628a5ec to d5cd6e9 Compare September 2, 2026 16:14
@chihumyum

Copy link
Copy Markdown
Contributor Author

@Astro-Han could you please review exact head d5cd6e9ab2e8b47f1b9e9eddd289c1302b658799?

I rebased it onto main@b714a392192961fef3d89d4358fb0a0858252804. The conflict resolution keeps the new Session Settings / Runtime Host composition alongside the App Update provider and regenerates the renderer architecture and Astryx ledgers from the combined source.

Exact-head local verification passes: rebuild; renderer architecture 71/71 plus the base ratchet; App Update 22/22; Desktop 1,982/1,982; UI 313/313; typecheck, lint, format, Desktop/UI Knip, ASF headers, AppShell hook scope, Astryx inventory; and the real Electron window smoke. The new hosted CI and Windows package checks are currently queued.

@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (c86da40d7, which includes #4571's About page rebuild). Exact head: 076fb612f579e0ed890b082b8e488bf134a9a793.

How the two real conflicts were resolved:

  • about-settings-page.tsx is refactor(desktop): rebuild the About page as labeled settings groups #4571's page verbatim; only update ownership changed. The status row is now AboutUpdateRow, rendered through AppUpdateAboutProjectionConsumer, and the page no longer calls window.maka.app.*. The dev-build decision (no status line) stays on the page as refactor(desktop): rebuild the About page as labeled settings groups #4571 put it.
  • aboutUpdateStatusDetail follows main's two-argument signature (no isDevBuild) and lives in the feature model; aboutChannelFacts stays in settings/about-update-status.ts because it is About copy, not update state. about-update-status.test.ts imports each from its owner.
  • Stories: About is back to main's dev-checkout form (its e2e asserts no update button); the previous play moved to a new AboutCheckForUpdates story on a packaged release. The story-side services read window.maka.app per call so the channel stories' updateStatus overrides reach the provider.
  • renderer-architecture.json and the Astryx inventory were regenerated.

Local verification on this head with Node 24: desktop test:dist 1988/1988, @maka/ui 313/313, typecheck (four projects), lint, format, check:renderer-architecture --base upstream/main, check:app-shell-hooks (42 hooks / 75 call sites), Astryx inventory, Knip (desktop, ui), ASF headers, git diff --check, and build:renderer — all green.

Automated update from Claude Code on behalf of the PR author.

@chihumyum
chihumyum force-pushed the refactor/app-update-feature-scope branch from 076fb61 to 750ee05 Compare September 2, 2026 18:40
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased again onto current main (ad18da42c, #4455). One real conflict, in packages/ui/stories/session-rail-harness.tsx: #4455 added the railSelection prop and forwards it to SessionRailProvider, this PR wraps that provider in SidebarUpdateProjectionProvider; the merge keeps both. The Astryx inventory was regenerated. Exact head: 750ee054c66babfe2107279a9bb3d979bbbaaf31.

Same local verification on this head with Node 24: desktop test:dist, @maka/ui test:dist, typecheck (four projects), lint, format, check:renderer-architecture --base upstream/main, check:app-shell-hooks, Astryx inventory, Knip (desktop, ui), ASF headers, git diff --check, build:renderer — all green.

Automated update from Claude Code on behalf of the PR author.

@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main 68cda0b54. The only non-generated part of the resolution is the ledger's hand-authored ownership policy: regeneration keeps policy fields from the base, so the app-lifecycle → app-shell-app-update.ts entry (this PR deletes that file) was removed again by hand, as in the original commit. Exact head: 8cc5de4fbd82d89aadddda60984c0a5989cdc27b; the full local verification is green on it.

Posted by Claude Code on behalf of the PR author.

@chihumyum
chihumyum force-pushed the refactor/app-update-feature-scope branch from 8cc5de4 to 8a30f75 Compare September 3, 2026 15:40
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased once more onto main b9748a773; the ledger was the only conflict, regenerated with the same hand-authored ownership edit replayed. Exact head: 8a30f7511ddf834be2526e6bca63a82a56e975da; the full local verification (desktop and ui suites) is green on it.

Posted by Claude Code on behalf of the PR author.

Astro-Han pushed a commit that referenced this pull request Sep 3, 2026
… namespaces through (#4590)

Nine renderer feature slices each restated the same twenty-line services context, and their Desktop adapters hand-wrote forwarders whose signatures equal the bridge methods they forward to. Every new slice paid that join cost again, and the copies could drift apart without any test noticing.

`application/contracts/feature-services.tsx` now exports `createServicesContext<S>(providerName)`, which returns the Provider and hook pair. Each slice's `services-context.tsx` shrinks to a few lines while keeping every exported name, type, and error message. Desktop adapters hand a bridge namespace through where the port is a structural subset of it (`sessions`, `newTasks`, `scheduledTasks`, `shellRuns`, `todo`, `attachments`), removing 33 identity forwarders. Adapter blocks that rename, guard, filter, or translate stay hand-written. Preload namespaces are plain objects with no `this` usage, so passing one through is runtime-safe, and the port types stay narrow, so nothing new is reachable from feature code.

`composition/desktop-feature-services.tsx` is untouched; the tenth slice from #4498 can adopt the factory after it lands.

No behavior change. Refs #4582

Generated-by: Claude Code
@github-actions github-actions Bot added effort/XXL Over 2500 readable lines and removed effort/XL Under 2500 readable lines labels Sep 3, 2026
ggbdpq pushed a commit to ggbdpq/maka that referenced this pull request Sep 4, 2026
… namespaces through (apache#4590)

Nine renderer feature slices each restated the same twenty-line services context, and their Desktop adapters hand-wrote forwarders whose signatures equal the bridge methods they forward to. Every new slice paid that join cost again, and the copies could drift apart without any test noticing.

`application/contracts/feature-services.tsx` now exports `createServicesContext<S>(providerName)`, which returns the Provider and hook pair. Each slice's `services-context.tsx` shrinks to a few lines while keeping every exported name, type, and error message. Desktop adapters hand a bridge namespace through where the port is a structural subset of it (`sessions`, `newTasks`, `scheduledTasks`, `shellRuns`, `todo`, `attachments`), removing 33 identity forwarders. Adapter blocks that rename, guard, filter, or translate stay hand-written. Preload namespaces are plain objects with no `this` usage, so passing one through is runtime-safe, and the port types stay narrow, so nothing new is reachable from feature code.

`composition/desktop-feature-services.tsx` is untouched; the tenth slice from apache#4498 can adopt the factory after it lands.

No behavior change. Refs apache#4582

Generated-by: Claude Code

Generated-by: GLM-5.3-Flash (ZCode)
@chihumyum
chihumyum force-pushed the refactor/app-update-feature-scope branch from 8a30f75 to a54ef20 Compare September 4, 2026 17:00
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main 2c948e4e3 (after #4315) and folded in the three follow-ups this PR was waiting on:

  • Registered with the controllerOwners guard: useAppUpdateControllerAppUpdateProvider (AppShell already mounts the provider directly, so no structural change was needed). The retired app-lifecycle → app-shell-app-update.ts ownership entry is removed again by hand, as before.
  • Adopted refactor(desktop): share the feature services context and pass bridge namespaces through #4590's shared services context: features/app-update/services-context.tsx is now the four-line createServicesContext form; exports and the error message are unchanged.
  • Absorbed feat(i18n): add Traditional Chinese locale #3853 (Traditional Chinese): the feature copy catalog is keyed zh-CN / zh-TW / en (the zh-TW strings are the ones feat(i18n): add Traditional Chinese locale #3853 had added to the shell catalog for these keys, which this PR removes from there along with zh-CN and en), localizedAppUpdateErrorMessage delegates to generalizedErrorMessageForLocale like the shell copy does, and aboutUpdateStatusDetail takes feat(i18n): add Traditional Chinese locale #3853's { isDevBuild, errorDetail } options so the About row localizes a raw updater error the same way main does. The AppShell conflict was the Session rail mount: main's Module Hub boundary form, minus the two update props this PR retires.

Exact head: a54ef209dfb5e7a568a10b0463e2a5b32f585492. Full local verification on it with Node 24: desktop test:dist, @maka/ui test:dist, typecheck (four projects), lint, format, check:renderer-architecture --base upstream/main (validating both registered owners), check:app-shell-hooks, Astryx inventory, Knip (desktop, ui), ASF headers, git diff --check, build:renderer — all green.

@chihumyum
chihumyum force-pushed the refactor/app-update-feature-scope branch from a54ef20 to 08c27ef Compare September 5, 2026 05:38
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main cbeb1a93c; the Astryx inventory was the only conflict and was regenerated (the ledger merged cleanly, with both registered owners and the retired ownership entry as before). Exact head: 08c27efda2d3960fc6ab37026158b17a565dd40c; the full local verification is green on it.

@chihumyum
chihumyum force-pushed the refactor/app-update-feature-scope branch from 08c27ef to 96a84dc Compare September 5, 2026 06:43
@chihumyum

Copy link
Copy Markdown
Contributor Author

The run on 08c27efda failed only in Desktop e2e, on session-workbar.spec.ts:54 ("the composer usage action opens Task trace in the right workbar"), with everything else green. That spec was rewritten by cbeb1a93c (the workbar shell rebuild this head sits on) and exercises the right workbar, which this PR does not touch; on this head it passes locally three times in a row (24/24 across the file). main's own run for cbeb1a93c is red in the same e2e job on a different spec (transcript-scroll-cost.spec.ts:239). Re-pushed with identical content to re-run; exact head 96a84dc5d884d71a238d0320e6b5e3c62ea08fcd.

@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 at 96a84dc. All four done-conditions from #4582 hold: the two effects, one state and two refs leave the shell render body (check-app-shell-hooks.mjs 14→12 / 15→14), the four window.maka.* update paths leave app-shell.tsx and the three in about-settings-page.tsx go with them, useAppUpdateController → AppUpdateProvider is in controllerOwners, and the same PR deletes app-shell-app-update.ts, the nine shell-copy keys, the updateReminder / onOpenUpdate threading through SessionRailChrome, and the second copy of AppUpdateStatus in preload and main. The hand-removed ownership entry is the one the checker would otherwise reject (check-renderer-architecture.mjs:2346), so the ledger edit is consistent with regeneration.

The case for the move is stronger than the earlier slices: main had two subscribers to the same status (AppShellContent and About), and the shell one fed download progress into a useState on the renderer root, which is what the 512 → 1 fibers number is measuring. It also fixes a real ordering bug on the About page: the old effect read before it subscribed with no fence, so a slow updateStatus() could overwrite a newer push. The revision counter and the subscribe-then-read test pin that. I compared the nine copy keys across zh-CN / zh-TW / en against the deleted shell-copy.ts entries: identical.

No P0–P2. Five small things, none blocking, take what you want in one pass:

  • app-update-projection-context.ts gives the About projection a silent default (inactiveAboutProjection), where module-hub-provider.tsx:118 throws Provider is missing. A stray About mount outside the Provider then sits on "not checked yet" forever and the button toasts. Make the desktop context nullable and throw on read; the packages/ui one can keep its default since it explicitly serves other hosts.
  • Three cases in app-update-boundary.test.ts add no constraint: exclusive owner and index/testing-only entry are what check-renderer-architecture.mjs:1865 and :2087 already enforce, and the "recognizes alias / namespace / re-export / dynamic import" case tests the helper against inline strings. Keep the cross-package sidebar binding, the raw-capability ban and the mount-point checks; drop those three (~70 lines). Same point we settled on #4315.
  • isDevBuild is dead in about-status-detail.ts (its own comment says there is no dev-build branch) and this PR threads it through a new AboutUpdateRow prop. Delete the option, the prop and the call site.
  • localizedAppUpdateErrorMessage in app-update-copy.ts only forwards to generalizedErrorMessageForLocale, the same shell it copies from shell-copy.ts. Call the original.
  • create-app-update-services.ts hands the feature the whole bridge.app namespace (icons, sessionProjectInfo, info included) and the adapter test asserts identity on it. create-goal-services.ts closes over the methods it needs; do the same here and drop the === assertion.

Two things before merge. The branch went CONFLICTING a few minutes ago because #4640 and #4823 both landed on renderer-architecture.json; please regenerate rather than hand-merge, and replay the ownership deletion. And test / package were still pending on this head when I read it; the approval stands across the rebase per CONTRIBUTING, but the exact head that merges needs green CI, including the session-workbar.spec.ts line that was red on main.

#4491 shares the ledger, app-shell.tsx, the hook gate and the Astryx inventory with this PR, so whichever lands second regenerates once; I would land this one first since it deletes more.

Manual acceptance still owed on a packaged build, both themes: About page across the eight update states with the percentage moving during downloading; the check button disabling and recovering, including closing and reopening Settings mid-check (the one semantic shift, since checking moved from the page to the Provider); the dev checkout still rendering no status row; the sidebar footer button appearing only for downloaded and versioned error; the install-failure toast and the running-tasks confirm, three locales.

Evidence boundary: static read at 96a84dc against main; no tests, build, Storybook or app run, so the fiber counts and suite results are the body's, not mine.

AI-assisted review: drafted with Maka; I verified the four done-conditions, the copy parity, the ownership edit and the checker overlap myself.

Make the About projection context nullable and throw on a read outside
AppUpdateProvider, drop the three boundary cases the architecture checker
already proves, remove the dead isDevBuild option and prop, call
generalizedErrorMessageForLocale directly instead of a forwarding wrapper,
and hand the feature only the five update capabilities its port names
instead of the whole app bridge namespace.
@chihumyum
chihumyum force-pushed the refactor/app-update-feature-scope branch from 96a84dc to 17ed533 Compare September 5, 2026 09:54
@chihumyum

Copy link
Copy Markdown
Contributor Author

Rebased onto main efbcc6c08 (ledger regenerated, the owner registration and the ownership deletion replayed) and took all five items in one follow-up commit, 17ed533d2:

  • The About projection context is nullable and AppUpdateAboutProjectionConsumer throws AppUpdateProvider is missing on a read outside the Provider; the packages/ui sidebar context keeps its default since it serves other hosts.
  • The three boundary cases the checker already proves are gone (escape-hatch helper, exclusive owner, index/testing-only entry); the sidebar binding, the raw-capability ban, and the mount-point checks stay.
  • isDevBuild is deleted from aboutUpdateStatusDetail, AboutUpdateRow, and the call site; the options object is now just { errorDetail? }.
  • localizedAppUpdateErrorMessage is gone; the Provider calls generalizedErrorMessageForLocale directly.
  • create-app-update-services.ts closes over the five capabilities the port names instead of handing the feature bridge.app; the adapter test now asserts each call reaches the bridge and the exact key set, instead of identity.

Exact head: 17ed533d2db16108ff85aae8d31ca46eb3b095b2. Full local verification with Node 24: typecheck (four projects), lint, format, check:renderer-architecture --base upstream/main, check:app-shell-hooks, Astryx inventory, Knip (desktop, ui), ASF headers, git diff --check, build:renderer, @maka/ui test:dist — green; desktop test:dist 2181/2182, where the one failure is shell-env.test "kills login-shell descendants when capture times out", a main-process case this PR does not touch that fails identically on plain main in my environment today after passing earlier in the day, so CI arbitrates.

The packaged-build manual acceptance you listed is still owed and is on the author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XXL Over 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants