Expose default oncallback#2667
Closed
Piyush-85 wants to merge 3 commits into
Closed
Conversation
…ck at all call sites Co-authored-by: Dan Haughey <dan@greenlynx.co.uk>
Contributor
📝 WalkthroughWalkthroughThe PR extends the ChangesonCallback Hook Contract and Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2667 +/- ##
==========================================
- Coverage 89.76% 89.74% -0.03%
==========================================
Files 70 70
Lines 8254 8285 +31
Branches 1759 1760 +1
==========================================
+ Hits 7409 7435 +26
- Misses 833 838 +5
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
EXAMPLES.md (1)
2547-2567: ⚡ Quick winAdd the
defaultOnCallbackparameter to the example signature and demonstrate its usage.The documentation describes
onCallbackas receiving 4 parameters, but the example only shows 3. Consider adding the fourth parameter to the signature and providing an example that demonstrates how to usedefaultOnCallback, such as running custom logic before delegating to the default behavior.📚 Suggested example demonstrating defaultOnCallback usage
export const auth0 = new Auth0Client({ - async onCallback(error, context, session) { + async onCallback(error, context, session, defaultOnCallback) { + // Example 1: Run custom logic then delegate to default behavior + if (!error && session) { + // Log successful authentication + console.log(`User ${session.user.sub} authenticated successfully`); + + // Delegate to the default onCallback behavior + return defaultOnCallback(error, context, session); + } + + // Example 2: Custom error handling, custom redirect for success const appBaseUrl = context.appBaseUrl ?? process.env.APP_BASE_URL; // redirect the user to a custom error page if (error) { return NextResponse.redirect( new URL(`/error?error=${error.message}`, appBaseUrl) ); } // complete the redirect to the provided returnTo URL return NextResponse.redirect( new URL(context.returnTo || "/", appBaseUrl) ); } });🤖 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 `@EXAMPLES.md` around lines 2547 - 2567, Update the example Auth0Client onCallback signature to include the fourth parameter defaultOnCallback and show delegating to it after custom logic: in the Auth0Client instantiation's async onCallback(error, context, session, defaultOnCallback) example, demonstrate checking error and doing custom behavior (e.g., logging or redirecting to a custom error page) and then calling await defaultOnCallback(error, context, session) to run the built-in behavior (or return its NextResponse) when appropriate; reference Auth0Client, onCallback, defaultOnCallback, NextResponse, context, session, and error so readers can find the handler and see how to run the default flow after custom work.V4_MIGRATION_GUIDE.md (1)
378-394: ⚡ Quick winDemonstrate the new
defaultOnCallbackparameter or clarify backward compatibility.The signature now includes
defaultOnCallbackas a fourth parameter, but the example doesn't use it. Since this is a migration guide, consider either:
- Showing how to use the new parameter (especially if it enables new patterns), or
- Adding a note that the parameter is optional and existing 3-parameter implementations will continue to work
📚 Suggested enhancement options
Option 1: Add example using defaultOnCallback
export const auth0 = new Auth0Client({ async onCallback(error, context, session, defaultOnCallback) { if (error) { console.error('Authentication error:', error); return NextResponse.redirect( new URL('/error', process.env.APP_BASE_URL) ); } // Custom logic after successful authentication if (session) { console.log(`User ${session.user.sub} logged in successfully`); } + // Delegate to default behavior for redirect handling + return defaultOnCallback(error, context, session); - return NextResponse.redirect( - new URL(context.returnTo || "/", process.env.APP_BASE_URL) - ); } });Option 2: Add backward compatibility note
export const auth0 = new Auth0Client({ async onCallback(error, context, session, defaultOnCallback) { + // Note: The defaultOnCallback parameter is new in v4. It allows you to + // run side effects and then delegate to the SDK's default redirect behavior. + // If you don't need it, you can omit it from your signature (backward compatible). + if (error) { console.error('Authentication error:', error); return NextResponse.redirect( new URL('/error', process.env.APP_BASE_URL) ); } // ... } });🤖 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 `@V4_MIGRATION_GUIDE.md` around lines 378 - 394, Update the onCallback example to either demonstrate using the new fourth parameter or explicitly state it's optional: modify the async onCallback(error, context, session, defaultOnCallback) example to call and await defaultOnCallback(error, context, session) when you want the default behavior (e.g., fallback redirect) or show how to wrap it (perform custom logic then return defaultOnCallback(...)); alternatively add a short sentence after the example clarifying that defaultOnCallback is optional and older 3-parameter implementations (error, context, session) remain supported.
🤖 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 `@src/server/auth-client.ts`:
- Around line 186-198: The popup branches call and await onCallback (the
developer hook) but discard its returned NextResponse, so cookies/headers set by
the hook are lost; update those branches in auth-client.ts to capture the
Promise<NextResponse> returned by onCallback (e.g., const hookResponse = await
onCallback(error, ctx, session, defaultOnCallbackPopup)) and then return that
hookResponse instead of ignoring it, and ensure you pass a popup-specific
default callback (e.g., defaultOnCallbackPopup) that builds the postMessage
response when invoking onCallback so the hook can override or augment it; apply
this change to the identified popup-handling branches that currently await
onCallback but do not return its value.
---
Nitpick comments:
In `@EXAMPLES.md`:
- Around line 2547-2567: Update the example Auth0Client onCallback signature to
include the fourth parameter defaultOnCallback and show delegating to it after
custom logic: in the Auth0Client instantiation's async onCallback(error,
context, session, defaultOnCallback) example, demonstrate checking error and
doing custom behavior (e.g., logging or redirecting to a custom error page) and
then calling await defaultOnCallback(error, context, session) to run the
built-in behavior (or return its NextResponse) when appropriate; reference
Auth0Client, onCallback, defaultOnCallback, NextResponse, context, session, and
error so readers can find the handler and see how to run the default flow after
custom work.
In `@V4_MIGRATION_GUIDE.md`:
- Around line 378-394: Update the onCallback example to either demonstrate using
the new fourth parameter or explicitly state it's optional: modify the async
onCallback(error, context, session, defaultOnCallback) example to call and await
defaultOnCallback(error, context, session) when you want the default behavior
(e.g., fallback redirect) or show how to wrap it (perform custom logic then
return defaultOnCallback(...)); alternatively add a short sentence after the
example clarifying that defaultOnCallback is optional and older 3-parameter
implementations (error, context, session) remain supported.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 520f2b1d-591e-42c0-96ed-8c5a9c9ee036
📒 Files selected for processing (4)
EXAMPLES.mdV4_MIGRATION_GUIDE.mdsrc/server/auth-client.test.tssrc/server/auth-client.ts
Comment on lines
+186
to
+198
| * If you do not need custom routing, simply return the result of calling | ||
| * the fourth argument (`defaultOnCallback`) to invoke the SDK's default behaviour. | ||
| * | ||
| * @param error - The error returned from Auth0 or during token exchange. | ||
| * `null` when the callback completed successfully. | ||
| * @param ctx - Context about the transaction that initiated the auth flow | ||
| * (e.g. `returnTo` URL, `responseType`, `challengeMode`). | ||
| * @param session - The `SessionData` that will be persisted after a | ||
| * successful callback. `null` when `error` is present. | ||
| * @param defaultOnCallback - The SDK's built-in callback handler. Call this | ||
| * to keep the default redirect/error behaviour while still running your own | ||
| * side effects (e.g. syncing the user to your database, setting extra | ||
| * cookies). Signature: `(error, ctx) => Promise<NextResponse>`. |
Contributor
There was a problem hiding this comment.
Popup callbacks still ignore the hook’s NextResponse.
In these branches, onCallback is awaited only for side effects and the returned response is discarded. That makes the new defaultOnCallback contract inaccurate for popup flows: cookies or headers added to the returned NextResponse never reach the client, and defaultOnCallback does not actually represent the popup flow’s default response. Consider threading a popup-specific default callback that builds the postMessage response and then returning the hook’s result from these branches.
Possible direction
- await this.onCallback(null, onCallbackCtx, mergedSession, this.defaultOnCallback.bind(this));
- const popupResponse = createAuthCompletePostMessageResponse({ ... });
- ...
- return popupResponse;
+ const defaultOnCallback = async () => {
+ const popupResponse = createAuthCompletePostMessageResponse({ ... });
+ ...
+ return popupResponse;
+ };
+
+ return this.onCallback(null, onCallbackCtx, mergedSession, defaultOnCallback);Also applies to: 1269-1274, 1319-1324, 2190-2195
🤖 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 `@src/server/auth-client.ts` around lines 186 - 198, The popup branches call
and await onCallback (the developer hook) but discard its returned NextResponse,
so cookies/headers set by the hook are lost; update those branches in
auth-client.ts to capture the Promise<NextResponse> returned by onCallback
(e.g., const hookResponse = await onCallback(error, ctx, session,
defaultOnCallbackPopup)) and then return that hookResponse instead of ignoring
it, and ensure you pass a popup-specific default callback (e.g.,
defaultOnCallbackPopup) that builds the postMessage response when invoking
onCallback so the hook can override or augment it; apply this change to the
identified popup-handling branches that currently await onCallback but do not
return its value.
Contributor
Author
|
After reviewing this more carefully, we've decided not to move ahead with exposing defaultOnCallback as a The bigger concern is the popup flow: defaultOnCallback would produce a redirect response there, which is Closing this out, but appreciate the contribution. |
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
3 participants
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.
Summary
The onCallback hook currently requires users to re-implement the SDK's default redirect/error behaviour from
scratch if they want any custom side effects (e.g. syncing the user to a database). This change passes the
SDK's built-in callback handler as a fourth argument (defaultOnCallback) to every onCallback invocation, so
custom hooks can run their own logic and then delegate to the default behaviour without duplicating it.
Changes:
Promise
flow added after this was originally authored)
Code and WebStorm
Usage
Previously, users had to manually inspect ctx.returnTo and construct a NextResponse.redirect(...) themselves
to replicate what the SDK does by default.
Co-authored-by: Dan Haughey (greenlynx) — original implementation in #2488