Skip to content

Commit 1f1ab4b

Browse files
KATO-Hiroclaude
andcommitted
fix: implement CodeRabbit findings (Phase 2)
- Fix prisma-db.md: clarify schema.prisma as source of truth for CHECK constraints - Fix spelling: "Parametrized" → "Parameterized" in testing-e2e.md and testing.md - Update median.ts to use MIN_VOTES_FOR_STATISTICS constant instead of hardcoded 3 - Fix globalThis.location state leak in vote_grade.test.ts by saving/restoring - Simplify getBaseUrl() by removing unreachable 'http://localhost' fallback - Move task_grade.test.ts from src/test/utils/ to src/lib/utils/ (adjacent pattern) - Add src/lib/**/*.test.ts to vite.config.ts test.include - Add 3 new testing patterns to rules/testing.md (multi-location tests, globalThis mocking, guard clause reachability) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
1 parent 4f16134 commit 1f1ab4b

8 files changed

Lines changed: 75 additions & 17 deletions

File tree

.claude/rules/prisma-db.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ user User @relation(fields: [userId], references: [id])
9898

9999
## DB-Level Value Constraints
100100

101-
Add `CHECK` constraints (via manual migration SQL) for count and invalid enum values. Document with inline `schema.prisma` comments (e.g., `/// CHECK: count >= 0`) `prisma-erd-generator` overwrites `ERD.md` on each migration.
101+
Add `CHECK` constraints via manual migration SQL. Document with inline `schema.prisma` comments (e.g., `/// CHECK: count >= 0`). Note: `prisma-erd-generator` may overwrite `ERD.md` on each migration—always keep the constraint definition in `schema.prisma` as the source of truth.
102102

103103
## Service Layer Error Handling
104104

@@ -125,9 +125,10 @@ Prisma does not support `@@check`. To add one:
125125

126126
1. `pnpm exec prisma migrate dev --create-only --name <description>` — generate migration without applying
127127
2. Edit the generated `migration.sql` to add the CHECK constraint manually
128-
3. `pnpm exec prisma migrate dev` — apply
128+
3. Add an inline comment in `schema.prisma` (e.g., `/// CHECK: constraint description`)
129+
4. `pnpm exec prisma migrate dev` — apply
129130

130-
Document the constraint in `prisma/ERD.md` (the only place it's visible):
131+
For visibility, also document complex constraints in `prisma/ERD.md`:
131132

132133
```mermaid
133134
%% XOR constraint: workbookplacement_xor_grade_category — exactly one of taskGrade or solutionCategory must be non-null

.claude/rules/testing-e2e.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ test.describe('logged-in user', () => {
3434
});
3535
```
3636

37-
## Parametrized Tests
37+
## Parameterized Tests
3838

3939
Playwright has no native `test.each`. Use `for...of` loops (official pattern):
4040

.claude/rules/testing.md

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ const mockGetUser = (statusCode, user?) => {
7575
};
7676
```
7777

78-
### Parametrized Tests
78+
### Parameterized Tests
7979

8080
Test enum boundaries + typical value, then separate test for distinct behavior:
8181

@@ -115,3 +115,50 @@ Target: 80% lines, 80% branches. Run `pnpm coverage`.
115115
## Test Files Ship with Code
116116

117117
Never defer tests. For non-trivial logic without explicit test requirement, add them anyway.
118+
119+
## Multiple Test Location Patterns
120+
121+
During migration, support both centralized (`src/test/`) and co-located (`src/features/`, `src/lib/`) tests.
122+
Configure `vite.config.ts` with explicit ordering:
123+
124+
```typescript
125+
include: [
126+
'src/lib/**/*.test.ts', // shared utilities (adjacent)
127+
'src/test/**/*.test.ts', // legacy centralized
128+
'src/features/**/*.test.ts', // feature co-location
129+
],
130+
```
131+
132+
## Mocking globalThis Properties
133+
134+
Save and restore `globalThis` state to prevent test leaks:
135+
136+
```typescript
137+
const original = globalThis.location;
138+
139+
beforeEach(() => {
140+
Object.defineProperty(globalThis, 'location', {
141+
value: { origin: 'http://test' },
142+
writable: true,
143+
});
144+
});
145+
146+
afterEach(() => {
147+
if (original !== undefined) {
148+
Object.defineProperty(globalThis, 'location', { value: original, writable: true });
149+
}
150+
});
151+
```
152+
153+
## Guard Clause Reachability
154+
155+
Ensure guard clauses don't make later code unreachable. Example anti-pattern:
156+
157+
```typescript
158+
// Bad: 'http://localhost' is unreachable
159+
if (location?.origin) return location.origin;
160+
if (!browser) return '';
161+
return 'http://localhost'; // Never reached in browser
162+
```
163+
164+
Simplify to remove dead code after the final guard.

src/features/votes/internal_clients/vote_grade.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import * as statusCodes from '$lib/constants/http-response-status-codes';
1010
const BASE_URL = 'http://localhost';
1111
const TASK_ID = 'abc_a';
1212

13+
const originalLocation = globalThis.location;
14+
1315
beforeEach(() => {
1416
nock.cleanAll();
1517
// Set globalThis.location for test environment
@@ -21,6 +23,13 @@ beforeEach(() => {
2123

2224
afterEach(() => {
2325
nock.cleanAll();
26+
// Restore original location to prevent test state leak
27+
if (originalLocation !== undefined) {
28+
Object.defineProperty(globalThis, 'location', {
29+
value: originalLocation,
30+
writable: true,
31+
});
32+
}
2433
});
2534

2635
describe('fetchMyVote', () => {

src/features/votes/internal_clients/vote_grade.ts

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import type { TaskGrade } from '$lib/types/task';
22
import { isValidTaskGrade } from '$lib/utils/task_grade';
3-
import { browser } from '$app/environment';
43

54
/**
65
* Fetches the current user's vote grade for a given task.
@@ -88,14 +87,10 @@ function getBaseUrl(): string {
8887
return globalThis.location.origin;
8988
}
9089

91-
if (!browser) {
92-
console.warn(
93-
'getBaseUrl() called in SSR context where browser APIs are unavailable. ' +
94-
'This should only be called from client-side code.',
95-
);
96-
97-
return '';
98-
}
90+
console.warn(
91+
'getBaseUrl() called in SSR context where browser APIs are unavailable. ' +
92+
'This should only be called from client-side code.',
93+
);
9994

100-
return 'http://localhost';
95+
return '';
10196
}

src/features/votes/utils/median.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { TaskGrade } from '@prisma/client';
22
import { getGradeOrder, taskGradeOrderInfinity } from '$lib/utils/task';
3+
import { MIN_VOTES_FOR_STATISTICS } from '$features/votes/constants/statistics';
34

45
/** Maps grade order (1=Q11 … 17=D6) back to the corresponding TaskGrade enum value. */
56
const ORDER_TO_TASK_GRADE: Map<number, TaskGrade> = new Map([
@@ -30,10 +31,14 @@ type GradeCounter = { grade: TaskGrade; count: number };
3031
* Returns `null` when the total vote count is below the minimum threshold.
3132
*
3233
* @param counters - Grade counters sorted by grade ascending.
33-
* @param minVotes - Minimum votes required to compute a median. Defaults to 3.
34+
* @param minVotes - Minimum votes required to compute a median. Defaults to MIN_VOTES_FOR_STATISTICS.
35+
*
3436
* @returns The median TaskGrade, or `null` if there are fewer than `minVotes` total votes.
3537
*/
36-
export function computeMedianGrade(counters: GradeCounter[], minVotes = 3): TaskGrade | null {
38+
export function computeMedianGrade(
39+
counters: GradeCounter[],
40+
minVotes = MIN_VOTES_FOR_STATISTICS,
41+
): TaskGrade | null {
3742
const total = counters.reduce((sum, counter) => sum + counter.count, 0);
3843
if (total < minVotes) {
3944
return null;

vite.config.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ export default defineConfig({
99
},
1010
test: {
1111
include: [
12+
'src/lib/**/*.test.ts', // shared utility tests
1213
'src/test/**/*.test.ts', // existing tests (phase transition)
1314
'src/features/**/*.test.ts', // feature co-location tests
1415
],

0 commit comments

Comments
 (0)