Skip to content

refactor(desktop): retire AppShell's direct Desktop bridge (R2 M5) - #5936

Merged
chihumyum merged 9 commits into
apache:mainfrom
chihumyum:refactor/transcript-attachment-bytes
Oct 3, 2026
Merged

chihumyum merged 9 commits into
apache:mainfrom
chihumyum:refactor/transcript-attachment-bytes

Conversation

@chihumyum

@chihumyum chihumyum commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

At c7fa6bb6a, the AppShell family still reached the Desktop bridge in 13 places across three files:

  • app-shell.tsx: 5 paths
  • app-shell-effects.ts: 7 paths
  • use-app-shell-session-list.ts: 1 path

This PR gives each one an owner. There is one commit per owner. Afterwards app-shell.tsx has no direct window.maka access, and the AppShell-family bridge count in the ledger goes from 43 to 30. The remaining 30 are the chat, command, project, revision, stop, turn and E2E-fixture actions, which other slices own.

Commit Bridge paths removed New owner / seam Who loses what
4f49aaf99 attachments.readBytes Conversation's attachment port. ComposerStagingServices gains readBytes; its Desktop adapter already returned bridge.attachments. StagedQuoteChatView reads it. AppShell loses the path and the onReadAttachmentBytes prop. ChatMessageSurface and StagedQuoteChatView drop the prop from their contracts, and a forced value is overridden.
5bd6132d3 settings.getClient, settings.subscribeClientChanged A WorkHub enablement authority (application/contracts/workhub-workspace/workhub-enablement.ts) with a Desktop source injected at composition. Workbar, the rail and the dock read it directly. WorkHubEnablementWatch hands AppShell only the on/off edges. AppShell loses the workHubEnabled state, its ref and its effect. Workbar's input loses workHub.enabled, WorkHubDock loses enabled, and the rail decides entry visibility itself.
602046260 onboarding.setMilestone An onboarding authority (application/contracts/onboarding/onboarding-authority.ts). It owns the poller, invalidations, refresh and the skip command. Desktop source: create-onboarding-source.ts, formerly onboarding-snapshot-bridge.ts. The rail reads send outcomes from it. AppShell receives a read-only projection through OnboardingProjectionRoot. AppShell loses useOnboardingSnapshot (and the poller with it), the bridge write, and the sessionSendOutcomes prop it threaded to the rail.
6d5500928 sessions.list A SessionCatalogSource injected into the session catalog at composition. use-app-shell-session-list.ts loses its bridge path.
093d29906 app.info, appWindow.subscribeCommand, connections.subscribeEvents, runtimeHostProfiles.subscribeChanges, sessions.subscribeChanges, settings.subscribeClientChanged, settings.subscribeExternalChanged ShellLifecycleSources (application/contracts/shell-lifecycle.ts) comes from a Desktop adapter that also writes data-os. ShellLifecycleSubscriptions is the single subscriber; Session changes come from the catalog's source. app-shell-effects.ts loses all seven paths and one effect. useAppShellBootstrapSubscriptions keeps the startup refreshes, hotkeys and mounted flag, and returns its handlers.
479ec0ecf sessions.listTurnLandmarks ConversationServices.sessions.listTurnLandmarks. The Desktop adapter already spreads bridge.sessions. ConversationLifecycle loses its listTurnLandmarks prop, so AppShell can no longer supply a reader.
deeff8d6b — Review follow-up (all four P3s from the automated review of 479ec0ecf), described below. —

Design decisions

  • Placement. WorkHub enablement and onboarding live in application-layer authorities. The issue author chose this; it follows the session catalog pattern. Each fact has readers in several features (enablement: Workbar, rail, dock; onboarding: rail, the shell's first-run gating, readiness), and features cannot import each other. The authorities read their source only while subscribed.
  • No hook renames. The renderer-architecture ratchet rejects any new hook name in an AppShell-family file, even one that replaces a removed hook. Retiring a hook therefore means moving the read out, not renaming it:
    • OnboardingProjectionRoot follows the render-prop root pattern of TaskEntryRoot and OverlaysRoot.
    • WorkHubEnablementWatch and ShellLifecycleSubscriptions follow CatalogRowWatch.
    • No AppShell or effects file gains a hook.
  • Retained root lifecycle. The bootstrap reactions remain a root application lifecycle. M5 item 3 allows this: they refresh sessions, settings mirrors, projects, memory and connections together. What moved is the environment access: one injected source and one subscriber.
  • Narrowest seams. readBytes and listTurnLandmarks use ports that already exist, and both Desktop adapters already supply the functions. Neither needed adapter code.
  • Providers are required (review follow-up). The onboarding, WorkHub-enablement and shell-lifecycle contexts throw when their provider is missing, like createServicesContext, instead of falling back to inert values. A catalog built without a source can still be committed to, but reading through its source rejects or throws. Tests pass explicit fakes; no story mounts these readers.
  • Testable handlers (review follow-up). The event-to-reaction mapping is the pure createShellLifecycleHandlers beside ShellLifecycleHandlers. The hook returns it, so the mapping is unit-tested without loading app-shell-effects.ts.
  • Direct contract import (review follow-up). AppShell imports WorkHubEnablementWatch from its contract, as it does OnboardingProjectionRoot and ShellLifecycleSubscriptions. features/workhub/index.ts is unchanged from main.

Behavior

There is no product, visual, IPC, storage, copy or shortcut change. Three internal differences:

  • The two settings-change handlers used to capture the first render's refreshConnections closure. They now refresh the current connection projections, as the Host-change handler already did.
  • A failed onboarding read is now a flag rather than localized text. That text was never rendered; only its truthiness was read. The snapshotErrorFallback onboarding copy key now has no reader. It stays in place so this PR changes no copy and does not collide with feat(i18n-ko): native surfaces and E2E fixtures #4515's ko catalog; removing it can be a follow-up.
  • The onboarding snapshot now outlives an AppShell remount, because the authority lives for the app and revalidates when a reader returns. Before, a remount started from null. The authority's code comment documents this.
  • The OnboardingSnapshot and DesktopOnboardingSessionUpdate types moved to src/shared/onboarding-snapshot.d.ts so the application layer can name them. Preload re-exports them unchanged.

Inventory (c7fa6bb6a → this branch; the branch is rebased on 1a66e4d5e, which adds only non-renderer files)

Before After
app-shell.tsx lines / import statements 2,019 / 79 1,991 / 80
Direct window.maka paths in app-shell.tsx 5 0
AppShell-family bridge references (legacyAppShell.files) 43 30
AppShell + AppShellContent hooks / call sites 28 / 42 27 / 39
nonTriviaTokens: app-shell.tsx / app-shell-effects.ts 9,080 / 1,361 8,903 / 886
controllerOwners 13 13
Legacy renderer files 190 189

Notes:

  • Gate entries removed: useOnboardingSnapshot, workHubEnabled (useState 8→7) and its useEffect (2→1).
  • controllerOwners unchanged: no React controller moved. The new owners are authorities created at composition.
  • Legacy file deleted: use-onboarding-snapshot.ts.

Retained root entries in this PR's scope

Proposed rows for the retained-root table in #5934:

Gate entry Consumer Owner Allowed capability Why at the root
useAppShellBootstrapSubscriptions startup refreshes, ⌘, / ⌘N, handlers for ShellLifecycleSubscriptions root application lifecycle no bridge; reacts to injected ShellLifecycleSources and the catalog source one event refreshes several regions
useAppShellHostEffects titlebar modal sync shell frame top-layer observation window chrome
useAppShellPersistenceEffects theme/palette application, navigation persistence appearance + navigation document theme, local storage appearance and navigation
useAppShellNavRefSync invocation-time navigation reads navigation ref mirror navigation
useState (workHubActive) dock visibility, chat surface, Workbar workspace, rail entry navigation local state navigation; enablement is the authority's
useShellConnections ×3 model controls and composer connections for the default, new-task and Session Hosts connection lifecycle (M5 item 3) snapshot reads; events via ShellLifecycleSubscriptions; onboarding seed three Host targets feed root chrome; extracting an owner is separate work
useAppShellSessionWorkspace catalog actions plus the Conversation target adapter session catalog + transitional Conversation adapter catalog source; transitional M3 adapter the transitional part retires with M3
useAppShellProjectContext project picker, titlebar, project actions workspace projection — project actions are slice C's; that slice decides between moving it and a row
useShellMemoryPill memory chip for the active Session chrome projection memory state reads chrome

Refs #4582

Merge with #5934 and #5935 (e542fbe28)

Merged upstream/main 255ae23ae (not rebased). Conflicts:

  • app-shell.tsx: imports only. Kept main's application-contract desktopSlashCommandAvailability plus this PR's onboarding and lifecycle imports.
  • The hook gate. Both sides' retirements apply: useNewTaskChoice, useTaskSubmissionReadiness and useOnboardingSnapshot are gone; useStableActions is 1 and useState 3.
  • composer-staging-owner.test.ts: imports only.
  • The ledger: main's copy, then --write.

Follow-ups the merge required:

  • Readiness refresh key. TaskReadinessProvider now reads the onboarding snapshot from the onboarding authority (useCurrentOnboardingSnapshot), so AppShell no longer passes refreshKey={onboarding.snapshot}. Its test harness delivers the key through a fake authority.
  • Retained-root table. Removed the rows for useOnboardingSnapshot, the workHubEnabled useState and the WorkHub-enablement useEffect. useAppShellHostEffects is now the titlebar modal sync only. useAppShellBootstrapSubscriptions is retained as an application lifecycle with no bridge access. The onboarding connection-seed useEffect and workHubActive rows stay.
  • refactor(desktop): own Composer readiness, new-task choices and submission below the shell #5935's composer-submission test. It stops passing the removed listTurnLandmarks prop to ConversationLifecycle.

On the merge: --strict-base against 255ae23ae passes. The hook gate is 25 / 30, AppShell-family bridge references are 20, and app-shell.tsx has 0. Typecheck, lint, format, both knip runs, both inventories and ASF pass, and desktop tests are 3,187 / 3,187.

Electron is 33 / 34. session-workbar.spec.ts:179 ("Terminal survives navigation and reload…", the post-reload toBeVisible at line 224) fails intermittently on main too, so it is not from this PR. Results of that spec alone, --repeat-each=8:

Head Failed
8ad836ce1 (main before #5934/#5935) 2 / 8
255ae23ae (current main) 4 / 8
63888f9fd (this PR before the merge) 1 / 8
e542fbe28 (this merge) 5 / 8, 8 / 15 overall

Review focus

Overlap with the other R2 slices. All of them edit app-shell.tsx, the ledger and the hook gate. Whichever lands second merges main and regenerates the ledger.

  • refactor(desktop): own Composer readiness, new-task choices and submission below the shell #5935 (Composer readiness). Expect manual merges in:

    • app-shell.tsx
    • scripts/check-app-shell-hooks.mjs (the useState line)
    • composition/desktop-feature-services.tsx (both add imports and providers)
    • features/workbar/controller/use-workbar-controller.ts, where both PRs edit the import block at lines 38–44

    chat-message-surface.tsx, the Conversation README, features/conversation/testing.ts and the slash-menu story are touched at non-adjacent lines. Its readiness can read the onboarding authority instead of a snapshot prop.

  • chore(desktop): check R2 root symbol uses and retained root hooks #5934 (root symbol allowlist and retained-root rows). The new root symbols here, all from application contracts, need allowlist entries: WorkHubEnablementWatch, OnboardingProjectionRoot, getOnboardingActivationCandidate and ShellLifecycleSubscriptions. features/workhub/index.ts is no longer touched, so that textual conflict is gone.

    The rows above are proposed for its table. If chore(desktop): check R2 root symbol uses and retained root hooks #5934 lands first, the main merge here must also update its table:

    • delete the rows for useOnboardingSnapshot, the workHubEnabled useState and the WorkHub-enablement useEffect;
    • keep the onboarding connection-seed useEffect, which now reads the onboarding prop, and workHubActive;
    • update the useAppShellHostEffects row, since its app.info read moved to ShellLifecycleSources and only the titlebar modal sync remains;
    • shrink rootSymbolUses.
  • The new src/shared/onboarding-snapshot.d.ts. Declaration files are outside the checker's source index, so it adds no legacyAppShellClosure entry. --strict-base passes against both c7fa6bb6a and 1a66e4d5e.

  • perf(desktop): bound transcript work on session switches #5712 (testikun).

    • No textual overlap in conversation-lifecycle.tsx: it edits lines 109–117, this PR lines 46 and 149.
    • One line overlaps in conversation-owner.test.ts:111, where the harness now injects listTurnLandmarks through stub services instead of a prop. Its rebase needs that one-line change.

External PRs. Not designed around, as the issue now records:

Naming. ComposerStagingServices now also serves a transcript read. Renaming it (e.g. ConversationAttachmentServices) can be a follow-up.

Verification

Run locally on Node 24.19.0 at deeff8d6b (on 1a66e4d5e), after a real npm install, node scripts/apply-dependency-patches.mjs, node scripts/install-electron-with-retry.mjs and npm --workspace @maka/desktop run build:workspace-deps:

  • Desktop tests. npm --workspace @maka/desktop run clean:main && … build:test && … test:dist: 3,165 / 3,165 pass (main has 3,143; 22 new). The readBytes tests and the rail WorkHub-entry test were checked by reverting the behavior. The other new suites exercise modules this PR adds, and their source checks fail on the base files. The new or changed suites:
    • composer-staging-owner.test.ts: transcript bytes through the port; a forced prop is overridden; the adapter; AppShell is free of the path.
    • workhub-enablement.test.ts: the authority reads only while subscribed; a failed read keeps the value; StrictMode edges; the adapter.
    • session-navigation-controller.test.ts:
      • the WorkHub entry is shown only while the switch is on and re-checked on select;
      • stale rows come from the onboarding authority.
    • onboarding-authority.test.ts: the poller suite moved here; plus the authority lifecycle, the failure flag, skip then refresh, the projection root, the Desktop invalidations, and AppShell without setMilestone.
    • session-catalog-source.test.ts: refresh through the catalog source; the adapter.
    • shell-lifecycle.test.ts:
      • one subscriber with the latest handlers and full cleanup;
      • createShellLifecycleHandlers routes each event to the same refreshes as before, including a Host that is not ready versus a ready default Host;
      • a missing provider throws;
      • the adapter's data-os cancel;
      • app-shell-effects.ts and app-shell.tsx contain no window.maka.
    • Missing-provider and detached-source cases in workhub-enablement, onboarding-authority and session-catalog-source.
    • conversation-owner.test.ts: landmarks injected through services only.
  • Electron E2E. npm --workspace @maka/desktop run build:with-deps && npx playwright test --config e2e/playwright.config.ts: 34 / 34 pass on 479ec0ecf (3.7 min) and again on the review follow-up deeff8d6b (4.0 min). This includes workhub-layout, workhub-reconstruction, workhub-pending-question, settings, session-local-recovery and streaming-remount. The run was on the same tree before the rebase onto 1a66e4d5e, whose 5 files are outside the renderer and Electron app.
  • Type closure. Re-adding onReadAttachmentBytes={window.maka.attachments.readBytes} to app-shell.tsx fails the renderer typecheck with TS2322.
  • Typecheck. npm run typecheck (preload, main, renderer, storybook): pass.
  • Architecture ledger. node apps/desktop/scripts/check-renderer-architecture.mjs --write, then npm run check:renderer-architecture -- --base 1a66e4d5e22067afd986d43fc634159e307a8e31 --strict-base: pass. It also passed against c7fa6bb6a before the rebase.
  • Other gates, all pass or current:
    • npm run check:app-shell-hooks: ok, 27 / 39; inventory edited by hand.
    • npm run lint, npm run format:check.
    • npx knip --workspace apps/desktop, npx knip --workspace packages/ui.
    • npm run windows:inventory (current, 119), npm run astryx:surface-inventory (ok), npm run check:asf-headers.

Not covered directly:

  • Storybook smoke (smoke:storybook) was not run. The two stories changed only in their service fixtures, and tsconfig.storybook.json typechecks.

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. The author chose the WorkHub enablement and onboarding placement and the landmark timing, 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 (internal only: the settings-change handlers use the current render; no product, visual, IPC or copy change)
  • No

…rsation port

AppShell passed window.maka.attachments.readBytes to the transcript as a
prop. Declare readBytes on Conversation's attachment port
(ComposerStagingServices), whose Desktop adapter already returns the
bridge's attachments namespace, and have StagedQuoteChatView, the
feature's transcript ChatView, take the reader from it. The reader
contracts of ChatMessageSurface and StagedQuoteChatView drop the prop,
so AppShell can no longer supply one; app-shell.tsx goes from five
direct bridge paths to four.

Refs apache#4582

Generated-by: Claude Opus 5.5
AppShell read the client WorkHub switch from the Desktop bridge into its
own state and passed it to Workbar, the session rail and the WorkHub dock.
Create a WorkHub enablement authority in application/contracts beside the
WorkHub workspace contract, with its Desktop source injected at
composition like the session catalog. Workbar, the rail and the dock read
it directly; the rail and WorkHub's main-window navigation check it again
before opening WorkHub. WorkHubEnablementWatch hands AppShell only the
on/off edges for the navigation it still owns (workHubActive and the
destination).

AppShell loses settings.getClient, settings.subscribeClientChanged, the
workHubEnabled state, its ref and its effect.

Refs apache#4582

Generated-by: Claude Opus 5.5
AppShell owned the onboarding snapshot: the poller, its invalidation
subscriptions, the refresh after Settings closes and the bridge write
behind "skip setup". Move the poller into an onboarding authority in
application/contracts, created at composition with a Desktop source
(the former onboarding-snapshot-bridge, now also writing the skip
milestone on the default Host). The session rail reads per-Session send
outcomes from it directly. AppShell receives a read-only projection plus
refresh and skip through OnboardingProjectionRoot, the same render-prop
pattern as the other shell roots, and derives first-run gating, the
default-Host connection seed and the activation candidate from it.

A failed read is now a flag. The localized error text was never shown,
so nothing that might carry paths or tokens is kept. The snapshot types
move to src/shared so the application layer can name them; preload
re-exports them unchanged.

AppShell loses onboarding.setMilestone, useOnboardingSnapshot and the
sessionSendOutcomes prop it threaded to the rail.

Refs apache#4582

Generated-by: Claude Opus 5.5
The shell's catalog refresh called window.maka.sessions.list itself.
The session catalog now takes a SessionCatalogSource (full lists and the
change feed), injected at composition from the Desktop session catalog
adapter, and the shell refreshes through catalog.source. A catalog built
without a source, as in tests and stories, is detached.

use-app-shell-session-list.ts loses its sessions.list bridge path.

Refs apache#4582

Generated-by: Claude Opus 5.5
…jected sources

app-shell-effects.ts subscribed to seven Desktop bridge paths directly:
app.info for the document's platform tag, the window menu, connection
events, Host profile changes, Session changes and both settings-change
feeds. The reactions stay a root application lifecycle, since they
refresh several regions at once, but the environment leaves the shell:

- ShellLifecycleSources (application/contracts/shell-lifecycle.ts) is
  supplied at composition by a Desktop adapter that also writes the
  data-os tag.
- ShellLifecycleSubscriptions is the one subscriber. It takes Session
  changes from the session catalog's own source.
- useAppShellBootstrapSubscriptions keeps the startup refreshes, the
  hotkeys and the mounted flag, and returns the handlers.

Every handler now reads the latest render. The two settings-change
handlers used to capture the first render's connection refresh; they now
refresh the current connection projections, as the Host-change handler
already did.

app-shell-effects.ts loses all seven bridge paths and one effect.

Refs apache#4582

Generated-by: Claude Opus 5.5
AppShell handed ConversationLifecycle an inline
window.maka.sessions.listTurnLandmarks reader. Declare listTurnLandmarks
on ConversationServices.sessions; the Desktop adapter already spreads the
bridge's sessions namespace, so it supplies the function unchanged. The
lifecycle passes it to the reading-position controller, and its prop is
gone, so AppShell can no longer supply a reader. The controller never
listed the reader as an effect dependency, so its identity becoming
stable changes nothing.

app-shell.tsx loses its last direct bridge path.

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 479ec0ecfac965d11a3af7d205331a7478490f23 (46 files, +1568/−681, 6 commits). The dispatch asked for a per-file pass on the retired bridge call sites — every former window.maka.* call must correspond item by item in its new owner — plus whether the ledger only tightens.

No P0–P3 findings.

The retired call sites. app-shell.tsx gives up five bridge paths, one each: attachments.readBytes, onboarding.setMilestone, sessions.listTurnLandmarks, settings.getClient, settings.subscribeClientChanged. Two of them (settings.getClient, settings.subscribeClientChanged) still have renderer callers elsewhere in the tree, so those removals are plainly a change of owner rather than a loss. The other three now have no call site anywhere under apps/desktop/src outside tests — which is the case worth stating precisely: the bridge methods are still referenced elsewhere in the repository (attachments.readBytes in 3 files, onboarding.setMilestone in 1, listTurnLandmarks in 12), so nothing appears orphaned at the contract level, and the description's per-commit table carries a "who loses what" column that declares the intent for each removal. I found no contradiction between that table and the tree; I did not attempt to prove that the three app-shell call sites were dead rather than merely unreferenced in the renderer.

The ledger only tightens, which is the property worth pinning. All three of those paths go from 1 to 0 in app-shell.tsx's ledger entry, the manifest diff is +14/−50 (net −36), and check-app-shell-hooks.mjs moves +2/−3 (net −1). Nothing is loosened, and the description's "43 → 30" for the AppShell-family bridge count is consistent with those numbers.

One thing worth knowing before merging these siblings. All four AppShell PRs share the base 1a66e4d5, so they are parallel rather than stacked. #5934 does not touch app-shell.tsx at all, so it cannot conflict on that file — but #5936 and #5937 overlap on it in three regions (@@ -204, @@ -240, @@ -249 in the AppShell / AppShellContent bodies), so whichever lands second will need a real merge there rather than a clean apply.

Gate on this head: test and label are green.

What I did not judge

  • Whether the three unreferenced app-shell call sites were dead or load-bearing before their removal; the description asserts the former and the tree does not contradict it, but I did not trace each capability end to end.
  • The internal correctness of the 7-path move in app-shell-effects.ts beyond the ledger accounting.
  • 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.

@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.

Independent review of 479ec0ecfac965d11a3af7d205331a7478490f23 (base 1a66e4d5e; it merges cleanly onto current main 887924e14).

Verdict: no P0, P1 or P2 found. Each of the 13 bridge paths reaches an owner, and none is dropped. I traced the three that no longer appear as a literal window.maka.* anywhere outside tests, end to end on this head:

  • attachments.readBytes (transcript image bytes, not sending). StagedQuoteChatView now reads useComposerStagingServices().readBytes (staged-quote-chat-view.tsx:29-30). createDesktopComposerStagingServices returns bridge.attachments whole, so it is the same preload function (preload.ts:3421, attachments:readBytes). ChatMessageSurface and StagedQuoteChatView drop the prop from their types, so the shell cannot reintroduce it.
  • sessions.listTurnLandmarks. ConversationLifecycle passes services.sessions.listTurnLandmarks (conversation-lifecycle.tsx:148). The Desktop Conversation adapter spreads ...bridge.sessions (create-conversation-services.ts:61), so it is the same preload function (preload.ts:2469). Both call sites (the index read with null, and lookupTurn) pass turnId explicitly, so the preload default does not matter. The reference is now stable where it used to be a fresh closure, and that does not affect the effect deps.
  • onboarding.setMilestone. The shell's onSkip calls onboarding.skipInitialOnboarding(), which goes to OnboardingAuthority.skipInitialOnboarding, then source.skipInitialOnboarding, then runOnDefaultRuntimeHost(host => bridge.onboarding.setMilestone('initial_onboarding','skipped',host)) (create-onboarding-source.ts:48-50). It then re-pulls, as onboarding.refresh() did. The try/catch and its toast in the shell are unchanged.

Other fidelity checks:

  • WorkHub enablement. The watch fires onEnabled on the first true read, which opens WorkHub and selects sessions at startup as becameEnabled did. It fires onDisabled (exitWorkHub) on a false edge. The old guard if (!workHubEnabledRef.current) return in openWorkHub moved to both of its callers: the rail entry re-checks isEnabled() on select, and WorkHubMainNavigation checks before onOpenWorkHub. No other caller of openWorkHub exists. A read that keeps reporting false no longer calls setWorkHubActive(false), but workHubActive cannot be true while the switch is off, so nothing changes in practice.
  • Lifecycle subscriptions. All six subscriptions are still made, with the same handlers. useEffectEvent gives the latest handler, which fixes the stale first-render closure in the settings mirrors as declared. Session changes now come from catalog.source, which wraps the same bridge.sessions.subscribeChanges. data-os tagging keeps its cancel.
  • Onboarding authority. The poller is moved verbatim. failed replaces only the truthiness of error, which was all the shell read.
  • Gate and ledger only tighten. useOnboardingSnapshot is removed, useEffect goes 2→1 and useState 8→7, and bridgePaths is now {} for app-shell.tsx, app-shell-effects.ts and use-app-shell-session-list.ts.

Local run on this head (build:test): the workhub-enablement, onboarding-authority, shell-lifecycle, session-catalog-source, session-navigation-controller, composer-staging-owner, conversation-owner, workbar-controller and onboarding-incremental-preload suites pass (116/116). CI is green.

P3: silent default contexts. OnboardingAuthority (IDLE), WorkHubEnablement (DISABLED), ShellLifecycleSources (DETACHED) and the catalog's DETACHED_SOURCE fall back to inert values when their provider is missing. Every other feature port uses createServicesContext, which throws. If a future composition drops OnboardingAuthorityProvider, snapshot stays null and failed stays false, so isOnboardingLoading stays true for a user with no Sessions and the launch overlay never clears. Nothing would fail loudly. Suggest throwing (at least in dev) when the provider is absent, and making tests and stories pass explicit fakes.

P3: undeclared lifetime change. The onboarding snapshot now outlives an AppShell remount because the authority is an app singleton. Before, a remount started from null. This is benign, and the code comment documents it, but the PR's "Behavior" list does not mention it.

P3: test gap, already acknowledged in the PR. The handler map returned by useAppShellBootstrapSubscriptions is untested. Moving that mapping into a pure factory next to ShellLifecycleHandlers would make it testable without fixing the extensionless imports.

P3: feature entry re-exports a contract. features/workhub/index.ts:28 re-exports WorkHubEnablementWatch from application/contracts. Under #5934's root-symbol rule, that attributes a contract component to the WorkHub feature entry. Importing it from the contract directly, as OnboardingProjectionRoot and ShellLifecycleSubscriptions already are, would be more honest.

Cross-PR (git merge-tree against 887924e14).

  • #5937: conflicts in renderer-architecture.json and in three app-shell.tsx hunks (@@ -204, -240, -249). Both PRs wrap AppShellContent in a render-prop root and add one prop, so the second to land nests both. I confirmed these are the only three conflict regions.
  • #5935: conflicts in app-shell.tsx, the ledger, scripts/check-app-shell-hooks.mjs and composer-staging-owner.test.ts.
  • #5934: a textual conflict in features/workhub/index.ts. #5934's checker will also need --write and the row edits listed in this PR's body.

This is an automated review by Claude (Anthropic), run on behalf of the maintainer. It is not an approval. Please verify the findings before acting on them.

Comment thread apps/desktop/src/renderer/features/workhub/index.ts Outdated
…he lifecycle handlers

Review follow-up for apache#5936:

- The onboarding, WorkHub-enablement and shell-lifecycle contexts throw
  when their provider is missing, as createServicesContext does, instead
  of falling back to inert values that would, for example, hold the
  first-run gate closed. A catalog built without a source still accepts
  commits but fails when read through that source. Tests pass explicit
  fakes.
- The event-to-reaction mapping moves into the pure
  createShellLifecycleHandlers beside ShellLifecycleHandlers, so it is
  unit-tested without loading app-shell-effects.ts.
- AppShell imports WorkHubEnablementWatch from its contract; the WorkHub
  feature entry no longer re-exports it.

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 deeff8d6b9c578c2c9799188cc24dc5ebf4b2718 (46 files, +1692/−697, 7 commits).

Both P3s from my previous pass are fixed, each verified in the code rather than from the reply.

  1. A missing provider no longer fails silently. The failure mode I flagged was that the onboarding authority's inert default meant a composition that forgot OnboardingAuthorityProvider would see snapshot: null, failed: false forever, so isOnboardingLoading would hold the launch overlay with no way to tell why. The authority now throws (OnboardingAuthorityProvider is missing), shell-lifecycle.ts throws for its own sources, and the feature-services helper throws ${providerName} is missing with a documented convention that the message stays <Feature>ServicesProvider is missing across slices. So the inert default is gone from all three, not only the one I named.
  2. The cross-PR symbol pollution is gone. My finding was that the WorkHub feature entry re-exported a component from application/contracts, which #5934's rootSymbolUses would then record as a WorkHub public-entry symbol even though WorkHub does not own it. At this head features/workhub/index.ts is byte-identical to main (empty diff), and AppShell imports WorkHubEnablementWatch directly from application/contracts/workhub-workspace/workhub-enablement.js. That removes the conflict in that file between the two PRs, which is what the dispatch asked me to check for this set.

The deep pass from my previous review is unaffected. The five retired bridge call sites, the ledger tightening (entries 1→0, manifest net −36, check-app-shell-hooks net −1) and the three sites that lost their last renderer caller while the bridge methods remain referenced elsewhere all still read the same; this head's increment is the fixes above.

Gate on this head: test and label are green; mergeable is true against the base branch, so the PR can still land on its own. Every commit carries a Generated-by: trailer and there is no Grok involvement.

What I could not judge

  • I did not execute the suites; the fixes are verified by reading the throwing paths, the empty diff against main, and the import site.
  • Whether the two sibling PRs still conflict on app-shell.tsx after this head — I measured three overlapping regions last round, and this change does not touch app-shell.tsx's structure, so I would expect the overlap to stand.

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.

…-attachment-bytes

# 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.

Reviewed 63888f9fd0c186555b758b562ef4c20eee571ccd.

The rebase is faithful, so my previous review's conclusions carry over unchanged. A git range-diff between the old and new ranges marks all eight commits of this PR as = — byte-identical — with the only differences being commits that came in from main. The tree difference I measured against my previously reviewed head was that base advancement alone. The gate is green on the new head.

Merge order and remaining conflicts (measured at the current heads, all four sharing one base): there is no overlap with #5934 beyond the generated ledger; against #5935 the overlap is twelve files, including app-shell.tsx, chat-message-surface.tsx, composition/desktop-feature-services.tsx, scripts/check-app-shell-hooks.mjs and features/conversation/testing.ts; against #5937 it is the ledger, app-shell-effects.ts and app-shell.tsx. So the #5934 → #5935 → #5936 → #5937 order is still required, and this PR should land after #5935 has been merged and its resolution reviewed — the two are ownership moves in the same file, not adjacent edits.

What I could not judge

  • Verified by range-diff and file sets; I did not run the suites or perform the merges.
  • The moved logic was not re-audited, since the rebase left it untouched.

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.

…-attachment-bytes

# Conflicts:
#	apps/desktop/renderer-architecture.json
#	apps/desktop/src/main/__tests__/composer-staging-owner.test.ts
#	apps/desktop/src/renderer/app-shell.tsx
#	scripts/check-app-shell-hooks.mjs
@chihumyum
chihumyum merged commit 9a2779b into apache:main Oct 3, 2026
1 check passed
chihumyum added a commit to chihumyum/maka that referenced this pull request Oct 3, 2026
Brings in apache#5936.

- app-shell.tsx: apache#5936 wraps AppShellContent in OnboardingProjectionRoot
  where this branch wraps it in ManualDiagnosticReportConsumer. Both
  are kept, the onboarding root outside, and AppShellContent takes both
  the onboarding projection and copyManualDiagnosticReport.
- renderer-architecture.json: taken from main's copy, the
  workspace-projection ownership entry removed again, and regenerated.
- README retained-root table: the useAppShellProjectContext row now
  describes the projection it still provides (project mutations and the
  folder commands are Task Entry's); the transitional-exports table
  lists ManualDiagnosticReportConsumer.

The hook gate matches the merged tree unchanged (25 hooks, 30 call
sites).

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

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants