refactor: centralize error mapping and add shared error notice#120
refactor: centralize error mapping and add shared error notice#120juanmahidalgo wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: refactor: centralize error mapping and add shared error notice
Clean, well-motivated refactoring. The shared errors.ts module and ErrorNotice component are well-designed — the golden rule of never surfacing raw backend text is consistently followed, and the role="alert" on the banner is a nice a11y touch. The intentional local overrides in Cart and MarketCheckout (for context-specific plural/market copy) are well-documented and make sense.
Findings
All minor — nothing blocking merge.
P2 — MyAssets.tsx:93-97 still duplicates isRejection logic inline
This file was touched by the PR (error renders → ErrorNotice), but the catch block at line 93-97 still manually checks err.code === 4001 || msg.includes('reject') || msg.includes('denied') instead of calling the new isRejection(e). It also references the old getCredits.errorCanceled key instead of errors.rejected — both have identical copy today, but they'll drift when a translator updates only one.
Suggestion: Replace with isRejection(e) + t('errors.rejected') to complete the migration in this file.
P2 — Cart.tsx and MarketCheckout.tsx use ad-hoc casts instead of the shared ErrLike type
Both local friendlyError functions cast with (e as { message?: string }) rather than importing the ErrLike type from ~/lib/errors. Not a bug, but inconsistent with the centralization goal — if ErrLike grows a new field later, these won't pick it up.
P2 — ErrorNotice default export is unused (see inline comment)
P2 — Defensive guard against null/undefined throws
The as ErrLike cast in isRejection / isInsufficient / friendlyError would crash if the thrown value is literally null or undefined (accessing .code on null). In practice wallet SDKs and fetch always throw Error objects, and the old code had the same pattern, so this isn't a regression — but a one-line guard (typeof e === 'object' && e !== null ? (e as ErrLike) : {}) would make it bulletproof.
Security
No issues. friendlyError never surfaces raw error text — every path returns a t() key or a caller-supplied t() fallback. No dangerouslySetInnerHTML, no ReDoS risk, no hardcoded secrets, no XSS vectors.
CI
Build + test + e2e still in progress at time of review. Title validation passed ✅
Verdict
Approved — solid refactoring with good documentation. The P2 items are all follow-up material; none blocks merge.
Reviewed by Jarvis 🤖 · Requested by juanmahidalgo via GitHub
| ) | ||
| } | ||
|
|
||
| export default ErrorNotice |
There was a problem hiding this comment.
[P2] All 11 consumers use the named import { ErrorNotice } — this default export is unused. Consider removing it to keep a single canonical import path.
| return t('getCredits.errorCanceled') | ||
| } | ||
| if (isRejection(e)) return t('errors.rejected') | ||
| const msg = ((e as { message?: string }).message ?? '').toLowerCase() |
There was a problem hiding this comment.
[P2] Nit: this inline cast (e as { message?: string }) duplicates the ErrLike type from ~/lib/errors (which is already imported). Using ErrLike would keep the typing consistent and pick up any future field additions.
Same applies to MarketCheckout.tsx:25.
(Would need to export the type: export type { ErrLike } from errors.ts)
2 participants
Stacked on #117 (i18n) — base branch is
fix/shop-i18n, so this diff is error-handling only. Retarget tomainonce #117 merges.Changes
Replaces the scattered, duplicated error handling with a small shared foundation.
~/lib/errors.ts— one place to map a thrown error to a safe, localized string.friendlyError(e, fallback, { sale })handles the universal wallet/abort rejection, and (for purchase flows) funds/availability failures; anything unrecognized returns the caller's own context-specificfallback. It never returns raw backend text. Also exports the previously-duplicatedisRejection/isInsufficienthelpers.~/components/ErrorNotice.tsx(+ styles) — a consistent inline error banner (alert icon + tinted surface +role="alert") replacing the bare red<p className="error">text that sat unstyled across the app. Renders nothing when there's no message.friendlyErrorcopies (Cart, BuyModal, MarketCheckout, SellModal, PrimaryListModal) onto the shared helpers, and swapped ~15 raw error<p>render sites across pages + modals to<ErrorNotice>.buyModal.error.soldOrRemoved/cantBuyOwnkeys (folded into a sharederrors.*namespace, en + es).Cart and MarketCheckout keep a thin local mapping (plural "listing changed" copy / the "refreshing the market" sold-out copy respectively) on top of the shared helpers, since those messages are context-specific.
Notes
friendlyError+ error render will adopt the shared foundation in a follow-up once feat: redesign get credits page with always-on packs and skeleton loaders #116 and feat: localize the shop with t() across all pages and components #117 land.Test plan
tsc -bcleanvite buildsucceedsvitest run— 651 tests pass