Open
Conversation
📝 WalkthroughWalkthroughThe Vue page Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/composables/useScriptTriggerConsent.ts`:
- Around line 20-23: The trigger promise in useScriptTriggerConsent (the promise
resolved by the accept() flow and later rejected on revocation) cannot be
rejected after it has been resolved, so the current reject() call is ineffective
and breaks the API contract; instead, remove the expectation of post-resolution
rejection: update the implementation to stop calling reject() after a resolved
promise and adjust the JSDoc to state that revocation is signaled via the
reactive consented property (exposed from useScriptTriggerConsent) rather than
by rejecting the trigger promise; ensure accept(), the trigger promise logic,
and any code paths that previously called resolve()/reject() are updated so the
promise only reflects the initial load outcome and consumers rely on consented
for revocation notifications.
- Around line 32-50: The watcher on consented (in useScriptTriggerConsent.ts) is
registered without immediate: true, so if consented.value is set to true earlier
(via the boolean branch, the Ref branch, or a quickly-resolving Promise) the
watcher never fires; update the watch(consented, ...) call to include {
immediate: true } so the callback runs immediately with the current value
(ensuring the promise/flow that waits on the consent transition resolves even
when consent was already granted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9b802ca-f48d-4225-a6ba-78fb21d0964e
📒 Files selected for processing (2)
playground/pages/features/cookie-consent.vuesrc/runtime/composables/useScriptTriggerConsent.ts
Comment on lines
+32
to
+50
| // Setup initial consent value | ||
| if (options?.consent) { | ||
| if (isRef(options?.consent)) { | ||
| watch(options.consent, (_val) => { | ||
| const val = toValue(_val) | ||
| consented.value = Boolean(val) | ||
| }, { immediate: true }) | ||
| } | ||
| // check for boolean primitive | ||
| else if (typeof options?.consent === 'boolean') { | ||
| consented.value = options?.consent | ||
| } | ||
| // consent is a promise | ||
| else if (options?.consent instanceof Promise) { | ||
| options?.consent.then((res) => { | ||
| consented.value = typeof res === 'boolean' ? res : true | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/runtime/composables/useScriptTriggerConsent.ts | head -80Repository: nuxt/scripts
Length of output: 3546
🌐 Web query:
Vue 3 watch callback immediate false does not trigger if value changed before registration
💡 Result:
In Vue 3 this is expected:
watch()is lazy by default: withimmediate: false(the default), the callback only runs for changes that happen after the watcher is created. If the value already changed before you register the watcher, there’s no “missed event queue” to replay, so nothing fires. Vue’s docs: “the callback won't be called until the watched source has changed.” [1]- Also, watchers should be created synchronously inside
setup(); if you create them later (e.g., after anawait, in a timeout, or in some async init), you can easily miss earlier changes. Vue explicitly warns that async-created watchers are a special case and synchronous creation is preferred. [1]
Fix options
- If you must react to the current value too, use
immediate: true:
watch(source, (nv, ov) => {
// runs once immediately, then on future changes
}, { immediate: true })[1]
- Register the watcher earlier (synchronously), and gate your logic instead of delaying registration:
const ready = ref(false)
watch(source, (nv) => {
if (!ready.value) return
// ...
})
onMounted(() => { ready.value = true })(avoids missing changes while still preventing early side effects)
- Use
watchEffect()if “run once now + rerun on dependency changes” is what you actually want:
watchEffect()runs immediately and tracks dependencies automatically. [1]
Sources:
[1] Vue docs — Watchers (lazy by default, immediate, watchEffect, and note about creating watchers synchronously in setup()). (vuejs.org)
Citations:
Consent is lost if granted before the watcher registers.
Lines 38, 42, and 47 can set consented.value = true synchronously. However, the watch(consented, ...) at line 53 is registered without immediate: true, meaning it only fires for changes that occur after the watcher is created. If consent is granted before line 53 executes, the watcher never sees the false → true transition and the promise stays pending indefinitely.
This occurs when:
options.consentis a booleantrueoptions.consentis aRefwith an initial value oftrueoptions.consentis aPromisethat resolves quickly
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/composables/useScriptTriggerConsent.ts` around lines 32 - 50, The
watcher on consented (in useScriptTriggerConsent.ts) is registered without
immediate: true, so if consented.value is set to true earlier (via the boolean
branch, the Ref branch, or a quickly-resolving Promise) the watcher never fires;
update the watch(consented, ...) call to include { immediate: true } so the
callback runs immediately with the current value (ensuring the promise/flow that
waits on the consent transition resolves even when consent was already granted).
Calling reject() on an already-resolved promise is silently ignored. Revocation is now documented as being signaled via the reactive `consented` ref instead.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/composables/useScriptTriggerConsent.ts (1)
55-80:⚠️ Potential issue | 🔴 CriticalRevocation does not stop an in-flight delayed load.
Once the
truetransition starts, the callback-based trigger, promise-based trigger, andonNuxtReadypath all keep an unconditionalresolve. Ifrevoke()is called before those complete, the stale callback still fulfills the trigger and can load the script after consent was withdrawn.Suggested fix
const promise = new Promise<void>((resolve) => { watch(consented, (newValue, oldValue) => { if (newValue && !oldValue) { // Consent granted - load script const runner = nuxtApp?.runWithContext || ((cb: () => void) => cb()) + const resolveIfStillConsented = () => { + if (consented.value) + runner(resolve) + } if (typeof options?.postConsentTrigger === 'function') { // check if function has an argument if (options?.postConsentTrigger.length === 1) { - options.postConsentTrigger(resolve) + options.postConsentTrigger(resolveIfStillConsented) return } // else it's returning a promise to await const val = (options.postConsentTrigger as (() => Promise<any>))() if (val instanceof Promise) { - return val.then(() => runner(resolve)) + return val.then(resolveIfStillConsented) } return } if (options?.postConsentTrigger === 'onNuxtReady') { const idleTimeout = options?.postConsentTrigger ? (nuxtApp ? onNuxtReady : requestIdleCallback) : (cb: () => void) => cb() - runner(() => idleTimeout(resolve)) + runner(() => idleTimeout(resolveIfStillConsented)) return } // other trigger not supported - runner(resolve) + resolveIfStillConsented() } - }) + }, { immediate: true }) }) as UseConsentScriptTriggerApi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/composables/useScriptTriggerConsent.ts` around lines 55 - 80, The watch handler currently calls resolve unconditionally in async paths, so revocation via revoke() can still allow a delayed callback to fulfill the trigger; update the async continuations to guard before calling resolve by checking the current consent state (the reactive consented ref) or a local "cancelled" flag that revoke() sets: for the callback-argument branch (options.postConsentTrigger with length===1) wrap the passed callback so it only calls resolve when consented.value is still true; for the promise branch, after val.then(...) verify consented.value before calling runner(resolve); for the onNuxtReady / requestIdleCallback path, wrap the resolve invocation with the same consent check; ensure revoke() sets the flag or updates the reactive so these checks prevent calling resolve when consent has been withdrawn.
♻️ Duplicate comments (1)
src/runtime/composables/useScriptTriggerConsent.ts (1)
55-56:⚠️ Potential issue | 🔴 CriticalInitial granted consent can still leave the trigger pending.
consented.valuecan already betruebefore this watcher is registered (options.consent === true, or a ref initialized totrue). Without{ immediate: true }, that transition is missed and the promise never resolves. This was already raised on an earlier revision and still applies here.Suggested fix
- watch(consented, (newValue, oldValue) => { + watch(consented, (newValue, oldValue) => { if (newValue && !oldValue) { // Consent granted - load script const runner = nuxtApp?.runWithContext || ((cb: () => void) => cb()) if (typeof options?.postConsentTrigger === 'function') { // check if function has an argument if (options?.postConsentTrigger.length === 1) { options.postConsentTrigger(resolve) return } // else it's returning a promise to await const val = (options.postConsentTrigger as (() => Promise<any>))() if (val instanceof Promise) { return val.then(() => runner(resolve)) } return } if (options?.postConsentTrigger === 'onNuxtReady') { const idleTimeout = options?.postConsentTrigger ? (nuxtApp ? onNuxtReady : requestIdleCallback) : (cb: () => void) => cb() runner(() => idleTimeout(resolve)) return } // other trigger not supported runner(resolve) } - }) + }, { immediate: true })Vue 3 watch() callback when the source is already true before watcher registration; does it require { immediate: true }? Please use the official Vue docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/composables/useScriptTriggerConsent.ts` around lines 55 - 56, The watcher on the consented ref inside useScriptTriggerConsent misses cases where consented.value is already true before the watcher registers, so update the watch(consented, ...) call to pass the options object { immediate: true } and ensure the callback handles the initial invocation (newValue true) to resolve the pending trigger/promise; locate the watch in useScriptTriggerConsent and modify it to invoke the same resolution logic when immediate fires so the promise never remains unresolved when options.consent or the ref starts true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/composables/useScriptTriggerConsent.ts`:
- Around line 15-19: The SSR branch in useScriptTriggerConsent currently returns
a bare Promise and thus lacks the new members revoke and consented; update the
import.meta.server path so it returns the same API shape as the client: an
object (or Promise resolving to it) containing a noop revoke function and a
reactive consented Ref<boolean> (initialized false) so SSR callers can safely
read consented.value and call revoke without runtime errors; locate the return
in the import.meta.server branch and add these members to match the client
return shape.
---
Outside diff comments:
In `@src/runtime/composables/useScriptTriggerConsent.ts`:
- Around line 55-80: The watch handler currently calls resolve unconditionally
in async paths, so revocation via revoke() can still allow a delayed callback to
fulfill the trigger; update the async continuations to guard before calling
resolve by checking the current consent state (the reactive consented ref) or a
local "cancelled" flag that revoke() sets: for the callback-argument branch
(options.postConsentTrigger with length===1) wrap the passed callback so it only
calls resolve when consented.value is still true; for the promise branch, after
val.then(...) verify consented.value before calling runner(resolve); for the
onNuxtReady / requestIdleCallback path, wrap the resolve invocation with the
same consent check; ensure revoke() sets the flag or updates the reactive so
these checks prevent calling resolve when consent has been withdrawn.
---
Duplicate comments:
In `@src/runtime/composables/useScriptTriggerConsent.ts`:
- Around line 55-56: The watcher on the consented ref inside
useScriptTriggerConsent misses cases where consented.value is already true
before the watcher registers, so update the watch(consented, ...) call to pass
the options object { immediate: true } and ensure the callback handles the
initial invocation (newValue true) to resolve the pending trigger/promise;
locate the watch in useScriptTriggerConsent and modify it to invoke the same
resolution logic when immediate fires so the promise never remains unresolved
when options.consent or the ref starts true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b63b7410-e606-4319-bf31-68463daec66d
📒 Files selected for processing (1)
src/runtime/composables/useScriptTriggerConsent.ts
Comment on lines
+15
to
+19
| revoke: () => void | ||
| /** | ||
| * Reactive reference to the consent state | ||
| */ | ||
| consented: Ref<boolean> |
There was a problem hiding this comment.
Keep the SSR return value API-compatible.
The new revoke/consented members are only attached on the client path. The import.meta.server branch at Line 28 still returns a bare Promise, so SSR callers get undefined for both and can break as soon as they read consented.value or bind revoke.
Suggested fix
export function useScriptTriggerConsent(options?: ConsentScriptTriggerOptions): UseConsentScriptTriggerApi {
- if (import.meta.server)
- return new Promise(() => {}) as UseConsentScriptTriggerApi
+ if (import.meta.server) {
+ const serverPromise = new Promise<void>(() => {}) as UseConsentScriptTriggerApi
+ serverPromise.accept = () => {}
+ serverPromise.revoke = () => {}
+ serverPromise.consented = ref(false)
+ return serverPromise
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/composables/useScriptTriggerConsent.ts` around lines 15 - 19, The
SSR branch in useScriptTriggerConsent currently returns a bare Promise and thus
lacks the new members revoke and consented; update the import.meta.server path
so it returns the same API shape as the client: an object (or Promise resolving
to it) containing a noop revoke function and a reactive consented Ref<boolean>
(initialized false) so SSR callers can safely read consented.value and call
revoke without runtime errors; locate the return in the import.meta.server
branch and add these members to match the client return shape.
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
Resolves #458
❓ Type of change
📚 Description
Users had no way to unload scripts when consent was revoked. Adds
revoke()method and reactiveconsentedref touseScriptTriggerConsent. The trigger promise rejects withError('Consent revoked')on revocation, signaling the script should be unloaded. Backward compatible with existingaccept()API.