fix: address settings-related hydration issues using prehydrate#1952
fix: address settings-related hydration issues using prehydrate#1952Adebesin-Cell wants to merge 11 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request consolidates search provider selection logic into a single SSR-safe composable. A new ChangesSearch Provider SSR Hydration Fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
app/composables/useSettings.ts (1)
77-88: Add explicit normalisation after merging persisted settings.The merge preserves stale/legacy values as-is. Please normalise constrained fields (for example
searchProvider,sidebar.collapsed, and numeric config bounds) before returning, so invalid old payloads cannot leak into runtime state.♻️ Suggested hardening
+function normaliseSettings(input: AppSettings): AppSettings { + return { + ...input, + searchProvider: input.searchProvider === 'npm' ? 'npm' : 'algolia', + sidebar: { + ...input.sidebar, + collapsed: Array.isArray(input.sidebar?.collapsed) + ? input.sidebar.collapsed.filter((v): v is string => typeof v === 'string') + : [], + }, + } +} + function readFromLocalStorage(): AppSettings { try { const raw = localStorage.getItem(STORAGE_KEY) if (raw) { const stored = JSON.parse(raw) - return { + return normaliseSettings({ ...DEFAULT_SETTINGS, ...stored, connector: { ...DEFAULT_SETTINGS.connector, ...stored.connector }, sidebar: { ...DEFAULT_SETTINGS.sidebar, ...stored.sidebar }, chartFilter: { ...DEFAULT_SETTINGS.chartFilter, ...stored.chartFilter }, - } + }) } } catch {} return { ...DEFAULT_SETTINGS } }Based on learnings: "defu only fills in null/undefined keys, so a stale persisted value survives the merge unchanged. Normalisation/migration of stale stored values must be done explicitly after hydration, not via defu defaults."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb8c5219-621a-43fb-9a9f-661ca5a5dd57
📒 Files selected for processing (2)
app/composables/useSettings.tsapp/utils/prehydrate.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| // Read localStorage eagerly but apply after mount to prevent hydration | ||
| // mismatch. During hydration, useState provides server-matching defaults. | ||
| // After mount, we swap in the user's actual preferences from localStorage. | ||
| // Uses nuxtApp.hook('app:mounted') instead of onMounted so it works even | ||
| // when useSettings() is first called from a plugin (no component context). |
There was a problem hiding this comment.
I don't think this is the right fix. right now, with onPrehydrate, in many cases we're able to reflect the user's preferences immediately without any flash after mounting.
There was a problem hiding this comment.
Hmm, good point. What approach do you think we could look toward here?
I tried a few iterations but was only able to get satisfactory results using the useState + localStorage pattern. Though I can see some flashes (related to client-side sorting, I believe)
The core issue is that useLocalStorage from VueUse causes hydration mismatches because the server renders with defaults while the client initializes with stored values immediately. Cos right now, when we load https://npmx.dev/~antfu on page load with npm registry as a setting.
The onPrehydrate script handles the visual/DOM side (accent color, background theme, collapsed sections), but the reactive state still needs to reconcile somehow.
Open to suggestions if you have a different pattern in mind, I might be missing something in the Nuxt hydration lifecycle that could help here.
…hydration # Conflicts: # app/composables/useSettings.ts # app/utils/prehydrate.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/composables/useSettings.ts (2)
82-96: Docstring mismatches the function's behaviour.The JSDoc for
normaliseSettingsdescribes reading fromlocalStorageand merging with defaults, but that is whatreadFromLocalStoragebelow does.normaliseSettingsonly sanitises thesearchProviderenum and thesidebar.collapsedarray.✏️ Proposed docstring
-/** - * Read settings from localStorage and merge with defaults. - */ +/** + * Sanitise potentially invalid persisted values (e.g. unknown + * `searchProvider` strings, non-string entries in `sidebar.collapsed`). + */ function normaliseSettings(input: AppSettings): AppSettings {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useSettings.ts` around lines 82 - 96, The JSDoc for normaliseSettings is incorrect: it does not read from localStorage or merge defaults but only sanitises fields; update the comment for the normaliseSettings function to state that it validates and normalises the provided AppSettings (ensuring searchProvider is 'npm' or 'algolia' and that sidebar.collapsed is a string[]), and remove any mention of localStorage or merging defaults (those belong to readFromLocalStorage); keep references to the same symbols (normaliseSettings, AppSettings, sidebar.collapsed, searchProvider) so reviewers can find the change.
140-157: Watcher triggers a redundantlocalStorage.setItemon every hydration.The
watchon line 149 is registered synchronously, thensettings.value = storedruns inside theapp:mountedhook on line 142. Because that reassignment is a mutation, the deep watcher fires and re-serialises the value back tolocalStorageon every page load — even when nothing has actually changed.You can avoid the round-trip by installing the watcher after the stored value has been applied, for example:
♻️ Proposed change
- if (nuxtApp.isHydrating) { - nuxtApp.hook('app:mounted', () => { - settings.value = stored - }) - } else { - settings.value = stored - } - - // Persist future changes back to localStorage - watch( - settings, - value => { - try { - localStorage.setItem(STORAGE_KEY, JSON.stringify(value)) - } catch {} - }, - { deep: true }, - ) + const startPersisting = () => { + watch( + settings, + value => { + try { + localStorage.setItem(STORAGE_KEY, JSON.stringify(value)) + } catch {} + }, + { deep: true }, + ) + } + + if (nuxtApp.isHydrating) { + nuxtApp.hook('app:mounted', () => { + settings.value = stored + startPersisting() + }) + } else { + settings.value = stored + startPersisting() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useSettings.ts` around lines 140 - 157, The watcher on settings is being registered before the stored value is applied during hydration, causing the initial assignment (in nuxtApp.hook('app:mounted') when nuxtApp.isHydrating is true) to trigger an unnecessary localStorage.setItem; to fix, register the watch only after the stored value has been applied — i.e. move the call to watch(settings, ...) so it runs inside the app:mounted hook (when using nuxtApp.hook('app:mounted', () => { settings.value = stored; /* then install watcher */ })) and in the non-hydration branch install the watcher after settings.value = stored; keep using STORAGE_KEY and the same try/catch serialization logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/composables/useSettings.ts`:
- Around line 82-96: The JSDoc for normaliseSettings is incorrect: it does not
read from localStorage or merge defaults but only sanitises fields; update the
comment for the normaliseSettings function to state that it validates and
normalises the provided AppSettings (ensuring searchProvider is 'npm' or
'algolia' and that sidebar.collapsed is a string[]), and remove any mention of
localStorage or merging defaults (those belong to readFromLocalStorage); keep
references to the same symbols (normaliseSettings, AppSettings,
sidebar.collapsed, searchProvider) so reviewers can find the change.
- Around line 140-157: The watcher on settings is being registered before the
stored value is applied during hydration, causing the initial assignment (in
nuxtApp.hook('app:mounted') when nuxtApp.isHydrating is true) to trigger an
unnecessary localStorage.setItem; to fix, register the watch only after the
stored value has been applied — i.e. move the call to watch(settings, ...) so it
runs inside the app:mounted hook (when using nuxtApp.hook('app:mounted', () => {
settings.value = stored; /* then install watcher */ })) and in the non-hydration
branch install the watcher after settings.value = stored; keep using STORAGE_KEY
and the same try/catch serialization logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e0c76ba-3e55-41d2-948f-81f491605b81
📒 Files selected for processing (2)
app/composables/useSettings.tsapp/utils/prehydrate.ts
…atch on org/user/search Revert the useSettings/prehydrate refactor and instead fix the actual cause of the empty-state on /org/[name] when the user has 'npm registry' selected: the async-data cache key embedded the localStorage-backed search provider, so SSR keyed by 'algolia' while client hydration keyed by 'npm', missing the SSR payload and dropping to the empty default. Introduces useEffectiveSearchProvider which defers the stored preference until app:mounted, keeping SSR and initial hydration on the same key. After mount the reactive key flips and useLazyAsyncData refetches once with the user's preference. URL ?p= still wins everywhere and remains cacheable per-URL. Closes npmx-dev#2753
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/composables/useGlobalSearch.ts`:
- Line 12: The navigation/URL update calls are still passing the reactive stored
setting (searchProvider.value) instead of the effective provider
(searchProviderValue) returned by useEffectiveSearchProvider(), which causes the
?p= override to be clobbered; update every call to updateUrlQueryImpl (and any
navigation functions that build the query) to pass searchProviderValue instead
of searchProvider.value so the effective provider (including URL overrides) is
used when updating the URL.
In `@app/composables/useSettings.ts`:
- Around line 238-243: The computed provider currently defaults to 'algolia'
when storedProviderReady is false, bypassing the established
DEFAULT_SETTINGS.searchProvider; update the computed (the return computed block
using normalizeSearchParam, storedProviderReady and searchProvider) to return
DEFAULT_SETTINGS.searchProvider as the fallback instead of the hard-coded
'algolia' so that when p is not 'npm' or 'algolia' and storedProviderReady is
false it uses DEFAULT_SETTINGS.searchProvider (preserving prior test/mount
behavior and keeping useEffectiveSearchProvider in sync with useSearchProvider).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e27f591d-1b82-451c-ba01-9bc02fc3c7b5
📒 Files selected for processing (4)
app/composables/npm/useOrgPackages.tsapp/composables/npm/useUserPackages.tsapp/composables/useGlobalSearch.tsapp/composables/useSettings.ts
… URL writes - useEffectiveSearchProvider falls back to DEFAULT_SETTINGS.searchProvider (not hardcoded 'algolia') so test env (default 'npm') no longer mismatches SSR vs hydration. - useGlobalSearch URL-write paths now use the effective provider so a ?p= override survives subsequent keystrokes/Enter.
|
Hey @danielroe @serhalp, found some time to look into this again. I reverted the initial The fix is now scoped to the data-fetching composables ( A new There's still a bit of delay for the first navigation while the npm provider refetches, worth a separate follow-up to keep the SSR data visible during that swap. |
3 participants
This is an attempt to fix a sudden flash and weird page display behavior, as seen here
Screen.Recording.2026-03-05.at.22.32.44.mov
The fix swaps useLocalStorage for useState (SSR-safe), paired with a localStorage sync after mount. During hydration, server and client agree on the same default values.
Closes #1948
AFTER FIX:
Screen.Recording.2026-03-05.at.22.32.58.mov