Skip to content

Commit 8658b0f

Browse files
KATO-Hiroclaude
andcommitted
docs: Extract plan teachings to rules and remove completed plan
- Compress coding-style.md (199 → 143 lines): Consolidate examples while preserving guidance. Add Layer-Specific Responsibility (service normalization vs UI filtering) and Function Composition patterns. - Compress testing.md (188 → 111 lines): Simplify patterns and sections. Add Fixture Sharing in describe() scope for test DRY principle. - Remove fix-solution-category-all-view/plan.md: Teachings migrated to rules; implementation complete (2058/2059 tests pass); no future decisions retained. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
1 parent 0d13b30 commit 8658b0f

3 files changed

Lines changed: 53 additions & 389 deletions

File tree

.claude/rules/coding-style.md

Lines changed: 25 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,7 @@ Before writing new logic, decide which layer it belongs to. Run this check at pl
2727
- **Lambda parameters**: no single-character names (e.g., use `placement`, `workbook`). Iterator index `i` is the only exception.
2828
- **`upsert`**: only use when the implementation performs both insert and update. For insert-only, use `initialize`, `seed`, or another accurate verb.
2929
- **Function verbs**: every function name must start with a verb. Noun-only names (`pointOnCircle`, `arcPath`) are ambiguous — use `calcPointOnCircle`, `buildArcPath`, etc. Common prefixes: `get` (read existing), `build`/`create` (construct new), `calc`/`compute` (derive by formula), `update`, `fetch`, `resolve`.
30-
- **`any`**: before using `any`, check the value's origin — adding a missing `@types/*` or `devDependency` often provides the correct type. When `any` seems unavoidable, use the narrowest alternative:
31-
32-
| Situation | Alternative |
33-
| ---------------------------------------------------------- | ------------------------------------------------------------------------ |
34-
| Assign to a property not on the type | `obj as T & { prop: U }` (intersection cast) |
35-
| Return type too complex to write manually | `ReturnType<typeof fn>` |
36-
| Partial mock: only specific properties matter | `Partial<T>`, `Pick<T, 'a' \| 'b'>`, or `satisfies` — prefer these first |
37-
| Partial mock: none of the above narrow the type far enough | `as unknown as T` — last resort; bypasses type checking entirely |
38-
| Inline `: any` annotation where inference reaches | Delete the annotation |
30+
- **`any`**: Before using `any`, check the value's origin. If unavoidable: use `ReturnType<typeof fn>` for complex returns, `Partial<T>` for partial objects, `obj as T & { prop: U }` for property extension. Last resort: `as unknown as T`.
3931

4032
- **UI labels**: if a label does not match actual behavior, update it or add an inline comment explaining the intentional mismatch.
4133
- **Constant names**: reflect what the value IS (content), not what it is used for (purpose). e.g., a set holding all enum tab values is `EXISTING_TABS`, not `VALID_TABS`.
@@ -50,25 +42,11 @@ Before writing new logic, decide which layer it belongs to. Run this check at pl
5042
- **Braces**: always use braces for single-statement `if` blocks. Never `if () return;` — write `if () { return; }`.
5143
- **Domain types over `string`**: when the Prisma schema uses an enum (e.g. `grade: TaskGrade`), the corresponding app-layer type must use the same enum — not `string`. A loose `string` type hides misspellings in fixtures and forces `as TaskGrade` casts throughout the codebase. When a field comes from an external source (form data, query params), validate and narrow it at the boundary; inside the app it must always be the domain type.
5244
- **Plural type aliases**: define `type Placements = Placement[]` instead of using `Placement[]` directly in signatures and variables.
53-
- **Empty `catch` blocks**: never use `catch { }` or `catch (_e)` to silence errors. Every `catch` must re-throw, log, or contain an explanatory comment justifying the suppression. Silent swallowing hides bugs and makes failures untraceable.
45+
- **Empty `catch` blocks**: never use `catch { }` to silence errors. Either re-throw, log, or add a comment justifying suppression. Silent swallowing hides bugs.
5446

5547
```typescript
56-
// Bad: silently discards the error
57-
try { ... } catch { }
58-
try { ... } catch (_e) { }
59-
60-
// Good: log and re-throw (adds context before propagating)
61-
try { ... } catch (error) {
62-
console.error('Operation failed:', error);
63-
throw error;
64-
}
65-
66-
// Good: intentional suppression with explanation
67-
try {
68-
localStorage.setItem(key, value);
69-
} catch {
70-
// localStorage may be unavailable (private browsing) — fall back to in-memory store
71-
}
48+
try { ... } catch (error) { console.error('...'); throw error; } // good
49+
try { localStorage.setItem(key, value); } catch { /* unavailable */ } // good + comment
7250
```
7351

7452
### No Hard-Coded Values
@@ -120,25 +98,28 @@ When a filter has an "all" or "unfiltered" state, omit the parameter entirely ra
12098

12199
### Type Guards: Precise Narrowing for Excluded Values
122100

123-
When a type guard intentionally excludes enum members, use `Exclude<T, 'VALUE'>` in the return type to match runtime behavior.
101+
When a type guard intentionally excludes enum members, use `Exclude<T, 'VALUE'>` in the return type to match runtime behavior. Caller code then trusts the type system; no `as` casts needed.
124102

125-
**Bad:** Type doesn't reflect runtime exclusion
103+
### Layer-Specific Responsibility: Normalization vs Filtering
126104

127-
```typescript
128-
function isSelectable(value: string | null): value is Category {
129-
return value !== null && ... && value !== PENDING; // Excludes PENDING at runtime
130-
}
131-
```
105+
When filtering data by multiple criteria (format + role):
106+
107+
- **Service layer**: Normalize raw data format (e.g., `null → PENDING`). Return all data; stay framework-agnostic and testable.
108+
- **UI layer**: Filter by role/context (e.g., "admin sees PENDING, user doesn't"). Apply display rules in component.
132109

133-
**Good:** Type matches runtime behavior
110+
**Benefit**: Services remain pure and unit-testable without mocking roles; UI logic explicit in one place.
111+
112+
### Function Composition: Single Responsibility
113+
114+
Separate independent transformations into distinct functions. Compose at call site.
134115

135116
```typescript
136-
function isSelectable(value: string | null): value is Exclude<Category, 'PENDING'> {
137-
return value !== null && ... && value !== PENDING;
138-
}
117+
// groupBySolutionCategory(): pure grouping (testable)
118+
// filterGroupsByRole(): pure filtering by role (testable)
119+
let filtered = $derived(filterGroupsByRole(groupBySolutionCategory(workbooks, map), role));
139120
```
140121

141-
**Benefit:** Caller code trusts the type system; no `as` casts needed.
122+
**Benefit**: Each function testable in isolation; reusable in different contexts.
142123

143124
## Documentation
144125

@@ -150,50 +131,13 @@ Write all project documentation (plans, dev-notes, guides, refactor notes) in Ja
150131

151132
### TSDoc
152133

153-
Add TSDoc comments to every exported function, type, and class. The minimum required fields are `@param` (for non-obvious parameters) and `@returns` (when the return value is not evident from the type). One-liner `/** ... */` is sufficient for simple cases; use multi-line only when behavior needs explanation.
154-
155-
For optional parameters with a default, state it explicitly in `@param`: `Defaults to false.`
156-
157-
```typescript
158-
/** Returns the URL slug for a workbook, falling back to the workbook ID. */
159-
export function getUrlSlugFrom(workbook: WorkbookList): string { ... }
160-
```
161-
162-
### Markdown Code Blocks
163-
164-
Always specify a language identifier on every fenced code block. Never write bare ` ``` `.
165-
166-
Common identifiers: `typescript`, `svelte`, `sql`, `bash`, `mermaid`, `json`, `prisma`, `html`, `css`.
167-
168-
### Svelte 5: Prefer Official Docs Over Training Knowledge
169-
170-
When Svelte 5 behavior is unclear, fetch official docs via WebFetch — do not rely on training knowledge. URL pattern: `https://svelte.dev/docs/svelte/{section}` (e.g. `/$effect`, `/stores`, `/what-are-runes`).
171-
172-
## Security
173-
174-
### Server-side Logging
175-
176-
Do not log user-identifiable or content data (titles, names, IDs that map to users) in server-side `console.log`. Use generic messages instead:
177-
178-
```typescript
179-
// Bad: leaks content and user identity
180-
console.log(`Created workbook "${workBook.title}" by user ${author.id}`);
181-
182-
// Good
183-
console.log('Workbook created successfully');
184-
```
185-
186-
Prefer placing the single authoritative log in the service layer; remove duplicate logs in route handlers that cover the same event.
187-
188-
## Code Review
189-
190-
### CodeRabbit Review: Severity Triage
134+
Add TSDoc to every exported function, type, class. Minimum: `@param` (non-obvious), `@returns` (not evident from type). One-liner OK; multi-line for complex behavior only.
191135

192-
Run `coderabbit review --plain` once after all phases are complete (not on every commit).
136+
### Documentation
193137

194-
**Triage by severity:**
138+
Write plans/dev-notes/guides in Japanese. Source code comments in English. Always specify language on code blocks (e.g., `typescript`, `sql`, `bash`). For Svelte 5 unclear behavior: fetch official docs via WebFetch, not training knowledge.
195139

196-
- **critical / high / potential_issue (medium)**: Write all findings verbatim to a `## CodeRabbit Findings` section in `plan.md`. The user decides which to fix before opening the PR. Do not fix any of these findings unilaterally.
197-
- **nitpick / info**: Defer to PR CI — CodeRabbit will re-comment on the open PR.
140+
## Security & Code Review
198141

199-
Writing medium-and-above findings to `plan.md` serves a dual purpose: it gives the user full visibility for a fix/defer decision, and it builds the implementer's understanding of recurring quality issues.
142+
- **Logging**: No user-identifiable data in `console.log`. Single authoritative log in service layer.
143+
- **CodeRabbit**: Run after all phases complete. Write `critical`/`high`/`medium` findings to `plan.md` verbatim; defer `nitpick` to PR CI.

.claude/rules/testing.md

Lines changed: 28 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -26,22 +26,7 @@ If a task description does not mention tests, add them anyway for any non-trivia
2626

2727
## Unused Imports in Test Files
2828

29-
An unused import in a test file is a signal that a test was planned but not yet written — not dead code to remove.
30-
31-
Before deleting such an import, check whether the corresponding test case is missing and add it:
32-
33-
```typescript
34-
// Bad: remove the import because it's unused
35-
import { ABCLikeProvider } from './contest_table_provider';
36-
37-
// Good: the import is unused because the test is missing — add the test
38-
test('expects to create ABCLike preset correctly', () => {
39-
const group = prepareContestProviderPresets().ABCLike();
40-
expect(group.getProvider(ContestType.ABC_LIKE)).toBeInstanceOf(ABCLikeProvider);
41-
});
42-
```
43-
44-
Removing the import silences the linter but leaves a coverage gap. Adding the test both satisfies the linter and improves coverage.
29+
Unused imports signal a missing test, not dead code. Before deleting, add the corresponding test case.
4530

4631
## Test Types
4732

@@ -67,7 +52,24 @@ Test stub parameter types must match the production function's signature — use
6752
- Use realistic fixture values (real task IDs, grade names) instead of placeholders like `'t1'`
6853
- Extract shared data into fixture files; inline is fine for single-use cases
6954
- After `.filter()` on fixtures, verify actual contents — same ID may refer to a different entity after fixture updates
70-
- **Description ↔ code path alignment**: when a test name describes a specific scenario (e.g. "tie-break"), verify the fixture actually exercises that code path. A test that passes without reaching the branch it claims to cover gives false confidence
55+
- **Description ↔ code path alignment**: when a test name describes a specific scenario (e.g. "tie-break"), verify the fixture actually exercises that code path
56+
57+
### Fixture Sharing in describe() Scope
58+
59+
When multiple tests in a `describe` block use identical fixture data, define it once at block scope instead of duplicating in each test. Benefit: DRY + fixture changes sync automatically. Only use if fixture is immutable within tests (no mutations).
60+
61+
```typescript
62+
describe('filterByRole', () => {
63+
const testGroups = [{ role: ADMIN }, { role: PENDING }];
64+
65+
test('admin sees all', () => {
66+
expect(filter(testGroups, ADMIN)).toHaveLength(2);
67+
});
68+
test('user sees filtered', () => {
69+
expect(filter(testGroups, USER)).toHaveLength(1);
70+
});
71+
});
72+
```
7173

7274
## Coverage
7375

@@ -80,108 +82,29 @@ Order `describe` blocks in service and utils test files to match the declaration
8082

8183
### Describe Block Organization: Multi-Scenario Functions
8284

83-
When a function behaves differently based on input type or mode, split `describe` blocks by scenario rather than mixing all cases flat.
84-
85-
**Bad:** All cases mixed
86-
87-
```typescript
88-
describe('buildWorkbooksUrl', () => {
89-
test('curriculum tab with grade produces correct URL', () => { ... });
90-
test('solution tab with category produces correct URL', () => { ... });
91-
test('curriculum tab without grade produces URL with tab only', () => { ... });
92-
test('created_by_user tab produces URL with tab only', () => { ... });
93-
});
94-
```
95-
96-
**Good:** Split by scenario
97-
98-
```typescript
99-
describe('buildWorkbooksUrl with curriculum tab', () => {
100-
test('produces URL with tab and grade when grade is provided', () => { ... });
101-
test('produces URL with tab only when grade is not provided', () => { ... });
102-
});
103-
104-
describe('buildWorkbooksUrl with solution tab', () => {
105-
test('produces URL with tab and category when category is provided', () => { ... });
106-
test('produces URL with tab only when category is null', () => { ... });
107-
});
108-
109-
describe('buildWorkbooksUrl with created_by_user tab', () => {
110-
test('produces URL with tab only', () => { ... });
111-
});
112-
```
113-
114-
**Benefit:** Test discovery improves, names less redundant, structure mirrors implementation.
85+
Split `describe` blocks by scenario (not all cases flat) when a function behaves differently by mode. Example: separate `describe('func with modeA')` and `describe('func with modeB')` rather than mixing all cases. Benefit: better test discovery and names.
11586

11687
## Service Layer Unit Tests
11788

11889
Service tests mock Prisma via `vi.mock('$lib/server/database', ...)` — no real DB mutations occur.
11990

12091
### Mock Helpers
12192

122-
Extract repeated mock patterns into helpers in the test file. Define the return type alias once and use it across all helpers:
123-
124-
```typescript
125-
type PrismaWorkBook = Awaited<ReturnType<typeof prisma.workBook.findUnique>>;
126-
type PrismaWorkBookRow = Awaited<ReturnType<typeof prisma.workBook.findMany>>[number];
127-
128-
function mockFindUnique(value: PrismaWorkBook) {
129-
vi.mocked(prisma.workBook.findUnique).mockResolvedValue(value);
130-
}
131-
132-
function mockFindMany(value: PrismaWorkBookRow[]) {
133-
vi.mocked(prisma.workBook.findMany).mockResolvedValue(
134-
value as unknown as Awaited<ReturnType<typeof prisma.workBook.findMany>>,
135-
);
136-
}
137-
138-
function mockCount(value: number) {
139-
vi.mocked(prisma.workBook.count).mockResolvedValue(value);
140-
}
141-
```
142-
143-
Extract `mockFindUnique`, `mockFindMany`, and `mockCount` as the standard trio for service tests that touch a single Prisma model. Add `mockCreate`, `mockTransaction`, and `mockDelete` when those operations are also tested.
144-
145-
### Cleanup for Integration Tests and Tests with Real Side Effects
93+
Extract repeated mock patterns into helpers. Use `mockFindUnique`, `mockFindMany`, `mockCount` as the standard trio for Prisma model tests. Add `mockCreate`, `mockTransaction`, `mockDelete` when needed.
14694

147-
This does not apply to standard service layer unit tests that use Prisma mocks.
95+
### Cleanup for Integration Tests
14896

149-
If a test performs real DB mutations, file system changes, external API calls, or other stateful side effects that persist beyond the test (e.g., integration tests, seed scripts), wrap assertions in `try/finally` — a failing assertion skips cleanup and contaminates later tests:
150-
151-
```typescript
152-
try {
153-
await doSomething();
154-
expect(result).toBe(expected);
155-
} finally {
156-
await restoreState();
157-
}
158-
```
97+
For tests with real side effects (DB mutations, file changes, API calls), wrap assertions in `try/finally` — a failing assertion skips cleanup and contaminates later tests.
15998

16099
### File Split for Testability
161100

162-
When a service file mixes DB operations and pure functions, split it into two files:
163-
164-
- `crud.ts` — DB operations (`getXxx`, `updateXxx`, `createXxx`); tests need Prisma mocks
165-
- `initializers.ts` — pure computation (grade grouping, priority assignment); tests need no mocks
166-
167-
Stop the split if internal helpers (e.g. `fetchUnplacedWorkbooks`) would be fragmented across files — cohesion matters more than the split itself.
168-
169-
## Component Vitest Unit Tests
170-
171-
E2E tests are complementary to, not a substitute for, unit tests. Add Vitest unit tests for any component logic (derived values, event handlers, utility calls) by extracting it to the nearest `utils/` file and testing there.
172-
173-
You may omit a component-level Vitest test when **both** conditions hold:
174-
175-
1. The component is template-only (no logic beyond prop bindings and simple `{#if}`/`{#each}` blocks that only render — no inline function calls, ternaries with side effects, derived computations, or nested logic)
176-
2. The component's rendering paths are covered by E2E tests
177-
178-
When a component contains extracted logic (e.g. derived values, event handlers, utility calls), add unit tests for that logic in the nearest `utils/` file instead of testing the component directly.
101+
When a service mixes DB operations and pure functions, split into `crud.ts` (DB; Prisma mocks needed) and `initializers.ts` (pure; no mocks). Skip split if cohesion suffers.
179102

180-
## Testing Extracted Utilities
103+
## Component & Utility Tests
181104

182-
- Add tests at extraction time, not later
183-
- For URL manipulation: assert the original URL is not mutated
184-
- For multi-column operations (e.g., DnD): assert both source and destination columns
105+
- Extract component logic to `utils/` and test there, not in the component.
106+
- Omit component Vitest test if template-only **and** E2E tests cover rendering paths.
107+
- Add utility tests at extraction time; assert immutability (URLs) and all affected parts (multi-column operations).
185108

186109
## HTTP Mocking
187110

0 commit comments

Comments
 (0)