refactor(core): improve user messages with clearer context#189
Open
halvaradop wants to merge 1 commit into
Open
refactor(core): improve user messages with clearer context#189halvaradop wants to merge 1 commit into
halvaradop wants to merge 1 commit into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR refactors the entire error handling system by replacing seven custom error classes with a unified ChangesAura Error System Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/core/src/shared/utils.ts (1)
93-98:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUnresolved provider env placeholders still become a live Basic header.
Lines 94-95 fall back to the raw arguments when the env lookup misses. In
packages/core/src/oauth/notion.ts, Lines 56-61, that turns absentNOTION_CLIENT_ID/NOTION_CLIENT_SECRETvars into literal credentials, so Line 97 never throws and the misconfiguration is only discovered after an outbound token request. This helper needs an explicit “resolve env key” vs “use literal credential” contract, and the fail-fast path should use a configuration-oriented Aura code instead ofAUTH_BASIC_CREDENTIALS_INVALID.🤖 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/src/shared/utils.ts` around lines 93 - 98, The helper createBasicAuthHeader currently falls back to the raw username/password when getEnv returns undefined (getEnv(username) ?? username), which lets unresolved env placeholders become live credentials; change the contract to require resolved env values: call getEnv(username) and getEnv(password) and if either returns undefined/null, do NOT use the literal argument — throw a configuration-oriented error (replace AuraAuthError with a config error class such as AuraConfigError) with a clear config-missing code (e.g., "CONFIG_ENV_VAR_MISSING") and message identifying which env key failed; ensure createBasicAuthHeader, getEnv usage, and the thrown error class/name are updated so callers like oauth/notion.ts fail fast on missing envs.packages/core/src/api/signOut.ts (1)
60-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix failure response contract (
redirectURL) and return an error status.Line 63 uses
redirectsURL(typo), and this failuretoResponsecurrently returns default 200. That breaks response shape consistency and can mask failures at HTTP level.Suggested fix
toResponse: () => { - return Response.json({ - success: false, - redirect: false, - redirectsURL: null, - }) + return Response.json( + { + success: false, + redirect: false, + redirectURL: null, + error: { code, message }, + }, + { headers, status: 400 } + ) },🤖 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/src/api/signOut.ts` around lines 60 - 64, The failure response in signOut.ts returns the wrong field name and a 200 status; update the Response.json call in the signOut (or its toResponse) failure branch to use the correct redirectURL property (not redirectsURL) and return a non-200 HTTP status (e.g., status: 500 or other appropriate error code) so the response shape and status indicate failure; ensure the payload still contains success: false and include an error message or null redirectURL as before.packages/core/src/api/credentials.ts (1)
80-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow the invalid-credentials branch to the explicit credentials error code.
Line 80 currently treats any
AuraAuthErroras invalid credentials. That mislabels unrelated auth/config/origin errors asINVALID_CREDENTIALSand returns the same 401 path.Suggested fix
- if (isAuraAuthError(error)) { + if (isAuraAuthError(error) && error.code === "AUTH_CREDENTIALS_INVALID") { logger?.log("INVALID_CREDENTIALS", { severity: "warning", structuredData: { path: "/signIn/credentials" }, }) return invalidCredentials }🤖 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/src/api/credentials.ts` around lines 80 - 85, The code currently treats any AuraAuthError as invalid credentials; change the condition so the invalid-credentials branch only triggers for the explicit credential error code (e.g., check error.code or error.errorCode equals the project's credential error constant) instead of any AuraAuthError. Concretely, update the if around isAuraAuthError(error) to something like isAuraAuthError(error) && error.code === <CREDENTIALS_ERROR_CODE>, then call logger?.log("INVALID_CREDENTIALS", ...) and return invalidCredentials; otherwise let other AuraAuthError cases fall through or be handled separately.packages/core/src/actions/callback/callback.ts (1)
41-53:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't report provider-declared OAuth errors as "missing parameters".
This branch runs when the callback contains an OAuth
errorpayload from the provider, e.g.access_deniedorserver_error. ThrowingAUTH_CALLBACK_MISSING_PARAMETERSturns an explicit upstream denial/failure into the wrong message and removes the distinction between "provider rejected the flow" and "the callback was malformed." Use a dedicated AuraAuthError code for authorization error responses instead.🤖 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/src/actions/callback/callback.ts` around lines 41 - 53, The code currently detects an OAuth provider error via OAuthAuthorizationErrorResponse.safeParse and then throws AuraAuthError with code "AUTH_CALLBACK_MISSING_PARAMETERS"; change this to throw a distinct authorization error that preserves provider details (e.g., throw new AuraAuthError({ code: "AUTH_CALLBACK_AUTHORIZATION_ERROR", meta: { error, error_description } })) so provider-declared errors (access_denied, server_error) are not misreported as missing parameters; update the thrown error in the same branch where OAuthAuthorizationErrorResponse, criticalAuthErrors, and logger are used and ensure the logger still records severity and structuredData.packages/core/src/actions/callback/access-token.ts (1)
25-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't collapse every missing precondition into
INVALID_OAUTH_PROVIDER_URL_CONFIG.This guard also fires when
clientSecret,code, orcodeVerifieris missing, so callers will get a provider-URL error for failures that are unrelated to the provider URL. Split the URL-only validation from the general callback/config preconditions and map them to different catalog codes.🤖 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/src/actions/callback/access-token.ts` around lines 25 - 36, Split the single guard into two checks: first validate the provider-URL related fields (e.g., redirectURI and any provider URL-specific input) and if those are missing log structuredData and throw AuraAuthError with code "INVALID_OAUTH_PROVIDER_URL_CONFIG"; then validate the remaining callback/config preconditions (clientId, clientSecret, code, codeVerifier, accessToken) and if any are missing log structuredData and throw a different AuraAuthError code such as "INVALID_OAUTH_CONFIGURATION". Keep the same logger structuredData keys (has_client_id, has_client_secret, has_access_token, has_redirect_uri, has_code, has_code_verifier) and apply them to both checks so the logs show which specific fields are absent; update the throw sites in access-token.ts accordingly.
🧹 Nitpick comments (1)
packages/core/test/actions/callback/access-token.test.ts (1)
66-68: ⚡ Quick winThese tests are over-coupled to mutable internal prose; assert stable error contract fields instead.
packages/core/test/actions/callback/access-token.test.ts#L66-L68: assert errorcode/type(or status+payload) rather than long internal message text.packages/core/test/actions/callback/access-token.test.ts#L97-L99: same refactor—prefer contract fields over prose.packages/core/test/actions/callback/access-token.test.ts#L135-L137: same refactor—replace exact full sentence assertion with stable identifiers.packages/core/test/actions/callback/userinfo.test.ts#L117-L119: switch to checking stable error identifiers.packages/core/test/actions/callback/userinfo.test.ts#L150-L152: switch to checking stable error identifiers.packages/core/test/actions/callback/userinfo.test.ts#L177-L179: switch to checking stable error identifiers.packages/core/test/actions/callback/userinfo.test.ts#L258-L260: switch to checking stable error identifiers.packages/core/test/actions/signIn/authorization.test.ts#L110-L112: prefer code/type assertions for rejection paths.packages/core/test/actions/signIn/authorization.test.ts#L151-L153: same refactor to stable contract assertions.packages/core/test/actions/signIn/authorization.test.ts#L165-L167: same refactor to stable contract assertions.packages/core/test/oauth.test.ts#L27-L29: assert stable structured error attributes instead of exact message.packages/core/test/oauth.test.ts#L54-L56: assert stable structured error attributes instead of exact message.packages/core/test/api/signOut.test.ts#L22-L24: assert stable error code/type for missing session token path.🤖 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/actions/callback/access-token.test.ts` around lines 66 - 68, Replace fragile assertions that match full error message prose with assertions on stable error contract fields (e.g., error.code, error.type, or status+payload) in the listed tests: packages/core/test/actions/callback/access-token.test.ts (the rejects.toThrow at L66-L68, and similar at L97-L99 and L135-L137), packages/core/test/actions/callback/userinfo.test.ts (L117-L119, L150-L152, L177-L179, L258-L260), packages/core/test/actions/signIn/authorization.test.ts (L110-L112, L151-L153, L165-L167), packages/core/test/oauth.test.ts (L27-L29, L54-L56), and packages/core/test/api/signOut.test.ts (L22-L24); locate the Promise rejection checks (e.g., the .rejects.toThrow calls) and change them to assert the error object has the expected stable fields (like expect(err.code).toBe(...) or expect(err.type).toBe(...) or expect(err.status).toBe(...) and/or inspect err.payload) instead of matching the full human-readable message.
🤖 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/actions/callback/access-token.ts`:
- Around line 70-94: The JSON.parse step (response.json()) needs its own
try/catch so parse failures are classified as response-format errors rather than
generic transport errors: wrap the response.json() call in a small try block and
if it throws, log via logger and throw an AuraAuthError with code
"INVALID_OAUTH_ACCESS_TOKEN_RES_FORMAT" in access-token.ts (affecting the logic
around OAuthAccessTokenResponse/OAuthAccessTokenErrorResponse handling), and
similarly throw "INVALID_OAUTH_USER_INFO_RES_FORMAT" in userinfo.ts (around the
OAuth user info response validation). Keep the outer try to handle
transport/network failures and preserve existing isAuraAuthError rethrows and
other error codes; only reclassify JSON parse exceptions to the new *_RES_FORMAT
codes and include the original parse error as the cause when creating the
AuraAuthError.
In `@packages/core/src/actions/signIn/authorization-url.ts`:
- Around line 23-27: The code currently constructs the provider URL with new
URL(baseURL) which can throw a native TypeError for malformed strings; wrap the
URL construction in a try/catch around the new URL(baseURL) call in the
authorization URL logic (the spot using authorizeConfig/baseURL and creating
url) and on any exception rethrow an AuraAuthError with code
"INVALID_OAUTH_PROVIDER_URL_CONFIG" (include the original error message/details
in the AuraAuthError payload or log) so malformed provider URLs surface as the
standardized AuraAuthError instead of leaking a native TypeError.
In `@packages/core/src/actions/signUp/signUp.ts`:
- Line 10: The fallback empty Zod schema uses z.object() which is not the
intended API in Zod v4; update the fallback in the sign-up action so that body:
config?.schema ?? z.object({}) uses an explicit empty shape. Locate the
occurrence of config?.schema and replace the z.object() fallback with
z.object({}) (e.g., in the signUp handler where body is assigned) to ensure a
proper empty-object schema.
In `@packages/core/src/api/signIn.ts`:
- Around line 77-80: When projecting AuraAuthError into API payloads, replace
uses of the internal error.message with the user-facing error.userMessage:
inside the isAuraAuthError(error) branches (e.g., in signIn.ts where code =
error.code and message = error.message), set message = error.userMessage; make
the same change in the corresponding Aura error branches in credentials.ts,
signOut.ts, signUp.ts, and updateSession.ts so each branch uses
error.userMessage when building the response object (preserve existing code =
error.code behavior and only swap the message source).
In `@packages/core/src/cookie.ts`:
- Around line 104-106: The current check in cookie handling only tests `if
(!cookies)` which misses empty arrays; update the condition where `cookies` is
validated (refer to the `cookies` variable and the throw of `AuraAuthError` with
code `"SET_COOKIE_NOT_FOUND"`) to treat an empty collection as "not found" as
well (e.g., check for `cookies` falsy OR `cookies.length === 0`) so that empty
responses trigger the `SET_COOKIE_NOT_FOUND` error instead of
`SET_COOKIE_INVALID_VALUE`.
In `@packages/core/src/shared/crypto.ts`:
- Around line 83-85: In verifyCSRF, the catch currently replaces every error
with AuraAuthError({ code: "CSRF_TOKEN_MISSING", cause: error }), which hides
explicit CSRF_TOKEN_MISMATCH errors; change the catch to rethrow the original
error when it's already an AuraAuthError (or when error.code ===
"CSRF_TOKEN_MISMATCH"), and only wrap/throw a new AuraAuthError with code
"CSRF_TOKEN_MISSING" for other unexpected failures—refer to the verifyCSRF
function and AuraAuthError usage to implement this conditional rethrowing logic
so mismatch errors are preserved.
In `@packages/core/src/shared/unstable_error.ts`:
- Around line 407-413: The CONFIG_BASE_URL_MISSING catalog entry currently has
empty message and userMessage which causes AuraAuthError.message to be blank and
toResponse() to emit an invalid empty-message response; update the
CONFIG_BASE_URL_MISSING object (the constant with key CONFIG_BASE_URL_MISSING)
to provide non-empty defaults for both message and userMessage (e.g. a concise
internal message and a user-facing string), keeping the type, statusCode, and
name unchanged so that AuraAuthError.message, toResponse(), and errorHandler.ts
produce a valid response (this same change should also be applied at the other
occurrence referenced around lines 729-733).
In `@packages/core/src/validator/registry.ts`:
- Around line 61-62: The three schema-type fallback throws that currently use
throw new AuraAuthError({ code: "SCHEMA_INVALID_MODE" }) are misclassifying
unsupported schema fallthroughs; replace those error instances so they throw
AuraAuthError with code "SCHEMA_UNSUPPORTED" instead (or alternatively add an
explicit runtime mode guard earlier if you intend to validate mode), i.e.,
locate each occurrence of throw new AuraAuthError({ code: "SCHEMA_INVALID_MODE"
}) in registry.ts (the schema-type fallback branches) and change the code value
to "SCHEMA_UNSUPPORTED".
---
Outside diff comments:
In `@packages/core/src/actions/callback/access-token.ts`:
- Around line 25-36: Split the single guard into two checks: first validate the
provider-URL related fields (e.g., redirectURI and any provider URL-specific
input) and if those are missing log structuredData and throw AuraAuthError with
code "INVALID_OAUTH_PROVIDER_URL_CONFIG"; then validate the remaining
callback/config preconditions (clientId, clientSecret, code, codeVerifier,
accessToken) and if any are missing log structuredData and throw a different
AuraAuthError code such as "INVALID_OAUTH_CONFIGURATION". Keep the same logger
structuredData keys (has_client_id, has_client_secret, has_access_token,
has_redirect_uri, has_code, has_code_verifier) and apply them to both checks so
the logs show which specific fields are absent; update the throw sites in
access-token.ts accordingly.
In `@packages/core/src/actions/callback/callback.ts`:
- Around line 41-53: The code currently detects an OAuth provider error via
OAuthAuthorizationErrorResponse.safeParse and then throws AuraAuthError with
code "AUTH_CALLBACK_MISSING_PARAMETERS"; change this to throw a distinct
authorization error that preserves provider details (e.g., throw new
AuraAuthError({ code: "AUTH_CALLBACK_AUTHORIZATION_ERROR", meta: { error,
error_description } })) so provider-declared errors (access_denied,
server_error) are not misreported as missing parameters; update the thrown error
in the same branch where OAuthAuthorizationErrorResponse, criticalAuthErrors,
and logger are used and ensure the logger still records severity and
structuredData.
In `@packages/core/src/api/credentials.ts`:
- Around line 80-85: The code currently treats any AuraAuthError as invalid
credentials; change the condition so the invalid-credentials branch only
triggers for the explicit credential error code (e.g., check error.code or
error.errorCode equals the project's credential error constant) instead of any
AuraAuthError. Concretely, update the if around isAuraAuthError(error) to
something like isAuraAuthError(error) && error.code ===
<CREDENTIALS_ERROR_CODE>, then call logger?.log("INVALID_CREDENTIALS", ...) and
return invalidCredentials; otherwise let other AuraAuthError cases fall through
or be handled separately.
In `@packages/core/src/api/signOut.ts`:
- Around line 60-64: The failure response in signOut.ts returns the wrong field
name and a 200 status; update the Response.json call in the signOut (or its
toResponse) failure branch to use the correct redirectURL property (not
redirectsURL) and return a non-200 HTTP status (e.g., status: 500 or other
appropriate error code) so the response shape and status indicate failure;
ensure the payload still contains success: false and include an error message or
null redirectURL as before.
In `@packages/core/src/shared/utils.ts`:
- Around line 93-98: The helper createBasicAuthHeader currently falls back to
the raw username/password when getEnv returns undefined (getEnv(username) ??
username), which lets unresolved env placeholders become live credentials;
change the contract to require resolved env values: call getEnv(username) and
getEnv(password) and if either returns undefined/null, do NOT use the literal
argument — throw a configuration-oriented error (replace AuraAuthError with a
config error class such as AuraConfigError) with a clear config-missing code
(e.g., "CONFIG_ENV_VAR_MISSING") and message identifying which env key failed;
ensure createBasicAuthHeader, getEnv usage, and the thrown error class/name are
updated so callers like oauth/notion.ts fail fast on missing envs.
---
Nitpick comments:
In `@packages/core/test/actions/callback/access-token.test.ts`:
- Around line 66-68: Replace fragile assertions that match full error message
prose with assertions on stable error contract fields (e.g., error.code,
error.type, or status+payload) in the listed tests:
packages/core/test/actions/callback/access-token.test.ts (the rejects.toThrow at
L66-L68, and similar at L97-L99 and L135-L137),
packages/core/test/actions/callback/userinfo.test.ts (L117-L119, L150-L152,
L177-L179, L258-L260), packages/core/test/actions/signIn/authorization.test.ts
(L110-L112, L151-L153, L165-L167), packages/core/test/oauth.test.ts (L27-L29,
L54-L56), and packages/core/test/api/signOut.test.ts (L22-L24); locate the
Promise rejection checks (e.g., the .rejects.toThrow calls) and change them to
assert the error object has the expected stable fields (like
expect(err.code).toBe(...) or expect(err.type).toBe(...) or
expect(err.status).toBe(...) and/or inspect err.payload) instead of matching the
full human-readable message.
🪄 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: 070c010d-9450-450a-bd03-d54f09121a37
📒 Files selected for processing (46)
packages/core/deno.jsonpackages/core/src/actions/callback/access-token.tspackages/core/src/actions/callback/callback.tspackages/core/src/actions/callback/userinfo.tspackages/core/src/actions/signIn/authorization-url.tspackages/core/src/actions/signIn/authorization.tspackages/core/src/actions/signUp/signUp.tspackages/core/src/api/credentials.tspackages/core/src/api/signIn.tspackages/core/src/api/signOut.tspackages/core/src/api/signUp.tspackages/core/src/api/updateSession.tspackages/core/src/client/client.tspackages/core/src/cookie.tspackages/core/src/jose.tspackages/core/src/oauth/index.tspackages/core/src/router/errorHandler.tspackages/core/src/session/jose-manager.tspackages/core/src/session/stateless.tspackages/core/src/session/strategy.tspackages/core/src/shared/crypto.tspackages/core/src/shared/errors.tspackages/core/src/shared/unstable_error.tspackages/core/src/shared/utils.tspackages/core/src/validator/registry.tspackages/core/src/validator/validator.tspackages/core/test/actions/callback/access-token.test.tspackages/core/test/actions/callback/callback.test.tspackages/core/test/actions/callback/userinfo.test.tspackages/core/test/actions/signIn/authorization.test.tspackages/core/test/actions/signIn/signIn.test.tspackages/core/test/actions/signOut/signOut.test.tspackages/core/test/api/signIn.test.tspackages/core/test/api/signInCredentials.test.tspackages/core/test/api/signOut.test.tspackages/core/test/api/signUp.test.tspackages/core/test/api/updateSession.test.tspackages/core/test/instance.test.tspackages/core/test/jose.test.tspackages/core/test/oauth.test.tspackages/elysia/package.jsonpackages/express/package.jsonpackages/hono/package.jsonpackages/next/package.jsonpackages/react-router/package.jsonpackages/react/package.json
💤 Files with no reviewable changes (1)
- packages/core/src/shared/errors.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.
Summary by CodeRabbit
Release Notes
New Features
AuraAuthError) with specific error codes and user-friendly error messages.Improvements
Chores
^0.8.0.0.7.2.