-
Notifications
You must be signed in to change notification settings - Fork 4
Implement Nimbus error handling components and utilities for consistent user-facing error messages #1613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 12-01-chore_migrate_build_system_to_vite_and_update_ts_config
Are you sure you want to change the base?
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces a comprehensive error-handling infrastructure across the Cyberstorm Remix and Thunderstore API packages. It adds a new NimbusErrorBoundary component with styling for displaying errors, utilities for normalizing errors from multiple sources into a standardized Possibly related PRsBased on the provided data, no strongly related PRs were identified with direct code-level connections to this error-handling implementation. Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (10)
apps/cyberstorm-remix/cyberstorm/utils/searchParamsUtils.ts (1)
8-21: Solid, defensive parsing; consider clarifying signed-int behavior and return typeThe implementation is robust and conservative: trims input, rejects mixed strings (
"10px"), guards againstNaNand unsafe integers, and normalizes invalid inputs toundefined. No correctness issues from what I can see.Two small, optional follow-ups to consider:
- If search params are expected to support negative or
+-prefixed values (e.g."‑1","+5"), the/^\d+$/check will currently reject them. Either broaden the pattern (e.g./^[+-]?\d+$/) or document that only non‑negative integers are valid.- For an exported utility that’s part of shared infra, adding an explicit return type can make the contract clearer:
export function parseIntegerSearchParam(value: string | null): number | undefined { ... }Please double-check existing and planned callers to confirm whether signed integers should be accepted or if non‑negative-only is the intended contract.
packages/thunderstore-api/src/errors/sanitizeServerDetail.ts (1)
1-40: Sanitizer behavior is solid; no changes requiredThe combination of control-character stripping, whitespace collapsing, and 400‑char truncation with an ellipsis gives you a safe, bounded string for UI use. The early returns on falsy/empty values keep things defensive. If you ever need different limits per call-site, you could consider making
MAX_LENGTHconfigurable, but that’s strictly optional.apps/cyberstorm-remix/cyberstorm/utils/errors/resolveRouteErrorPayload.ts (1)
1-72: Route error normalization is coherent; consider hiding rawErrormessagesThe overall flow—RouteErrorResponse →
UserFacingErrorPayloadparsing →ApiError→ generic payload—is clear and matches the documented strategy.One behavioral nit: for generic
Errorinstances you currently surfaceerror.messagedirectly as the headline. That can leak fairly technical messages (“Invariant failed…”) to end users, which may not match the “consistent, user‑friendly” copy you’re aiming for. You might prefer a generic headline for these cases and keep the original message only in logging or context.For example (optional):
- if (error instanceof Error) { - return { - headline: error.message || "Something went wrong.", - description: undefined, - category: "server", - status: 500, - }; - } + if (error instanceof Error) { + return { + headline: "Something went wrong.", + description: undefined, + category: "server", + status: 500, + }; + }Everything else in this helper looks good to me.
docs/error-handling.md (1)
1-198: Docs align with implementation; consider making snippets copy‑pasteableThe narrative matches the behavior in
handleLoaderError,userFacingErrorResponse, andresolveRouteErrorPayloadwell. One minor nit: in the first loader example you callthrowUserFacingPayloadResponsebut don’t show its import, which makes the snippet slightly less copy‑pasteable for newer contributors. Adding that import (and similar ones elsewhere) would make this doc even more self‑contained.apps/cyberstorm-remix/cyberstorm/utils/errors/handleLoaderError.ts (1)
1-88: Loader error handling logic is solid; double‑checkfindLastsupportThe mapping flow (default mappings + route‑specific overrides, last‑match wins) and payload construction look good and match the docs. Rethrowing
Responseunchanged and delegating unmapped cases tothrowUserFacingErrorResponsegives a clear, predictable path.One thing to verify:
Array.prototype.findLastis still relatively new. If your Remix server or bundler targets environments that don’t support it out of the box, this could break at runtime. If there’s any doubt, consider a more broadly compatible pattern:- const mapping = allOptions.findLast((candidate) => { - const statuses = Array.isArray(candidate.status) - ? candidate.status - : [candidate.status]; - return statuses.includes(error.statusCode); - }); + const mapping = [...allOptions].reverse().find((candidate) => { + const statuses = Array.isArray(candidate.status) + ? candidate.status + : [candidate.status]; + return statuses.includes(error.statusCode); + });Otherwise, the structure and typing of
LoaderErrorMapping/HandleLoaderErrorOptionslook good.apps/cyberstorm-remix/cyberstorm/utils/errors/loaderMappings.ts (1)
24-37: Confirm whether CONFLICT and RATE_LIMIT should be indefaultErrorMappings
CONFLICT_MAPPING(409) andRATE_LIMIT_MAPPING(429) are defined but not included indefaultErrorMappings. If callers rely ondefaultErrorMappingsas a complete baseline, consider adding them or documenting that they’re opt‑in only to avoid silent omission of these common cases.Also applies to: 71-81
apps/cyberstorm-remix/cyberstorm/utils/errors/userFacingErrorResponse.ts (1)
72-107: Consider tightening the runtime payload validation
isUserFacingErrorPayloadonly checks thatheadlineandcategoryare strings. If you expect to rely on payloads coming from untrusted sources, consider also validating that:
categoryis one of the knownUserFacingErrorCategoryvalues, andstatus(when present) is a number, andcontext(when present) is a plain object.This would make
parseUserFacingErrorPayloadmore defensive without changing the external API.packages/thunderstore-api/src/errors/userFacingError.ts (1)
199-216: Align 409/conflict categorization with loader mappings
categorizeStatusdoesn’t treat409specially and will currently return"unknown", whereasCONFLICT_MAPPINGinloaderMappings.tsuses status409with category"server". For consistency between API error categorization and loader mappings, consider adding a409branch here (likely mapping to"server"or a dedicated"conflict"category if you introduce one).apps/cyberstorm-remix/cyberstorm/utils/errors/NimbusErrorBoundary/NimbusErrorBoundary.tsx (2)
130-177: Minor UX/accessibility nits for the fallback surfaceThe fallback is functionally sound (safe payload resolution, sane default copy, robust retry behavior), but you might consider:
- Using a heading element (
<h1>/<h2>) instead of a bare<p>fortitleto improve semantics.- Optionally exposing a
role="alert"or similar on the container when appropriate.These are small tweaks that can be deferred but would improve accessibility.
183-215: Use the safe resolver inNimbusDefaultRouteErrorBoundaryand generalize the JSDoc
NimbusDefaultRouteErrorBoundarycallsresolveRouteErrorPayloaddirectly, whereasNimbusErrorBoundaryFallbackwraps it insafeResolveRouteErrorPayloadto avoid mapper failures crashing the UI. For consistency and extra robustness, consider using the safe wrapper here as well, and update the JSDoc to reflect that this component is generic (the comment still references the wiki route).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/cyberstorm-remix/cyberstorm/utils/errors/NimbusErrorBoundary/NimbusErrorBoundary.css(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/errors/NimbusErrorBoundary/NimbusErrorBoundary.tsx(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/errors/NimbusErrorBoundary/index.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/errors/handleLoaderError.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/errors/loaderMappings.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/errors/resolveRouteErrorPayload.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/errors/userFacingErrorResponse.ts(1 hunks)apps/cyberstorm-remix/cyberstorm/utils/searchParamsUtils.ts(1 hunks)docs/error-handling.md(1 hunks)packages/thunderstore-api/src/errors/sanitizeServerDetail.ts(1 hunks)packages/thunderstore-api/src/errors/userFacingError.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/thunderstore-api/src/errors/sanitizeServerDetail.ts (1)
.yarn/releases/yarn-1.19.0.cjs (1)
cleaned(42026-42046)
apps/cyberstorm-remix/cyberstorm/utils/errors/handleLoaderError.ts (4)
packages/thunderstore-api/src/errors/userFacingError.ts (2)
UserFacingErrorCategory(9-16)mapApiErrorToUserFacingError(92-159)apps/cyberstorm-remix/cyberstorm/utils/errors/userFacingErrorResponse.ts (4)
CreateUserFacingErrorResponseOptions(29-33)UserFacingErrorPayload(11-17)throwUserFacingPayloadResponse(55-67)throwUserFacingErrorResponse(38-50)apps/cyberstorm-remix/cyberstorm/utils/errors/loaderMappings.ts (1)
defaultErrorMappings(71-81)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)
apps/cyberstorm-remix/cyberstorm/utils/errors/resolveRouteErrorPayload.ts (3)
packages/thunderstore-api/src/errors/userFacingError.ts (2)
UserFacingError(44-62)mapApiErrorToUserFacingError(92-159)apps/cyberstorm-remix/cyberstorm/utils/errors/userFacingErrorResponse.ts (2)
UserFacingErrorPayload(11-17)parseUserFacingErrorPayload(72-91)packages/thunderstore-api/src/errors.ts (1)
ApiError(14-64)
packages/thunderstore-api/src/errors/userFacingError.ts (2)
packages/thunderstore-api/src/errors.ts (4)
ApiError(14-64)RequestBodyParseError(66-70)RequestQueryParamsParseError(72-76)ParseError(78-82)packages/thunderstore-api/src/errors/sanitizeServerDetail.ts (1)
sanitizeServerDetail(29-40)
apps/cyberstorm-remix/cyberstorm/utils/errors/userFacingErrorResponse.ts (1)
packages/thunderstore-api/src/errors/userFacingError.ts (4)
UserFacingErrorCategory(9-16)MapUserFacingErrorOptions(18-22)mapApiErrorToUserFacingError(92-159)UserFacingError(44-62)
apps/cyberstorm-remix/cyberstorm/utils/errors/NimbusErrorBoundary/NimbusErrorBoundary.tsx (2)
packages/cyberstorm/src/utils/utils.ts (1)
classnames(34-38)apps/cyberstorm-remix/cyberstorm/utils/errors/resolveRouteErrorPayload.ts (1)
resolveRouteErrorPayload(31-72)
apps/cyberstorm-remix/cyberstorm/utils/errors/loaderMappings.ts (1)
apps/cyberstorm-remix/cyberstorm/utils/errors/handleLoaderError.ts (1)
LoaderErrorMapping(17-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Generate visual diffs
🔇 Additional comments (7)
apps/cyberstorm-remix/cyberstorm/utils/errors/NimbusErrorBoundary/index.ts (1)
1-1: Barrel export is straightforward and appropriateRe-exporting from
"./NimbusErrorBoundary"keeps consumers on a single, stable path; no changes needed here.apps/cyberstorm-remix/cyberstorm/utils/errors/NimbusErrorBoundary/NimbusErrorBoundary.css (1)
1-45: Nimbus error boundary styling looks consistent and self-containedUse of
@layer nimbus-components, design tokens, and hover state is clean and should integrate well with the existing design system; no functional or maintainability concerns from this snippet.apps/cyberstorm-remix/cyberstorm/utils/errors/loaderMappings.ts (1)
3-37: Loader mappings are clear and consistentStatuses and categories for the predefined mappings look sensible and align with expected user-facing copy; factories keep server and not-found cases reusable without over-configuring options.
apps/cyberstorm-remix/cyberstorm/utils/errors/userFacingErrorResponse.ts (1)
38-67: Centralized Response throwing looks solid
throwUserFacingErrorResponseandthrowUserFacingPayloadResponsecleanly centralize mapping toUserFacingErrorplusResponseconstruction with sane defaults (status override, JSON body,no-storecaching). This should keep loader error handling consistent across call sites.packages/thunderstore-api/src/errors/userFacingError.ts (2)
92-159: VerifyApiErrorexposes the fields used here
mapApiErrorToUserFacingErrorrelies onApiErrorhavingstatusCode,statusText,serverMessage, andcontext?.sessionWasUsed. TheApiErrorsnippet inerrors.tsonly showsresponseandresponseJson, so just confirm that the additional fields/getters exist (or are added in this PR) so this code type‑checks and doesn’t fall back to"unknown"unexpectedly.
164-188: Not-found helper is well-structured
createResourceNotFoundErrornicely sanitizes labels/identifiers, constructs clear copy, and enriches context withresourceand optionalidentifier. This should make 404s much easier to handle consistently at the UI layer.apps/cyberstorm-remix/cyberstorm/utils/errors/NimbusErrorBoundary/NimbusErrorBoundary.tsx (1)
77-123: Error boundary core implementation looks goodThe class-based
NimbusErrorBoundaryfollows the standard React error boundary pattern (getDerivedStateFromError,componentDidCatch, local error state) and cleanly wires inonError,onReset, and a pluggable fallback component. Props surface and reset behavior look straightforward to use.
apps/cyberstorm-remix/cyberstorm/utils/errors/NimbusErrorBoundary/NimbusErrorBoundary.tsx
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| export const VALIDATION_MAPPING: LoaderErrorMapping = { | ||
| status: 422, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our backend is currently more likely to return 400 Bad Request than this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely yes, but that's alright. For specific cases there can always be additional/overriding mappings added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's alright. Go check the backend code base. Plenty of endpoints return 400. AFAICT none return 422. So 422 is the specific case that can be taken care when needed. 400 is the common case that shouldn't require boilerplate on UI components. Or am I misunderstanding something here?
| * Creates a reusable server-error mapping with configurable copy and status. | ||
| */ | ||
| export function createServerErrorMapping( | ||
| headline = "Something went wrong.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this and the 404 counterpart below use the text's used in figma design?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK those texts are more placeholders, let's keep this like this for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figma designs have now been updated and the alternative error messages have been cleared out. Let's use the texts there. If you want to implement the glitch effect shown on Figma at the same go, these can be done separately, but in that case create a task for them.
apps/cyberstorm-remix/cyberstorm/utils/errors/resolveRouteErrorPayload.ts
Show resolved
Hide resolved
apps/cyberstorm-remix/cyberstorm/utils/errors/handleLoaderError.ts
Outdated
Show resolved
Hide resolved
|
|
||
| Client loaders stream data to the browser but should produce the same payloads and tone as their server counterparts. Treat them exactly like server loaders: | ||
|
|
||
| 1. Validate required params (`namespaceId`, `teamName`, etc.) and throw `throwUserFacingPayloadResponse` immediately when missing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these params ever be missing in the clientLoader (or loader for that matter)? If the parameter is missing from the URL does the request even match the route definition? If not, the clientLoader is never called but RR shows 404 via some magic? (Not sure about this but I think it seemed this way when I tested things.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No but the check needs to be in place for typing. And one can add additional checks e.g. if team name includes bad words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would then perhaps prefer params.namespaceId! etc. I usually don't like using non-null assertion, but in this case it would reduce boilerplate and make the code more readable. Maybe we should make this a squad decision?
| @@ -0,0 +1,198 @@ | |||
| # Error Handling | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Roffenlund would you mind review just this file without checking any of the related code. I want to know if it given you a clear picture how you should write loaders and components without checking the implementation.
55d7eb7 to
0e9d28f
Compare
2d3f2a3 to
e63509b
Compare
…nt user-facing error messages
e63509b to
e20d99e
Compare
0e9d28f to
19be637
Compare
| ); | ||
|
|
||
| if (error instanceof ApiError && allOptions.length) { | ||
| const mapping = allOptions.findLast((candidate) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that status is just number and not an array, I think this cleans up to:
const mapping = allOptions.findLast(
(mapping) => mapping.status === error.response.status
);| 2. Call `getLoaderTools()` so you reuse the configured `DapperTs` instance *and* hydrated session tools. The helper now returns `{ dapper, sessionTools }` for client and server paths. | ||
| 3. Wrap awaited work in `try/catch` and delegate to `handleLoaderError`. For Promises you hand off to Suspense, attach `.catch(error => handleLoaderError(error, { mappings }))` so rejections surface Nimbus copy. | ||
| 4. Share error-mapping constants between server and client loaders to prevent drift. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would something like this be clearer:
2. Use `const { dapper, sessionTools } = getLoaderTools()` to reuse preconfigured `DapperTs` instance and hydrated session tools.No need to mention "server paths" in the clientLoader section of the doc. And if sessionTools is worth mentioning here, it should be used in a code sample too (one code sample or a separate one where sessionTools is used).
3. If you need to await promise to resolve in the loader, wrap the promise in `try/catch` and use `handleLoaderError` in the `catch` block to show consistent error messages.
4. If there's no need to await in the loader, return an unresolved Promise to be used by Suspense. Attach `.catch(error => handleLoaderError(error, { mappings }))` to the promise to show consistent error messages.A bit more verbose but makes the points clearer I think?
5. Share error-mapping constants between server and client loaders to show consistent error messages.I still think Daniel should review the readme with fresh eyes.
| ### Example pattern | ||
|
|
||
| ```tsx | ||
| const teamNotFoundMappings = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example seems to have nothing to do with Teams?
| }; | ||
| } | ||
|
|
||
| throwUserFacingPayloadResponse({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this reuse (fixed) teamNotFoundMapping?
| }); | ||
|
|
||
| it("trims before truncating when slice ends in space", () => { | ||
| // "a" * 399 + " " + "b" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: IMO unnecessary comments.
| const category = categorizeStatus(error.response.status); | ||
| const headline = sanitizeServerDetail(error.message) ?? fallbackHeadline; | ||
| const sanitizedServerDetail = sanitizeServerDetail(error.message ?? ""); | ||
| const sanitizedFallback = sanitizeServerDetail(fallbackDescription); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we sanitize the fallback, isn't it something we control to begin with?
| const headline = sanitizeServerDetail(error.message) ?? fallbackHeadline; | ||
| const sanitizedServerDetail = sanitizeServerDetail(error.message ?? ""); | ||
| const sanitizedFallback = sanitizeServerDetail(fallbackDescription); | ||
| const descriptionCandidate = sanitizedServerDetail || sanitizedFallback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing "candidate" about this, this will be straight up the description.
| }); | ||
| } | ||
|
|
||
| return new UserFacingError({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Déjà vu but this could probably be combined with the isNetworkError(error) branch above.
| typeof error === "object" && | ||
| error !== null && | ||
| "message" in error && | ||
| typeof (error as { message: unknown }).message === "string" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to my comment in the unit tests for this function: is the four-line check really needed? Why don't we just check error instanceof Error here?
| export * from "./errors"; | ||
| export * from "./errors/index"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to consider adding to your cleanup task list: maybe move the ApiError from errors.ts to error/errors.ts oslt?

No description provided.