feat: add server-synced user preferences infrastructure (#484)#1189
feat: add server-synced user preferences infrastructure (#484)#1189gusa4grr wants to merge 5 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
haven't reviewed the code yet, but on the proposed changes:
The usual pattern is this:
we should then end up in a state where our prefs save to local storage and take effect immediately, but meanwhile get synced to the server in the background. i think we probably also need a toggle somewhere to disable sync, in case you want different prefs per machine. same way chrome works today |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
@43081j 👋🏻 Updated the MR description and pushed one more commit. if you have time - please look throught the code 🙂 Much appreciated! I will try to find time during the work week to finalise this, as not much is left 🤞🏻 |
gusa4grr
force-pushed
the
feat-484-persist-user-preferences
branch
from
96ad934 to
55c4157
Compare
| } | ||
| } | ||
|
|
||
| export function useRelativeDates() { |
There was a problem hiding this comment.
we should probably split these up into app/composables/preferences/whatever.ts
e.g. app/composables/preferences/useRelativeDates.ts
to follow convention of the rest of the repo
gusa4grr
force-pushed
the
feat-484-persist-user-preferences
branch
from
55c4157 to
262f23d
Compare
gusa4grr
force-pushed
the
feat-484-persist-user-preferences
branch
from
add3eba to
3c8b850
Compare
gusa4grr
force-pushed
the
feat-484-persist-user-preferences
branch
from
3c8b850 to
955381d
Compare
gusa4grr
force-pushed
the
feat-484-persist-user-preferences
branch
from
955381d to
e56d0d9
Compare
📝 WalkthroughWalkthroughReplaces the legacy useSettings composable with a split model: a client-only useUserLocalSettings for ephemeral UI state and a server-backed user preferences system (provider, sync client, server APIs, KV-backed store, shared schema). Adds localStorage/storage abstractions, new userPreferences composables (accent color, background theme, color mode, search provider, relative dates), updates components and tests to consume the new APIs, removes useSettings and usePreferencesProvider, and adds a client plugin to initialise preference sync and apply stored colour mode. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
app/composables/userPreferences/useUserPreferencesState.ts (1)
1-9: Align the “read-only” wording with the mutable return value.The docstring promises read-only access, but the returned ref is writable. Either clarify the wording or enforce immutability so expectations are consistent.
shared/schemas/userPreferences.ts (1)
35-38: Type definitions could drift from schema.The types
AccentColorIdandBackgroundThemeIdare derived from the constants, whilstColorModePreferenceandSearchProviderare manually defined literals. Consider deriving these from the schema for consistency:♻️ Suggested approach
+import type { InferOutput } from 'valibot' + +// Derive types from schemas to prevent drift +export type ColorModePreference = InferOutput<typeof ColorModePreferenceSchema> +export type SearchProvider = InferOutput<typeof SearchProviderSchema> export type AccentColorId = keyof typeof ACCENT_COLORS.light export type BackgroundThemeId = keyof typeof BACKGROUND_THEMES -export type ColorModePreference = 'light' | 'dark' | 'system' -export type SearchProvider = 'npm' | 'algolia'app/composables/useUserPreferencesSync.client.ts (2)
137-144: Route guard uses fire-and-forget pattern which may lose data.The
void flushPendingSync()call allows navigation to proceed without awaiting the save. If the network request fails or is slow, the user may navigate away before their preferences are persisted.Consider awaiting the flush to ensure data is saved before navigation:
♻️ Suggested fix
function setupRouteGuard(getPreferences: () => UserPreferences): void { router.beforeEach(async (_to, _from, next) => { if (hasPendingChanges && isAuthenticated.value) { - void flushPendingSync(getPreferences()) + await flushPendingSync(getPreferences()) } next() }) }
32-41: Silent error handling may mask connectivity issues.
fetchServerPreferencescatches all errors and returnsnull, making it indistinguishable from "user has no saved preferences" vs "network/server error". Consider logging errors or updating the sync state to reflect fetch failures.server/utils/preferences/user-preferences-store.ts (1)
23-35: Minor: Double timestamp assignment inmerge.Line 30 sets
updatedAt, then line 33 callsthis.set()which setsupdatedAtagain on line 18. This is functionally correct but slightly redundant.♻️ Optional simplification
async merge(did: string, partial: Partial<UserPreferences>): Promise<UserPreferences> { const existing = await this.get(did) const base = existing ?? { ...DEFAULT_USER_PREFERENCES } const merged: UserPreferences = { ...base, ...partial, - updatedAt: new Date().toISOString(), } - await this.set(did, merged) + await this.storage.setItem(did, { + ...merged, + updatedAt: new Date().toISOString(), + }) return merged }app/composables/useUserPreferencesProvider.ts (1)
77-95: Avoid echoing server‑loaded prefs back to the server.
In the auth watcher, assigningpreferences.valuetriggers the deep watch and schedules a sync, even though the data just came from the server. This creates redundant PUTs and can churn timestamps. Consider suppressingscheduleSyncwhile applying server prefs.♻️ Suggested refactor
const isSyncing = computed(() => status.value === 'syncing') const isSynced = computed(() => status.value === 'synced') const hasError = computed(() => status.value === 'error') + let isApplyingServerPrefs = false async function initSync(): Promise<void> { @@ watch( preferences, newPrefs => { - if (isAuthenticated.value) { + if (isAuthenticated.value && !isApplyingServerPrefs) { scheduleSync(newPrefs) } }, { deep: true }, ) watch(isAuthenticated, async newIsAuth => { if (newIsAuth) { const serverPrefs = await loadFromServer() if (serverPrefs) { const merged = { ...defaultValue, ...preferences.value, ...serverPrefs } if (!arePreferencesEqual(preferences.value, merged)) { - preferences.value = merged + isApplyingServerPrefs = true + preferences.value = merged + isApplyingServerPrefs = false } } } })
| export function useLocalStorageHashProvider<T extends object>(key: string, defaultValue: T) { | ||
| const provider = createLocalStorageProvider<T>(key) | ||
| const data = ref<T>(defaultValue) | ||
|
|
||
| onMounted(() => { | ||
| const stored = provider.get() | ||
| if (stored) { | ||
| data.value = defu(stored, defaultValue) | ||
| } | ||
| }) | ||
|
|
||
| function save() { | ||
| provider.set(data.value) | ||
| } | ||
|
|
||
| function reset() { | ||
| data.value = { ...defaultValue } | ||
| provider.remove() | ||
| } | ||
|
|
||
| function update<K extends keyof T>(key: K, value: T[K]) { | ||
| data.value[key] = value | ||
| save() |
There was a problem hiding this comment.
Prevent shared default mutations in the storage provider
data is initialised with defaultValue by reference and reset() only shallow‑clones, so nested arrays/objects can mutate the defaults and make resets or new instances pick up modified values. Deep‑clone defaults on init/reset and when merging stored values.
🛠️ Suggested fix
export function useLocalStorageHashProvider<T extends object>(key: string, defaultValue: T) {
const provider = createLocalStorageProvider<T>(key)
- const data = ref<T>(defaultValue)
+ const data = ref<T>(structuredClone(defaultValue))
onMounted(() => {
const stored = provider.get()
if (stored) {
- data.value = defu(stored, defaultValue)
+ data.value = defu(stored, structuredClone(defaultValue))
}
})
function reset() {
- data.value = { ...defaultValue }
+ data.value = structuredClone(defaultValue)
provider.remove()
}| async function initSync(): Promise<void> { | ||
| if (syncInitialized || import.meta.server) return | ||
| syncInitialized = true | ||
|
|
||
| setupRouteGuard(() => preferences.value) | ||
| setupBeforeUnload(() => preferences.value) | ||
|
|
||
| if (isAuthenticated.value) { | ||
| const serverPrefs = await loadFromServer() | ||
| if (serverPrefs) { | ||
| const merged = { ...preferences.value, ...serverPrefs } | ||
| if (!arePreferencesEqual(preferences.value, merged)) { | ||
| preferences.value = merged | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent initSync from getting stuck on load failures.
If loadFromServer() throws (offline/5xx), initSync exits before watchers/guards are registered, yet syncInitialized stays true, so subsequent calls no‑op. Wrap the fetch in a try/catch and ensure setup continues even on failure (or reset the flag).
🛠️ Suggested fix
async function initSync(): Promise<void> {
if (syncInitialized || import.meta.server) return
syncInitialized = true
setupRouteGuard(() => preferences.value)
setupBeforeUnload(() => preferences.value)
- if (isAuthenticated.value) {
- const serverPrefs = await loadFromServer()
- if (serverPrefs) {
- const merged = { ...preferences.value, ...serverPrefs }
- if (!arePreferencesEqual(preferences.value, merged)) {
- preferences.value = merged
- }
- }
- }
+ if (isAuthenticated.value) {
+ try {
+ const serverPrefs = await loadFromServer()
+ if (serverPrefs) {
+ const merged = { ...preferences.value, ...serverPrefs }
+ if (!arePreferencesEqual(preferences.value, merged)) {
+ preferences.value = merged
+ }
+ }
+ } catch (_err) {
+ // allow init to complete; sync status is handled by useUserPreferencesSync
+ }
+ }As per coding guidelines: “Use error handling patterns consistently.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function initSync(): Promise<void> { | |
| if (syncInitialized || import.meta.server) return | |
| syncInitialized = true | |
| setupRouteGuard(() => preferences.value) | |
| setupBeforeUnload(() => preferences.value) | |
| if (isAuthenticated.value) { | |
| const serverPrefs = await loadFromServer() | |
| if (serverPrefs) { | |
| const merged = { ...preferences.value, ...serverPrefs } | |
| if (!arePreferencesEqual(preferences.value, merged)) { | |
| preferences.value = merged | |
| } | |
| } | |
| } | |
| async function initSync(): Promise<void> { | |
| if (syncInitialized || import.meta.server) return | |
| syncInitialized = true | |
| setupRouteGuard(() => preferences.value) | |
| setupBeforeUnload(() => preferences.value) | |
| if (isAuthenticated.value) { | |
| try { | |
| const serverPrefs = await loadFromServer() | |
| if (serverPrefs) { | |
| const merged = { ...preferences.value, ...serverPrefs } | |
| if (!arePreferencesEqual(preferences.value, merged)) { | |
| preferences.value = merged | |
| } | |
| } | |
| } catch (_err) { | |
| // allow init to complete; sync status is handled by useUserPreferencesSync | |
| } | |
| } | |
| } |
| // Read user preferences from localStorage | ||
| const preferences = JSON.parse(localStorage.getItem('npmx-user-preferences') || '{}') | ||
|
|
||
| const accentColorId = settings.accentColorId | ||
| const accentColorId = preferences.accentColorId |
There was a problem hiding this comment.
Guard JSON.parse to avoid prehydrate crashes on malformed localStorage.
If a user (or browser) stores invalid JSON, the current parse will throw and short‑circuit the prehydrate logic. You already guard other parsing (package manager), so mirroring that here keeps behaviour consistent and resilient.
🛠️ Suggested hardening
// Read user preferences from localStorage
- const preferences = JSON.parse(localStorage.getItem('npmx-user-preferences') || '{}')
+ const safeParse = (raw) => {
+ if (!raw) return {}
+ try {
+ const parsed = JSON.parse(raw)
+ return parsed && typeof parsed === 'object' && !Array.isArray(parsed) ? parsed : {}
+ } catch {
+ return {}
+ }
+ }
+ const preferences = safeParse(localStorage.getItem('npmx-user-preferences'))
@@
- const sidebar = JSON.parse(localStorage.getItem('npmx-settings') || '{}')
- document.documentElement.dataset.collapsed = sidebar.sidebar?.collapsed?.join(' ') ?? ''
+ const sidebar = safeParse(localStorage.getItem('npmx-settings'))
+ document.documentElement.dataset.collapsed =
+ Array.isArray(sidebar.sidebar?.collapsed) ? sidebar.sidebar.collapsed.join(' ') : ''As per coding guidelines, “Use error handling patterns consistently”.
Also applies to: 53-54
gusa4grr
force-pushed
the
feat-484-persist-user-preferences
branch
from
e56d0d9 to
21882ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
app/pages/settings.vue (1)
66-100: Remove the non‑essential template comment.
Line 66 is descriptive rather than explaining complex logic; consider dropping it to keep templates lean.As per coding guidelines: “Add comments only to explain complex logic or non-obvious implementations”.♻️ Suggested tidy‑up
- <!-- Sync status indicator for authenticated users -->test/nuxt/components/HeaderConnectorModal.spec.ts (1)
104-106: Reset the wholemockUserLocalSettingsobject to avoid cross‑test leakage.
Onlyconnectoris reset; if future tests mutatesidebar, state will bleed between tests.Proposed fix
function resetMockState() { mockState.value = { connected: false, connecting: false, npmUser: null, avatar: null, operations: [], error: null, lastExecutionTime: null, } - mockUserLocalSettings.value.connector = { - autoOpenURL: false, - } + mockUserLocalSettings.value = { + sidebar: { + collapsed: [], + }, + connector: { + autoOpenURL: false, + }, + } }
| onPrehydrate(el => { | ||
| const settings = JSON.parse(localStorage.getItem('npmx-settings') || '{}') | ||
| const id = settings.accentColorId | ||
| const preferences = JSON.parse(localStorage.getItem('npmx-user-preferences') || '{}') | ||
| const id = preferences.accentColorId |
There was a problem hiding this comment.
Guard JSON.parse in onPrehydrate to prevent crashes.
If localStorage.getItem('npmx-user-preferences') contains malformed JSON, the parse will throw and break the prehydrate logic. Wrap in try/catch for resilience.
🛠️ Suggested fix
onPrehydrate(el => {
- const preferences = JSON.parse(localStorage.getItem('npmx-user-preferences') || '{}')
- const id = preferences.accentColorId
+ let id = null
+ try {
+ const preferences = JSON.parse(localStorage.getItem('npmx-user-preferences') || '{}')
+ id = preferences?.accentColorId
+ } catch {
+ // Ignore malformed JSON
+ }
if (id) {As per coding guidelines, "Use error handling patterns consistently".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onPrehydrate(el => { | |
| const settings = JSON.parse(localStorage.getItem('npmx-settings') || '{}') | |
| const id = settings.accentColorId | |
| const preferences = JSON.parse(localStorage.getItem('npmx-user-preferences') || '{}') | |
| const id = preferences.accentColorId | |
| onPrehydrate(el => { | |
| let id = null | |
| try { | |
| const preferences = JSON.parse(localStorage.getItem('npmx-user-preferences') || '{}') | |
| id = preferences?.accentColorId | |
| } catch { | |
| // Ignore malformed JSON | |
| } |
| function setAccentColor(id: AccentColorId | null) { | ||
| if (id) { | ||
| document.documentElement.style.setProperty('--accent-color', `var(--swatch-${id})`) | ||
| } else { | ||
| document.documentElement.style.removeProperty('--accent-color') | ||
| } | ||
| preferences.value.accentColorId = id | ||
| } |
There was a problem hiding this comment.
Guard DOM access for SSR safety.
Similar to useBackgroundTheme, setAccentColor directly accesses document.documentElement.style, which will throw during SSR.
🛠️ Suggested fix
function setAccentColor(id: AccentColorId | null) {
+ if (import.meta.server) {
+ preferences.value.accentColorId = id
+ return
+ }
if (id) {
document.documentElement.style.setProperty('--accent-color', `var(--swatch-${id})`)
} else {
document.documentElement.style.removeProperty('--accent-color')
}
preferences.value.accentColorId = id
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function setAccentColor(id: AccentColorId | null) { | |
| if (id) { | |
| document.documentElement.style.setProperty('--accent-color', `var(--swatch-${id})`) | |
| } else { | |
| document.documentElement.style.removeProperty('--accent-color') | |
| } | |
| preferences.value.accentColorId = id | |
| } | |
| function setAccentColor(id: AccentColorId | null) { | |
| if (import.meta.server) { | |
| preferences.value.accentColorId = id | |
| return | |
| } | |
| if (id) { | |
| document.documentElement.style.setProperty('--accent-color', `var(--swatch-${id})`) | |
| } else { | |
| document.documentElement.style.removeProperty('--accent-color') | |
| } | |
| preferences.value.accentColorId = id | |
| } |
| function setBackgroundTheme(id: BackgroundThemeId | null) { | ||
| if (id) { | ||
| document.documentElement.dataset.bgTheme = id | ||
| } else { | ||
| document.documentElement.removeAttribute('data-bg-theme') | ||
| } | ||
| preferences.value.preferredBackgroundTheme = id | ||
| } |
There was a problem hiding this comment.
Guard DOM access for SSR safety.
setBackgroundTheme directly accesses document.documentElement, which will throw during SSR. Either guard the DOM manipulation or ensure this composable is only ever called client-side.
🛠️ Suggested fix
function setBackgroundTheme(id: BackgroundThemeId | null) {
+ if (import.meta.server) {
+ preferences.value.preferredBackgroundTheme = id
+ return
+ }
if (id) {
document.documentElement.dataset.bgTheme = id
} else {
document.documentElement.removeAttribute('data-bg-theme')
}
preferences.value.preferredBackgroundTheme = id
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function setBackgroundTheme(id: BackgroundThemeId | null) { | |
| if (id) { | |
| document.documentElement.dataset.bgTheme = id | |
| } else { | |
| document.documentElement.removeAttribute('data-bg-theme') | |
| } | |
| preferences.value.preferredBackgroundTheme = id | |
| } | |
| function setBackgroundTheme(id: BackgroundThemeId | null) { | |
| if (import.meta.server) { | |
| preferences.value.preferredBackgroundTheme = id | |
| return | |
| } | |
| if (id) { | |
| document.documentElement.dataset.bgTheme = id | |
| } else { | |
| document.documentElement.removeAttribute('data-bg-theme') | |
| } | |
| preferences.value.preferredBackgroundTheme = id | |
| } |
| const { preferences } = useUserPreferencesState() | ||
| const settingsLocale = preferences.value.selectedLocale | ||
|
|
||
| if ( | ||
| settingsLocale && | ||
| // Check if the value is a supported locale | ||
| locales.value.map(l => l.code).includes(settingsLocale) && | ||
| // Check if the value is not a current locale | ||
| settingsLocale !== locale.value | ||
| ) { | ||
| setLocale(settingsLocale) | ||
| const matchedLocale = locales.value.map(l => l.code).find(code => code === settingsLocale) | ||
|
|
||
| if (matchedLocale && matchedLocale !== locale.value) { | ||
| setLocale(matchedLocale) |
There was a problem hiding this comment.
Guard against unset preferences during early boot.
If preferences.value can be null/undefined before hydration, Line 10 will throw and prevent locale initialisation. A small null-guard keeps this safe.
Proposed fix
- const settingsLocale = preferences.value.selectedLocale
+ const settingsLocale = preferences.value?.selectedLocale📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { preferences } = useUserPreferencesState() | |
| const settingsLocale = preferences.value.selectedLocale | |
| if ( | |
| settingsLocale && | |
| // Check if the value is a supported locale | |
| locales.value.map(l => l.code).includes(settingsLocale) && | |
| // Check if the value is not a current locale | |
| settingsLocale !== locale.value | |
| ) { | |
| setLocale(settingsLocale) | |
| const matchedLocale = locales.value.map(l => l.code).find(code => code === settingsLocale) | |
| if (matchedLocale && matchedLocale !== locale.value) { | |
| setLocale(matchedLocale) | |
| const { preferences } = useUserPreferencesState() | |
| const settingsLocale = preferences.value?.selectedLocale | |
| const matchedLocale = locales.value.map(l => l.code).find(code => code === settingsLocale) | |
| if (matchedLocale && matchedLocale !== locale.value) { | |
| setLocale(matchedLocale) |
| // Get preferences and disable includeTypesInInstall directly | ||
| const { preferences } = useUserPreferencesState() | ||
| preferences.value.includeTypesInInstall = false |
There was a problem hiding this comment.
Avoid leaking preference state across tests.
useUserPreferencesProvider caches a module‑level ref, so mutating includeTypesInInstall here can persist into later tests even if localStorage is cleared. Reset it after the assertion (or in an afterEach) to keep tests isolated.
🧹 Suggested fix
expect(showTypesInInstall.value).toBe(false)
expect(fullInstallCommand.value).toBe('npm install express')
+ preferences.value.includeTypesInInstall = true| it('initializes with default values when storage is empty', () => { | ||
| mountWithSetup(() => { | ||
| const { preferences } = usePackageListPreferences() | ||
| onMounted(() => { | ||
| expect(preferences.value).toEqual(DEFAULT_PREFERENCES) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| it('loads and merges values from localStorage', () => { | ||
| mountWithSetup(() => { | ||
| const stored = { viewMode: 'table' } | ||
| setLocalStorage(stored) | ||
| const { preferences } = usePackageListPreferences() | ||
| onMounted(() => { | ||
| expect(preferences.value.viewMode).toBe('table') | ||
| expect(preferences.value.paginationMode).toBe(DEFAULT_PREFERENCES.paginationMode) | ||
| expect(preferences.value.pageSize).toBe(DEFAULT_PREFERENCES.pageSize) | ||
| expect(preferences.value.columns).toEqual(DEFAULT_PREFERENCES.columns) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| it('handles array merging by replacement', () => { | ||
| mountWithSetup(() => { | ||
| const stored = { columns: [] } | ||
| setLocalStorage(stored) | ||
| const { preferences } = usePackageListPreferences() | ||
| onMounted(() => { | ||
| expect(preferences.value.columns).toEqual([]) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Assertions inside onMounted are not awaited; tests can silently pass.
The test body finishes before onMounted runs, so failures may be missed. Consider awaiting nextTick() and asserting in the test body.
Suggested pattern (apply similarly to other tests)
-import { defineComponent, onMounted } from 'vue'
+import { defineComponent, nextTick } from 'vue'
-function mountWithSetup(run: () => void) {
- return mount(
+async function mountWithSetup<T>(setupFn: () => T) {
+ let result: T
+ const wrapper = mount(
defineComponent({
name: 'TestHarness',
setup() {
- run()
+ result = setupFn()
return () => null
},
}),
{ attachTo: document.body },
)
+ await nextTick()
+ return { wrapper, result: result! }
}
-it('initializes with default values when storage is empty', () => {
- mountWithSetup(() => {
- const { preferences } = usePackageListPreferences()
- onMounted(() => {
- expect(preferences.value).toEqual(DEFAULT_PREFERENCES)
- })
- })
-})
+it('initializes with default values when storage is empty', async () => {
+ const { result, wrapper } = await mountWithSetup(() => usePackageListPreferences())
+ expect(result.preferences.value).toEqual(DEFAULT_PREFERENCES)
+ wrapper.unmount()
+})
| onPrehydrate(el => { | ||
| const settings = JSON.parse(localStorage.getItem('npmx-settings') || '{}') | ||
| const id = settings.preferredBackgroundTheme | ||
| const preferences = JSON.parse(localStorage.getItem('npmx-user-preferences') || '{}') |
There was a problem hiding this comment.
here too we should try/catch the parse call and probably just fall back to {} like we do if it isn't set
gusa4grr
force-pushed
the
feat-484-persist-user-preferences
branch
from
21882ac to
7bd9568
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/nuxt/components/compare/PackageSelector.spec.ts (1)
112-113:⚠️ Potential issue | 🟡 MinorGuard against
undefinedbefore callingtrigger.The
.find()method on an array can returnundefinedif no element matches. Using!without a preceding check could cause a runtime error if the component's markup changes and no button contains the expected icon class.Suggested fix
const removeButton = component.findAll('button').find(b => b.find('.i-lucide\\:x').exists()) + expect(removeButton).toBeDefined() await removeButton!.trigger('click')As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
| @@ -0,0 +1,4 @@ | |||
| export function useRelativeDates() { | |||
There was a problem hiding this comment.
maybe this should also be called useRelativeDatesPreference?
since nuxt magic will make it global, so the name might end up ambiguous in other contexts
|
|
||
| function arePreferencesEqual(a: UserPreferences, b: UserPreferences): boolean { | ||
| const keys = Object.keys(DEFAULT_USER_PREFERENCES) as (keyof typeof DEFAULT_USER_PREFERENCES)[] | ||
| return keys.every(key => a[key] === b[key]) |
There was a problem hiding this comment.
what if we ever add or remove a key? should we also check the length here?
- introduce the foundational layer for persisting user preferences to the server - add UserPreferencesSchema and shared types for user preferences - add client-only sync composable with debounced saves, route guard flush, and sendBeacon fallback - integrate server sync into useSettings and migrate to shared UserPreferences type - extract generic localStorage helpers, migrate consumers, remove usePreferencesProvider
- extract sidebar collapsed state into separate `usePackageSidebarPreferences` composable - add `preferences-sync.client.ts` plugin for early color mode + server sync init - wrap theme select in `<ClientOnly>` to prevent SSR hydration mismatch - show sync status indicator on settings page for authenticated users - add `useColorModePreference` composable to sync color mode with `@nuxtjs/color-mode`
gusa4grr
force-pushed
the
feat-484-persist-user-preferences
branch
from
7bd9568 to
eec29e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
app/composables/useLocalStorageHashProvider.ts (1)
31-34: Rename thekeyparameter inupdateto avoid shadowing the outerkeyargument.The
key: Kparameter insideupdateshadows thekey: stringparameter of the enclosinguseLocalStorageHashProvider. There is no runtime bug (the outerkeyis already captured inprovider), but it is confusing and will triggerno-shadowlinting rules.♻️ Proposed rename
- function update<K extends keyof T>(key: K, value: T[K]) { - data.value[key] = value + function update<K extends keyof T>(field: K, value: T[K]) { + data.value[field] = value save() }shared/schemas/userPreferences.ts (1)
4-6:picklistcasts lose literal-type inference;UserPreferences.accentColorIdinfers asstring, notAccentColorId.
Object.keys()returnsstring[]and casting to[string, ...string[]]satisfies valibot's non-empty constraint, but throws away the string-literal union.InferOutput<typeof UserPreferencesSchema>then givesaccentColorId: string | null | undefinedrather thanAccentColorId | null | undefined, diverging from the separately exported type alias.Derive the keys with a type-preserving utility instead:
♻️ Proposed refactor
-const AccentColorIdSchema = picklist(Object.keys(ACCENT_COLORS.light) as [string, ...string[]]) +const accentColorKeys = Object.keys(ACCENT_COLORS.light) as [AccentColorId, ...AccentColorId[]] +const AccentColorIdSchema = picklist(accentColorKeys) -const BackgroundThemeIdSchema = picklist(Object.keys(BACKGROUND_THEMES) as [string, ...string[]]) +const backgroundThemeKeys = Object.keys(BACKGROUND_THEMES) as [BackgroundThemeId, ...BackgroundThemeId[]] +const BackgroundThemeIdSchema = picklist(backgroundThemeKeys)Note: the type aliases for
AccentColorIdandBackgroundThemeIdmust be declared before their use here (move theexport typelines above the schema constants).server/utils/preferences/user-preferences-store.ts (1)
42-48: Module-level singleton offers no benefit in serverless runtimes.In cold-start environments (Cloudflare Workers, Vercel Edge, AWS Lambda), modules are re-evaluated per invocation, so
storeInstanceis alwaysnullon entry. The pattern is harmless for correctness —UserPreferencesStoreis stateless beyond delegating to the KV storage — but the singleton comment and null-check add noise without delivering caching. If the singleton was intended to amortise any initialisation cost, note that it will not do so in serverless deployments.
| let dataRef: RemovableRef<HydratedUserPreferences> | null = null | ||
| let syncInitialized = false |
There was a problem hiding this comment.
Module-level syncInitialized prevents re-initialisation across test cases.
syncInitialized is a module-level singleton. Once initSync sets it to true in one test, all subsequent tests in the same process will see it as true and skip the watch registration. Tests for the dependents listed in the summary (use-preferences-provider.spec.ts, etc.) must call vi.resetModules() between cases — or the flag needs to be exported and reset, or moved into a ref that tests can mutate via a provided injection.
| const { | ||
| status, | ||
| lastSyncedAt, | ||
| error, | ||
| loadFromServer, | ||
| scheduleSync, | ||
| setupRouteGuard, | ||
| setupBeforeUnload, | ||
| } = useUserPreferencesSync() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "useUserPreferencesProvider.ts" -o -name "useUserPreferencesSync.ts" -o -name "useUserPreferencesSync.client.ts" | head -10Repository: npmx-dev/npmx.dev
Length of output: 159
🏁 Script executed:
fd -t f "useUserPreferences" --type ts --type tsx | head -20Repository: npmx-dev/npmx.dev
Length of output: 230
🏁 Script executed:
git ls-files | grep -i userpreference | head -20Repository: npmx-dev/npmx.dev
Length of output: 641
🏁 Script executed:
cat -n app/composables/useUserPreferencesProvider.tsRepository: npmx-dev/npmx.dev
Length of output: 4107
🏁 Script executed:
head -20 app/composables/useUserPreferencesSync.client.tsRepository: npmx-dev/npmx.dev
Length of output: 732
🏁 Script executed:
cat -n app/composables/userPreferences/useUserPreferencesState.tsRepository: npmx-dev/npmx.dev
Length of output: 337
🏁 Script executed:
cat -n app/composables/useUserPreferencesProvider.ts | head -60 | tail -20Repository: npmx-dev/npmx.dev
Length of output: 758
🏁 Script executed:
rg 'import.*useUserPreferencesSync' app/composables/useUserPreferencesProvider.ts -C 2Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
rg 'useUserPreferencesState\|useUserPreferencesProvider' app --type vue -l | head -10Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
cat -n app/composables/useUserPreferencesSync.client.ts | head -50Repository: npmx-dev/npmx.dev
Length of output: 1861
🏁 Script executed:
head -40 app/composables/useUserPreferencesProvider.ts | cat -nRepository: npmx-dev/npmx.dev
Length of output: 1884
🏁 Script executed:
rg 'useUserPreferencesState\|useUserPreferencesProvider' app -C 1 | head -60Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
find app -name "*.vue" -type f | xargs grep -l 'useUserPreferencesState\|useUserPreferencesProvider' 2>/dev/null | head -10Repository: npmx-dev/npmx.dev
Length of output: 104
🏁 Script executed:
head -20 app/pages/search.vueRepository: npmx-dev/npmx.dev
Length of output: 716
🏁 Script executed:
head -20 app/pages/settings.vueRepository: npmx-dev/npmx.dev
Length of output: 799
🏁 Script executed:
grep -n 'import.*useUserPreferencesSync' app/composables/useUserPreferencesProvider.tsRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
cat -n app/composables/useUserPreferencesProvider.ts | grep -A 5 -B 5 'import'Repository: npmx-dev/npmx.dev
Length of output: 1235
🏁 Script executed:
cat -n app/composables/useUserPreferencesProvider.ts | sed -n '1,60p'Repository: npmx-dev/npmx.dev
Length of output: 2582
🏁 Script executed:
rg 'import' app/composables/useUserPreferencesProvider.ts -nRepository: npmx-dev/npmx.dev
Length of output: 319
🏁 Script executed:
grep -n 'useUserPreferencesSync' app/composables/useUserPreferencesProvider.tsRepository: npmx-dev/npmx.dev
Length of output: 94
🏁 Script executed:
cat -n app/composables/useUserPreferencesProvider.ts | head -110 | tail -60Repository: npmx-dev/npmx.dev
Length of output: 1979
🏁 Script executed:
wc -l app/composables/useUserPreferencesProvider.tsRepository: npmx-dev/npmx.dev
Length of output: 110
🏁 Script executed:
cat app/composables/useUserPreferencesProvider.tsRepository: npmx-dev/npmx.dev
Length of output: 3337
🏁 Script executed:
rg 'useUserPreferencesSync' app/composables/ -B 5 -A 5Repository: npmx-dev/npmx.dev
Length of output: 2528
🏁 Script executed:
grep -n 'import\|from' app/composables/useUserPreferencesProvider.ts | head -5Repository: npmx-dev/npmx.dev
Length of output: 319
🏁 Script executed:
cat app/composables/userPreferences/useUserPreferencesSyncStatus.tsRepository: npmx-dev/npmx.dev
Length of output: 471
🏁 Script executed:
cat app/nuxt.config.ts 2>/dev/null | head -50Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
find app -name 'nuxt.config.*' -type f -exec cat {} \;Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
rg 'nuxt.config' . --type ts | head -5Repository: npmx-dev/npmx.dev
Length of output: 214
🏁 Script executed:
find . -maxdepth 2 -name 'nuxt.config.*'Repository: npmx-dev/npmx.dev
Length of output: 99
🏁 Script executed:
cat nuxt.config.tsRepository: npmx-dev/npmx.dev
Length of output: 10325
🏁 Script executed:
grep -n 'ssr\|server\|client' nuxt.config.tsRepository: npmx-dev/npmx.dev
Length of output: 289
Potential SSR crash: useUserPreferencesSync() is client-only but called unconditionally.
useUserPreferencesSync is defined in useUserPreferencesSync.client.ts. In Nuxt 4, .client.ts composables are excluded from the server bundle. If useUserPreferencesProvider is invoked during SSR (transitively via useUserPreferencesState() called in settings.vue or search.vue), line 54 will attempt to call an undefined function and the destructuring assignment will throw a TypeError.
The import.meta.server guard in initSync() (line 61) does not protect this unconditional call on line 54, which executes in the composable body during module evaluation on the server.
Consider guarding the call or wrapping the return with a no-op object on the server:
Suggested guard
+ if (import.meta.server) {
+ const noopStatus = ref<'idle' | 'syncing' | 'synced' | 'error'>('idle')
+ const noopDate = ref<string | null>(null)
+ const noopError = ref<Error | null>(null)
+ return {
+ data: preferences,
+ isAuthenticated: computed(() => false),
+ isSyncing: computed(() => false),
+ isSynced: computed(() => false),
+ hasError: computed(() => false),
+ syncError: noopError,
+ lastSyncedAt: noopDate,
+ initSync: async () => {},
+ }
+ }
+
const {
status,
lastSyncedAt,Alternatively, rename this file to useUserPreferencesProvider.client.ts if the composable is strictly client-only.
| async function loadFromServer(): Promise<UserPreferences> { | ||
| if (!isAuthenticated.value) { | ||
| return { ...DEFAULT_USER_PREFERENCES } | ||
| } | ||
|
|
||
| state.status.value = 'syncing' | ||
| const serverPreferences = await fetchServerPreferences() | ||
|
|
||
| if (serverPreferences) { | ||
| showSyncedStatus() | ||
| return serverPreferences | ||
| } | ||
|
|
||
| state.status.value = 'idle' | ||
| return { ...DEFAULT_USER_PREFERENCES } | ||
| } |
There was a problem hiding this comment.
loadFromServer silently returns defaults when the server call fails.
When fetchServerPreferences swallows a network/server error and returns null, loadFromServer resets status to 'idle' without setting state.error. An authenticated user whose prefs failed to load gets defaults with no indication — potentially overwriting locally staged changes on the next sync.
🛠️ Suggested fix
if (serverPreferences) {
showSyncedStatus()
return serverPreferences
}
- state.status.value = 'idle'
+ state.status.value = 'error'
+ state.error.value = 'Failed to load preferences from server'
return { ...DEFAULT_USER_PREFERENCES }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function loadFromServer(): Promise<UserPreferences> { | |
| if (!isAuthenticated.value) { | |
| return { ...DEFAULT_USER_PREFERENCES } | |
| } | |
| state.status.value = 'syncing' | |
| const serverPreferences = await fetchServerPreferences() | |
| if (serverPreferences) { | |
| showSyncedStatus() | |
| return serverPreferences | |
| } | |
| state.status.value = 'idle' | |
| return { ...DEFAULT_USER_PREFERENCES } | |
| } | |
| async function loadFromServer(): Promise<UserPreferences> { | |
| if (!isAuthenticated.value) { | |
| return { ...DEFAULT_USER_PREFERENCES } | |
| } | |
| state.status.value = 'syncing' | |
| const serverPreferences = await fetchServerPreferences() | |
| if (serverPreferences) { | |
| showSyncedStatus() | |
| return serverPreferences | |
| } | |
| state.status.value = 'error' | |
| state.error.value = 'Failed to load preferences from server' | |
| return { ...DEFAULT_USER_PREFERENCES } | |
| } |
| function setupRouteGuard(getPreferences: () => UserPreferences): void { | ||
| router.beforeEach(async (_to, _from, next) => { | ||
| if (hasPendingChanges && isAuthenticated.value) { | ||
| void flushPendingSync(getPreferences()) | ||
| } | ||
| next() | ||
| }) |
There was a problem hiding this comment.
setupRouteGuard accumulates navigation guards on repeated calls.
router.beforeEach returns a remove function that is discarded here. Each call adds another guard, so flushPendingSync fires once per registered guard on every navigation, causing duplicate saves.
🛠️ Suggested fix — deduplicate via module-level flag
+let routeGuardRegistered = false
function setupRouteGuard(getPreferences: () => UserPreferences): void {
+ if (routeGuardRegistered) return
+ routeGuardRegistered = true
+
router.beforeEach(async (_to, _from, next) => {
if (hasPendingChanges && isAuthenticated.value) {
void flushPendingSync(getPreferences())
}
next()
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function setupRouteGuard(getPreferences: () => UserPreferences): void { | |
| router.beforeEach(async (_to, _from, next) => { | |
| if (hasPendingChanges && isAuthenticated.value) { | |
| void flushPendingSync(getPreferences()) | |
| } | |
| next() | |
| }) | |
| let routeGuardRegistered = false | |
| function setupRouteGuard(getPreferences: () => UserPreferences): void { | |
| if (routeGuardRegistered) return | |
| routeGuardRegistered = true | |
| router.beforeEach(async (_to, _from, next) => { | |
| if (hasPendingChanges && isAuthenticated.value) { | |
| void flushPendingSync(getPreferences()) | |
| } | |
| next() | |
| }) | |
| } |
| window.addEventListener('beforeunload', () => { | ||
| if (hasPendingChanges && isAuthenticated.value) { | ||
| const preferences = getPreferences() | ||
| navigator.sendBeacon( | ||
| '/api/user/preferences', | ||
| new Blob([JSON.stringify(preferences)], { type: 'application/json' }), | ||
| ) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
setupBeforeUnload leaks listeners — no deduplication or cleanup.
Every call to useUserPreferencesSync() that subsequently calls setupBeforeUnload() stacks a new beforeunload/visibilitychange listener with no removeEventListener. In a SPA with component re-mounts or hot reload this accumulates, firing the save beacon multiple times per unload.
🛠️ Suggested fix — module-level guard
+let beforeUnloadListenerRegistered = false
function setupBeforeUnload(getPreferences: () => UserPreferences): void {
- if (import.meta.server) return
+ if (import.meta.server || beforeUnloadListenerRegistered) return
+ beforeUnloadListenerRegistered = true
- window.addEventListener('beforeunload', () => {
+ window.addEventListener('visibilitychange', () => {
+ if (document.visibilityState !== 'hidden') return
if (hasPendingChanges && isAuthenticated.value) {
// ...
}
})
}| const setLocale: typeof setNuxti18nLocale = locale => { | ||
| settings.value.selectedLocale = locale | ||
| preferences.value.selectedLocale = locale | ||
| return setNuxti18nLocale(locale) | ||
| } |
There was a problem hiding this comment.
preferences written before locale navigation is confirmed.
preferences.value.selectedLocale = locale is persisted synchronously, but if setNuxti18nLocale fails (e.g., navigation guard rejects), the stored locale is left pointing to an unvisited locale. Consider writing the preference only after the navigation resolves successfully, or reverting on failure.
| async set(did: string, preferences: UserPreferences): Promise<void> { | ||
| const withTimestamp: UserPreferences = { | ||
| ...preferences, | ||
| updatedAt: new Date().toISOString(), | ||
| } | ||
| await this.storage.setItem(did, withTimestamp) | ||
| } | ||
|
|
||
| async merge(did: string, partial: Partial<UserPreferences>): Promise<UserPreferences> { | ||
| const existing = await this.get(did) | ||
| const base = existing ?? { ...DEFAULT_USER_PREFERENCES } | ||
|
|
||
| const merged: UserPreferences = { | ||
| ...base, | ||
| ...partial, | ||
| updatedAt: new Date().toISOString(), | ||
| } | ||
|
|
||
| await this.set(did, merged) | ||
| return merged |
There was a problem hiding this comment.
merge returns an updatedAt that differs from what is stored.
merge computes updatedAt at T1 (line 30) and includes it in merged, then calls set(did, merged). Inside set, updatedAt is overwritten with a fresh timestamp T2 (line 18) before being persisted. The returned merged therefore carries T1, whilst the KV entry carries T2. Any caller that caches the return value and later compares it to a fresh get will observe an apparent modification it never made.
The cleanest fix is to have set own the timestamp and return the persisted object:
🛠️ Proposed fix
- async set(did: string, preferences: UserPreferences): Promise<void> {
+ async set(did: string, preferences: UserPreferences): Promise<UserPreferences> {
const withTimestamp: UserPreferences = {
...preferences,
updatedAt: new Date().toISOString(),
}
await this.storage.setItem(did, withTimestamp)
+ return withTimestamp
}
async merge(did: string, partial: Partial<UserPreferences>): Promise<UserPreferences> {
const existing = await this.get(did)
const base = existing ?? { ...DEFAULT_USER_PREFERENCES }
- const merged: UserPreferences = {
- ...base,
- ...partial,
- updatedAt: new Date().toISOString(),
- }
-
- await this.set(did, merged)
- return merged
+ return this.set(did, { ...base, ...partial })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async set(did: string, preferences: UserPreferences): Promise<void> { | |
| const withTimestamp: UserPreferences = { | |
| ...preferences, | |
| updatedAt: new Date().toISOString(), | |
| } | |
| await this.storage.setItem(did, withTimestamp) | |
| } | |
| async merge(did: string, partial: Partial<UserPreferences>): Promise<UserPreferences> { | |
| const existing = await this.get(did) | |
| const base = existing ?? { ...DEFAULT_USER_PREFERENCES } | |
| const merged: UserPreferences = { | |
| ...base, | |
| ...partial, | |
| updatedAt: new Date().toISOString(), | |
| } | |
| await this.set(did, merged) | |
| return merged | |
| async set(did: string, preferences: UserPreferences): Promise<UserPreferences> { | |
| const withTimestamp: UserPreferences = { | |
| ...preferences, | |
| updatedAt: new Date().toISOString(), | |
| } | |
| await this.storage.setItem(did, withTimestamp) | |
| return withTimestamp | |
| } | |
| async merge(did: string, partial: Partial<UserPreferences>): Promise<UserPreferences> { | |
| const existing = await this.get(did) | |
| const base = existing ?? { ...DEFAULT_USER_PREFERENCES } | |
| return this.set(did, { ...base, ...partial }) | |
| } |
| relativeDates: optional(boolean()), | ||
| /** Include @types/* package in install command for packages without built-in types */ | ||
| includeTypesInInstall: optional(boolean()), | ||
| /** Accent color theme ID (e.g., "rose", "amber", "emerald") */ |
There was a problem hiding this comment.
JSDoc example lists "rose" which is not a valid AccentColorId.
Valid values are coral | amber | emerald | sky | violet | magenta (from ACCENT_COLORS). "rose" is not in that set.
🛠️ Fix
- /** Accent color theme ID (e.g., "rose", "amber", "emerald") */
+ /** Accent color theme ID (e.g., "coral", "amber", "emerald") */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Accent color theme ID (e.g., "rose", "amber", "emerald") */ | |
| /** Accent color theme ID (e.g., "coral", "amber", "emerald") */ |
2 participants
Note: I came from a React background, quite a newbie to the Nuxt/Vue ecosystem. Please let me know if any patterns are misplaced. Happy to learn and adjust!
Core implementation details:
useUserLocalSettingscomposablepreferences-sync.client.tsplugin for early color mode + server sync init<ClientOnly>to prevent SSR hydration mismatchuseColorModePreferencecomposable to sync color mode with@nuxtjs/color-modeToDo:
searchProvider, which was added while this MR was openconnector, which was added while this MR was openQuestions:
npmx-settingsLS, as those will now live in separate place?client.tssuffix for useUserPreferencesSync.client.ts to ensure it is client-side only. Is this the standard convention to prevent server-side execution?preferences-sync.client.tsplugin for nowI noticed initPreferencesOnPrehydrate, which retrieves some settings from LS on the client, but it doesn't appear to support data fetching. Few other places also using
onPrehydrate.I am curious as we can load preferences during SSR too and can hydrate client with the preferences right away (if logged in). What files/places should I look at, any suggestions?
Closes #484
Needs to be discussed - Done ✅
During the implementation, I identified inconsistent local storage usage across the app:
npmx-color-mode- used for color mode. It is adjustable via the settings page, but is also evaluated bylighthouseand referenced in thenuxt.configcolorModepropertynpmx-list-prefs- used in search to modify the viewing experiencenpmx-settings- contains settings found in/settingsroute as well as unrelated sidebar states on package page (see image below)Based on the feature requirements, I decided to create the
user-preferencesschema specifically for the/settingspage configuration. However, the currentuseSettingshook combines both user-preferences and "sidebar states".I want to align with the team on the execution strategy before finalizing these changes.
Proposed plan:
npmx-color-mode), but connect to user-preference serviceThe solution I am drafting centralizes these options into a single user-preference service. However, if we include items outside of /settings, we need to consider: