fix(google-sign-in): initialize/renderButton/prompt helpers#734
Merged
fix(google-sign-in): initialize/renderButton/prompt helpers#734
Conversation
Extends `useScriptGoogleSignIn()` with three helpers that wrap the most common GSI flows. Schema options are merged into every call so callers don't repeat `clientId`, `loginUri`, `uxMode`, etc., and `initialize()` is internally guarded to avoid the warning Google logs on re-init after a button has been rendered. Adds playground pages exercising locale switching, navigation/remount, the legacy proxy mode, and redirect UX, plus a `same-origin-allow-popups` COOP route rule so the popup callback can post back to the page. Docs gain a Composable API section and notes on COOP and redirect mode.
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:
📝 WalkthroughWalkthroughAdds a high-level composable API useScriptGoogleSignIn() that returns GoogleSignInHelpers with initialize, renderButton, and prompt. Implements per-script shared state on window.__nuxtScriptsGsi, a mapSchemaToIdConfig() mapper, guarded and idempotent initialization (including a stable callback proxy for non-redirect flows), and routes GIS calls through instance.onLoaded. Adds GoogleSignInHelpers to registry-types.json. Updates documentation and multiple playground Nuxt pages to use the new helpers (locale switching, navigation, redirect/popup modes, One Tap, and a legacy proxy demo) and adds a COOP route rule for demo routes. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/script/src/runtime/registry/google-sign-in.ts`:
- Around line 183-215: The renderButton and prompt handlers call
accounts.id.renderButton/accounts.id.prompt without verifying that ensureInit
actually initialized GSI; update ensureInit to return a boolean (true when
gid.initialize ran and state.initialized set) or simply check
getState().initialized after calling ensureInit, and only call
accounts.id.renderButton or accounts.id.prompt when initialized is true; modify
instance.renderButton and instance.prompt (inside their instance.onLoaded
callbacks) to guard with this check and bail out (or surface an error) when
initialization did not succeed so GSI methods are never invoked before
initialize().
🪄 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: 4c32936d-0688-404d-973b-243f73982cf2
📒 Files selected for processing (9)
docs/content/scripts/google-sign-in.mdpackages/script/src/registry-types.jsonpackages/script/src/runtime/registry/google-sign-in.tsplayground/nuxt.config.tsplayground/pages/third-parties/google-sign-in/locale.vueplayground/pages/third-parties/google-sign-in/navigation.vueplayground/pages/third-parties/google-sign-in/nuxt-scripts.vueplayground/pages/third-parties/google-sign-in/proxy-legacy.vueplayground/pages/third-parties/google-sign-in/redirect-mode.vue
`ensureInit()` now returns a boolean, and the helpers bail out when initialization couldn't run (missing client_id, non-client environment, or `window.google` not yet available). Calling `renderButton` or `prompt` before `initialize` triggers GSI's "Failed to render button before calling initialize" error and breaks the sign-in flow silently.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/script/src/runtime/registry/google-sign-in.ts`:
- Around line 177-221: The code currently auto-initializes Google Identity
without ensuring a required callback, locking state.initialized and preventing
later initialize({ callback }) from working; fix by changing ensureInit to
accept a flag (e.g. requireCallback?: boolean) and, after merging
state.schemaConfig and extra in ensureInit, return false (and avoid setting
state.initialized) if requireCallback is true and merged.callback is missing;
update instance.prompt to call ensureInit(undefined, true) (since prompt/One
Tap/popup require a callback) and keep instance.renderButton calling
ensureInit(undefined, false) or the default so rendering still works without a
callback; leave instance.initialize to call ensureInit(config) so explicit
initialization can register the callback and mark initialized via the same path.
🪄 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: cc14ae79-7bf4-4c73-be5c-7a4dcbc6d874
📒 Files selected for processing (1)
packages/script/src/runtime/registry/google-sign-in.ts
Stash runtime config from explicit `initialize({ callback })` calls
separately from schema config. `ensureInit()` now refuses to run when
the merged config lacks a `callback` (and ux_mode isn't 'redirect'), so
a stray `renderButton()` or `prompt()` that fires before the user calls
`initialize()` no longer locks GIS into a callback-less config that
silently breaks One Tap and popup flows.
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 `@packages/script/src/runtime/registry/google-sign-in.ts`:
- Around line 186-195: getState() stores per-instance config into a
server-global (globalThis) which lets initialize({ callback }) mutate shared
__nuxtScriptsGsi state and leak request-scoped callbacks/configs during SSR;
change the logic so that getState() does not persist mutable
request/client-specific data on globalThis when import.meta.client is false—use
a transient in-memory object only for client runtime or return a fresh GsiState
for server calls, and ensure initialize() writes callbacks/schema only when
running on the client (check import.meta.client) or store server-side values in
a request-scoped context instead of __nuxtScriptsGsi; update references to
getState, initialize, key, and __nuxtScriptsGsi accordingly (also apply same fix
to the similar block around lines 221-225).
- Around line 180-184: The initialize() flow updates state.runtimeConfig but
never calls gid.initialize() after the first GIS initialize, so remounted
components keep invoking a stale original callback; fix by creating and
registering a stable proxy callback with gid.initialize() once (e.g., register a
closure variable like proxyCallback that calls the latest
state.runtimeConfig.callback) inside ensureInit(), then on subsequent
initialize() calls update state.runtimeConfig.callback to the newest app
callback so the proxy dispatches to the fresh handler; apply the same pattern
where callbacks are set (references to initialize, runtimeConfig, ensureInit,
gid.initialize, and any code around lines 203-205 and 221-228) so One Tap and
button flows always forward to the latest callback across remounts.
🪄 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: 7bf9210e-a26a-4034-87f3-4e4291302395
📒 Files selected for processing (1)
packages/script/src/runtime/registry/google-sign-in.ts
`getState()` no longer falls back to `globalThis` on the server, so
`initialize({ callback })` can't leak request-scoped callbacks into
shared state across concurrent SSR renders. The helpers no-op on the
server (their work happens inside `onLoaded`, which is client-only).
Register a stable proxy callback once with `gid.initialize()` and have it
dispatch to `runtimeConfig.callback`. Remounted components updating their
handler via `initialize({ callback })` now see the new handler fire, since
GIS only honors the first `initialize()` call.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/script/src/runtime/registry/google-sign-in.ts`:
- Around line 164-168: The cached state in clientInit/initialize is being
mutated without ensuring runtimeConfig exists, causing Object.assign(undefined,
config) to throw; update the logic around w.__nuxtScriptsGsi[key] (created in
clientInit) to always backfill a default runtimeConfig before any mutation or
Object.assign calls (e.g., ensure w.__nuxtScriptsGsi[key] has a runtimeConfig
object prior to assigning schemaConfig via mapSchemaToIdConfig and prior to
calling initialize()), so that initialize() and any later Object.assign
operations are safe on remount/HMR.
🪄 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: 1371130b-9de4-4f24-802f-a56bc1cb6119
📒 Files selected for processing (1)
packages/script/src/runtime/registry/google-sign-in.ts
`clientInit()` seeds `__nuxtScriptsGsi[key]` without `runtimeConfig`. If a remount hits `getState()` after `clientInit` ran first, the cached entry short-circuits the fallback and `Object.assign(undefined, config)` would throw. Always backfill `runtimeConfig` (and `schemaConfig`) in both code paths.
harlan-zw
changed the title
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
N/A
❓ Type of change
📚 Description
Extends
useScriptGoogleSignIn()with three high-level helpers (initialize,renderButton,prompt) that wrap the most common Google Identity Services flows. Schema options passed to the composable are merged into every call soclientId,loginUri,uxMode, etc. don't have to be repeated, andinitialize()is internally guarded to avoid the warning Google logs on re-init after a button has already been rendered (which broke remounts on navigation/locale change).Also adds playground pages covering locale switching, navigation/remount, the legacy proxy mode, and redirect UX, plus a
same-origin-allow-popupsCOOP route rule so the popup callback can post back to the page. Docs gain a Composable API section and notes on COOP and redirect mode.