Two related weaknesses surfaced during the review of #483 (artist hero, #482). Both are pre-existing and shared across the codebase, so they were deliberately left out of that PR rather than fixed for one hook only — fixing a single call site would have given the illusion of coverage while 20+ others stayed exposed.
1. set_profile_setting is not scoped to a profile (TOCTOU)
commands/profile.rs::set_profile_setting takes key / value / value_type and resolves the pool itself:
pub async fn set_profile_setting(state, key, value, value_type) -> AppResult<()> {
let pool = state.require_profile_pool().await?;
// INSERT ... ON CONFLICT DO UPDATE
}Every JS caller guards the write against a profile switch by comparing a captured id with the live one — the pattern in useCoverSlideshow and its copies:
const write = writeChainRef.current.then(async () => {
if (activeProfileIdRef.current !== profileId) return;
await setProfileSetting(KEY, next ? "true" : "false", "bool");
...
});That check happens before the await. A switch_profile landing between the guard and require_profile_pool() acquiring the lease writes the preference into the wrong profile. The window is small and the blast radius is one UI preference (no data loss), but it's a genuine correctness gap and it exists on every write path.
Fix: take an expected profile id on the command and validate it while holding the same lock used to acquire the active pool (see state.rs) — a JS-side guard fundamentally cannot close this, only the backend can. Mismatch should be a no-op or an explicit error the caller can ignore, not a write.
Scope: 22 call sites across 18 files (setProfileSetting in src/), including every preference hook, ThemeContext, SkinContext and lib/shortcuts.ts. get_profile_setting deserves the same treatment for symmetry — a read racing a switch returns another profile's value.
2. The preference-hook pattern is copy-pasted 7×
These all reimplement the same machinery — serialized write chain, profile-switch guards, rollback to the last backend-confirmed value, a window event to re-read across mounted consumers:
The review of #483 hardened only the last one, which now carries fixes the other six don't:
- A read in flight clobbering an optimistic toggle — a read started at mount (or by the broadcast event) applied its result unconditionally, overwriting a toggle clicked meanwhile.
useArtistHero now tags each read with a token bumped by every read and every write.
- A stale
resolved marker across profiles — only relevant to hooks whose default is ON (useArtistHero is the only one today, hence resolved + the reset of the outgoing profile's value on switch). The others default to OFF, so an unresolved frame paints nothing and the bug is invisible — but it becomes real the day any of them flips its default.
Fix: factor a single useProfileBooleanSetting(key, { default }) (plus a value-typed sibling for useVisualizerColor / useHiddenKpis) carrying the hardened logic, and port the seven hooks onto it. That also makes #1 a one-place change on the JS side.
Why now
Neither is urgent — #1 needs a rare interleaving, #2 is latent for six of the seven hooks. But both compound: every new preference hook copies the pattern again and adds a call site to #1. Worth doing before the next one lands.
Found during: #483 review.
Two related weaknesses surfaced during the review of #483 (artist hero, #482). Both are pre-existing and shared across the codebase, so they were deliberately left out of that PR rather than fixed for one hook only — fixing a single call site would have given the illusion of coverage while 20+ others stayed exposed.
1.
set_profile_settingis not scoped to a profile (TOCTOU)commands/profile.rs::set_profile_settingtakeskey/value/value_typeand resolves the pool itself:Every JS caller guards the write against a profile switch by comparing a captured id with the live one — the pattern in
useCoverSlideshowand its copies:That check happens before the
await. Aswitch_profilelanding between the guard andrequire_profile_pool()acquiring the lease writes the preference into the wrong profile. The window is small and the blast radius is one UI preference (no data loss), but it's a genuine correctness gap and it exists on every write path.Fix: take an expected profile id on the command and validate it while holding the same lock used to acquire the active pool (see
state.rs) — a JS-side guard fundamentally cannot close this, only the backend can. Mismatch should be a no-op or an explicit error the caller can ignore, not a write.Scope: 22 call sites across 18 files (
setProfileSettinginsrc/), including every preference hook,ThemeContext,SkinContextandlib/shortcuts.ts.get_profile_settingdeserves the same treatment for symmetry — a read racing a switch returns another profile's value.2. The preference-hook pattern is copy-pasted 7×
These all reimplement the same machinery — serialized write chain, profile-switch guards, rollback to the last backend-confirmed value, a
windowevent to re-read across mounted consumers:useScrollLongTitlesuseArtistBioCollapseduseHiddenKpisuseWebRadioFavoritesuseCoverSlideshowuseVisualizerColoruseArtistHeroThe review of #483 hardened only the last one, which now carries fixes the other six don't:
useArtistHeronow tags each read with a token bumped by every read and every write.resolvedmarker across profiles — only relevant to hooks whose default is ON (useArtistHerois the only one today, henceresolved+ the reset of the outgoing profile's value on switch). The others default to OFF, so an unresolved frame paints nothing and the bug is invisible — but it becomes real the day any of them flips its default.Fix: factor a single
useProfileBooleanSetting(key, { default })(plus a value-typed sibling foruseVisualizerColor/useHiddenKpis) carrying the hardened logic, and port the seven hooks onto it. That also makes #1 a one-place change on the JS side.Why now
Neither is urgent — #1 needs a rare interleaving, #2 is latent for six of the seven hooks. But both compound: every new preference hook copies the pattern again and adds a call site to #1. Worth doing before the next one lands.
Found during: #483 review.