Use redirectToHandler in StackHandler and disallow string default URL target#1472
Conversation
… target
- StackHandler now calls redirectToHandler(page) via the internals API
instead of raw redirect() when a route's URL is not configured for the
handler. This ensures proper cross-domain auth and redirect-back handling.
- DefaultHandlerUrlTarget no longer accepts plain strings. Users must use
{ type: "hosted" } or { type: "handler-component" } explicitly.
A runtime check throws a descriptive error for JS users who pass strings.
- Added JSDoc documentation to HandlerUrlOptions and DefaultHandlerUrlTarget
summarizing valid values for each key.
Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNarrow ChangesDefault handler URL type narrowing and asynchronous redirect refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
| if (typeof configuredUrls?.default === "string") { | ||
| throw new StackAssertionError(`The 'default' URL option must be an object (eg. { type: "hosted" } or { type: "handler-component" }), but received a string: "${configuredUrls.default}". If you meant to use hosted URLs, use { type: "hosted" } instead.`); | ||
| } |
There was a problem hiding this comment.
Removed the runtime string check.
| * **`default`** — fallback target for every key not set individually: | ||
| * - `{ type: "handler-component" }` — use the local `StackHandler` (default). | ||
| * - `{ type: "hosted" }` — use Stack's hosted auth pages. |
There was a problem hiding this comment.
we will probably change the default here with the next breaking version so make that clear
There was a problem hiding this comment.
Updated — both JSDoc blocks now note "current default, may change in the next breaking version".
Greptile SummaryThis PR refactors how
Confidence Score: 4/5Safe to merge — the redirect refactor is well-contained and URL validation flows through the unchanged The core redirect logic, type narrowing, and runtime validation are all correct. The only finding is a test name that contradicts its assertions, which could mislead future contributors but does not affect runtime behavior. url-targets.test.ts — the renamed test description does not match what its assertions verify. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant StackHandlerClient
participant renderComponent
participant AppInternals
Browser->>StackHandlerClient: renders /handler/sign-in
StackHandlerClient->>renderComponent: path, shouldRedirectToPage
renderComponent->>renderComponent: shouldRedirectToPage('signIn')?
alt URL is not local handler target
renderComponent-->>StackHandlerClient: "{ redirectToPage: 'signIn' }"
StackHandlerClient->>AppInternals: "runAsynchronously(redirectToHandler('signIn', {replace:true}))"
AppInternals->>AppInternals: planRedirectToHandler (cross-domain / redirect-back)
AppInternals->>Browser: navigate to resolved URL
StackHandlerClient-->>Browser: return null
else URL is local handler target
renderComponent-->>StackHandlerClient: SignIn component
StackHandlerClient-->>Browser: render component
end
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/template/src/lib/stack-app/url-targets.test.ts:133-148
**Misleading test name — says "does not inherit" but asserts the opposite**
The test name `"does not inherit a hosted default target for the OAuth callback"` is the inverse of what the assertions verify. Both `urls.signIn` and `urls.oauthCallback` are expected to be hosted URLs, meaning the `{ type: "hosted" }` default IS inherited by `oauthCallback`. The old test with this pattern guarded against an absolute-string default leaking into `oauthCallback`, but now strings are disallowed and the code explicitly propagates the hosted type. A reader will trust the name over the assertions and misunderstand the intended behavior.
Reviews (1): Last reviewed commit: "Use redirectToHandler in StackHandler an..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
|
|
||
| it("does not inherit a hosted default target for the OAuth callback", () => { | ||
| vi.stubEnv("NEXT_PUBLIC_STACK_HOSTED_HANDLER_DOMAIN_SUFFIX", ".example-stack-hosted.test"); | ||
|
|
||
| const urls = resolveHandlerUrls({ | ||
| projectId: "project-id", | ||
| urls: { | ||
| default: "https://app.example.test/handler", | ||
| default: { type: "hosted" }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(urls.signIn).toBe("https://app.example.test/handler"); | ||
| expect(urls.oauthCallback).toBe("/handler/oauth-callback"); | ||
| expect(urls.signIn).toBe("https://project-id.example-stack-hosted.test/handler/sign-in"); | ||
| expect(urls.oauthCallback).toBe("https://project-id.example-stack-hosted.test/handler/oauth-callback"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The test name "does not inherit a hosted default target for the OAuth callback" is the inverse of what the assertions verify. Both urls.signIn and urls.oauthCallback are expected to be hosted URLs, meaning the { type: "hosted" } default IS inherited by oauthCallback. The old test with this pattern guarded against an absolute-string default leaking into oauthCallback, but now strings are disallowed and the code explicitly propagates the hosted type. A reader will trust the name over the assertions and misunderstand the intended behavior.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/template/src/lib/stack-app/url-targets.test.ts
Line: 133-148
Comment:
**Misleading test name — says "does not inherit" but asserts the opposite**
The test name `"does not inherit a hosted default target for the OAuth callback"` is the inverse of what the assertions verify. Both `urls.signIn` and `urls.oauthCallback` are expected to be hosted URLs, meaning the `{ type: "hosted" }` default IS inherited by `oauthCallback`. The old test with this pattern guarded against an absolute-string default leaking into `oauthCallback`, but now strings are disallowed and the code explicitly propagates the hosted type. A reader will trust the name over the assertions and misunderstand the intended behavior.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — renamed to "inherits a hosted default target for the OAuth callback".
![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
🧹 Nitpick comments (1)
packages/template/src/lib/stack-app/url-targets.test.ts (1)
125-133: ⚡ Quick winUse inline snapshots for the updated assertions.
These new assertions use regex/
toBe; please switch to inline snapshots for consistency with the test guideline in this repo.As per coding guidelines
**/*.test.{ts,tsx,js,jsx}: Prefer .toMatchInlineSnapshot over other selectors in tests; check snapshot-serializer.ts to understand snapshot formatting and non-deterministic value handling.Also applies to: 145-146
🤖 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/template/src/lib/stack-app/url-targets.test.ts` around lines 125 - 133, Update the test assertions to use inline snapshots: replace the regex/toBe-based thrown-error assertions for resolveHandlerUrls (the test named "rejects a string value for the default URL option" and the second failing assertion around the later block) with Jest inline-snapshot matchers — e.g., use expect(() => resolveHandlerUrls(...)).toThrowErrorMatchingInlineSnapshot(`"expected error message snapshot"`) or expect(...).toMatchInlineSnapshot(`"snapshot"`) as appropriate so the tests follow the repo's snapshot guideline and the snapshot-serializer formatting; ensure the snapshots contain the exact error text produced by resolveHandlerUrls.
🤖 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/template/src/components-page/stack-handler-client.tsx`:
- Around line 305-307: The render path in StackHandlerClient currently calls
runAsynchronously(stackApp[stackAppInternalsSymbol].redirectToHandler(...))
during render when renderComponent returns { redirectToPage }, which can trigger
repeated async redirects or silent failures; move the redirect invocation into a
React effect (useEffect) that depends on redirectToPage (matching the existing
redirect effect pattern) and call
stackApp[stackAppInternalsSymbol].redirectToHandler(redirectToPage, { replace:
true }) from inside that effect; surface failures to the user instead of only
logging by using runAsynchronouslyWithAlert (or rendering an explicit
error/fallback UI) to handle rejected promises; ensure you leave the render
return as null while the effect performs the redirect so redirects run only as a
side effect and failures are visible.
In `@packages/template/src/lib/stack-app/url-targets.test.ts`:
- Line 135: The test heading starting with it("does not inherit a hosted default
target for the OAuth callback", ...) is misleading because the assertions on
oauthCallback (lines ~145-146) expect hosted inheritance; rename the test title
to accurately state the asserted behavior (e.g., "inherits a hosted default
target for the OAuth callback" or similar) so the test name matches the
assertions in url-targets.test.ts and update only the string passed to the
it(...) call that refers to oauthCallback.
---
Nitpick comments:
In `@packages/template/src/lib/stack-app/url-targets.test.ts`:
- Around line 125-133: Update the test assertions to use inline snapshots:
replace the regex/toBe-based thrown-error assertions for resolveHandlerUrls (the
test named "rejects a string value for the default URL option" and the second
failing assertion around the later block) with Jest inline-snapshot matchers —
e.g., use expect(() =>
resolveHandlerUrls(...)).toThrowErrorMatchingInlineSnapshot(`"expected error
message snapshot"`) or expect(...).toMatchInlineSnapshot(`"snapshot"`) as
appropriate so the tests follow the repo's snapshot guideline and the
snapshot-serializer formatting; ensure the snapshots contain the exact error
text produced by resolveHandlerUrls.
🪄 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
Run ID: 2aa6eb61-7edd-44f4-a9e7-c1c338ffe568
📒 Files selected for processing (6)
packages/stack-shared/src/interface/handler-urls.tspackages/template/src/components-page/stack-handler-client.tsxpackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/interfaces/client-app.tspackages/template/src/lib/stack-app/url-targets.test.tspackages/template/src/lib/stack-app/url-targets.ts
…ge, fix test name Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
…unAsynchronouslyWithAlert Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
1 participant
StackHandler now calls
redirectToHandler(page)via internals instead of rawredirect()when a route's URL is not configured for the handler. This ensures proper cross-domain auth and redirect-back parameter handling.DefaultHandlerUrlTargetno longer accepts plain strings — must use{ type: "hosted" }or{ type: "handler-component" }. Runtime validation throws a descriptive error for JS users who accidentally pass a string.Added JSDoc documentation to
HandlerUrlOptionsandDefaultHandlerUrlTargetsummarizing valid values for each key.Link to Devin session: https://app.devin.ai/sessions/8db35e6835b849c38ebafde2f243fb31
Requested by: @N2D4
Summary by CodeRabbit
Breaking Changes
New Features
Improvements