fix(script): harden lifecycle and SDK loading - #850
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
# Conflicts: # packages/script/src/module.ts # packages/script/src/runtime/server/instagram-embed.ts # packages/script/src/runtime/server/proxy-handler.ts # packages/script/src/runtime/server/utils/cached-upstream.ts # pnpm-lock.yaml
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change adds explicit cancellation and teardown across devtools, script instances, triggers, registries, components, and server handlers. It bounds retained devtools data, request bodies, and caches. It adds Unhead source-less loader detection and NPM loading support. It updates Google Maps and third-party component cleanup. Tests cover lifecycle behavior, server limits, cache configuration, and Unhead v3 compatibility. Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/devtools-app/composables/state.ts (1)
135-154: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the current script entry after a deduplicated fetch.
A second
syncScriptscall can replacescripts.valuewhile this request is active. The second call skips the request becausescriptFetchescontainsscriptSizeKey.When the first request completes, Line 144 updates the obsolete
scriptobject from the first call. The current entry can remain withoutscript.size.Update the current entries in
scripts.value, or copy cached size and error values into each new script object.Proposed fix
if (res.size) { scriptSizes[scriptSizeKey] = res.size - script.size = res.size + for (const currentScript of Object.values(scripts.value)) { + if (currentScript.src === scriptSizeKey) + currentScript.size = res.size + } } if (res.error) { scriptErrors[scriptSizeKey] = res.error instanceof Error ? res.error.message : String(res.error) - script.error = scriptErrors[scriptSizeKey] + for (const currentScript of Object.values(scripts.value)) { + if (currentScript.src === scriptSizeKey) + currentScript.error = scriptErrors[scriptSizeKey] + } }🤖 Prompt for 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. In `@packages/devtools-app/composables/state.ts` around lines 135 - 154, Update the fetch completion logic in syncScripts to locate the current script entry in scripts.value by scriptSizeKey before assigning size or error fields, rather than updating the stale script captured by the original request. Preserve the existing deduplicated request and cache updates, and ensure the replacement entry receives any fetched size and error values.
🧹 Nitpick comments (5)
packages/script/src/runtime/composables/useScript.ts (1)
404-404: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
!existscondition.The function returns at Line 325 when
existsis true. This branch is unreachable withexists === true, so!existsis always true here.♻️ Proposed cleanup
- if (import.meta.client && debugEnabled && !exists) { + if (import.meta.client && debugEnabled) {🤖 Prompt for 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. In `@packages/script/src/runtime/composables/useScript.ts` at line 404, In the branch guarded by import.meta.client and debugEnabled, remove the redundant !exists condition because the earlier return already excludes exists === true; preserve the existing branch behavior and other guards.packages/script/src/runtime/composables/useScriptTriggerInteraction.ts (1)
59-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the inner
cleanupbinding.The inner
const cleanupat Line 60 shadows the outercleanupdefined at Line 32. Both names describe different scopes. Usestopfor the per-listener handle.♻️ Proposed rename
events.forEach((event) => { - const cleanup = useEventListener( + const stop = useEventListener( target, event, () => { settle(true) }, { once: true, passive: true }, ) - cleanupFns.push(cleanup) + cleanupFns.push(stop) })🤖 Prompt for 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. In `@packages/script/src/runtime/composables/useScriptTriggerInteraction.ts` around lines 59 - 69, Rename the per-listener const binding inside the events.forEach callback from cleanup to stop, and update the cleanupFns.push argument accordingly; leave the outer cleanup binding unchanged.packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue (1)
389-405: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCreate the
importLibrarywatcher in a detached effect scope.
createAbortablePromisecalls its setup synchronously, sowatchis registered under the currently active component setup scope. A child can callmapsApi.importLibrary('marker')while mounting; if that child unmounts before the script/API loads, Vue stops the watcher and the cachedlibrariespromise never settles unless the parent component abortslifecycleController. Create the watcher with a detachedeffectScope(true)and stop that scope on abort/cleanup.🤖 Prompt for 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. In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue` around lines 389 - 405, Update the importLibrary promise setup around createAbortablePromise so the mapsApi watcher is created inside a detached effectScope(true), preventing child component unmounts from stopping it. Keep a reference to the scope and stop it during abort/cleanup alongside the existing watcher cleanup, while preserving the current resolution and rejection behavior.test/unit/youtube-player-lifecycle.test.ts (1)
39-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the
removewrapper is installed.The current assertions pass if
shared.removeremainsmocks.remove. All equality checks still pass, andfirst.remove()still calls the base mock once. Assert thatdecoratedRemovediffers frommocks.removebefore creating the second handle.Proposed assertion
const first = useScriptYouTubePlayer({}) const decoratedRemove = mocks.shared.remove + expect(decoratedRemove).not.toBe(mocks.remove) const second = useScriptYouTubePlayer({})🤖 Prompt for 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. In `@test/unit/youtube-player-lifecycle.test.ts` around lines 39 - 53, In the test “decorates the shared remove method only once across Vue proxies,” assert immediately after capturing decoratedRemove that it is different from mocks.remove, before creating the second handle. Keep the existing identity and invocation assertions unchanged.test/unit/google-maps-lifecycle.test.ts (1)
493-500: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake detached-watcher cleanup observable.
Line 493 does not prove that
waitForMapsReadyinstalled the detached watcher before abort. After line 496 rejects, a leaked watcher can still callresolve(), butcreateAbortablePromiseignores it because it already settled. This test can pass ifscope.stop()is removed. Wait for observable watcher registration, then assert that later ref updates cause no watcher reads or callbacks.Verify this with a mutation test that removes
return () => scope.stop()frompackages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.ts; this test must fail.🤖 Prompt for 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. In `@test/unit/google-maps-lifecycle.test.ts` around lines 493 - 500, Strengthen the test around waitForMapsReady so it waits for observable detached-watcher registration before calling controller.abort(). After rejection, spy on or otherwise track watcher reads/callbacks, mutate mapsApi and map, and assert no further reads or callbacks occur. Ensure the test fails if the cleanup returned by waitForMapsReady, including scope.stop(), is removed.
🤖 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 `@packages/script/src/devtools.ts`:
- Around line 100-118: Update the request-body handling in onData so chunks are
buffered without decoding each one; after the request ends, decode the complete
byte sequence once before JSON parsing. Preserve the existing
DEVTOOLS_API_MAX_BODY_SIZE limit, cleanup, oversized-response behavior, and
normal parsing flow.
In `@packages/script/src/runtime/components/ScriptCarbonAds.vue`:
- Around line 88-91: Update the unmount cleanup in ScriptCarbonAds.vue so it no
longer removes or nulls the component root referenced by carbonadsEl; leave that
container in place for Vue and page-transition cleanup. Retain removal of only
the injected script element, following the established behavior documented in
ScriptGoogleMaps.vue.
In `@packages/script/src/runtime/components/ScriptLemonSqueezy.vue`:
- Around line 41-65: Replace the single activeLemonSqueezyOwner flow with a
module-level registry and shared dispatcher that tracks each live component’s
event handler and fans events out to all registered handlers. Add registration
in onLoaded using the component owner and its lemonSqueezyEvent emitter,
unregister in onBeforeUnmount, and clear Lemon.js’s global handler only when the
registry is empty. Ensure the dispatcher is re-installed after script reloads
through Setup.
In `@packages/script/src/runtime/registry/usercentrics.ts`:
- Around line 123-180: Re-arm the readiness lifecycle in the stable instance
load path: reset readyApi and readyPromise, create a fresh readyController,
clear disposed, and re-attach the app:unmount cleanup hook after
instance.remove() has disposed the prior state. Ensure the subsequent load still
performs consent setup rather than being skipped due to the removed-instance
state, while preserving existing readiness and cleanup behavior.
In `@packages/script/src/runtime/registry/youtube-player.ts`:
- Around line 41-43: Move readyPromise, readyController, and the YouTube
API-ready handler restoration cleanup from the per-call useScriptYouTubePlayer
closure onto the shared script instance passed through clientInit. Reuse that
instance state across calls, ensure remove() aborts the shared controller and
restores the shared global handler, and avoid replacing readiness state when
another useScriptYouTubePlayer call reuses the instance.
In `@test/unit/script-trigger-lifecycle.test.ts`:
- Around line 22-25: Update the test cleanup around the navigator.serviceWorker
mutation to restore the original property descriptor after each test. Capture
the descriptor before the replacement in the relevant setup, then have afterEach
restore it or delete the own property when none existed; keep the existing timer
and mock cleanup unchanged.
---
Outside diff comments:
In `@packages/devtools-app/composables/state.ts`:
- Around line 135-154: Update the fetch completion logic in syncScripts to
locate the current script entry in scripts.value by scriptSizeKey before
assigning size or error fields, rather than updating the stale script captured
by the original request. Preserve the existing deduplicated request and cache
updates, and ensure the replacement entry receives any fetched size and error
values.
---
Nitpick comments:
In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Around line 389-405: Update the importLibrary promise setup around
createAbortablePromise so the mapsApi watcher is created inside a detached
effectScope(true), preventing child component unmounts from stopping it. Keep a
reference to the scope and stop it during abort/cleanup alongside the existing
watcher cleanup, while preserving the current resolution and rejection behavior.
In `@packages/script/src/runtime/composables/useScript.ts`:
- Line 404: In the branch guarded by import.meta.client and debugEnabled, remove
the redundant !exists condition because the earlier return already excludes
exists === true; preserve the existing branch behavior and other guards.
In `@packages/script/src/runtime/composables/useScriptTriggerInteraction.ts`:
- Around line 59-69: Rename the per-listener const binding inside the
events.forEach callback from cleanup to stop, and update the cleanupFns.push
argument accordingly; leave the outer cleanup binding unchanged.
In `@test/unit/google-maps-lifecycle.test.ts`:
- Around line 493-500: Strengthen the test around waitForMapsReady so it waits
for observable detached-watcher registration before calling controller.abort().
After rejection, spy on or otherwise track watcher reads/callbacks, mutate
mapsApi and map, and assert no further reads or callbacks occur. Ensure the test
fails if the cleanup returned by waitForMapsReady, including scope.stop(), is
removed.
In `@test/unit/youtube-player-lifecycle.test.ts`:
- Around line 39-53: In the test “decorates the shared remove method only once
across Vue proxies,” assert immediately after capturing decoratedRemove that it
is different from mocks.remove, before creating the second handle. Keep the
existing identity and invocation assertions unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e3d2723-6248-4f88-bbc0-82974490c355
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (47)
.nuxtrcpackages/devtools-app/composables/rpc.tspackages/devtools-app/composables/state.tspackages/devtools-app/utils/fetch.tspackages/script/src/devtools.tspackages/script/src/module.tspackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vuepackages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.tspackages/script/src/runtime/components/ScriptCarbonAds.vuepackages/script/src/runtime/components/ScriptCrisp.vuepackages/script/src/runtime/components/ScriptIntercom.vuepackages/script/src/runtime/components/ScriptLemonSqueezy.vuepackages/script/src/runtime/components/ScriptPayPalButtons.vuepackages/script/src/runtime/components/ScriptPayPalMessages.vuepackages/script/src/runtime/components/ScriptStripePricingTable.vuepackages/script/src/runtime/components/ScriptVimeoPlayer.vuepackages/script/src/runtime/composables/useScript.tspackages/script/src/runtime/composables/useScriptEventPage.tspackages/script/src/runtime/composables/useScriptTriggerIdleTimeout.tspackages/script/src/runtime/composables/useScriptTriggerInteraction.tspackages/script/src/runtime/composables/useScriptTriggerServiceWorker.tspackages/script/src/runtime/devtools-standalone-bridge.client.tspackages/script/src/runtime/npm-script-stub.tspackages/script/src/runtime/registry/speedcurve.tspackages/script/src/runtime/registry/usercentrics.tspackages/script/src/runtime/registry/youtube-player.tspackages/script/src/runtime/server/instagram-embed.tspackages/script/src/runtime/server/proxy-handler.tspackages/script/src/runtime/server/utils/cache-config.tspackages/script/src/runtime/server/utils/cached-upstream.tspackages/script/src/runtime/utils/abortable-promise.tspackages/script/src/runtime/utils/after-next-paint.tspnpm-workspace.yamltest/e2e/unhead-v3-compat.test.tstest/fixtures/unhead-v3/app.vuetest/fixtures/unhead-v3/nuxt.config.tstest/fixtures/unhead-v3/package.jsontest/fixtures/unhead-v3/public/fixture-api.jstest/unit/abortable-promise.test.tstest/unit/cached-upstream-lifecycle.test.tstest/unit/google-maps-lifecycle.test.tstest/unit/npm-script-stub-lifecycle.test.tstest/unit/script-trigger-lifecycle.test.tstest/unit/speedcurve-after-next-paint.test.tstest/unit/use-script-lifecycle.test.tstest/unit/youtube-player-lifecycle.test.tsvitest.config.ts
harlan-zw
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/script/src/module.ts (5)
1209-1222: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winInitialize the bounded cache mount when cache consumption is possible.
enabledEndpointscomes only from enabled registry scripts withserverHandlers, but the proxy handler is registered unconditionally. Proxy-only registry scripts such asplausibleAnalytics,cloudflareWebAnalytics, orvimeoPlayercan hit Nitro’s default unbounded memory storage forNUXT_SCRIPTS_CACHE_BASEif they use the cached upstream fetch helpers; base the mount initialization on actual proxy/cache capability.🤖 Prompt for 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. In `@packages/script/src/module.ts` around lines 1209 - 1222, Update the cache mount initialization around enabledEndpoints so it runs whenever proxy or other cached-fetch capability can access NUXT_SCRIPTS_CACHE_BASE, not only when enabled registry scripts expose serverHandlers. Use the existing proxy/cache capability signals and preserve the bounded lru-cache configuration plus application-supplied mount behavior.
171-175: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not create a new secret after a lock timeout.
withProxySecretFileLockthrowsETIMEDOUTat Line 173. This catch treats the timeout as a generic write failure and returns a newmemory-generatedsecret. Another process may already own and persist a different secret. Signed proxy URLs can then fail across processes or after restart.Handle
ETIMEDOUTseparately. Re-read the persisted secret after a retry, or fail setup. Use an in-memory secret only when no competing lock owner exists.Also applies to: 269-273
🤖 Prompt for 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. In `@packages/script/src/module.ts` around lines 171 - 175, Update the catch handling around withProxySecretFileLock in the proxy secret setup flow to handle ETIMEDOUT separately instead of falling back to a new memory-generated secret. After a timeout, retry reading the persisted secret and use it if available; otherwise fail setup, ensuring memory-generated secrets are only used when no competing lock owner exists.
158-170: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake stale-lock recovery ownership-safe.
Lines 165-170 can delete a live lock. If one process pauses for more than
PROXY_SECRET_LOCK_TIMEOUT_MS, another process unlinks the lock and acquires the same path. Both.envupdates can then run concurrently. The original process can also remove the replacement lock during cleanup.Use an ownership-aware lock implementation. Do not remove a lock only because its
mtimeMsis old.🤖 Prompt for 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. In `@packages/script/src/module.ts` around lines 158 - 170, Update the lock acquisition flow around existingLock and the unlink cleanup to use ownership-aware locking rather than deleting any lock solely because its mtimeMs exceeds PROXY_SECRET_LOCK_TIMEOUT_MS. Ensure stale-lock recovery verifies the lock still belongs to the process before removal, and prevent the original process from deleting a replacement lock during cleanup while preserving safe acquisition and concurrent .env update protection.
220-225: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep
resolveProxySecretsynchronous.
resolveProxySecretis exported and is currently only called synchronously inpackages/script/src/module.ts:1253. Making itasyncchanges the exported API contract without a documented bump, and callers must wrap it withawait.🤖 Prompt for 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. In `@packages/script/src/module.ts` around lines 220 - 225, Remove the async modifier from the exported resolveProxySecret function and keep its return behavior synchronous, preserving the existing ResolvedProxySecret | undefined contract for its synchronous callers.
253-260: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: Internal
Write the generated proxy secret with restrictive permissions.
resolveProxySecret()auto-writesNUXT_SCRIPTS_PROXY_SECRETinto.env, while created files get platform defaults and existing files keep their current permissions. A shared POSIX workspace can expose the HMAC secret to local users who can use it to forge signed proxy URLs or page tokens.When generating the secret directly to
.env, create the file with0o600and re-apply that mode when updating existing files. Keep the fallback to in-memory-only when this cannot be written securely.🤖 Prompt for 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. In `@packages/script/src/module.ts` around lines 253 - 260, Update the .env write logic in resolveProxySecret so newly generated files use mode 0o600 and existing files are re-applied to that mode after updates, including both replacement and append paths. If secure writing or permission enforcement fails, preserve the existing fallback to an in-memory-only secret.
🤖 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 `@packages/script/src/module.ts`:
- Around line 633-635: Update the unheadSourceLessScriptLoader initialization to
guard the scriptsTypesPath file read, including unreadable paths and
directories, so any filesystem error results in false and preserves
compatibility mode; retain the existing existence check and
hasUnheadSourceLessLoader behavior for successfully readable files.
In `@packages/script/src/runtime/utils/youtube-readiness.ts`:
- Around line 35-47: Update the previous readiness callback invocation in
onReady to call previousReady with target as its this receiver, preserving the
existing error handling and finally resolve flow.
---
Outside diff comments:
In `@packages/script/src/module.ts`:
- Around line 1209-1222: Update the cache mount initialization around
enabledEndpoints so it runs whenever proxy or other cached-fetch capability can
access NUXT_SCRIPTS_CACHE_BASE, not only when enabled registry scripts expose
serverHandlers. Use the existing proxy/cache capability signals and preserve the
bounded lru-cache configuration plus application-supplied mount behavior.
- Around line 171-175: Update the catch handling around withProxySecretFileLock
in the proxy secret setup flow to handle ETIMEDOUT separately instead of falling
back to a new memory-generated secret. After a timeout, retry reading the
persisted secret and use it if available; otherwise fail setup, ensuring
memory-generated secrets are only used when no competing lock owner exists.
- Around line 158-170: Update the lock acquisition flow around existingLock and
the unlink cleanup to use ownership-aware locking rather than deleting any lock
solely because its mtimeMs exceeds PROXY_SECRET_LOCK_TIMEOUT_MS. Ensure
stale-lock recovery verifies the lock still belongs to the process before
removal, and prevent the original process from deleting a replacement lock
during cleanup while preserving safe acquisition and concurrent .env update
protection.
- Around line 220-225: Remove the async modifier from the exported
resolveProxySecret function and keep its return behavior synchronous, preserving
the existing ResolvedProxySecret | undefined contract for its synchronous
callers.
- Around line 253-260: Update the .env write logic in resolveProxySecret so
newly generated files use mode 0o600 and existing files are re-applied to that
mode after updates, including both replacement and append paths. If secure
writing or permission enforcement fails, preserve the existing fallback to an
in-memory-only secret.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fd8896f-a7ac-42fa-ae0a-8ba5aae8628f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
packages/script/src/devtools.tspackages/script/src/module.tspackages/script/src/runtime/components/ScriptCarbonAds.vuepackages/script/src/runtime/components/ScriptLemonSqueezy.vuepackages/script/src/runtime/registry/posthog.tspackages/script/src/runtime/registry/usercentrics.tspackages/script/src/runtime/registry/youtube-player.tspackages/script/src/runtime/server/proxy-handler.tspackages/script/src/runtime/server/utils/cached-upstream.tspackages/script/src/runtime/unhead-features.tspackages/script/src/runtime/utils.tspackages/script/src/runtime/utils/usercentrics-consent.tspackages/script/src/runtime/utils/youtube-readiness.tspackages/script/src/unhead-features.tspnpm-workspace.yamltest/e2e/unhead-v3-compat.test.tstest/fixtures/unhead-v3/app.vuetest/nuxt-runtime/consent-default.nuxt.test.tstest/nuxt-runtime/script-component-lifecycle.nuxt.test.tstest/unit/cached-upstream-lifecycle.test.tstest/unit/devtools-lifecycle.test.tstest/unit/script-trigger-lifecycle.test.tstest/unit/unhead-features.test.tstest/unit/usercentrics-lifecycle.test.tstest/unit/utils.test.tstest/unit/youtube-player-lifecycle.test.ts
💤 Files with no reviewable changes (1)
- packages/script/src/runtime/components/ScriptCarbonAds.vue
🚧 Files skipped from review as they are similar to previous changes (8)
- pnpm-workspace.yaml
- packages/script/src/runtime/server/utils/cached-upstream.ts
- test/unit/cached-upstream-lifecycle.test.ts
- packages/script/src/runtime/registry/usercentrics.ts
- test/unit/script-trigger-lifecycle.test.ts
- packages/script/src/runtime/registry/youtube-player.ts
- packages/script/src/runtime/server/proxy-handler.ts
- packages/script/src/devtools.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/cached-upstream-lifecycle.test.ts (1)
2-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert every configured cache bound.
Lines 25-28 check
driverandmaxonly.ensureNuxtScriptsCacheStoragealso setsmaxSizeandmaxEntrySize, so the test can pass after either limit is removed.Import the existing size-limit constants and assert both properties.
Suggested test update
import { ensureNuxtScriptsCacheStorage, NUXT_SCRIPTS_CACHE_BASE, NUXT_SCRIPTS_CACHE_MAX_ENTRIES, + NUXT_SCRIPTS_CACHE_MAX_SIZE, + NUXT_SCRIPTS_CACHE_MAX_ENTRY_SIZE, } from '../../packages/script/src/runtime/server/utils/cache-config' expect(nitroOptions.storage?.[NUXT_SCRIPTS_CACHE_BASE]).toEqual(expect.objectContaining({ driver: 'lru-cache', max: NUXT_SCRIPTS_CACHE_MAX_ENTRIES, + maxSize: NUXT_SCRIPTS_CACHE_MAX_SIZE, + maxEntrySize: NUXT_SCRIPTS_CACHE_MAX_ENTRY_SIZE, }))Also applies to: 25-28
🤖 Prompt for 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. In `@test/unit/cached-upstream-lifecycle.test.ts` around lines 2 - 6, Update the cache storage test around ensureNuxtScriptsCacheStorage to import the existing size-limit constants and assert maxSize and maxEntrySize alongside driver and max. Ensure the test verifies every configured cache bound so removing either size limit causes failure.
🤖 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.
Nitpick comments:
In `@test/unit/cached-upstream-lifecycle.test.ts`:
- Around line 2-6: Update the cache storage test around
ensureNuxtScriptsCacheStorage to import the existing size-limit constants and
assert maxSize and maxEntrySize alongside driver and max. Ensure the test
verifies every configured cache bound so removing either size limit causes
failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cafbe91d-cecc-463e-be3f-e692f421d72b
📒 Files selected for processing (7)
packages/script/src/module.tspackages/script/src/runtime/server/utils/cache-config.tspackages/script/src/runtime/utils/youtube-readiness.tspackages/script/src/unhead-features.tstest/unit/cached-upstream-lifecycle.test.tstest/unit/unhead-features.test.tstest/unit/youtube-player-lifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/unit/unhead-features.test.ts
- packages/script/src/runtime/utils/youtube-readiness.ts
- test/unit/youtube-player-lifecycle.test.ts
- packages/script/src/module.ts
1 participant
🔗 Linked issue
Alternative to #832
Related to unjs/unhead#925 and unjs/unhead@36dcf318
❓ Type of change
📚 Description
Applies the lifecycle hardening from #829 without raising the current Nuxt or Unhead peer floors. Shared
useScriptresources now own cleanup for app hooks, consumer scopes, readiness callbacks, observers, request buffers, and bounded caches.NPM mode registry SDKs use keyed source-less loaders when the installed Unhead package exports
UseScriptLoaderInput. This gives PostHog and future NPM backed providers native deduplication and cancellation. A small adapter keeps existing SDK method return values, including methods retained before load.Unhead 3.3.1 contains the source-less loader support from
36dcf318. The pinned 3.3.1 browser fixture verifies the native path, one initialization across duplicate calls, and return preserving proxies. Unhead v2, 3.3.0, and other releases without that exported capability keep the existing local stub.✅ Verification
pnpm lintpnpm typecheckpnpm buildpnpm test:run: 99 files passed, 1105 tests passed, 6 skipped, 3 todo