refactor: move from NuxtApp augment to NuxtPayload#240
Conversation
commit: |
📝 WalkthroughWalkthroughThis pull request refactors the storage location of hints and state data structures within the Nuxt application. Hints previously stored directly on the NuxtApp instance ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (2)
src/runtime/web-vitals/plugin.client.ts (1)
40-42: Consider cachingnuxtApp.payload.__hints.webvitalsin a local variable.The third-party-scripts plugin caches the ref locally (
const scripts = nuxtApp.payload.__hints.thirdPartyScripts). The same pattern would reduce repetition here:♻️ Suggested refactor
nuxtApp.payload.__hints = defu(nuxtApp.payload.__hints, { webvitals: { lcp: ref([]), inp: ref([]), cls: ref([]), }, }) + const webvitals = nuxtApp.payload.__hints.webvitals nuxtApp.hook('hints:webvitals:sync', (webvitals) => { - webvitals.lcp.value = [...nuxtApp.payload.__hints.webvitals.lcp.value] - webvitals.inp.value = [...nuxtApp.payload.__hints.webvitals.inp.value] - webvitals.cls.value = [...nuxtApp.payload.__hints.webvitals.cls.value] + webvitals.lcp.value = [...nuxtApp.payload.__hints.webvitals.lcp.value] + webvitals.inp.value = [...nuxtApp.payload.__hints.webvitals.inp.value] + webvitals.cls.value = [...nuxtApp.payload.__hints.webvitals.cls.value] })Note: The hook callback parameter shadows the outer
webvitalsname, so the sync hook body must still use the full path (or rename the parameter). But the metric push sites can use the local:- nuxtApp.payload.__hints.webvitals.inp.value.push(metric) + webvitals.inp.value.push(metric)(Apply similarly for
lcpandclspushes.)Also applies to: 54-54, 68-68, 133-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/web-vitals/plugin.client.ts` around lines 40 - 42, Cache nuxtApp.payload.__hints.webvitals into a local constant (e.g. const webvitalsHints = nuxtApp.payload.__hints.webvitals) and use webvitalsHints.lcp.value, webvitalsHints.inp.value and webvitalsHints.cls.value when assigning to webvitals.lcp.value / webvitals.inp.value / webvitals.cls.value to avoid repeated deep access; be careful that any hook callback parameter named webvitals may shadow the outer variable so either rename the parameter or keep full path inside the hook while using the cached webvitalsHints for the metric push sites (apply same change for the other occurrences mentioned).src/runtime/types.d.ts (1)
41-56:__hintsand its sub-properties are typed as non-optional, but initialized piecemeal by separate plugins.Each plugin initializes only its own slice via
defu(e.g., the hydration plugin only setshydration: []). Between plugin setup calls, properties likewebvitalsorthirdPartyScriptswon't exist yet, even though the type declares them as required. If plugin execution order changes or a consumer readspayload.__hintsbefore all plugins run, this will produce a runtime error despite TypeScript being satisfied.Consider either:
- Making each sub-property optional (
hydration?: ...,webvitals?: ..., etc.), or- Adding a single centralized initialization plugin (with high priority) that sets up the full
__hintsobject once.Option 1: Make sub-properties optional
interface NuxtPayload { __hints: { lazyHydrationState?: LazyHydrationState - hydration: LocalHydrationMismatch[] - lazyComponents: DirectImportInfo[] - webvitals: { + hydration?: LocalHydrationMismatch[] + lazyComponents?: DirectImportInfo[] + webvitals?: { lcp: Ref<LCPMetricWithAttribution[]> inp: Ref<INPMetricWithAttribution[]> cls: Ref<CLSMetricWithAttribution[]> } - thirdPartyScripts: Ref<{ + thirdPartyScripts?: Ref<{ element: HTMLScriptElement loaded: boolean }[]> } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/types.d.ts` around lines 41 - 56, The NuxtPayload.__hints object declares required sub-properties that are populated piecemeal by separate plugins, risking runtime undefined access; update the NuxtPayload interface so each sub-property under __hints (e.g., lazyHydrationState, hydration, lazyComponents, webvitals, thirdPartyScripts) is optional (use ? types) OR alternatively implement a single high-priority initialization plugin that creates a complete __hints object before other plugins run; modify the declaration for NuxtPayload::__hints or add the init plugin to ensure all consumers can safely access those fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/runtime/lazy-load/composables.test.ts`:
- Around line 92-96: The test reads nuxtApp.payload.__hints?.lazyHydrationState
which types state as possibly undefined, causing TypeScript errors when
accessing properties even though setup!() guarantees initialization; remove the
optional chaining so you read nuxtApp.payload.__hints.lazyHydrationState (and
similarly at the other occurrences around the reads at lines referencing state)
so state is non-nullable, then keep the runtime expect(state).toBeDefined()
assertions for safety — update the accesses in this file (the variable named
state and the expression nuxtApp.payload.__hints?.lazyHydrationState) to use the
non-optional form.
---
Nitpick comments:
In `@src/runtime/types.d.ts`:
- Around line 41-56: The NuxtPayload.__hints object declares required
sub-properties that are populated piecemeal by separate plugins, risking runtime
undefined access; update the NuxtPayload interface so each sub-property under
__hints (e.g., lazyHydrationState, hydration, lazyComponents, webvitals,
thirdPartyScripts) is optional (use ? types) OR alternatively implement a single
high-priority initialization plugin that creates a complete __hints object
before other plugins run; modify the declaration for NuxtPayload::__hints or add
the init plugin to ensure all consumers can safely access those fields.
In `@src/runtime/web-vitals/plugin.client.ts`:
- Around line 40-42: Cache nuxtApp.payload.__hints.webvitals into a local
constant (e.g. const webvitalsHints = nuxtApp.payload.__hints.webvitals) and use
webvitalsHints.lcp.value, webvitalsHints.inp.value and webvitalsHints.cls.value
when assigning to webvitals.lcp.value / webvitals.inp.value /
webvitals.cls.value to avoid repeated deep access; be careful that any hook
callback parameter named webvitals may shadow the outer variable so either
rename the parameter or keep full path inside the hook while using the cached
webvitalsHints for the metric push sites (apply same change for the other
occurrences mentioned).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
client/app/composables/host.tsclient/app/plugins/hydration.tssrc/runtime/hydration/composables.tssrc/runtime/hydration/plugin.client.tssrc/runtime/lazy-load/composables.tssrc/runtime/lazy-load/plugin.client.tssrc/runtime/third-party-scripts/plugin.client.tssrc/runtime/types.d.tssrc/runtime/web-vitals/plugin.client.tstest/runtime/lazy-load/composables.test.ts
| const nuxtApp = useNuxtApp() | ||
| const state = nuxtApp.payload._lazyHydrationState | ||
| const state = nuxtApp.payload.__hints?.lazyHydrationState | ||
| expect(state).toBeDefined() | ||
| expect(state.directImports.get('Child')).toEqual(imports[0]) | ||
| }) |
There was a problem hiding this comment.
state is typed T | undefined after ?. — unguarded property accesses may cause TypeScript errors.
nuxtApp.payload.__hints?.lazyHydrationState infers LazyHydrationState | undefined. expect(state).toBeDefined() guards the runtime path but TypeScript's control-flow analysis does not narrow through Vitest assertions, so state.directImports (line 95) is a potential strict-mode type error. The same pattern repeats at lines 124 and 145 (line 145 lacks even a toBeDefined() guard before property access at 147–149).
Since setup!() is called before each read, __hints.lazyHydrationState is guaranteed to be initialized. Drop the optional chain to get a non-nullable type:
🛡️ Proposed fix (same pattern applies to lines 124 and 145)
- const state = nuxtApp.payload.__hints?.lazyHydrationState
+ const state = nuxtApp.payload.__hints!.lazyHydrationState
expect(state).toBeDefined()
expect(state.directImports.get('Child')).toEqual(imports[0])📝 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 nuxtApp = useNuxtApp() | |
| const state = nuxtApp.payload._lazyHydrationState | |
| const state = nuxtApp.payload.__hints?.lazyHydrationState | |
| expect(state).toBeDefined() | |
| expect(state.directImports.get('Child')).toEqual(imports[0]) | |
| }) | |
| const nuxtApp = useNuxtApp() | |
| const state = nuxtApp.payload.__hints!.lazyHydrationState | |
| expect(state).toBeDefined() | |
| expect(state.directImports.get('Child')).toEqual(imports[0]) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/runtime/lazy-load/composables.test.ts` around lines 92 - 96, The test
reads nuxtApp.payload.__hints?.lazyHydrationState which types state as possibly
undefined, causing TypeScript errors when accessing properties even though
setup!() guarantees initialization; remove the optional chaining so you read
nuxtApp.payload.__hints.lazyHydrationState (and similarly at the other
occurrences around the reads at lines referencing state) so state is
non-nullable, then keep the runtime expect(state).toBeDefined() assertions for
safety — update the accesses in this file (the variable named state and the
expression nuxtApp.payload.__hints?.lazyHydrationState) to use the non-optional
form.
1 participant
🔗 Linked issue
❓ Type of change
📚 Description