perf: memory leak hardening#829
Open
harlan-zw wants to merge 6 commits into
Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe changes add cancellation and deterministic teardown for devtools connections, script fetching, Google Maps operations, third-party integrations, registries, and trigger composables. Server request bodies and upstream responses receive bounded, streaming handling, while upstream caches use dedicated bounded storage. Devtools payloads and network data are capped. New lifecycle, abort, cache, and animation cleanup tests cover the updated behavior. Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/script/src/runtime/npm-script-stub.ts (1)
39-55: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn the in-flight initialization to concurrent callers.
Once the first call sets
hasInitialized, a concurrentawait stub.load()returns immediately even thoughclientInit()is still running. Preserve an in-flight promise so every caller waits for the same initialization result.🤖 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/npm-script-stub.ts` around lines 39 - 55, Update the NpmScriptStub load method to store the in-flight initialization promise when the first caller begins loading, and have concurrent callers return or await that same promise instead of exiting immediately after hasInitialized is set. Preserve the existing initialization status transitions and clientInit execution, including sharing its fulfillment or rejection with all callers.packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue (1)
385-430: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep the abort race active through the actual library import
Right now the abort listener is removed as soon as
api.importLibrary(key)is handed off, so an unmount while that promise is still pending can resolve after disposal instead of rejecting withAbortError. Wrap the returned import promise itself and clean up on settle.🤖 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 385 - 430, Update importLibrary so the lifecycle abort listener remains active until the actual api.importLibrary(key) promise settles, rather than cleaning up when the import is merely started. Wrap the returned import promise in the existing settle/cleanup flow, reject with AbortError if disposal wins the race, and preserve normal resolution or rejection when the import settles first.packages/devtools-app/composables/state.ts (1)
135-154: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winApply fetch results to the current script record.
If
syncScriptsreplaces a script while its source fetch is pending, deduplication skips a new fetch, but this closure updates only the oldscriptobject. The active record can therefore miss its size/error indefinitely.Proposed fix
.then((res) => { - if (controller.signal.aborted || !activeSources.has(scriptSizeKey)) + const currentScript = scripts.value[key] + if (controller.signal.aborted || currentScript?.src !== scriptSizeKey) return if (res.size) { scriptSizes[scriptSizeKey] = res.size - script.size = res.size + currentScript.size = res.size } if (res.error) { scriptErrors[scriptSizeKey] = res.error instanceof Error ? res.error.message : String(res.error) - script.error = scriptErrors[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 handler in syncScripts so results are applied to the current active script record retrieved by scriptSizeKey, rather than only the captured script object. Preserve the existing abort and active-source checks, and ensure both size and error fields are written to the replacement record when it differs from the original.packages/script/src/runtime/server/utils/cache-config.ts (1)
1-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove
maxSizeandmaxEntrySizeconstants.As noted in
packages/script/src/module.ts, configuringmaxSizefor thelru-cachestorage driver via a serialized Nitro configuration will cause runtime crashes because asizeCalculationfunction cannot be passed. Once those configurations are removed from the module, these size limit constants will be unused and should be removed.♻️ Proposed fix
/** * Conservative defaults for the in-process fallback. Applications can replace * this mount with Redis, KV, filesystem, or their own storage configuration. */ export const NUXT_SCRIPTS_CACHE_MAX_ENTRIES = 500 -export const NUXT_SCRIPTS_CACHE_MAX_SIZE = 32 * 1024 * 1024 -export const NUXT_SCRIPTS_CACHE_MAX_ENTRY_SIZE = 8 * 1024 * 1024🤖 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/server/utils/cache-config.ts` around lines 1 - 11, Remove the NUXT_SCRIPTS_CACHE_MAX_SIZE and NUXT_SCRIPTS_CACHE_MAX_ENTRY_SIZE exports from the cache configuration module, leaving NUXT_SCRIPTS_CACHE_BASE and NUXT_SCRIPTS_CACHE_MAX_ENTRIES unchanged.
🤖 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/devtools-app/composables/rpc.ts`:
- Around line 35-38: Update cleanupConnection to reset all connection state
during teardown: set isConnected to false and clear the stored host $fetch
alongside devtools.value and connection cleanups. Ensure subsequent standalone
connection attempts can establish a fresh client instead of reusing the
destroyed connection.
In `@packages/devtools-app/utils/fetch.ts`:
- Around line 1-5: Update fetchScript so errors from getResponseSize(),
including aborts during reader.read(), are caught and handled through the same
rejection path as initial fetch failures. Ensure cancellation while reading the
response body does not produce an unhandled promise rejection, while preserving
the existing response-processing behavior.
In `@packages/script/src/devtools.ts`:
- Around line 86-120: Update the oversized-body branch in onData to attach a
no-op error listener to req after cleanup and before req.resume() begins
draining. Keep the existing cleanup, response, and draining behavior unchanged
while ensuring errors emitted during the drain are safely handled.
In `@packages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.ts`:
- Around line 162-181: Update the load invocation within the promise around the
abort listener so synchronous exceptions from load() are caught and passed
through settle(() => reject(error)). Preserve the existing async
resolution/rejection handlers and cleanup behavior, ensuring the abort listener
is removed for both synchronous and asynchronous load failures.
In `@packages/script/src/runtime/components/ScriptLemonSqueezy.vue`:
- Around line 45-52: Track which ScriptLemonSqueezy instance currently owns the
shared LemonSqueezy event handler when Setup is called, and update the
onBeforeUnmount cleanup to reset the global handler only when the unmounting
instance remains the active owner. Ensure an older instance cannot clear a newer
instance’s handler while preserving cleanup for the current owner.
In `@packages/script/src/runtime/components/ScriptPayPalButtons.vue`:
- Around line 107-110: Ensure the PayPal initialization rejection handlers
suppress late failures after disposal by returning early when disposed before
mutating state or emitting. Update the catch path in
packages/script/src/runtime/components/ScriptPayPalButtons.vue (lines 107-110)
and apply the same guard in
packages/script/src/runtime/components/ScriptPayPalMessages.vue (lines 111-112),
while preserving the existing fulfillment guards.
In `@packages/script/src/runtime/composables/useScript.ts`:
- Around line 512-518: Update the instance.reload implementation to reconnect
the network observer in a finally block around _origReload(), so
observeNetworkRequests runs whether the reload resolves or rejects. Preserve the
existing cleaned guard, disconnect-before-reload behavior, and return the
original reload result while allowing rejection to propagate.
- Around line 366-383: Update instance.reload to apply the same validation gate
as load: check err and reject immediately before calling currentRemove() or
creating the reloaded script. Preserve the existing reload behavior for valid
scripts, but do not replace the validation trigger with 'client' when validation
has failed.
In `@packages/script/src/runtime/registry/usercentrics.ts`:
- Around line 135-170: The readiness cleanup in cleanupReadyListener does not
settle the pending readyPromise or prevent asynchronous isInitialized()
continuations from invoking onReady after teardown. Add a disposed guard checked
by the readiness continuation and onReady, and settle the pending promise during
cleanup by rejecting or otherwise resolving it; ensure cleanup is idempotent and
clears the stored resolver.
In `@packages/script/src/runtime/registry/youtube-player.ts`:
- Around line 59-70: Register teardown for the YouTube API readiness wrapper
created in the setup flow, using the owning lifecycle scope or
instance.remove(). When the script instance is removed before readiness,
conditionally restore previousReady or delete window.onYouTubeIframeAPIReady,
and release the captured callback state; preserve the existing onReady
restoration behavior when the API loads normally.
In `@packages/script/src/runtime/server/proxy-handler.ts`:
- Around line 117-122: Update the async cancel method to guarantee
reader.releaseLock() executes even when reader.cancel(reason) rejects, using a
finally-style cleanup around the cancellation call while preserving cleanup(),
controller.abort(), and the existing cancellation reason.
---
Outside diff comments:
In `@packages/devtools-app/composables/state.ts`:
- Around line 135-154: Update the fetch completion handler in syncScripts so
results are applied to the current active script record retrieved by
scriptSizeKey, rather than only the captured script object. Preserve the
existing abort and active-source checks, and ensure both size and error fields
are written to the replacement record when it differs from the original.
In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Around line 385-430: Update importLibrary so the lifecycle abort listener
remains active until the actual api.importLibrary(key) promise settles, rather
than cleaning up when the import is merely started. Wrap the returned import
promise in the existing settle/cleanup flow, reject with AbortError if disposal
wins the race, and preserve normal resolution or rejection when the import
settles first.
In `@packages/script/src/runtime/npm-script-stub.ts`:
- Around line 39-55: Update the NpmScriptStub load method to store the in-flight
initialization promise when the first caller begins loading, and have concurrent
callers return or await that same promise instead of exiting immediately after
hasInitialized is set. Preserve the existing initialization status transitions
and clientInit execution, including sharing its fulfillment or rejection with
all callers.
In `@packages/script/src/runtime/server/utils/cache-config.ts`:
- Around line 1-11: Remove the NUXT_SCRIPTS_CACHE_MAX_SIZE and
NUXT_SCRIPTS_CACHE_MAX_ENTRY_SIZE exports from the cache configuration module,
leaving NUXT_SCRIPTS_CACHE_BASE and NUXT_SCRIPTS_CACHE_MAX_ENTRIES 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
Run ID: f41aff63-1513-4ae9-8b6f-a5aef7b9ad96
📒 Files selected for processing (33)
packages/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/after-next-paint.tstest/unit/google-maps-lifecycle.test.tstest/unit/speedcurve-after-next-paint.test.tsvitest.config.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.ts (1)
162-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated abort/settle scaffolding.
The
settled/onAbort/cleanup/settleblock is duplicated verbatim between the twoawait new Promise(...)sections. Consolidating into a small helper (e.g.createAbortableSettle(signal)returning{ settle, cleanup }) would reduce the risk of the two copies drifting when one is edited but not the other.♻️ Proposed refactor sketch
+function createAbortableSettle(signal: AbortSignal | undefined, onAbort: () => Error) { + let settled = false + let listener: (() => void) | undefined + const cleanup = () => { + if (listener) + signal?.removeEventListener('abort', listener) + } + const settle = (fn: () => void, install: (reject: (e: Error) => void) => void) => { + if (settled) + return + settled = true + cleanup() + fn() + } + return { settle, cleanup, install: (reject: (e: Error) => void) => { + listener = () => settle(() => reject(onAbort()), () => {}) + signal?.addEventListener('abort', listener, { once: true }) + } } +}Also applies to: 203-232
🤖 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/useGoogleMapsResource.ts` around lines 162 - 190, Extract the duplicated settled/onAbort/cleanup/settle logic from both await new Promise sections into a shared helper, such as createAbortableSettle, near the surrounding useGoogleMapsResource utilities. Have it accept the abort signal and return the reusable settle and cleanup operations, then update both promise blocks to use the helper while preserving their existing resolve/reject behavior and abort handling.
🤖 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/runtime/registry/youtube-player.ts`:
- Around line 78-83: Update the global YouTube API ready callback around
previousReady so exceptions from the previously installed callback are caught
and do not escape into the YouTube IIFE; preserve the finally behavior that
always calls resolve().
In `@test/unit/npm-script-stub-lifecycle.test.ts`:
- Around line 1-10: Restore the console.log spy created in the “does not retain
callbacks registered after initialization fails” test after it completes, using
the test suite’s existing cleanup convention or a scoped mock pattern so
subsequent tests receive the original console.log behavior.
---
Nitpick comments:
In `@packages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.ts`:
- Around line 162-190: Extract the duplicated settled/onAbort/cleanup/settle
logic from both await new Promise sections into a shared helper, such as
createAbortableSettle, near the surrounding useGoogleMapsResource utilities.
Have it accept the abort signal and return the reusable settle and cleanup
operations, then update both promise blocks to use the helper while preserving
their existing resolve/reject behavior and abort handling.
🪄 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
Run ID: a742e923-20ca-4cdd-8142-b69943067cf5
📒 Files selected for processing (19)
packages/devtools-app/composables/rpc.tspackages/devtools-app/utils/fetch.tspackages/script/src/devtools.tspackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vuepackages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.tspackages/script/src/runtime/components/ScriptLemonSqueezy.vuepackages/script/src/runtime/components/ScriptPayPalButtons.vuepackages/script/src/runtime/components/ScriptPayPalMessages.vuepackages/script/src/runtime/composables/useScript.tspackages/script/src/runtime/npm-script-stub.tspackages/script/src/runtime/registry/usercentrics.tspackages/script/src/runtime/registry/youtube-player.tspackages/script/src/runtime/server/proxy-handler.tstest/unit/cached-upstream-lifecycle.test.tstest/unit/fetch-script-lifecycle.test.tstest/unit/google-maps-lifecycle.test.tstest/unit/npm-script-stub-lifecycle.test.tstest/unit/script-trigger-lifecycle.test.tstest/unit/use-script-lifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/script/src/runtime/components/ScriptPayPalMessages.vue
- packages/script/src/runtime/components/ScriptPayPalButtons.vue
- packages/script/src/devtools.ts
- packages/script/src/runtime/registry/usercentrics.ts
- packages/script/src/runtime/npm-script-stub.ts
- packages/script/src/runtime/server/proxy-handler.ts
- packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue
- packages/script/src/runtime/composables/useScript.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/runtime/utils/abortable-promise.ts`:
- Around line 27-44: The resolve path in createAbortablePromise must keep abort
handling active when given a promise or thenable. Update resolve/finish so
cleanup, listener removal, and settlement occur only after the adopted value
settles, while preserving direct-value behavior and rejection propagation; add a
regression test covering abort before the inner promise settles.
🪄 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
Run ID: daed13a4-e068-4560-b43e-32d4648ab74f
📒 Files selected for processing (6)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vuepackages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.tspackages/script/src/runtime/registry/usercentrics.tspackages/script/src/runtime/registry/youtube-player.tspackages/script/src/runtime/utils/abortable-promise.tstest/unit/abortable-promise.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/script/src/runtime/registry/usercentrics.ts
- packages/script/src/runtime/registry/youtube-player.ts
- packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
No linked issue.
❓ Type of change
📚 Description
Closes retention paths found while profiling devtools and runtime teardown. Connections now drop hooks and in-flight requests; component and registry cleanup releases callbacks and ignores late async completions; proxy bodies stream with bounded buffering and cancellation; Nuxt Scripts caches use a bounded LRU mount.
A shared abortable-promise primitive now owns settle, abort, and disposer behavior for the Google Maps, YouTube, and Usercentrics readiness paths. The matching upstream
useScriptlifecycle contract is proposed in unjs/unhead#840; this PR keeps the local compatibility layer because Nuxt Scripts currently supports both Unhead 2.x and 3.x.Regression tests cover shared useScript instances, trigger disposal, Google Maps teardown, abortable readiness, cache mounts, and failed NPM initialization.