diff --git a/.claude/rules/coding-style.md b/.claude/rules/coding-style.md index 516d6b631..33ac38cae 100644 --- a/.claude/rules/coding-style.md +++ b/.claude/rules/coding-style.md @@ -27,15 +27,7 @@ Before writing new logic, decide which layer it belongs to. Run this check at pl - **Lambda parameters**: no single-character names (e.g., use `placement`, `workbook`). Iterator index `i` is the only exception. - **`upsert`**: only use when the implementation performs both insert and update. For insert-only, use `initialize`, `seed`, or another accurate verb. - **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`. -- **`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: - - | Situation | Alternative | - | ---------------------------------------------------------- | ------------------------------------------------------------------------ | - | Assign to a property not on the type | `obj as T & { prop: U }` (intersection cast) | - | Return type too complex to write manually | `ReturnType` | - | Partial mock: only specific properties matter | `Partial`, `Pick`, or `satisfies` — prefer these first | - | Partial mock: none of the above narrow the type far enough | `as unknown as T` — last resort; bypasses type checking entirely | - | Inline `: any` annotation where inference reaches | Delete the annotation | +- **`any`**: Before using `any`, check the value's origin. If unavoidable: use `ReturnType` for complex returns, `Partial` for partial objects, `obj as T & { prop: U }` for property extension. Last resort: `as unknown as T`. - **UI labels**: if a label does not match actual behavior, update it or add an inline comment explaining the intentional mismatch. - **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 - **Braces**: always use braces for single-statement `if` blocks. Never `if () return;` — write `if () { return; }`. - **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. - **Plural type aliases**: define `type Placements = Placement[]` instead of using `Placement[]` directly in signatures and variables. -- **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. +- **Empty `catch` blocks**: never use `catch { }` to silence errors. Either re-throw, log, or add a comment justifying suppression. Silent swallowing hides bugs. ```typescript -// Bad: silently discards the error -try { ... } catch { } -try { ... } catch (_e) { } - -// Good: log and re-throw (adds context before propagating) -try { ... } catch (error) { - console.error('Operation failed:', error); - throw error; -} - -// Good: intentional suppression with explanation -try { - localStorage.setItem(key, value); -} catch { - // localStorage may be unavailable (private browsing) — fall back to in-memory store -} +try { ... } catch (error) { console.error('...'); throw error; } // good +try { localStorage.setItem(key, value); } catch { /* unavailable */ } // good + comment ``` ### No Hard-Coded Values @@ -101,60 +79,65 @@ Within a file, order declarations as follows: Place a private helper immediately after the single export that uses it. Place helpers shared by two or more exports at the end of the file. -## Documentation +### URL Parameter Patterns -### Language Policy - -Write all project documentation (plans, dev-notes, guides, refactor notes) in Japanese. Write all source code comments, TSDoc, commit messages, and test titles in English. This keeps documentation readable for the team while keeping code comments universally accessible and searchable. +#### null-as-ALL: Omitting Params for "All" State -**Exception**: The `## CodeRabbit Findings` section in `plan.md` must quote findings verbatim in their original language (English). Do not translate CodeRabbit output. +When a filter has an "all" or "unfiltered" state, omit the parameter entirely rather than using a magic value (e.g., "ALL", "\*"). -### TSDoc +**Pattern:** -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. +- Parse function defaults to `null` when param is absent +- `null` → "show all" (no filter applied) +- URL: `/workbooks?tab=solution` (no `categories=`) +- Browser back button naturally restores default "all" view -For optional parameters with a default, state it explicitly in `@param`: `Defaults to false.` +**Benefit:** Cleaner URLs, intuitive history behavior, smaller sessionStorage footprint. -```typescript -/** Returns the URL slug for a workbook, falling back to the workbook ID. */ -export function getUrlSlugFrom(workbook: WorkbookList): string { ... } -``` +**Example:** `parseWorkBookCategory()` defaults to `null` = all categories -### Markdown Code Blocks +### Type Guards: Precise Narrowing for Excluded Values -Always specify a language identifier on every fenced code block. Never write bare ` ``` `. +When a type guard intentionally excludes enum members, use `Exclude` in the return type to match runtime behavior. Caller code then trusts the type system; no `as` casts needed. -Common identifiers: `typescript`, `svelte`, `sql`, `bash`, `mermaid`, `json`, `prisma`, `html`, `css`. +### Layer-Specific Responsibility: Normalization vs Filtering -### Svelte 5: Prefer Official Docs Over Training Knowledge +When filtering data by multiple criteria (format + role): -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`). +- **Service layer**: Normalize raw data format (e.g., `null → PENDING`). Return all data; stay framework-agnostic and testable. +- **UI layer**: Filter by role/context (e.g., "admin sees PENDING, user doesn't"). Apply display rules in component. -## Security +**Benefit**: Services remain pure and unit-testable without mocking roles; UI logic explicit in one place. -### Server-side Logging +### Function Composition: Single Responsibility -Do not log user-identifiable or content data (titles, names, IDs that map to users) in server-side `console.log`. Use generic messages instead: +Separate independent transformations into distinct functions. Compose at call site. ```typescript -// Bad: leaks content and user identity -console.log(`Created workbook "${workBook.title}" by user ${author.id}`); - -// Good -console.log('Workbook created successfully'); +// groupBySolutionCategory(): pure grouping (testable) +// filterGroupsByRole(): pure filtering by role (testable) +let filtered = $derived(filterGroupsByRole(groupBySolutionCategory(workbooks, map), role)); ``` -Prefer placing the single authoritative log in the service layer; remove duplicate logs in route handlers that cover the same event. +**Benefit**: Each function testable in isolation; reusable in different contexts. + +## Documentation + +### Language Policy + +Write all project documentation (plans, dev-notes, guides, refactor notes) in Japanese. Write all source code comments, TSDoc, commit messages, and test titles in English. This keeps documentation readable for the team while keeping code comments universally accessible and searchable. -## Code Review +**Exception**: The `## CodeRabbit Findings` section in `plan.md` must quote findings verbatim in their original language (English). Do not translate CodeRabbit output. + +### TSDoc -### CodeRabbit Review: Severity Triage +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. -Run `coderabbit review --plain` once after all phases are complete (not on every commit). +### Documentation -**Triage by severity:** +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. -- **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. -- **nitpick / info**: Defer to PR CI — CodeRabbit will re-comment on the open PR. +## Security & Code Review -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. +- **Logging**: No user-identifiable data in `console.log`. Single authoritative log in service layer. +- **CodeRabbit**: Run after all phases complete. Write `critical`/`high`/`medium` findings to `plan.md` verbatim; defer `nitpick` to PR CI. diff --git a/.claude/rules/testing-e2e.md b/.claude/rules/testing-e2e.md index babeef13b..5347acea8 100644 --- a/.claude/rules/testing-e2e.md +++ b/.claude/rules/testing-e2e.md @@ -74,6 +74,26 @@ for (const grade of GRADES) { After an interaction that changes element state (active tab, toggle, selection), assert the _new_ state — not just that the element is visible, which may have been true before the interaction. Assert an active CSS class, `aria-selected`, or similar attribute instead of `toBeVisible()`. +### Prefer Semantic Attributes Over CSS Classes + +Assert element state via accessibility attributes (`aria-pressed`, `aria-selected`, `data-*`), not CSS classes (which are implementation details and break on style refactors). + +**Bad:** Brittle to styling changes + +```typescript +await expect(button).toHaveClass(/text-primary-700/); +``` + +**Good:** Resilient to refactors + +```typescript +await expect(button).toHaveAttribute('aria-pressed', 'true'); +// or +await expect(button).toHaveAttribute('data-active', 'true'); +``` + +**Note:** If component library doesn't expose the attribute, add it (or contribute PR). Teaching tests brittle selectors is not sustainable. + ## Flowbite Toggle Flowbite's `Toggle` renders an `sr-only` `` inside a `