fix(theme): stop double PUT /api/theme on token edits (cave-gvtw)#3745
Merged
Conversation
Every theme selection, mode flip, and token commit in AppearanceSection writes updateAppPreferences, whose store notify already runs RemoteThemeController's reconcile -> publishCurrentTheme -> PUT /api/theme. The section's own on-change effect fired a second, byte-identical PUT for each edit (measured at t=4433 in the cave-hkfq repro) and opened a 409 window when another device wrote between the two. Drop the settings-side on-change effect and make the controller the sole change-driven publisher. persistThemeTokens stays for the manual "Resync to phone" button (user-invoked healing when the daemon lost state). New source pins keep the effect from being reintroduced and document the single-publisher ownership. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Eliminates redundant theme-token publishing by removing the settings-side on-change /api/theme PUT, making RemoteThemeController the sole automatic publisher while keeping a manual “Resync to phone” escape hatch.
Changes:
- Removed the AppearanceSection
useEffectthat auto-calledpersistThemeTokens()on token/theme changes. - Updated
persistThemeTokens’s doc comment and nearby state-identity comment to reflect the new single-publisher model. - Added test pins to prevent reintroducing the automatic persist side effect and to enforce the manual-only contract documentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/settings-shell.tsx | Removes the settings-driven auto-publish effect; retains manual resync and updates explanatory comments. |
| src/components/settings-appearance.test.ts | Adds regression tests/pins for the single-publisher ownership model and updates related assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+442
to
+446
| assert.doesNotMatch( | ||
| settings, | ||
| /void persistThemeTokens\(\)/, | ||
| "no automatic persistThemeTokens call may remain — RemoteThemeController is the sole on-change publisher (cave-gvtw double PUT)", | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Both
persistThemeTokens(AppearanceSection's on-change effect) andRemoteThemeController.publishCurrentThemePUT/api/themeafter every token edit — byte-identicaltokenOnlypayloads (measured double PUT at t=4433 in the cave-hkfq repro). Wasteful, and the second write opens a 409 window if another device writes between them.Fix
Single-publisher ownership: RemoteThemeController (mounted in the root layout) is the sole change-driven publisher. Every selection, mode flip, and token commit in the settings section already lands in
updateAppPreferences; the store notify runs the controller's reconcile, whose runtime signature ([theme.id, resolvedMode, custom]) changes for exactly those edits → oneflushAppPreferences()→ one PUT (withresolvedMode, which the settings-side PUT never carried).persistThemeTokenseffect.persistThemeTokensfor the manual Resync to phone button — user-invoked healing when the daemon lost state.setCustomDataidentity-preservation comment (still needed for render/reloadKey stability, no longer about echo PUTs).settings-appearance.test.ts: novoid persistThemeTokens()may reappear; the manual-only contract comment must stay.Validation
node --testwith alias loader:settings-appearance,theme-token-hex,remote-theme-controller,theme-store,ios-theme-api,ios-theme-override— 11/11 passpnpm typecheckclean,pnpm lintclean (design gates included)Closes bead
cave-gvtw.