refactor: cleanup, race condition mitigation, reduce useEffect

[SukkaW]

Summary

• replace a few array finds in the render phase: the underlying data is nearly immutable
• drop a few useEffect
• migrate to useSyncExternalStore and useSWR

Verification

All tests that were passed still pass. All checks pass.

Checklist

• pnpm check:ci and pnpm test both pass (plus cargo fmt / clippy / test for Rust changes)
• I ran the affected surface and observed the change working
• If a wire message changed: WIRE_PROTOCOL_VERSION is bumped
• New code and assets are my own work, or their origin and license compatibility are noted above
• Docs and comments are updated where behavior changed

[greptile-apps]

Greptile Summary

The PR replaces several effect-backed state mirrors and render-time lookups with SWR, useSyncExternalStore, and cached maps.

• Migrates desktop process-level queries to immutable SWR caches.
• Reworks browser webview navigation and readiness subscriptions.
• Simplifies shared simulator, session, settings, and presentation state handling.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains from the prior review thread.

The previously reported unused import is invalid because noop remains used by both rejected-write handlers, and no blocking failure remains.

Important Files Changed

Filename	Overview
apps/desktop/src/renderer/src/shell/browser/browser-webview-pane.tsx	Replaces mirrored navigation state with direct webview snapshots and event-driven external-store subscriptions.
apps/desktop/src/renderer/src/shell/browser/webview-registry.ts	Adds synchronous readiness tracking while retaining promise-based command coordination.
apps/desktop/src/renderer/src/shell/desktop-shell.tsx	Caches process-lifetime application version and editor data with immutable SWR queries.
packages/client/workbench/src/simulator/consent.ts	Migrates simulator consent state seeding to SWR while preserving live broadcast updates and error handling.
packages/client/core/src/react.tsx	Updates shared React client bindings as part of the external-state cleanup.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  External[External state and IPC] --> SWR[SWR immutable caches]
  External --> Store[External-store subscriptions]
  SWR --> React[React surfaces]
  Store --> React
  Events[Runtime events] --> SWR
  Events --> Store

Loading

_{Reviews (3): Last reviewed commit: "chore(webview): bind settings tables to ..." | Re-trigger Greptile}

[pullfrog]

ℹ️ The three refactors are sound and the central race-condition claim checks out. Two minor suggestions inline, plus a few notes below that need no code change to merge.

Reviewed changes

• Read the full diff for all 14 files (982 lines) end-to-end.
• Traced every new SWR cache key to its actual React-tree cache scope via packages/client/workbench/src/runtime/provider.tsx.
• Verified the race-condition claim against the installed swr 2.4.2 source rather than docs: dist/index/index.mjs contains the mutation-timestamp guard (startAt <= mutationInfo[0] || startAt <= mutationInfo[1] || mutationInfo[1] === 0 → finishRequestAndUpdateState(); return false). So mutate(value, { revalidate: false }) from a push callback genuinely discards a fetch started earlier. The claim holds.
• Ran the touched tests: webview-registry.test.ts, use-sessions.test.tsx, provider-cache-policy.test.tsx → 9/9 pass.
• Confirmed all 10 SETTINGS_ROUTES entries have a matching webview route and a matching settings.tabs.* message, so the new activeLabel derivation is behaviorally equivalent to the deleted find.

🧪 No test for the new readyNow flag

apps/desktop/src/renderer/src/__tests__/webview-registry.test.ts already exists with 4 cases, and readyNow / isBrowserWebviewReady is now the load-bearing input to a useSyncExternalStore snapshot — a wrong value here silently disables the whole browser toolbar. Two cases (markBrowserWebviewReady → true, markBrowserWebviewUnready → back to false) would be the cheapest possible proof.

For what it's worth I traced the reset path myself and it is correct: markBrowserWebviewUnready replaces the entry with a fresh unreadyEntry(...), so readyNow does get cleared. It just isn't pinned by a test.

📝 "fetch once, cache forever" is overstated

The comments on 'desktop:app-version' (about-tab.tsx) and 'desktop:shell-editors' (desktop-shell.tsx) say the value is cached forever, but both components mount inside WorkbenchEndpointCacheBoundary, which (a) remounts a fresh Map cache on endpoint change and (b) runs ReadyRevalidator's blanket mutate(trueFn) once per ready generation. mutate bypasses useSWRImmutable's revalidateIfStale/OnFocus/OnReconnect: false — it calls revalidate() unconditionally — so these keys are re-fetched on every daemon reconnect.

Behaviorally harmless (the reads are cheap and main-cached, and the answer doesn't change). But per the repo's comment discipline — "constraint or trap only" — a comment that describes behavior the runtime doesn't actually provide is worse than no comment.

⏸️ refreshInterval stops polling while the window is hidden

SETUP_REPROBE_MS polling in panel.tsx now runs through SWR, and refreshWhenHidden defaults to false, so the loop pauses whenever document.visibilityState === 'hidden'. The old raw setInterval ticked unconditionally. The comment right above it says a step finishing in Xcode "should tick itself off without the user restarting anything" — and the thing being waited on is exactly the case where the user has tabbed away to Xcode for several minutes.

revalidateOnFocus (default true on the non-immutable simulator-status hook) fires an immediate revalidation on window raise, so this self-corrects the moment the user comes back and I don't think it's a correctness bug. Flagging it because it's a deliberate-looking comment now describing slightly different behavior; refreshWhenHidden: true restores the old semantics if that's what was intended.

✅ Checked and cleared — no action needed

Recording these so they don't get re-litigated:

• useSyncExternalStore subscribe ordering in browser-webview-pane.tsx: a dom-ready firing between the layout effect and the passive subscribe cannot be missed — React re-reads the snapshot immediately after subscribing.
• boundMutate identity: it's useCallback(..., []) in swr, so the [client, mutate] / [mutate] effect deps do not resubscribe per render. This matters for panel.tsx, which re-renders per stream frame.
• Two hooks sharing 'simulator-status' with different refreshInterval: each hook instance owns an independent timer; the settings card cannot start or stop the panel's polling.
• Keys omitting client identity (simulator-status, -consent, -devices): swr reads the fetcher through a per-render ref, and these keys live in the endpoint-scoped cache, so no cross-endpoint serving is possible.
• readGuest's blanket try/catch → false: matches the pre-PR IDLE_NAV fallback, not a regression.
• swr in devDependencies only for apps/desktop: pre-existing (cloud-auth/use-cloud-account.ts already imported it), out of scope here.

ℹ️ Nitpicks

• packages/client/workbench/src/simulator/panel.tsx:18 still imports useEffect from foxact/use-abortable-effect, but after this PR both remaining effects in the file are synchronous subscribe/cleanup — no signal is used. consent.ts and window-controls.tsx switched to React's useEffect in this same PR, so it reads as an inconsistency. (It's functionally fine: foxact's wrapper does invoke a returned destructor.)
• The PR checklist leaves "I ran the affected surface and observed the change working" unchecked. Every changed surface — the Electron <webview> pane, the window controls, the iOS Simulator panel — is one only a real run can validate, and the useSyncExternalStore rewrite in particular is not something typecheck or the existing tests cover.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

ℹ️ Both prior inline asks are addressed correctly — threads replied to and resolved. One small type-safety suggestion inline, nothing blocking.

Reviewed changes

Delta since the previous review (df6bfd31..d98e9c63, one commit) — two files:

• Single settings label table. SETTINGS_TAB_LABEL_KEYS is now the one source both the sidebar items and the page header read from, so the messaging → imChannel remap can no longer diverge between the two. Verified all 10 keys line up with SETTINGS_ROUTES and with the settings.tabs.* messages, and that activeKey === undefined still guards the header lookup.
• Screen-mask cache is now immutable. useSWRImmutable + shouldRetryOnError: false replaces plain useSWR with three hand-set flags.

I re-checked the one thing the immutable switch could have broken, against the installed swr 2.4.2 source rather than docs: shouldDoInitialRevalidation ends return isUndefined(data) || revalidateIfStale (node_modules/swr/dist/index/index.mjs ~L315-330). So a successful mask is served from cache with no background refetch — the fix works — while a failed one (no cached data) is still retried when the user switches back to that udid. shouldRetryOnError: false does not permanently poison a device, which was the one regression risk in that change.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

✅ No new issues found. The last open ask is addressed, and addressed better than it was asked — thread resolved, nothing left blocking.

Reviewed changes

Delta since the previous review (d98e9c63..e863c4b8, one commit) — one file, apps/webview/src/routes/settings/settings-layout.tsx:

• Bound both settings tables to one SettingsTabKey union. SETTINGS_ROUTES drops its Record<string, string> annotation so its literal keys survive inference, type SettingsTabKey = keyof typeof SETTINGS_ROUTES derives the key set from it, and SETTINGS_TAB_LABEL_KEYS is now Record<SettingsTabKey, string>. Deriving the union from the routes table rather than hand-writing it (what I suggested) is the better call — there is now exactly one place the tab-key set is written down, so the union itself cannot drift either.
• Typed the inverted path map. Object.entries widens keys to string, so the entries are asserted back to Array<[SettingsTabKey, string]>; SETTINGS_TAB_BY_PATH infers Map<string, SettingsTabKey> and activeKey narrows to SettingsTabKey | undefined, which is what makes the t(SETTINGS_TAB_LABEL_KEYS[activeKey]) lookup at line 166 typed rather than incidentally correct.

I verified the guard by falsification rather than by reading it: adding a driftprobe key to SETTINGS_ROUTES alone makes pnpm exec tsc --build --noEmit apps/webview fail with TS2741: Property 'driftprobe' is missing in type '{ general: string; … }' but required in type 'Record<"agents" | … | "driftprobe" | …, string>'. On the real code both pnpm exec eslint apps/webview/src/routes/settings/settings-layout.tsx and the same tsc --build exit 0.

I also checked the one thing that looked like a shortcut, and it isn't one: the first.key as SettingsTabKey cast at line 181 is forced by the shared type, not laziness. filterSettingsNavGroups (packages/client/workbench/src/settings/search.ts:9) is non-generic — readonly SettingsSidebarNavGroup[] → SettingsSidebarNavGroup[], whose item key is string — so the literal key types cannot survive the call no matter how the local tables are typed. Runtime behavior is identical to the pre-PR Record<string, string> index, and the inline comment says exactly why the cast is there. Removing it would mean making the shared workbench helper generic, which is correctly out of scope here.

  ^{｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}