Skip to content

Two parallel, divergent error-handling libraries (error-handler.ts vs errorHandler.ts) coexist, risking inconsistent error UX #87

Description

@prodbycorne

Problem

The codebase contains two separately-maintained error-handling modules with confusingly similar names:

  • src/lib/error-handler.ts (474 lines) — a class-hierarchy design (SmartDropError abstract base with FreighterError, RPCError, ContractError, ValidationError, ConfigError, UnknownError subclasses), each carrying code, userMessage, isTransient, isCritical. This is the module documented in ERROR_HANDLING.md and consumed by ErrorContext.tsx's setupGlobalErrorHandlers, StellarWalletContext.tsx (throws FreighterError instances), and soroban.ts (SecurityError).
  • src/lib/errorHandler.ts (382 lines) — a completely separate, enum-based design (ErrorType enum, a plain AppError interface, an isAppError type guard) with no shared types, base classes, or utilities in common with the first file.

Both files independently implement overlapping concerns (categorizing Freighter/RPC/contract errors into user-friendly messages, transient/retry classification) using incompatible data shapes — a component that does error instanceof FreighterError (the class-based module's pattern) will never match an error constructed via the enum-based module's AppError shape, and vice versa. This means whichever components/hooks were written against errorHandler.ts (the enum module) cannot interoperate with the useErrorHandler()/useToast() machinery documented in ERROR_HANDLING.md and used by ConnectWalletButton, UnlockModal, etc. If any component still imports from errorHandler.ts, its errors won't be recognized as SmartDropError instances by the rest of the app's error-handling pipeline, silently falling through to generic/UnknownError handling instead of the specific, user-friendly messaging the codebase clearly intends.

Acceptance Criteria

  • Audit every import of @/lib/errorHandler vs @/lib/error-handler across the codebase and determine which (if either) is genuinely dead code versus still wired into a live component.
  • Migrate any remaining consumers of the enum-based errorHandler.ts onto the class-based error-handler.ts design that ERROR_HANDLING.md documents as the system of record, preserving behavior.
  • Delete the now-unused module once consolidation is complete, and update ERROR_HANDLING.md if anything about the merged behavior changes.
  • Add a lint rule or CI check (e.g. a simple grep-based check, or an ESLint no-restricted-imports rule) preventing a re-introduction of a second, parallel error-handling module.

Relevant Files

  • src/lib/error-handler.ts — the class-hierarchy error system documented in ERROR_HANDLING.md and used by StellarWalletContext/ErrorContext
  • src/lib/errorHandler.ts — a separate, incompatible enum-based error system with no shared types with the file above
  • ERROR_HANDLING.md — documents only the class-based system, silent on the existence of the second module

Additional Notes

Re-audited every import site in src/ directly (grep -rn "error-handler\|errorHandler" src --include=*.ts --include=*.tsx). This produced a more precise and more actionable finding than the original body states:

src/lib/errorHandler.ts (the enum-based module) has zero importers anywhere in src/. Every live consumer — src/context/ErrorContext.tsx (setupGlobalErrorHandlers), src/context/StellarWalletContext.tsx (FreighterError), src/components/ErrorBoundary/ErrorBoundary.tsx (UnknownError, errorLogger), src/hooks/useToast.ts, src/hooks/useLockFlow.ts (normalizeError), and src/lib/soroban.ts (SecurityError) — imports exclusively from @/lib/error-handler (the class-hierarchy module, kebab-case filename). There is no file anywhere importing from @/lib/errorHandler (camelCase, no importers found even via a path-agnostic grep for the bare specifier), and no test file references it either (find . -iname "*errorHandler*" only turns up the source file itself, no errorHandler.test.ts).

This changes the shape of the fix: this is not "two parallel libraries at risk of inconsistent UX because some component might be on the wrong one" — it's one dead file with zero consumers, and one live file that everything already uses. The risk described in the original body (a component throwing an AppError shape that fails an instanceof SmartDropError check) does not currently manifest anywhere in the codebase as it stands today; the risk is prospective (a future contributor might import the wrong one by name-similarity, or might resurrect a stale PR branch that used the old module) rather than a live bug. This is good news for scope: the fix is very likely a straight deletion, not a migration.

Edge cases to cover

  • Before deleting, confirm errorHandler.ts isn't dynamically imported (import(...) string-based) anywhere, which a static grep would miss — worth a quick grep -rn "errorHandler" --include=*.json sweep of config files too (e.g. a webpack/next alias) in case something points at it indirectly.
  • Check for barrel re-exports: if any index.ts re-exports * from './errorHandler' without a direct consumer importing the symbols, a naive "grep for direct importers" pass (as done above) could still miss it if another module re-exports and that re-export is unused too — worth one more grep pass for from ['"]\.\/errorHandler['"] and from ['"]\./lib/errorHandler['"] relative-path forms specifically, since the audit above only checked the @/lib/... alias form.
  • Since ERROR_HANDLING.md documents only error-handler.ts already, no doc update should be needed beyond perhaps a one-line note in the file's own history/CHANGELOG (if one exists) recording the removal, so a future contributor doesn't wonder why a similarly-named module briefly existed.

Implementation sketch

  1. Re-run the import audit one more time immediately before deletion (things may have changed) with both alias and relative-path grep forms.
  2. Delete src/lib/errorHandler.ts.
  3. Add an ESLint no-restricted-imports rule (or a simple CI grep step) blocking any future @/lib/errorHandler (camelCase) import, so a re-introduction under the confusingly similar name gets caught before merge.
  4. No consumer migration needed given the zero-importer finding — if a future audit finds a consumer this pass missed, treat that as new information requiring the originally-scoped "migrate then delete" path instead.

Test plan

  • CI: add a lint rule / grep check test asserting no file imports @/lib/errorHandler (or any relative path resolving to the deleted file); this doubles as regression protection against reintroduction.
  • Manual: run the full existing test suite (npm test / vitest run) after deletion to confirm nothing was silently relying on the file via a path this audit didn't catch (e.g. a dynamic import or a test-only import).
  • No new business-logic tests are needed since this is dead-code removal, not a behavior change.

Cross-references

Metadata

Metadata

Assignees

No one assigned

    Labels

    GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial CampaignCampaign: Official CampaignOfficial Campaign | FWC26Campaign: Official Campaign | FWC26bugSomething isn't workinguxUser experience, interaction design, loading statesvery hardExtremely hard — deep expertise, careful design, and significant time required

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions