Skip to content

fix(desktop): keep Settings stable during Runtime Host refresh - #3450

Open
chihumyum wants to merge 3 commits into
apache:mainfrom
chihumyum:fix/settings-loading-flash
Open

fix(desktop): keep Settings stable during Runtime Host refresh#3450
chihumyum wants to merge 3 commits into
apache:mainfrom
chihumyum:fix/settings-loading-flash

Conversation

@chihumyum

@chihumyum chihumyum commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Keep last-ready masked Settings snapshots in renderer memory so reopening Settings does not flash a warning or collapse Runtime Host-owned rows during revalidation.
  • Bind Runtime Host settings, connections, and mutation authority to the selected Host key and lifecycle epoch, so reconnect or same-key replacement cannot retain stale write authority.
  • Keep Desktop-owned preferences available while Host-owned controls remain visible but inert until the current Runtime Host generation is verified.
  • Retire one-shot provider catalog navigation only after ProvidersPanel consumes it, and visibly mute cached Host pages while their authority fence is active.
  • Preserve error and unavailable feedback without changing IPC contracts or persistent storage.

Behaviour

Before fixing, the yellow banner flashes each time the settings page is opened.

before.mp4

After fixing, no yellow banner was ever shown.

after.mp4

Verification

  • npm run lint
  • npm run format:check
  • npm run build
  • npm run typecheck
  • npm --workspace @maka/desktop test — 1118/1118 passed
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 172/172 passed
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • npm run astryx:surface-inventory — 191 files, 1 documented exclusion
  • Settings cases in the full Electron run — 4/4 passed
  • npm --workspace @maka/desktop run e2e — 41 passed, 1 skipped

Review focus

  • Runtime Host epoch changes and stale read/write fencing
  • masked snapshot caching and secret lifetime
  • mixed ownership boundaries on General, Projects, and Data
  • one-shot provider catalog intent ownership across warm-cache loading

AI use

Select exactly one:

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

Tool(s) and scope: Codex — implementation, race analysis, unit/Storybook/Electron
E2E coverage, regression verification, and pull request preparation. The human
contributor reviewed the contribution and accepts responsibility for 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

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review of exact head 54651d6b0aeca844f527f75cfe00f4a8d6b6c9fc against current main@d62857a8357e9160926726a2a13096bc2dc2b91d.

The defect remains on main: every Settings mount starts the Runtime Host catalog/settings/connections resources cold, so the mixed-ownership General page briefly renders defaults/loading feedback before Host data arrives. The patch keeps only masked read snapshots in renderer memory, keys Host data by profileId:hostId, prunes an old epoch when the catalog changes, and keeps cached Host controls inert until the current mount verifies the target. The separate read/write tickets also prevent a late old-Host result from populating the newly selected Host. The reopen E2E observes the transient DOM, so it would catch the original flash rather than only asserting the settled page. I found no actionable P0-P2 defect; git diff --check is clean.

Required conclusions:

  1. Optimal for the actual problem: yes. The implementation is larger than the symptom, but stable cross-unmount rendering requires a cache, and the masking, epoch key, verification bit, and interaction fence are necessary to avoid turning stale UI into write authority.
  2. Production code to delete: the PR replaces the older parallel resource unions/ticket refs; none further identified.
  3. Tests to delete/replace: none identified. The helper tests and transient-DOM E2E cover distinct contracts.
  4. Deeper refactor: no; keeping the cache outside AppShell avoids lifting Settings-only state into a broader owner.
  5. Ready to merge: not yet. No hosted test check is reported on this exact head, and automated review is not approval.
  6. Residual risks/gaps: this changes user-visible loading/error behavior and the security-sensitive Host write boundary across General, Projects, and Data. The supplied tests cover cache epochs and the default General reopen, but independent human review should still exercise non-default Host selection and failure/retry behavior.

This affects user-visible behavior and Runtime Host authority fencing, so CONTRIBUTING.md requires independent human judgment on the exact head.

@chihumyum
chihumyum force-pushed the fix/settings-loading-flash branch from 54651d6 to 43e53f7 Compare August 22, 2026 05:51

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated re-review of exact head 43e53f751ba9b604b994d75bf88c8a2781972e08 against main@f1f4e71a9540a4da23159052c924fee72202e989.

No new actionable implementation defect was found relative to the previously reviewed revision. git range-diff shows the same one-commit settings stabilization adapted onto the newer base; the keyed snapshot/cache/request-authority structure still keeps one Host's data from being exposed through another, retains last-ready content during revalidation, and fences mutations until the selected Host is freshly verified.

The head is not mergeable, however. Current main and this PR both modify apps/desktop/e2e/settings.spec.ts, settings-surface.tsx, and nav-sidebar.css; the E2E conflict specifically places main's “wide settings gutters scroll the whole main pane” test against this PR's reopen-stability test. Rebase and preserve both behavioral contracts rather than choosing one side. Re-run the exact-head check after resolution.

Required conclusions:

  1. Optimal for the actual problem: yes; the cache remains renderer-memory, keyed by bridge/Host identity, and mutations stay behind fresh authority.
  2. Production code to delete: none identified.
  3. Tests to delete/replace: none; preserve both the main scroll-ownership E2E and this PR's reopen-stability E2E.
  4. Deeper refactor: no.
  5. Ready to merge: no while conflicting; there is also no hosted test result or independent human approval on this head.
  6. Residual risks: conflict resolution is the primary regression risk, particularly losing main's newly merged settings scroll ownership or this PR's revalidation coverage.

This changes user-visible Settings behavior. Independent human review is required; this automated review is not approval.

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

Thanks — the cache ownership and Runtime Host fencing are thoughtfully structured, and I did not find a cross-Host authority or stale-write correctness issue on exact head 43e53f751ba9b604b994d75bf88c8a2781972e08. I found two non-blocking user-facing gaps below.

Since this head currently conflicts with main in the same Settings surface, please preserve these final-state requirements while rebasing. After the semantic rebase, these findings remain P2 and do not by themselves block approval.

AI-assisted review disclosure: Codex delegated independent review passes; I verified the reported paths and conclusions against this exact head.

Comment thread apps/desktop/src/renderer/settings/settings-surface.tsx Outdated
@chihumyum
chihumyum force-pushed the fix/settings-loading-flash branch from 43e53f7 to fa01d91 Compare August 22, 2026 06:41
@chihumyum

Copy link
Copy Markdown
Contributor Author

@Astro-Han Thanks for the careful review. I rebased onto the current main and addressed both P2 findings in fa01d917d:

  • The provider-catalog landing intent is now retired only when ProvidersPanel actually enters the catalog route via onInitialCatalogConsumed. A warm-cache Story delays the child load, verifies that the catalog still opens, and verifies the intent is one-shot after remount.
  • The cached Runtime Host page now has a visible muted state while the existing inert / aria-busy authority fence is active, without changing layout or reintroducing the loading Banner. Storybook asserts the computed opacity as well as the semantic fence.

The semantic rebase also preserves both #3453 scroll ownership and this PR’s reopen-stability contract. Exact-head verification: desktop tests 1076/1076, Storybook smoke 169/169, Settings E2E 4/4, and full Electron E2E 41 passed / 1 skipped.

Could you please take another look when convenient?

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

Thanks for the substantial cleanup of the settings cache and intent flow. The exact-head test is green, and the earlier findings are improved, but the current page-level boundary still mixes Desktop-owned and Host-owned availability, and the refresh generation does not yet revoke write authority. I left the two final-state requirements inline.\n\nAI-assisted review disclosure: Codex delegated independent state and test reviews; I reconciled the conflicting conclusions against the exact-head control flow and live PR state before posting.

Comment thread apps/desktop/src/renderer/settings/settings-surface.tsx
Comment thread apps/desktop/src/renderer/settings/settings-resource-state.ts
@chihumyum
chihumyum force-pushed the fix/settings-loading-flash branch from fa01d91 to 1ac77bf Compare August 22, 2026 10:30
@chihumyum

chihumyum commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

@Astro-Han Thanks for the follow-up review. I rebased the branch onto main@f83b469b5, addressed the generation-fence finding in 1ac77bf98, and replied inline.

The follow-up binds Runtime Host reads and writes to lifecycle epoch as well as profileId:hostId, synchronously revokes old-generation authority on replacement/reconnect, and keeps background-profile lifecycle tombstones from being bypassed by a stale catalog. I also re-traced the mixed-page finding: General, Projects, and Data bypass the outer Host-only branch, and explicit Stories now cover language/notification availability during Host revalidation.

Exact-head local verification is green: lint, format, build, typecheck, both knip workspaces, Astryx inventory (191 files), Desktop tests (1118/1118), Storybook build/render smoke (172/172), and Electron E2E (41 passed, 1 repository-defined skip). The new hosted CI run is awaiting maintainer approval before its jobs can start.

Could you please re-review the new exact head when convenient?

— Codex, posting on behalf of @chihumyum

@chihumyum
chihumyum force-pushed the fix/settings-loading-flash branch 2 times, most recently from de55a42 to 29153de Compare August 22, 2026 13:37
@chihumyum
chihumyum force-pushed the fix/settings-loading-flash branch from 29153de to 280d833 Compare August 22, 2026 14:07

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

Re-reviewed at the new head 280d833a after the rebase. We re-verified each of our earlier findings against this head rather than carrying them forward, and one of them does not survive.

Withdrawing our P1 about the ownership seam — you were right and we were wrong. Our claim was that the outer RuntimeHostInteractionBoundary renders the whole SettingsPageBody inert and so blocks desktop-side controls on pages that mix client and Host concerns. That premise does not hold: requiresRuntimeHost is sectionScope === 'runtime-host', and SETTINGS_SECTION_SCOPES marks general, projects and data as 'mixed', so isInteractive={!requiresRuntimeHost || runtimeHostContentVerified} evaluates to true on exactly the pages we were worried about. The inert branch is unreachable there. We should have traced the scope table before filing it.

Two P2 from the earlier rounds are confirmed fixed. The write-authority/epoch one: settings-request-authority.ts now binds each ticket to (key, targetEpoch, targetRevision, generation) and selectTarget bumps every generation when the key or epoch changes, so a superseded mutation response is rejected before commit. The catalog-intent one: the parent now waits for the child's onInitialProviderCatalogConsumed callback before clearing providerCatalogRequested (settings-surface.tsx:998-1000).

One P2 from the earlier round is still open — the boundary is invisible. RuntimeHostInteractionBoundary still applies only inert plus aria-busy. Assistive technology is informed; a sighted user is not. During a slow reconnect the page looks fully usable and silently swallows clicks. A muted/loading treatment on the wrapper would close it.

One new P2, inline — and it is worth noting that this is where the risk our withdrawn P1 was gesturing at actually lives. The mechanism we described was wrong, but there is a real gap on the mixed pages; it is just an inconsistency between them rather than an outer-boundary problem.

Everything else on the rebase increment checks out — the epoch fix survived intact.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

Comment thread apps/desktop/src/renderer/settings/projects-settings-page.tsx
@chihumyum
chihumyum requested a review from Astro-Han August 22, 2026 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants