Merge pull request #3281 from AtCoder-NoviSteps/#3280

refactor: Extract workbook list and add tests (#3280)

Author: KATO-Hiro

.claude/rules/coding-style.md

@@ -8,6 +8,15 @@
 - **`any`**: before using `any`, check the value's origin — adding a missing `@types/*` or `devDependency` often provides the correct type.
 - **UI labels**: if a label does not match actual behavior, update it or add an inline comment explaining the intentional mismatch.
 
+## TSDoc
+
+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 behaviour needs explanation.
+
+```typescript
+/** Returns the URL slug for a workbook, falling back to the workbook ID. */
+export function getUrlSlugFrom(workbook: WorkbookList): string { ... }
+```
+
 ## Syntax
 
 - **Braces**: always use braces for single-statement `if` blocks. Never `if () return;` — write `if () { return; }`.
@@ -43,6 +52,20 @@ the server state is correct but the client-side update is wrong.
 **Diagnostic**: "Not reflected live, but fixed after reload" → suspect the optimistic
 update payload, not the reactivity system.
 
+## Server-side Logging
+
+Do not log user-identifiable or content data (titles, names, IDs that map to users) in server-side `console.log`. Use generic messages instead:
+
+```typescript
+// Bad: leaks content and user identity
+console.log(`Created workbook "${workBook.title}" by user ${author.id}`);
+
+// Good
+console.log('Workbook created successfully');
+```
+
+Prefer placing the single authoritative log in the service layer; remove duplicate logs in route handlers that cover the same event.
+
 ## Async Rollback: Capture State Before `await`
 
 Capture `$state` values before the first `await` for safe rollback. A concurrent update can overwrite the variable while awaiting:

.claude/rules/prisma-db.md

@@ -33,6 +33,26 @@ paths:
 
 Use `prisma.$transaction()` for multi-step operations.
 
+## Parallel Queries
+
+When `+page.server.ts` `load` makes multiple independent queries, run them concurrently with `Promise.all` to reduce page load latency:
+
+```typescript
+// NG: sequential — each awaits the previous
+const workbooks = await getWorkBooksWithAuthors();
+const tasks = await getTasksByTaskId();
+const results = await getTaskResultsOnlyResultExists(userId, true);
+
+// OK: all three fire at once
+const [workbooks, tasks, results] = await Promise.all([
+  getWorkBooksWithAuthors(),
+  getTasksByTaskId(),
+  getTaskResultsOnlyResultExists(userId, true),
+]);
+```
+
+Only applies when queries are **independent** (no output of one used as input to another).
+
 ## N+1 Queries
 
 Replace per-item DB calls in loops with a bulk fetch + `Map`:

.claude/rules/svelte-components.md

@@ -39,6 +39,30 @@ Referencing `$props()` inside `$state()` initializer triggers "This reference on
 let count = $state(untrack(() => initialCount)); // intentional: prop is initial seed only
 ```
 
+## `$effect` — Store Reading
+
+Inside `$effect`, use `$store` syntax, not `get(store)`. `get()` bypasses the signal graph — the effect will not re-run when the store updates:
+
+```svelte
+// Bad: get() takes a snapshot; effect won't react to store changes
+$effect(() => {
+  const grade = get(myStore).get(key) ?? fallback;
+});
+
+// Good: $store subscribes and re-runs the effect on updates
+$effect(() => {
+  const grade = $myStore.get(key) ?? fallback;
+});
+```
+
+## `$derived` — No Arrow Wrapper
+
+Use `$derived(expr)`, not `$derived(() => expr)`. The arrow form makes the derived value a _function_, not a reactive value — dependencies may not be tracked and the template call site is confusing.
+
+## `{@const}` Placement
+
+`{@const}` must be an **immediate child** of a block statement (`{#if}`, `{#each}`, `{:else}`, `{#snippet}`, etc.). Placing it inside an HTML element is a compile error:
+
 ## `{#snippet}` Placement
 
 Define snippets at the **top level**, outside component tags. Inside a tag = named slot = type error:
@@ -57,6 +81,8 @@ Define snippets at the **top level**, outside component tags. Inside a tag = nam
 Prefer `{#snippet}` when: (1) needs direct `$state` access, (2) pure display only, (3) same-file DRY.
 Promote to component when: independent state/lifecycle needed, exceeds ~30 lines, or reused across files.
 
+**Sibling consistency** — when one sibling block warrants snippet extraction, extract its parallel siblings too, even if they are short. A parent template that mixes inline markup with `{@render}` calls is harder to scan than one where every top-level section is a named snippet.
+
 ## Component Boundaries
 
 - One component, one responsibility: don't mix display, state management, and data fetching
@@ -79,7 +105,17 @@ export function buildUpdatedUrl(url: URL, activeTab: ActiveTab): URL { ... }
 replaceState(buildUpdatedUrl($page.url, activeTab), {});
 ```
 
-## Empty-list Fallback in `{#each}`
+## `{#each}` — Keys and Empty-list Fallback
+
+Always provide a key expression when the list or its items may change dynamically. This is especially critical when the block contains an inner `{#if}` — without a key, Svelte reuses DOM nodes by position, so filtering can silently bind data to the wrong element:
+
+```svelte
+{#each workbooks as workbook (workbook.id)}
+  {#if canRead(workbook)}
+    <Row {workbook} />
+  {/if}
+{/each}
+```
 
 Use `{:else}` to render a placeholder when the list is empty — no wrapper conditional needed:
 
@@ -91,6 +127,10 @@ Use `{:else}` to render a placeholder when the list is empty — no wrapper cond
 {/each}
 ```
 
+## Directory Structure: `list/` Subdirectories
+
+Consider introducing a `list/` subdirectory (or other domain-scoped subdirectory) when the component count in a directory starts to feel unwieldy — roughly 20 files is a reasonable prompt to reconsider. Below that threshold, flat organization is preferred — subdirectories add navigation cost without proportional benefit.
+
 ## Eliminate Branching with Records
 
 Replace `if`/ternary chains with `Record<EnumType, T>`:
@@ -103,3 +143,17 @@ const TAB_CONFIGS: Record<ActiveTab, TabConfig> = {
 ```
 
 Use the enum type as the key type, not `string`.
+
+When not all enum keys need an entry, use `Partial<Record<K, V>>` as a **type annotation** — not `satisfies`. `as const satisfies Partial<Record<K, V>>` preserves the narrowed literal type, so indexing with other enum values causes a type error:
+
+```typescript
+// NG: satisfies narrows the type — obj[key] errors for keys not in the literal
+const map = { [WorkBookType.SOLUTION]: SolutionTable }
+  as const satisfies Partial<Record<WorkBookType, Component<Props>>>;
+
+// OK: type annotation makes map[key] return Component<Props> | undefined
+const map: Partial<Record<WorkBookType, Component<Props>>> = {
+  [WorkBookType.SOLUTION]: SolutionTable,
+};
+// Safe to guard with: {#if map[type]} or if (map[type])
+```

.claude/rules/svelte-docs.md

@@ -0,0 +1,18 @@
+---
+description: Svelte official documentation reference
+paths:
+  - 'src/**/*.svelte'
+  - 'src/**/*.ts'
+---
+
+# Svelte Official Documentation
+
+When Svelte 5 behavior is unclear, fetch the official docs directly via WebFetch instead of relying on training knowledge.
+
+URL pattern: `https://svelte.dev/docs/svelte/{section}`
+
+Examples:
+
+- `$effect` behavior → `https://svelte.dev/docs/svelte/$effect`
+- Stores usage → `https://svelte.dev/docs/svelte/stores`
+- Runes overview → `https://svelte.dev/docs/svelte/what-are-runes`

.claude/rules/testing.md

@@ -9,6 +9,10 @@ paths:
 
 # Testing
 
+## Test Titles
+
+Write all test titles in English. Use descriptive sentences that state the expected behavior (e.g., `'returns empty array when workbooks is empty'`). Japanese is only acceptable in inline comments or fixture strings that represent real user-facing content.
+
 ## Test Integrity
 
 - Never delete, comment out, or weaken assertions (e.g. `toEqual` → `toBeDefined`) to make tests pass
@@ -26,6 +30,7 @@ paths:
 - Use `toBe(true)` / `toBe(false)` over `toBeTruthy()` / `toBeFalsy()`
 - For DB query tests, assert `orderBy`, `include`, and other significant parameters with `expect.objectContaining` — not just `where`
 - Enum membership: `in` traverses the prototype chain; use `Object.hasOwn(Enum, value)` instead
+- **E2E state transitions**: 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()`
 
 ## Cleanup in Tests
 
@@ -45,19 +50,42 @@ try {
 - Use realistic fixture values (real task IDs, grade names) instead of placeholders like `'t1'`
 - Extract shared data into fixture files; inline is fine for single-use cases
 - After `.filter()` on fixtures, verify actual contents — same ID may refer to a different entity after fixture updates
+- **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
 
 ## Mock Helpers
 
-Extract repeated mock patterns into a helper in the test file:
+Extract repeated mock patterns into helpers in the test file. For Prisma service tests, define the return type alias once and use it across all helpers:
 
 ```typescript
-function mockFindMany(value: WorkBookPlacements) {
-  vi.mocked(prisma.workBookPlacement.findMany).mockResolvedValue(
-    value as unknown as Awaited<ReturnType<typeof prisma.workBookPlacement.findMany>>,
+type PrismaWorkBook = Awaited<ReturnType<typeof prisma.workBook.findUnique>>;
+type PrismaWorkBookRow = Awaited<ReturnType<typeof prisma.workBook.findMany>>[number];
+
+function mockFindUnique(value: PrismaWorkBook) {
+  vi.mocked(prisma.workBook.findUnique).mockResolvedValue(value);
+}
+
+function mockFindMany(value: PrismaWorkBookRow[]) {
+  vi.mocked(prisma.workBook.findMany).mockResolvedValue(
+    value as unknown as Awaited<ReturnType<typeof prisma.workBook.findMany>>,
   );
 }
+
+function mockCount(value: number) {
+  vi.mocked(prisma.workBook.count).mockResolvedValue(value);
+}
 ```
 
+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.
+
+## Component Vitest Unit Tests
+
+Omit Vitest unit tests for a Svelte component when **both** conditions hold:
+
+1. The component is template-only (no logic beyond prop bindings and basic conditionals)
+2. The component is covered by E2E tests
+
+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.
+
 ## Testing Extracted Utilities
 
 - Add tests at extraction time, not later
@@ -81,3 +109,17 @@ Stop the split if internal helpers (e.g. `fetchUnplacedWorkbooks`) would be frag
 ## HTTP Mocking
 
 Use Nock for external HTTP calls. See `src/test/lib/clients/` for examples.
+
+## Flowbite Toggle in E2E Tests
+
+Flowbite's `Toggle` renders an `sr-only` `<input type="checkbox">` inside a `<label>`. Clicking the input directly fails because the visual `<span>` sibling intercepts pointer events. Click the label wrapper instead:
+
+```typescript
+const toggleInput = page.locator('input[aria-label="<aria-label value>"]');
+const toggleLabel = page.locator('label:has(input[aria-label="<aria-label value>"])');
+
+await toggleLabel.click();
+await expect(toggleInput).toBeChecked({ checked: true });
+```
+
+The same pattern applies to any Flowbite component that visually overlays its native input (e.g. `Checkbox`, `Radio`).

.claude/skills/session-close/SKILL.md

@@ -0,0 +1,10 @@
+---
+name: session-close
+description: Standard closing routine for an implementation session. Verifies tests, updates the plan checklist, proposes rule/skill additions, checks for bloat, and detects repeated instructions.
+disable-model-invocation: true
+argument-hint: '[plan-file-path]'
+---
+
+Run the session-close routine described in [instructions.md](instructions.md).
+
+Arguments: path to the active `plan.md` (optional — defaults to the most recently modified `docs/dev-notes/**/plan.md`).

.claude/skills/session-close/instructions.md

@@ -0,0 +1,60 @@
+# Session Close — Detailed Instructions
+
+## Step 1: Verify Tests and Types
+
+Run both checks and fix any failures before proceeding:
+
+```bash
+pnpm test:unit
+pnpm test:integration
+pnpm check
+```
+
+Only errors introduced by this session need fixing. Pre-existing errors (visible in git diff baseline) may be left as-is with a note.
+
+## Step 2: Update plan.md
+
+Target file: the path passed as `$ARGUMENTS`, or the most recently modified `docs/dev-notes/**/plan.md`.
+
+- Mark completed tasks: `- [ ]` → `- [x]`
+- If all tasks in the plan are done, append a one-line completion note, then delete the file or replace its body with a single-line summary. Stale plan files must not be left behind.
+
+## Step 3: Propose Rule / Skill Additions
+
+Read all files in `.claude/rules/` and `.claude/skills/`. Then review the session's changes and identify lessons that meet **all** of the following criteria:
+
+1. Generic enough to apply in future sessions (not specific to this PR's domain)
+2. Not already covered by an existing rule or skill
+3. Grounded in something that actually happened in this session (a bug caught, a type error, a pattern extracted)
+
+Present each candidate as:
+
+```
+→ Add to `<filename>`: under section `<section>`
+<code example>
+<one-sentence rationale>
+```
+
+Do not apply changes until the user confirms.
+
+## Step 4: Validate Rules / Skills for Bloat
+
+For each file in `.claude/rules/`:
+
+- Flag sections that duplicate content in another rule file
+- Flag files exceeding 150 lines where consolidation is possible
+- Flag outdated or project-specific content that no longer applies
+
+Present a concrete diff proposal for each issue. Do not apply without confirmation.
+
+## Step 5: Detect Repeated User Instructions
+
+Scan the conversation history for instructions the user gave more than once across this or prior sessions (visible in memory or chat context). Categorize each as:
+
+| Pattern                                | Suggested fix                             |
+| -------------------------------------- | ----------------------------------------- |
+| Always applies to every session        | Add to `AGENTS.md` workflow               |
+| Applies to a specific file type        | Add to the relevant `.claude/rules/` file |
+| A multi-step workflow called on demand | Promote to a new Skill                    |
+
+Report findings as a short bulleted list. Do not modify files without confirmation.

AGENTS.md

@@ -12,8 +12,7 @@ Always prefer simplicity over pathological correctness. YAGNI, KISS, DRY. No bac
 2. Before writing a new function, search `src/lib/utils/`, `src/lib/services/`, `src/features/*/utils/` and `src/features/*/services/` for existing implementations; extract shared logic there when it appears in 2+ places
 3. Write tests first, then implement production code, then verify with `pnpm test:unit`
 4. Review critically after implementing: flag YAGNI violations, over-abstraction, missing tests
-5. Record reusable insights in `.claude/rules/` or `docs/guides/` after the session
-6. Discard or summarize completed plans; don't leave stale TODOs
+5. Run `/session-close` at the end of each session: updates plan checklist, proposes rule/skill additions, checks for bloat, and detects repeated instructions
 
 ## Tech Stack
 
@@ -69,9 +68,10 @@ prisma/schema.prisma # Database schema
 ## Key Conventions
 
 - **Svelte 5 Runes**: Use `$props()`, `$state()`, `$derived()` in all new components
+- **Service layer**: Services return data or `null`; never call `error()` or `redirect()`. HTTP error translation belongs in the route handler — the service must stay framework-agnostic and unit-testable.
 - **Server data**: `+page.server.ts` → `+page.svelte` via `data` prop
 - **Forms**: Superforms + Zod validation
-- **Tests**: Write tests before implementation (TDD). Use `@quramy/prisma-fabbrica` for factories, Nock for HTTP mocking
+- **Tests**: Write tests before implementation (TDD). Use `@quramy/prisma-fabbrica` for factories only in `prisma/seed.ts` and Playwright global setup (`tests/global-setup.ts`). For service-layer unit tests, mock the DB with `vi.mock('$lib/server/database', ...)` — do not use fabbrica there. Use Nock for HTTP mocking
 - **Naming**: `camelCase` variables, `PascalCase` types/components, `snake_case` files/routes, `kebab-case` directories
 - **Pre-commit**: Lefthook runs Prettier + ESLint (bypass: `LEFTHOOK=0 git commit`)
 

docs/guides/claude-code.md

@@ -65,9 +65,16 @@ paths:
 
 **本プロジェクトの skills（`.claude/skills/`）:**
 
-| スキル           | 用途                                                                   |
-| ---------------- | ---------------------------------------------------------------------- |
-| `/refactor-plan` | Issue 番号またはパスを渡してリファクタリング計画を出力（実装はしない） |
+| スキル           | 用途                                                                                                         |
+| ---------------- | ------------------------------------------------------------------------------------------------------------ |
+| `/refactor-plan` | Issue 番号またはパスを渡してリファクタリング計画を出力（実装はしない）                                       |
+| `/session-close` | セッション終了時のルーティン：テスト確認 → plan.md 更新 → rules 候補提示 → 肥大化チェック → 繰り返し指示検出 |
+
+**プロジェクトローカルスキルと `Skill` ツールの違い:**
+
+- `Skill` ツール（Claude 内部のツール）はシステムプロンプトに列挙されたビルトインスキルのみ対象
+- `.claude/skills/` のプロジェクトローカルスキルは `/skill-name` スラッシュコマンドでのみ起動（Claude Code CLI が直接読み込む）
+- `/session-close` などを Claude に `Skill` ツール経由で呼ばせようとしても動作しない
 
 ### Hooks
 

eslint.config.mjs

@@ -71,7 +71,10 @@ export default [
         },
       ],
       // Add TypeScript ESLint rules manually
-      '@typescript-eslint/no-unused-vars': ['error', { argsIgnorePattern: '^_' }],
+      '@typescript-eslint/no-unused-vars': [
+        'error',
+        { argsIgnorePattern: '^_', varsIgnorePattern: '^_' },
+      ],
       '@typescript-eslint/no-explicit-any': 'warn',
       // Disable some strict Svelte rules that are too aggressive
       'svelte/require-each-key': 'warn',