Skip to content

refactor(desktop): move session settings ownership below AppShell - #5509

Merged
Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:refactor/session-settings-owner
Sep 25, 2026
Merged

Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:refactor/session-settings-owner

Conversation

@chihumyum

@chihumyum chihumyum commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Session-setting write intents currently live in AppShell, so writes for inactive Sessions also re-render the shell. Move their controller into a feature-owned SessionSettingsProvider, registered in controllerOwners, and leave the shell one equality-selected read of the active Session's model/thinking, permission, Plan and orchestration overlays plus stable commands.

Move the Plan transition policy and its authoritative getPlanState read behind feature services. Preserve optimistic rollback, revision-based retirement, captured-owner permission confirmation and the original Session target during Plan confirmation. Shell bridge paths drop from 18 to 17; the existing settings hook remains as a necessary read. Session Collaboration and Plan panel subscriptions remain outside this slice.

Refs #4582.

Verification

Rebased onto main feb9cf22f. Preserve #5532's direct Session catalog subscription inside the feature-owned controller, adapting the provider contract and tests to the current API. Regenerate the architecture ledger and Astryx inventory from the resolved source.

  • Node 24: build:test, Desktop test:dist 2820/2820, Desktop typecheck (including stories), production renderer build.
  • Renderer architecture 112/112 and exact-base check; AppShell hook gate; lint, format, Desktop/UI Knip, ASF headers, Astryx and Windows inventories, git diff --check.
  • New provider tests cover inactive-Session render isolation, active optimistic rollback, navigation during bypass/Plan confirmations, clearing pending intents, StrictMode cleanup and retained commands after unmount. The new catalog regression confirms that only the acknowledged Session overlay retires on catalog observations, without re-rendering the shell or frame.
  • Plan policy tests cover active execution refusal, canceled discard, normal mode changes and Host read errors; existing settings/controller tests retain model/thinking and causal-retirement coverage.
  • Merge-tree against the current base is clean.

No UI layout or intended user-visible behavior change. Interactive Electron/manual UI acceptance was not run; the behavior and render-scope evidence above comes from feature-level React tests.

AI use

Select exactly one:

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

Tool(s) and scope: OpenAI Codex implemented the ownership migration, tests and documentation, performed source review and validation, and prepared this PR under the contributor's direction. The commit includes Generated-by: Codex.

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/L Under 1000 readable lines label Sep 19, 2026
@chihumyum
chihumyum force-pushed the refactor/session-settings-owner branch 3 times, most recently from eeb463e to 8df8b98 Compare September 25, 2026 07:12

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

Adversarial review at 8df8b988e — line-by-line semantic diff of the moved controller plus a worktree build (20/20 focused tests, renderer tsc, architecture checks all green). No P0–P2.

Verified: writeSessionPlanMode is a faithful move of commitPlanMode — same fresh getPlanState IPC read (no snapshot), same refusal ordering, and Plan confirmation still targets the parameter sessionId with abandonPlanProposal keyed to the proposal id — the original-Session-target guarantee holds mid-await. Optimistic rollback and revision retirement live in the unchanged shared intent hook; captured-owner permission confirmation keeps its exact ordering (owner-change-mid-confirm is covered by a test). The equality-selected read covers exactly the four fields the shell consumed, with a stable reference — foreign-session writes wake neither shell nor frame. The provider's all-sessions catalog subscription is correct (it's the write owner and must observe retirement for any session). controllerOwners registration, bridge paths 18→17, and hook-site counts all check out.

Two P3s, both deferrable: ui/use-session-setting-intent.ts:57-67 hand-rolls the state-keyed snapshot cache + isEqual carry-forward that useExternalStoreSelector already provides with identical semantics — swapping deletes ~30 lines and one parallel mechanism; and (pre-existing) a write refusal still double-toasts — the specific refusal message plus the generic onWriteError failure — faithfully preserved from main, only worth touching if refusal-shouldn't-report-failure is considered a bug.

Move setting write intents and Plan transition policy into the feature owner. Keep a selected-session overlay read and stable commands in the shell, with ownership and render-scope coverage.

Generated-by: Codex
@chihumyum
chihumyum force-pushed the refactor/session-settings-owner branch from 8df8b98 to 83a7a35 Compare September 25, 2026 08:30
@Astro-Han
Astro-Han merged commit 6a9e8ae into apache:main Sep 25, 2026
1 check passed
@chihumyum
chihumyum deleted the refactor/session-settings-owner branch September 25, 2026 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants