Skip to content

Keep rapid settings edits from reverting each other#4494

Open
colonelpanic8 wants to merge 1 commit into
pingdotgg:mainfrom
colonelpanic8:t3code/fix-provider-settings-toggle
Open

Keep rapid settings edits from reverting each other#4494
colonelpanic8 wants to merge 1 commit into
pingdotgg:mainfrom
colonelpanic8:t3code/fix-provider-settings-toggle

Conversation

@colonelpanic8

@colonelpanic8 colonelpanic8 commented Jul 25, 2026

Copy link
Copy Markdown

What Changed

useUpdateSettingsTarget now retains each in-flight server settings patch and replays it over the server value (via the same applyServerSettingsPatch the server runs) until no write is outstanding for that environment.

  • New apps/web/src/hooks/pendingServerSettings.ts holds the per-environment overlay: retain on dispatch, release when the write settles, drop the overlay only when the last outstanding write settles.
  • useMergedSettings applies the overlay before merging client settings, so usePrimarySettings / useEnvironmentSettings reflect the edit immediately.
  • Unit tests for the overlay semantics in pendingServerSettings.test.ts.

Why

Server settings only change locally once the server echoes settingsUpdated back over the websocket. Until then, readers still see pre-edit settings, so a second edit issued inside that window is computed from stale state — and because keys such as providerInstances are whole-map replacements rather than deep merges (applyServerSettingsPatch), the second write silently reverts the first.

Reproduction before this change, in Settings → Providers: toggle Codex off, then toggle Claude off a moment later. Codex's switch snaps back on and its enabled: false never reaches settings.json — only the Claude entry is persisted. The same race applies to every provider-instance edit (binary path, custom models, accent color, add/delete), plus favorites and providerModelPreferences, since those patches are also built from the settings snapshot.

The doc comment on useUpdateSettingsTarget already claimed server keys were patched optimistically; this makes that true.

UI Changes

No visual changes. Behavioral only: a provider toggle now flips immediately and stays flipped instead of reverting when another edit follows quickly.

Verified in a local build (server + web from this branch, driven through the browser): toggling two providers in immediate succession leaves both switches off and persists both entries to settings.json; an off/on/other three-click sequence composes correctly. On main the same first sequence loses the first toggle.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes — n/a, no visual change
  • I included a video for animation/interaction changes — n/a, no motion change

Validation: vp test run src/hooks/pendingServerSettings.test.ts (6 passed), vp test run src/hooks src/components/settings src/providerInstances.test.ts (86 passed), vp run typecheck in apps/web, vp lint apps/web/src/hooks/, vp fmt --check.


Note

Medium Risk
Touches how all server-backed settings are read and written in the web app; behavior is localized to the settings hooks but affects any whole-map patch (providers, favorites, etc.).

Overview
Fixes a race where back-to-back server settings edits could undo each other because the UI only updated after the websocket settingsUpdated echo, and patches like providerInstances replace the whole map.

Adds pendingServerSettings: each dispatched server patch is retained per environment, replayed with applyServerSettingsPatch for reads, and cleared only when the last in-flight RPC settles (including on failure). useMergedSettings applies that overlay before merging client settings; useUpdateSettingsTarget calls retainPendingServerPatch on dispatch and releasePendingServerPatch in finally instead of relying on atom optimistic updates alone.

Includes unit tests for overlay composition, refcounting, environment scoping, and subscriptions.

Reviewed by Cursor Bugbot for commit 4df7b36. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Prevent rapid server settings edits from reverting each other with an optimistic patch overlay

  • Introduces pendingServerSettings.ts, a module that tracks in-flight server setting patches per environment using a retain/release counter model.
  • When a server settings update is dispatched, the patch is retained as an optimistic overlay and applied immediately in useMergedSettings so the UI reflects the change before the server responds.
  • The overlay persists until all outstanding writes for that environment settle (success or failure), preventing concurrent edits from reverting each other.
  • usePendingServerPatches bridges the patch state into React via useSyncExternalStore so components re-render when pending patches change.
  • Risk: failed writes drop the overlay immediately, so a failed update will revert the UI to the server value even if other in-flight writes for that environment are still pending.

Macroscope summarized 4df7b36.

Server settings only change locally once the server echoes `settingsUpdated`
back over the websocket, and `useUpdateSettingsTarget` applied nothing
locally in the meantime. Any edit issued inside that round trip was
therefore computed from pre-edit settings, and because keys such as
`providerInstances` are whole-value replacements rather than deep merges,
the second write silently reverted the first: toggle one provider off,
toggle a second off a moment later, and the first provider's switch snaps
back on and never reaches settings.json.

Retain in-flight patches per environment and replay them over the server
value with the same `applyServerSettingsPatch` the server runs, so reads
and follow-up patches build on the edit the user just made. Patches are
dropped once no write is outstanding — including after a failed write, so
the server value stays authoritative.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c749d816-6d81-4d6c-a3fa-b60106c1404d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 25, 2026

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4df7b36. Configure here.

environmentId,
input: { patch: serverPatch },
}).finally(() => {
releasePendingServerPatch(environmentId);

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.

Overlay drops before settings echo

High Severity

Optimistic pending server patches are cleared when the RPC promise settles, but the authoritative primaryServerSettingsAtom updates later via a websocket event. This gap briefly exposes stale server settings, causing follow-up edits to potentially revert earlier in-flight changes.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 4df7b36. Configure here.

@macroscopeapp

macroscopeapp Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

An unresolved high-severity comment identifies a potential timing bug where patches are released when the RPC settles but before the websocket echo arrives, which could still cause the race condition this PR aims to fix. This substantive correctness concern warrants human review.

You can customize Macroscope's approvability policy. Learn more.

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

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant