PoC: AppError - Type-Safe Error Handling by CatchMe2 · Pull Request #243 · lokalise/node-core

CatchMe2 · 2025-12-17T09:22:35Z

Changes

Introduces AppError, a new error handling system that addresses key limitations in the current InternalError/PublicNonRecoverableError approach.

Questions for Review

How about moving it to a dedicated @lokalise/error package? Error can be created in the browser, or we might use defineError in some shared contract package that is then pulled in FE.
Are there any additional error types we should include in ErrorType?

Checklist

Apply one of following labels; major, minor, patch or skip-release
I've updated the documentation, or no changes were necessary
I've updated the tests, or no changes were necessary

Summary by CodeRabbit

New Features
- Introduced a type-safe, protocol-agnostic error framework with structured error definitions, HTTP status mapping, and Zod schema validation for error details.
Documentation
- Added comprehensive error handling documentation with migration guides, usage examples, and best practices.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-17T09:22:45Z

Note

`.coderabbit.yaml` has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)

Validation error: Unrecognized key(s) in object: 'tools'

⚙️ Configuration instructions

Please see the configuration documentation for more information.
You can also validate your configuration using the online YAML validator.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Walkthrough

Introduces a protocol-agnostic, type-safe error framework with literal error codes, structured definitions, HTTP status mapping, and Zod schema integration for type-safe error details. Includes a generic AppError class with factory pattern and comprehensive documentation.

Changes

Cohort / File(s)	Summary
Error Framework Implementation `src/errors/AppError.ts`	New error system with ErrorType categorization, ErrorDefinition interface, generic AppError class with static factory method, HTTP status mapping, and Zod-based type-safe details inference.
Framework Documentation `src/errors/README.md`	Comprehensive guide covering error design patterns, protocol mapping (HTTP/gRPC), migration from legacy error patterns, and usage examples with Zod schemas.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Type inference and generic constraints: Verify correct behavior of InferDetails type inference and generic AppError class variance
HTTP status mapping: Confirm ErrorType-to-status mapping is complete and accurate for all error categories
Zod schema integration: Validate type-safe details handling with optional vs. required schema branches
Documentation accuracy: Ensure README examples and patterns align with actual implementation behavior and design intent

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: introducing a type-safe error handling system called AppError, which matches the primary focus of the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	The pull request description covers the main changes, includes a clear checklist with all items completed, and raises thoughtful review questions about architectural decisions.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch add-app-error-poc

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/errors/AppError.ts (1)
152-158: Consider setting a meaningful class name for debuggability.

The anonymous class returned by from will have the name of the definition's code in stack traces only if consumers extend it with a named class. For cases where AppError.from(def) is used directly without extending, the error name in stack traces will be empty or generic.

This is minor since the documented pattern encourages creating named subclasses, but you could optionally set the class name:
   static from<const T extends ErrorDefinition>(definition: T) {
-    return class extends AppError<T> {
+    const ErrorClass = class extends AppError<T> {
       constructor(options: AppErrorOptions<InferDetails<T>>) {
         super(definition, options)
       }
     }
+    Object.defineProperty(ErrorClass, 'name', { value: definition.code })
+    return ErrorClass
   }

📜 Review details

Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 088a0ca and bfedd8a.

📒 Files selected for processing (2)

src/errors/AppError.ts (1 hunks)
src/errors/README.md (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/errors/AppError.ts (1)

src/errors/EnhancedError.ts (1)

EnhancedError (60-88)

🪛 GitHub Actions: ci

src/errors/AppError.ts

[error] 2-171: Code coverage for this file is 0% (lines 2-171 are uncovered).

🔇 Additional comments (11)

src/errors/AppError.ts (7)

1-4: LGTM!

The import of Zod as a type-only import is appropriate since it's only used for type inference. The ValueOf utility type is a clean pattern for extracting union types from const objects.

6-34: Well-designed protocol-agnostic error categorization.

The error types cover common scenarios and the as const assertion ensures literal type inference. The pattern of exporting both the const object and a derived union type is idiomatic TypeScript.

44-53: LGTM!

The HTTP status mapping is correct, and using Record<ErrorType, number> ensures compile-time exhaustiveness checking if new error types are added.

64-73: LGTM!

The interface is well-structured. Using z.ZodTypeAny for the optional details schema provides flexibility while enabling type inference through defineError.

84-84: LGTM!

The const type parameter ensures that string literals like 'PROJECT_NOT_FOUND' remain as literal types rather than widening to string. This is the key enabler for type discrimination.

94-96: LGTM!

The conditional type correctly infers details from the Zod schema when present, falling back to undefined otherwise.

106-111: LGTM!

The conditional type elegantly handles both cases: making details optional when no schema is defined, and required when a schema exists. This provides excellent developer experience.

src/errors/README.md (4)

1-52: Clear and compelling motivation.

The problem statement effectively demonstrates the type safety gap in the old approach, and the listed benefits accurately reflect the implementation.

152-180: LGTM!

The protocol mapping examples clearly demonstrate the flexibility of the error type system. The gRPC mapping example provides a good template for users to implement their own protocol mappings.

185-227: LGTM!

The examples clearly demonstrate the conditional requirement pattern for error details. The distinction between errors with and without schemas is well illustrated.

229-312: Comprehensive migration guide and best practices.

The before/after comparisons make migration straightforward, and the best practices section provides actionable guidance for using the new error system effectively.

src/errors/AppError.ts

src/errors/README.md

drdaemos · 2025-12-17T09:40:48Z

src/errors/AppError.ts

+  /** Protocol-agnostic error type */
+  readonly type: T['type']
+  /** Whether error details are safe to expose externally */
+  readonly isPublic: T['isPublic']


won't that be boolean in 100% of times?

Yeah, but currently it will be typed as literal true/false. It could be useful to filter out non-public errors in some error union.

drdaemos · 2025-12-17T09:44:18Z

src/errors/AppError.ts

+   * @param definition - Error definition with const assertion for literal types
+   * @returns Error class with definition bound
+   */
+  static from<const T extends ErrorDefinition>(definition: T) {


How .from() would be different from a simple *Error extends AppError and you just fill everything internally, while type: ErrorType should theoretically allow to distinguish different errors?

Using .from() covers having literal types.
Defining error type/code with abstract&override is prone to missing readonly modifier, making error non type-safe. We would have to create some biome custom rule to look for such cases which seems a bit more complex.

drdaemos · 2025-12-17T09:47:01Z

src/errors/README.md

+  detailsSchema: z.object({ name: z.string() }),
+})
+
+export class ProjectNotFoundError extends AppError.from(projectNotFoundDef) {}


I don't have strong objective concerns, but this reads "weird".

Probably a more natural way for me to describe an error would be:

export class ProjectNotFoundError extends AppError { override readonly type = ErrorType.NOT_FOUND ... } export class AppError { abstract readonly type: ErrorType }

It was my initial implementation, but unfortunately it doesn't allow us to use the error definition for OpenAPI response typing and is prone to unintentionally missing readonly, which makes the error non-typesafe.

With defineError we can use the error definition for HTTP response typing and the error class creation for BE purposes.

CarlosGamero · 2025-12-22T11:23:30Z

src/errors/AppError.ts

+ *
+ * @template T - Error definition with literal code type
+ */
+export class AppError<T extends ErrorDefinition> extends EnhancedError {


Could we have some generic default here? I mean, to avoid having to create a type for each error 😓

I'm afraid that it would lead to overusing the generic solution, decreasing the type-safety.

We can continue using ErrorDefinition as it is; the only difference is whether we need to add it manually or not. I don’t have a strong preference. If it becomes annoying, we can always add it later.

CarlosGamero · 2025-12-22T11:30:24Z

It would be beneficial to maintain backward compatibility with InternalError and PublicNonRecoverableError. Migrating entirely to the new error type could require significant effort and trigger a large chain of updates, so a backwards-compatible approach would be preferable.

I think we can achieve this as follows:

InternalError → extend AppError, setting isPublic to false, type to internal, while preserving the existing properties.
PublicNonRecoverableError → follow a similar approach, but map httpStatusCode to the corresponding error type in the constructor.

This would allow us to deprecate the old error classes while keeping them fully functional. At the same time, we can prepare the error-handling mechanism to support only the new error type, without requiring an immediate migration.

kibertoad · 2025-12-22T22:11:25Z

src/errors/AppError.ts

+ */
+export interface ErrorDefinition {
+  /** Unique error code - becomes a literal type for type discrimination */
+  code: string


please give an example of what a code might be

PROJECT_NAME_DUPLICATED, PROJECT_NOT_FOUND

kibertoad · 2025-12-22T22:11:58Z

src/errors/AppError.ts

+  /** Whether this error is safe to expose to external consumers */
+  isPublic: boolean
+  /** Optional Zod schema for type inference and OpenAPI schema generation */
+  detailsSchema?: z.ZodTypeAny


what is the behaviour if schema is not provided? no openapi schema generation or some default schema?

If no schema is provided, the error details will be strictly typed as undefined, meaning no openapi schema. If we need some sort of freeform details, we can use z.json().

chore: add AppError PoC

bfedd8a

CatchMe2 added the skip-release label Dec 17, 2025

CatchMe2 requested review from a team, CarlosGamero, drdaemos and kibertoad as code owners December 17, 2025 09:22

coderabbitai bot reviewed Dec 17, 2025

View reviewed changes

src/errors/AppError.ts Show resolved Hide resolved

src/errors/README.md Show resolved Hide resolved

drdaemos reviewed Dec 17, 2025

View reviewed changes

CarlosGamero reviewed Dec 22, 2025

View reviewed changes

kibertoad reviewed Dec 22, 2025

View reviewed changes

Conversation

CatchMe2 commented Dec 17, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

.coderabbit.yaml has unrecognized properties

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarlosGamero commented Dec 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CatchMe2 commented Dec 17, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 17, 2025 •

edited

Loading

`.coderabbit.yaml` has unrecognized properties