refactor(core): align redirect options and response values across APIs#174
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes redirect options into ChangesRedirect handling refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/api/updateSession.ts`:
- Around line 43-60: The branch returning { headers: newHeaders, session,
success: isSuccess, ... } must include an error field when refreshSession()
yields session === null so it complies with
UpdateSessionAPIReturn/AuthActionAPIReturn; update the return object (and the
inner toResponse payload) to set error to a descriptive string or error object
whenever isSuccess is false (e.g., "Session refresh failed" or include
underlying reason), and ensure the Response.status remains 401 for failures;
look for refreshSession(), UpdateSessionAPIReturn, toResponse and ensure both
the top-level return value and the Response.json body include the error property
on non-throwing failures.
🪄 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: b1d75242-6238-47c9-b9fd-dc37bce75c1b
📒 Files selected for processing (7)
packages/core/src/@types/api.tspackages/core/src/actions/signIn/signIn.tspackages/core/src/actions/updateSession/updateSession.tspackages/core/src/api/updateSession.tspackages/core/src/schemas.tspackages/core/test/actions/updateSession/updateSession.test.tspackages/core/test/api/updateSession.test.ts
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)
packages/core/test/api/signInCredentials.test.ts (1)
56-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis fixture is exercising missing
BASE_URL, not the invalid-authorize branch.Because
BASE_URLis still unset here, the assertion now locks inINVALID_OAUTH_CONFIGURATION, which duplicates the dedicated URL-configuration test below instead of verifying the "missing required fields" path. Stub a valid base URL in this case so it actually covers the malformedauthorizereturn.🤖 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/core/test/api/signInCredentials.test.ts` around lines 56 - 84, The test "invalid authorize by missing required fields" is failing because BASE_URL is unset and triggers INVALID_OAUTH_CONFIGURATION; to fix it, stub a valid base URL before creating the auth instance so the test exercises the malformed authorize return path: set process.env.BASE_URL (or pass a baseUrl option if createAuth supports it) prior to calling createAuth(...) and then call api.signInCredentials(...) as-is to assert the missing-required-fields behavior from the authorize function; ensure changes reference the test name, createAuth, the credentials.authorize stub, and api.signInCredentials.
🧹 Nitpick comments (2)
packages/core/test/api/signOut.test.ts (1)
96-118: ⚡ Quick winAlso assert that
redirect: falseclears theLocationheader.This branch is specifically about suppressing server-side redirects, so the API test should lock that down as well instead of only checking the JSON shape.
Suggested fix
const out = await api.signOut({ headers: { Cookie: `aura-auth.session_token=${sessionToken}; aura-auth.csrf_token=${csrfToken}`, }, redirect: false, redirectTo: "/dashboard", }) + expect(out.headers.get("Location")).toBeNull() expect(out).toEqual({ success: true, redirect: false, redirectURL: "/dashboard", headers: expect.any(Headers), toResponse: expect.any(Function), })🤖 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/core/test/api/signOut.test.ts` around lines 96 - 118, The test "signOut with redirect: false and redirectTo" currently only asserts the JSON shape; add an assertion that the response headers do not include a Location header when redirect is false. After calling api.signOut in the test, inspect the returned out.headers (the Headers object) and assert that out.headers.get("Location") is null (or strictly falsy) to ensure server-side redirects are suppressed; update the test around the api.signOut invocation in signOut.test.ts accordingly.packages/core/test/api/signInCredentials.test.ts (1)
157-173: ⚡ Quick winThe
redirect: falsepath is not being tested here.This case never passes
redirect: false, so it currently re-tests the default redirect-enabled behavior and leaves the non-redirect API contract uncovered.Suggested fix
test("signIn with redirect: false and valid redirectTo", async () => { vi.stubEnv("BASE_URL", "https://example.com") const signIn = await api.signInCredentials({ payload: { username: "johndoe", password: "1234567890", }, + redirect: false, redirectTo: "https://example.com/dashboard", }) + expect(signIn.headers.get("Location")).toBeNull() expect(signIn).toEqual({ success: true, - redirect: true, - redirectURL: null, + redirect: false, + redirectURL: "/dashboard", headers: expect.any(Headers), toResponse: expect.any(Function), }) })🤖 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/core/test/api/signInCredentials.test.ts` around lines 157 - 173, The test currently calls api.signInCredentials without passing redirect: false so it exercises the default redirect-enabled path; update or add the test for api.signInCredentials (the "signIn with redirect: false and valid redirectTo" case) to pass redirect: false in the call and assert the non-redirect contract: expect success true, redirect false, redirectURL to equal the provided redirectTo (or appropriate non-null value), headers to be a Headers instance, and toResponse to be a function; keep the BASE_URL stub (vi.stubEnv("BASE_URL", "https://example.com")) as needed and mirror existing assertions but with redirect:false and the expected redirectURL value.
🤖 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/core/src/api/signOut.ts`:
- Around line 59-66: The error response in toResponse (inside signOut.ts) has a
typo and inconsistent structure; change the JSON body to match the success-path
shape by removing the headers field, rename redirectsURL to redirectURL, and
return a Response with an explicit non-200 status (e.g., status: 500) and any
HTTP headers passed via the Response init headers option rather than inside the
JSON body; update the toResponse implementation to return Response.json({
success: false, redirect: false, redirectURL: null }, { status: 500, headers })
so clients receive the correct contract.
---
Outside diff comments:
In `@packages/core/test/api/signInCredentials.test.ts`:
- Around line 56-84: The test "invalid authorize by missing required fields" is
failing because BASE_URL is unset and triggers INVALID_OAUTH_CONFIGURATION; to
fix it, stub a valid base URL before creating the auth instance so the test
exercises the malformed authorize return path: set process.env.BASE_URL (or pass
a baseUrl option if createAuth supports it) prior to calling createAuth(...) and
then call api.signInCredentials(...) as-is to assert the missing-required-fields
behavior from the authorize function; ensure changes reference the test name,
createAuth, the credentials.authorize stub, and api.signInCredentials.
---
Nitpick comments:
In `@packages/core/test/api/signInCredentials.test.ts`:
- Around line 157-173: The test currently calls api.signInCredentials without
passing redirect: false so it exercises the default redirect-enabled path;
update or add the test for api.signInCredentials (the "signIn with redirect:
false and valid redirectTo" case) to pass redirect: false in the call and assert
the non-redirect contract: expect success true, redirect false, redirectURL to
equal the provided redirectTo (or appropriate non-null value), headers to be a
Headers instance, and toResponse to be a function; keep the BASE_URL stub
(vi.stubEnv("BASE_URL", "https://example.com")) as needed and mirror existing
assertions but with redirect:false and the expected redirectURL value.
In `@packages/core/test/api/signOut.test.ts`:
- Around line 96-118: The test "signOut with redirect: false and redirectTo"
currently only asserts the JSON shape; add an assertion that the response
headers do not include a Location header when redirect is false. After calling
api.signOut in the test, inspect the returned out.headers (the Headers object)
and assert that out.headers.get("Location") is null (or strictly falsy) to
ensure server-side redirects are suppressed; update the test around the
api.signOut invocation in signOut.test.ts accordingly.
🪄 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: 1a0bea56-07d5-4452-a8ee-c41eb56e29b5
📒 Files selected for processing (17)
packages/core/src/@types/api.tspackages/core/src/actions/signInCredentials/signInCredentials.tspackages/core/src/actions/signOut/signOut.tspackages/core/src/api/credentials.tspackages/core/src/api/signOut.tspackages/core/src/api/updateSession.tspackages/core/src/schemas.tspackages/core/test/actions/signInCredentials/signInCredentials.test.tspackages/core/test/actions/signOut/signOut.test.tspackages/core/test/api/signInCredentials.test.tspackages/core/test/api/signOut.test.tspackages/core/test/api/updateSession.test.tspackages/core/test/presets.tspackages/elysia/package.jsonpackages/express/package.jsonpackages/hono/package.jsonpackages/integration/package.json
✅ Files skipped from review due to trivial changes (4)
- packages/hono/package.json
- packages/integration/package.json
- packages/express/package.json
- packages/elysia/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/api/updateSession.ts
- packages/core/test/api/updateSession.test.ts
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.
Description
New features are coming soon....
Summary by CodeRabbit
redirectandredirectTooptions for sign-in and session update endpoints to control post-authentication redirect behaviorSummary by CodeRabbit