Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .husky/pre-commit
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
#!/usr/bin/env sh

# Add bun to PATH
export PATH="$HOME/.bun/bin:$PATH"

# Auto-fix format and lint issues, then re-stage
bunx biome check --write . || exit 1
git add -u
Expand Down
18 changes: 13 additions & 5 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,17 @@ bun run db:seed # Seed test data
- **Private helpers**: camelCase with descriptive names

### Error Handling
- **Custom error classes** extending `Error` (e.g., `HeatDoesNotExistError`)
- **Type guards** for domain errors: `isDomainError(error)`
- **Error middleware** wrapper: `withErrorHandling(async () => { ... })`
- **Domain errors** map to HTTP status codes (400/404/500)
- **Always log errors** with context
- **`neverthrow` Result types**: Domain services return `Promise<Result<T, E>>` instead of throwing
- **Custom error classes** extending `Error` (e.g., `HeatDoesNotExistError`) — used as the `E` in `Result<T, E>`
- **oRPC `.errors()` for typed error contracts**: Define `NOT_FOUND`, `BAD_REQUEST`, `UNAUTHORIZED`, `FORBIDDEN` on procedures
- **`result.match(onOk, onErr)`**: Functional Result handling — use `.match()` instead of imperative `if (result.isErr())`
- **`throwDomainError(error, errors)`**: Maps domain errors to oRPC typed errors; replaces `DOMAIN_ERROR_MAP` + `unwrapOrThrow`
- **`withResultTransaction(db, fn)`**: Result-aware Drizzle transaction wrapper — rollbacks on `err()`, returns the Result
- **`domainErrorMapper` middleware**: Safety net for unexpected infrastructure errors → 500
- **`getDomainErrorStatusCode(error)`**: Maps domain errors to HTTP status codes for legacy REST routes
- **Error union types**: `HeatServiceError`, `BracketServiceError` for type-safe error handling
- **Pattern**: Domain services return `err(...)`, handlers use `result.match()` with `throwDomainError` for the error branch
- **API-level errors**: Use `throw errors.NOT_FOUND()` / `throw errors.FORBIDDEN()` for auth/existence checks (API concerns)

### Domain-Driven Design Patterns
- **Repository pattern**: Interfaces in `domain/`, implementations in `infrastructure/`
Expand Down Expand Up @@ -281,3 +287,5 @@ __tests__/
❌ **Don't** import from `infrastructure/` in domain code → Inject dependencies instead
❌ **Don't** start transactions in domain services → API handlers own transaction lifecycle
❌ **Don't** call `getDb()` in repositories → Accept connection via constructor
❌ **Don't** use `new ORPCError(...)` directly → Use typed `errors.CODE()` from oRPC procedures
❌ **Don't** use imperative `if (result.isErr())` → Use `result.match()` for cleaner handling
4 changes: 2 additions & 2 deletions __tests__/api/heat-routes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ describe("Heat API Routes", () => {
expect(data.error).toContain("Validation error");
});

it("should return 400 if heat does not exist", async () => {
it("should return 404 if heat does not exist", async () => {
const heatId = getUniqueHeatId("nonexistent");
const request = createWaveScoreRequest(heatId, {
scoreUUID: "wave-1",
Expand All @@ -388,7 +388,7 @@ describe("Heat API Routes", () => {
});

const response = await handleAddWaveScore(request);
expect(response.status).toBe(400);
expect(response.status).toBe(404);

const data = (await response.json()) as { error: string };
expect(data.error).toContain("does not exist");
Expand Down
21 changes: 20 additions & 1 deletion __tests__/api/middleware/error-handling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
BracketAlreadyExistsError,
DivisionNotFoundError,
InsufficientParticipantsError,
TooManyParticipantsError,
} from "../../../src/domain/bracket/bracket-service.js";
import {
HeatAlreadyExistsError,
Expand All @@ -19,6 +20,8 @@ import {
RiderAlreadyInHeatError,
RiderNotInHeatError,
ScoreMustBeInValidRangeError,
ScoreNotFoundError,
ScoreTypeMismatchError,
ScoreUUIDAlreadyExistsError,
} from "../../../src/domain/heat/errors.js";

Expand All @@ -33,6 +36,8 @@ describe("error-handling middleware", () => {
expect(isHeatDomainError(new ScoreUUIDAlreadyExistsError("test"))).toBe(true);
expect(isHeatDomainError(new InvalidHeatRulesError())).toBe(true);
expect(isHeatDomainError(new RiderAlreadyInHeatError("test", "test"))).toBe(true);
expect(isHeatDomainError(new ScoreNotFoundError("test"))).toBe(true);
expect(isHeatDomainError(new ScoreTypeMismatchError("test", "wave", "jump"))).toBe(true);
});

it("should return false for non-heat errors", () => {
Expand All @@ -49,6 +54,7 @@ describe("error-handling middleware", () => {
expect(isBracketDomainError(new BracketAlreadyExistsError("test"))).toBe(true);
expect(isBracketDomainError(new DivisionNotFoundError("test"))).toBe(true);
expect(isBracketDomainError(new InsufficientParticipantsError(1))).toBe(true);
expect(isBracketDomainError(new TooManyParticipantsError(65))).toBe(true);
});

it("should return false for non-bracket errors", () => {
Expand Down Expand Up @@ -78,20 +84,33 @@ describe("error-handling middleware", () => {
expect(getDomainErrorStatusCode(error)).toBe(404);
});

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);
});
Comment on lines +87 to +95
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. ScoreNotFoundError is recognized by isHeatDomainError
  2. ScoreTypeMismatchError is recognized by isHeatDomainError and maps to 400
  3. TooManyParticipantsError is recognized by isBracketDomainError and 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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot re-review after my recent push


it("should return 400 for heat domain errors", () => {
expect(getDomainErrorStatusCode(new HeatAlreadyExistsError("test"))).toBe(400);
expect(getDomainErrorStatusCode(new HeatDoesNotExistError("test"))).toBe(400);
expect(getDomainErrorStatusCode(new NonUniqueRiderIdsError())).toBe(400);
expect(getDomainErrorStatusCode(new RiderNotInHeatError("test", "test"))).toBe(400);
expect(getDomainErrorStatusCode(new ScoreMustBeInValidRangeError(15))).toBe(400);
expect(getDomainErrorStatusCode(new ScoreUUIDAlreadyExistsError("test"))).toBe(400);
expect(getDomainErrorStatusCode(new InvalidHeatRulesError())).toBe(400);
expect(getDomainErrorStatusCode(new RiderAlreadyInHeatError("test", "test"))).toBe(400);
expect(getDomainErrorStatusCode(new ScoreTypeMismatchError("test", "wave", "jump"))).toBe(
400
);
});

it("should return 400 for bracket domain errors (except DivisionNotFoundError)", () => {
expect(getDomainErrorStatusCode(new BracketAlreadyExistsError("test"))).toBe(400);
expect(getDomainErrorStatusCode(new InsufficientParticipantsError(1))).toBe(400);
expect(getDomainErrorStatusCode(new TooManyParticipantsError(65))).toBe(400);
});
});

Expand Down
106 changes: 55 additions & 51 deletions __tests__/domain/bracket/bracket-service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,35 @@ import {
DivisionNotFoundError,
generateBracketForDivision,
InsufficientParticipantsError,
TooManyParticipantsError,
} from "../../../src/domain/bracket/bracket-service.js";
import type { Bracket, Division } from "../../../src/domain/contest/types.js";

describe("generateBracketForDivision", () => {
// Note: Tests that create real heats in the event store are skipped
// and covered by integration tests instead

it("should throw DivisionNotFoundError if division does not exist", async () => {
it("should return err(DivisionNotFoundError) if division does not exist", async () => {
const mockDivisionRepo = {
getDivisionById: mock(() => Promise.resolve(null)),
};
const mockBracketRepo = {};
const mockParticipantRepo = {};
const mockHeatRepo = {};

await expect(
generateBracketForDivision("non-existent-division", {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
})
).rejects.toThrow(DivisionNotFoundError);
const result = await generateBracketForDivision("non-existent-division", {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
});

expect(result.isErr()).toBe(true);
expect(result._unsafeUnwrapErr()).toBeInstanceOf(DivisionNotFoundError);
expect(mockDivisionRepo.getDivisionById).toHaveBeenCalledWith("non-existent-division");
});

it("should throw InsufficientParticipantsError if division has less than 2 participants", async () => {
it("should return err(InsufficientParticipantsError) if division has less than 2 participants", async () => {
const mockDivision: Division = {
id: "division-1",
contestId: "contest-1",
Expand All @@ -52,19 +53,19 @@ describe("generateBracketForDivision", () => {
};
const mockHeatRepo = {};

await expect(
generateBracketForDivision("division-1", {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
})
).rejects.toThrow(InsufficientParticipantsError);
const result = await generateBracketForDivision("division-1", {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
});

expect(result.isErr()).toBe(true);
expect(result._unsafeUnwrapErr()).toBeInstanceOf(InsufficientParticipantsError);
expect(mockParticipantRepo.getRiderIdsByDivisionId).toHaveBeenCalledWith("division-1");
});

it("should throw InsufficientParticipantsError with correct message for 1 participant", async () => {
it("should return err(InsufficientParticipantsError) with correct message for 1 participant", async () => {
const mockDivision: Division = {
id: "division-1",
contestId: "contest-1",
Expand All @@ -85,21 +86,20 @@ describe("generateBracketForDivision", () => {
};
const mockHeatRepo = {};

try {
await generateBracketForDivision("division-1", {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
});
expect.unreachable("Should have thrown error");
} catch (error) {
expect(error).toBeInstanceOf(InsufficientParticipantsError);
expect((error as Error).message).toBe("Division has 1 participants, need at least 2");
}
const result = await generateBracketForDivision("division-1", {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
});

expect(result.isErr()).toBe(true);
const error = result._unsafeUnwrapErr();
expect(error).toBeInstanceOf(InsufficientParticipantsError);
expect(error.message).toBe("Division has 1 participants, need at least 2");
});

it("should throw BracketAlreadyExistsError if bracket already exists for division", async () => {
it("should return err(BracketAlreadyExistsError) if bracket already exists for division", async () => {
const mockDivision: Division = {
id: "division-1",
contestId: "contest-1",
Expand Down Expand Up @@ -128,19 +128,19 @@ describe("generateBracketForDivision", () => {
const mockParticipantRepo = {};
const mockHeatRepo = {};

await expect(
generateBracketForDivision("division-1", {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
})
).rejects.toThrow(BracketAlreadyExistsError);
const result = await generateBracketForDivision("division-1", {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
});

expect(result.isErr()).toBe(true);
expect(result._unsafeUnwrapErr()).toBeInstanceOf(BracketAlreadyExistsError);
expect(mockBracketRepo.getBracketByDivisionId).toHaveBeenCalledWith("division-1");
});

it("should throw error if division has more than 64 participants", async () => {
it("should return err(TooManyParticipantsError) if division has more than 64 participants", async () => {
const mockDivision: Division = {
id: "division-1",
contestId: "contest-1",
Expand All @@ -163,14 +163,17 @@ describe("generateBracketForDivision", () => {
};
const mockHeatRepo = {};

await expect(
generateBracketForDivision("division-1", {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
})
).rejects.toThrow("Division has 65 participants, maximum is 64");
const result = await generateBracketForDivision("division-1", {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
});

expect(result.isErr()).toBe(true);
const error = result._unsafeUnwrapErr();
expect(error).toBeInstanceOf(TooManyParticipantsError);
expect(error.message).toBe("Division has 65 participants, maximum is 64");
});

// These tests require event store and are covered by integration tests
Expand Down Expand Up @@ -211,14 +214,15 @@ describe("generateBracketForDivision", () => {
completeHeat: mock(() => Promise.resolve()),
};

const bracketId = await generateBracketForDivision(`division-${testId}`, {
const result = await generateBracketForDivision(`division-${testId}`, {
divisionRepository: mockDivisionRepo as any,
bracketRepository: mockBracketRepo as any,
divisionParticipantRepository: mockParticipantRepo as any,
heatRepository: mockHeatRepo as any,
});

expect(bracketId).toBe(`bracket-${testId}`);
expect(result.isOk()).toBe(true);
expect(result._unsafeUnwrap()).toBe(`bracket-${testId}`);
expect(mockBracketRepo.createBracket).toHaveBeenCalledWith({
divisionId: `division-${testId}`,
name: "Single Elimination",
Expand Down
Loading