refactor: replace exception-based error handling with neverthrow Result types#62
refactor: replace exception-based error handling with neverthrow Result types#62
Conversation
…lt types Domain services (HeatService, generateBracketForDivision) now return Promise<Result<T, E>> instead of throwing, making error paths explicit and type-safe. API handlers use unwrapOrThrow() at the oRPC boundary and result.isErr() checks in legacy REST routes. - Add neverthrow dependency and src/domain/result.ts re-export - Convert 6 HeatService methods and generateBracketForDivision to Result - Add HeatServiceError/BracketServiceError union types, TooManyParticipantsError - Create unwrapOrThrow() utility for oRPC handlers - Simplify domainErrorMapper middleware to infrastructure-error safety net - Fix heat-repository to return early/empty instead of throwing generic errors - Map HeatDoesNotExistError and ScoreNotFoundError to 404 (was 400) - Update all domain, integration, and middleware tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the error handling architecture from exception-based to type-safe Result types using the neverthrow library. Domain services (HeatService and generateBracketForDivision) now return Promise<Result<T, E>> instead of throwing exceptions, making error paths explicit and type-safe. API handlers use unwrapOrThrow() at the oRPC boundary and result.isErr() checks in legacy REST routes.
Changes:
- Adds neverthrow dependency and introduces Result-based error handling in domain services
- Converts 6 HeatService methods and generateBracketForDivision to return Result types
- Creates error union types (HeatServiceError, BracketServiceError) and adds TooManyParticipantsError
- Updates all API handlers (oRPC and REST) to handle Results, with unwrapOrThrow utility for oRPC
- Refactors infrastructure repositories to return early instead of throwing, and updates error mappings (HeatDoesNotExistError, ScoreNotFoundError now map to 404)
- Updates all tests (domain, integration, middleware) to work with Result types
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/domain/result.ts | Re-exports neverthrow Result types for domain layer |
| src/domain/heat/heat-service.ts | Converts 6 methods to return Result instead of throwing |
| src/domain/heat/errors.ts | Adds HeatServiceError union type, deprecates BadUserRequestError |
| src/domain/heat/index.ts | Exports new error types and HeatServiceError |
| src/domain/bracket/bracket-service.ts | Converts generateBracketForDivision to Result, adds TooManyParticipantsError and BracketServiceError union |
| src/infrastructure/repositories/heat-repository.ts | Changes error handling to early returns instead of throwing |
| src/api/orpc/unwrap-result.ts | New utility to convert Result to thrown ORPCError at API boundary |
| src/api/orpc/domain-error-mapper.ts | Simplifies middleware to infrastructure error safety net, exports DOMAIN_ERROR_MAP |
| src/api/orpc/routes/scores.ts | Updates handlers to use unwrapOrThrow for Result-based services |
| src/api/orpc/routes/heats.ts | Updates completeHeat to use unwrapOrThrow |
| src/api/orpc/routes/brackets.ts | Updates generate handler to use unwrapOrThrow |
| src/api/routes/heat-routes.ts | Adds result.isErr() checks and getDomainErrorStatusCode usage |
| src/api/routes/bracket-routes.ts | Updates to handle Result from generateBracketForDivision |
| src/api/middleware/error-handling.ts | Adds ScoreNotFoundError to 404 mappings, maps HeatDoesNotExistError to 404 |
| package.json | Adds neverthrow ^8.2.0 dependency |
| bun.lock | Locks neverthrow at 8.2.0 |
| tests/domain/heat/heat-service.test.ts | Updates all tests to check Result.isOk()/isErr() instead of expect().toThrow() |
| tests/domain/bracket/bracket-service.test.ts | Updates tests for Result-based bracket generation |
| tests/integration/bracket-generation.test.ts | Updates integration tests to work with Results |
| tests/api/middleware/error-handling.test.ts | Adds tests for HeatDoesNotExistError and ScoreNotFoundError 404 mapping |
| tests/api/heat-routes.test.ts | Updates status code expectation from 400 to 404 for missing heat |
| AGENTS.md | Documents new Result-based error handling patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("should return 404 for HeatDoesNotExistError", () => { | ||
| const error = new HeatDoesNotExistError("test-heat"); | ||
| expect(getDomainErrorStatusCode(error)).toBe(404); | ||
| }); | ||
|
|
||
| it("should return 404 for ScoreNotFoundError", () => { | ||
| const error = new ScoreNotFoundError("test-score"); | ||
| expect(getDomainErrorStatusCode(error)).toBe(404); | ||
| }); |
There was a problem hiding this comment.
Test coverage is missing for the newly added error types. While the test verifies that ScoreNotFoundError maps to 404 (lines 87-90), it should also verify that:
ScoreNotFoundErroris recognized byisHeatDomainErrorScoreTypeMismatchErroris recognized byisHeatDomainErrorand maps to 400TooManyParticipantsErroris recognized byisBracketDomainErrorand maps to 400
Once the missing error types are added to the type guards (see related comment on error-handling.ts), add corresponding test cases to ensure these errors are properly classified as domain errors.
| // Map domain errors to HTTP status codes | ||
| export function getDomainErrorStatusCode(error: Error): number { | ||
| // 404 errors | ||
| if (error instanceof DivisionNotFoundError) { | ||
| if ( | ||
| error instanceof DivisionNotFoundError || | ||
| error instanceof HeatDoesNotExistError || | ||
| error instanceof ScoreNotFoundError | ||
| ) { | ||
| return 404; | ||
| } | ||
|
|
There was a problem hiding this comment.
The ScoreNotFoundError was added to the getDomainErrorStatusCode function to map it to 404, but it's missing from the HeatDomainError type union (lines 23-32) and the isHeatDomainError type guard (lines 42-53). Similarly, ScoreTypeMismatchError is not imported or included anywhere, and TooManyParticipantsError is missing from BracketDomainError and isBracketDomainError.
While the oRPC error mapper correctly handles all these errors, the REST API error handling middleware needs these updates to properly classify these errors as domain errors. Without these additions, if these errors are thrown (e.g., from the transaction in handleCompleteHeat), they'll be treated as generic 500 errors instead of 400/404.
The following additions are needed:
- Import ScoreTypeMismatchError and TooManyParticipantsError
- Add ScoreNotFoundError and ScoreTypeMismatchError to HeatDomainError union
- Add them to isHeatDomainError type guard
- Add TooManyParticipantsError to BracketDomainError union
- Add it to isBracketDomainError type guard
src/api/routes/heat-routes.ts
Outdated
| const result = await heatService.completeHeat(heatId, new Date()); | ||
| if (result.isErr()) { | ||
| const status = getDomainErrorStatusCode(result.error); | ||
| throw Object.assign(result.error, { _statusCode: status }); | ||
| } |
There was a problem hiding this comment.
The code throws the error with a _statusCode property attached (line 470), but the withErrorHandling middleware (error-handling.ts lines 84-108) doesn't check for or use this property. This means the status code will be determined by the getDomainErrorStatusCode function based on the error type.
However, if the thrown error is not in the domain error type guards (like ScoreNotFoundError or ScoreTypeMismatchError which are currently missing - see related comment), it will be treated as a generic 500 error, ignoring the intended status code.
Consider either:
- Updating
withErrorHandlingto check for the_statusCodeproperty and use it if present, OR - Ensuring all possible errors from
completeHeatare properly included in the domain error type guards (which they currently aren't), OR - Not throwing inside the transaction and instead returning the error response after checking result.isErr() outside the transaction (though this prevents transaction rollback on domain errors)
|
|
||
| if (!heat) { | ||
| throw new Error(`Heat ${heatId} not found`); | ||
| return; // Heat existence is validated upstream by domain service |
There was a problem hiding this comment.
The comment states "Heat existence is validated upstream by domain service", but this is misleading. The domain service validates that the source heat exists (line 186-188 in heat-service.ts), not the destination heats referenced here.
When addRiderToHeat is called with metadata.winnerDestinationHeatId or metadata.loserDestinationHeatId, these are different heats that may not exist. While the bracket generation creates heats in reverse order to ensure destinations exist, this silent return could mask bugs or data corruption issues where a destination heat is missing.
Consider either:
- Adding explicit validation that destination heats exist before calling
addRiderToHeat, OR - Having the repository return an error/Result type so callers can detect and handle missing destination heats, OR
- At minimum, updating the comment to accurately reflect what is actually validated (e.g., "Destination heats are expected to exist from bracket generation; silently skip if missing")
| return; // Heat existence is validated upstream by domain service | |
| // Destination heats are expected to exist from bracket generation; silently skip if missing | |
| return; |
Covers page structure, Playwright screenshot generation, CI workflow, GitHub Pages deployment, and maintenance instructions for LLMs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… match - Add throwDomainError() utility mapping domain errors to oRPC typed errors - Add withResultTransaction() for Result-aware Drizzle transaction rollback - Update context.ts to use .errors() for typed error contracts - Convert scores.ts, heats.ts, brackets.ts to use result.match() pattern - Update heat-routes.ts handleCompleteHeat to use withResultTransaction - Simplify domain-error-mapper.ts by removing DOMAIN_ERROR_MAP - Delete unwrap-result.ts (replaced by throwDomainError) - Update AGENTS.md with new error handling patterns Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…yParticipantsError Co-authored-by: danbim <300638+danbim@users.noreply.github.com>
Add missing error type coverage in domain error type guards
Validate destination heat existence before advancing riders
Domain services (HeatService, generateBracketForDivision) now return Promise<Result<T, E>> instead of throwing, making error paths explicit and type-safe. API handlers use unwrapOrThrow() at the oRPC boundary and result.isErr() checks in legacy REST routes.