Conversation
問題集一覧のリファクタリング計画（Phase 0-8）を追加。N+1クエリ修正、サービス層からSvelteKit依存除去、コンポーネント分割などの実装方針を記録。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Phase 0-6の実装: - N+1クエリ修正（getWorkBooksWithAuthors追加） - サービス層からSvelteKit error()を除去、nullを返すよう変更 - WorkBookBaseTableをCurriculumTable/SolutionTable/CreatedByUserTableに分割 - WorkBookListをRecord<WorkBookType, Component>ルックアップに変更 - +page.svelte・+page.server.tsを簡略化 - utils/workbooks.tsにgetWorkBooksByType・buildTaskResultsByWorkBookId追加 - テスト追加（workbook_tasks.test.ts、utils/workbooks.test.ts拡充） - Svelte 5 $derivedパターン修正とloggedInUser型修正 - eslint.config.mjsにvarsIgnorePattern追加（デストラクチャの_プレフィックス対応） Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
完了したフェーズのチェックボックスを更新し、残作業（Phase 7-8）を整理。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erage (#3280) - Phase 7: テスト強化（workbooks.test.ts追加、workbooks.test.ts・workbook_tasks.test.ts拡充） - Phase 8: E2Eテスト追加（tests/workbooks_list.test.ts、loginAsAdminをhelpers/auth.tsに抽出） - テーブルコンポーネントから共通セルを抽出（GradeTableBodyCell、WorkbookAuthorActionsCell、WorkbookCompletionCell、WorkbookProgressCell） - coding-style.mdにTSDocルール追加 - testing.mdにルール追加 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
完了したフェーズをすべてチェック済みに更新し、計画を整理。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
) - CurriculumWorkBookList.svelte を分離（グレードフィルター・補充問題集トグルをカプセル化） - WorkBookList.svelte を簡略化（CURRICULUM以外は単純テーブル表示） - +page.server.ts でクエリを Promise.all 並列化 - workbooks.ts / workbooks.test.ts / workbooks.test.ts をリファクタリング - rules: prisma-db.md に並列クエリパターン追加、svelte-components.md に Partial<Record> パターン追加、testing.md にモックヘルパー改善 - skills: session-close スキル追加 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
完了したすべてのフェーズを整理し、計画を最終化。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- CurriculumWorkBookList.svelte をリファクタリング（ロジック整理） - EmptyWorkbookList.svelte を抽出 - WorkBookList.svelte を簡略化 - utils/workbooks.ts・テストを整理 - svelte-components.md・testing.md にルール追記 - 完了済み計画ドキュメントを削除 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
WalkthroughThis pull request refactors the workbook list feature by splitting a monolithic table component into specialized, type-specific variants and introduces session-close automation. It adds documentation rules for coding practices, updates the service layer with a new data-enrichment function, extends utilities with workbook-filtering operations, and includes comprehensive tests for services and E2E scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
src/routes/workbooks/edit/[slug]/+page.server.ts (2)
105-105: Remove success-path console logging from the action handler.This looks like debug output and can add noise in server logs; prefer structured logger usage only when needed.
Suggested patch
- console.log(`Updated workbook ${workBookId}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/workbooks/edit/`[slug]/+page.server.ts at line 105, Remove the debug console.log in the action handler (the line that reads console.log(`Updated workbook ${workBookId}`))—delete this success-path console logging; if you need structured logging instead, replace it with the existing server logger (e.g., call processLogger.info or the module's logger) and include context like workBookId, otherwise just remove the statement so the handler no longer emits console output on success.
94-99: Align action slug validation withloadto avoid 404-on-invalid-input behavior.Malformed slugs currently fall through to
NOT_FOUNDin the action, whileloadreturnsBAD_REQUEST. Keeping these consistent improves API semantics and error handling.Suggested patch
const workBook = form.data; const slug = params.slug.toLowerCase(); + + if (!parseWorkBookId(slug) && !parseWorkBookUrlSlug(slug)) { + error(BAD_REQUEST, '不正な問題集idです。'); + } + const workBookWithAuthor = await workBooksCrud.getWorkbookWithAuthor(slug); if (!workBookWithAuthor) { error(NOT_FOUND, `問題集id: ${slug} は見つかりませんでした。`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/workbooks/edit/`[slug]/+page.server.ts around lines 94 - 99, The action currently converts params.slug and calls workBooksCrud.getWorkbookWithAuthor without performing the same slug validation used in load, causing malformed slugs to produce NOT_FOUND; update the action to run the same slug validation logic as load (reuse the same helper function or regex check used in load) and if the slug is invalid return error(BAD_REQUEST, ...) instead of proceeding, then only call workBooksCrud.getWorkbookWithAuthor and return NOT_FOUND if the validated slug is not found.src/routes/workbooks/+page.server.ts (1)
31-31: Use object shorthand.The property
workbooks: workbookscan be simplified to justworkbooks.Proposed fix
return { - workbooks: workbooks, + workbooks, tasksMapByIds: tasksMapByIds,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/workbooks/`+page.server.ts at line 31, Replace the explicit property assignment "workbooks: workbooks" with the object shorthand "workbooks" in the returned object (wherever the load or return object is constructed in +page.server.ts), i.e., use the shorthand property name for the variable workbooks instead of repeating the identifier.tests/helpers/auth.ts (1)
21-21: Fragile selector:.nth(1)depends on DOM order.Using
.nth(1)to select the second button assumes a specific page structure with multiple "ログイン" buttons. If the page layout changes, this will silently click the wrong element or fail. Consider using a more specific selector (e.g.,data-testid) if feasible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/auth.ts` at line 21, The selector using page.getByRole('button', { name: 'ログイン' }).nth(1).click() is brittle because it relies on DOM order; replace it with a deterministic locator (e.g., a data-testid or a more specific role/attribute) so the test targets the intended login button. Update the code that performs the click (the expression using page.getByRole(...).nth(1)) to use a stable selector such as page.getByTestId('login-button') or page.locator('button[data-testid="login-button"]') and ensure the app adds that data-testid to the correct login button; keep the rest of the sign-in flow unchanged.tests/workbooks_list.test.ts (1)
61-69: These skips let fixture changes hide regressions.If the current dataset has no replenishment toggle or no visible workbook rows, the suite exits as skipped instead of proving the behavior. Seed deterministic workbook data, or navigate to a known grade/record so these assertions always execute.
Also applies to: 96-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workbooks_list.test.ts` around lines 61 - 69, The test currently skips when no replenishment toggle or rows are visible (the block using toggleInput.isVisible, toggleInput.isChecked, toggleLabel.click, and TIMEOUT), which hides regressions; instead ensure deterministic test setup: seed or stub the workbook data (or navigate to a known grade/record) in the test setup so a replenishment workbook and visible toggle always exist, remove the test.skip branch, and assert the toggle behavior directly; apply the same change to the similar block referencing toggleInput/toggleLabel around lines 96-103 so both sections rely on seeded fixtures or a fixed navigation target rather than conditional skipping.src/features/workbooks/services/workbooks.test.ts (1)
99-197: Please add direct coverage forgetWorkbookWithAuthor.
src/features/workbooks/services/workbooks.ts:101-123now returnsnullfor invalid or missing slugs, but this suite never locks down that contract. A few cases for numeric IDs, URL slugs, invalid slugs, and missing authors would protect the new loader behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/workbooks/services/workbooks.test.ts` around lines 99 - 197, Add unit tests directly for getWorkbookWithAuthor to assert it returns null for invalid/missing slugs and correctly resolves for numeric IDs and valid urlSlugs; specifically add tests that call getWorkbookWithAuthor with (1) a numeric id when prisma.workBook.findUnique returns the workbook with user, (2) a valid urlSlug when findUnique returns a workbook, (3) an invalid/nonexistent slug when findUnique returns null and assert null is returned, and (4) a case where the workbook exists but the related user is null and assert authorName becomes 'unknown'. Use the existing test helpers like mockFindUnique, mockFindMany/asPrismaWorkBookWithUser and prepareWorkBook to stub prisma responses and mirror how other tests (createWorkBook/updateWorkBook/deleteWorkBook) assert calls to prisma.src/features/workbooks/utils/workbooks.ts (1)
128-133: Avoid per-call empty-array allocation ingetTaskResultOn Line 132,
?? []creates a new array on every miss. Prefer a shared immutable fallback for stable references and lower allocation churn.♻️ Proposed refactor
+const EMPTY_TASK_RESULTS: TaskResults = []; + export function getTaskResult( workbookId: number, taskResults: Map<number, TaskResults>, ): TaskResults { - return taskResults.get(workbookId) ?? []; + return taskResults.get(workbookId) ?? EMPTY_TASK_RESULTS; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/workbooks/utils/workbooks.ts` around lines 128 - 133, getTaskResult currently returns taskResults.get(workbookId) ?? [] which allocates a new empty array on every cache miss; replace the per-call allocation with a shared immutable fallback constant (e.g., define a top-level EMPTY_TASK_RESULTS: TaskResults = Object.freeze([]) or similar) and return taskResults.get(workbookId) ?? EMPTY_TASK_RESULTS so callers get a stable, non-allocating empty value; update references in getTaskResult and ensure TaskResults typing matches the shared empty value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/session-close/instructions.md:
- Line 52: The instruction text contains a typo: the imperative "Categories"
should be the verb "Categorize"; update the sentence that starts "Scan the
conversation history for instructions the user gave more than once..." to
replace "Categories each as:" with "Categorize each as:" so the grammar is
correct (edit the line in .claude/skills/session-close/instructions.md where
that phrase appears).
In `@src/features/workbooks/components/list/CreatedByUserTable.svelte`:
- Line 36: Replace the typoged CSS class on the TableHeadCell component: change
the class attribute value from "ext-left min-w-[240px] max-w-[1440px] px-0" to
use the correct "text-left" instead of "ext-left" in CreatedByUserTable.svelte
(look for the TableHeadCell that currently renders "回答状況"); ensure no other
instances of "ext-left" remain.
- Line 43: The {`#each` workbooks as workbook} block in CreatedByUserTable.svelte
needs a stable unique key to allow Svelte to reconcile DOM updates; update the
each block to include a unique identifier expression for each workbook (for
example use workbook.id or another stable unique property on the workbook
object) and if no unique id exists, add one on the data model rather than using
the array index; ensure the keyed each uses the chosen property so Svelte can
track items correctly when reordering, filtering, or removing workbooks.
In `@src/features/workbooks/components/list/CurriculumTable.svelte`:
- Line 33: The {`#each` workbooks as workbook} block in CurriculumTable.svelte
needs a unique key for proper DOM reconciliation; update the each block to
include a stable unique identifier (e.g., use workbook.id or workbook.uuid) like
the fix used in CreatedByUserTable.svelte so Svelte can track items
correctly—locate the {`#each` workbooks as workbook} block and add the key
expression (workbook.<uniqueIdProperty>) matching the workbook objects.
- Line 26: The TableHeadCell in CurriculumTable.svelte has a typo in its class
name ("ext-left"); update the class on the TableHeadCell component (the element
with class "ext-left min-w-[240px] max-w-[1440px] px-0") to use "text-left"
instead of "ext-left" (apply the same fix pattern used in
CreatedByUserTable.svelte to ensure consistent alignment classes).
In `@src/features/workbooks/components/list/CurriculumWorkBookList.svelte`:
- Around line 43-65: The $effect currently reads taskGradesByWorkBookTypeStore
with non-reactive get(), so selectedGrade won't update when the store changes;
replace the get() usage with a reactive subscription (use the $store reactive
syntax for taskGradesByWorkBookTypeStore or call
taskGradesByWorkBookTypeStore.subscribe inside the effect) and assign the
curriculum grade to selectedGrade there; ensure filterByGradeMode still calls
taskGradesByWorkBookTypeStore.updateTaskGrade(WorkBookType.CURRICULUM, grade) so
updates propagate to the reactive subscription.
In `@src/features/workbooks/components/list/SolutionTable.svelte`:
- Line 35: The {`#each`} block in SolutionTable.svelte iterates "workbooks as
workbook" without a key, causing inefficient DOM updates; update that {`#each`} to
include a unique key expression (preferably a stable property like workbook.id
or workbook.uuid, falling back to the array index only if no stable id exists)
so Svelte can track items correctly.
- Line 28: The TableHeadCell instances use a typoged class name "ext-left" which
should be "text-left"; update the class attribute on the TableHeadCell in
SolutionTable.svelte (and the equivalent TableHeadCell usages in the other two
table components) to replace "ext-left" with "text-left" so the left-alignment
styling is applied correctly.
In `@src/features/workbooks/utils/workbooks.test.ts`:
- Around line 269-299: The test for calcWorkBookGradeModes never exercises the
tie-break branch because the fixture sets TaskGrade.Q7 five times but Q6 only
once; update the tasksMapByIds and/or workBookTasks so Q6 has the same frequency
as Q7 (e.g., add additional entries with TaskGrade.Q6 or reduce Q7 occurrences)
to create an actual tie, then assert the function returns the easier grade
(TaskGrade.Q7) for workbook id 1; reference calcWorkBookGradeModes, TaskGrade,
tasksMapByIds, and createWorkBookListBase when locating and adjusting the
fixture.
In `@src/routes/workbooks/create/`+page.server.ts:
- Line 71: Replace the raw success console.log that prints workBook.title and
author.id with a privacy-safe, non-user-specific log: stop emitting
workBook.title and author.id directly and instead log a generic message (e.g.,
"Workbook created") and, if needed, only non-sensitive structured metadata
(e.g., workbook id, truncated title hash, or boolean flags). Locate the
console.log call that references workBook.title and author.id and change it to a
sanitized structured log or use the existing application logger (e.g.,
processLogger/your logger) to emit non-identifying fields only.
In `@tests/helpers/auth.ts`:
- Around line 7-14: The auth helpers use incorrect hardcoded usernames causing
test logins to fail; update loginAsAdmin to call loginAs(page, 'admin_test') and
update loginAsUser to call loginAs(page, 'guest_test') so they match the seeded
users; locate and edit the two functions loginAsAdmin and loginAsUser in
tests/helpers/auth.ts to replace 'admin' with 'admin_test' and 'guest' with
'guest_test'.
In `@tests/workbooks_list.test.ts`:
- Around line 33-44: The test "clicking a grade filter on the curriculum tab
switches the workbook list" currently re-checks visibility of the 9Q button
after clicking, which doesn't prove the filter changed; update the assertion
after the click to verify the active state or list content change instead.
Specifically, after await page.getByRole('button', { name: '9Q' }).click(),
assert that the 9Q button is active (e.g., has aria-pressed="true" or an
"active" CSS class) using page.getByRole('button', { name: '9Q'
}).toHaveAttribute(...) or page.getByRole(...).toHaveClass(...), or assert the
workbook list re-renders to show a 9Q-specific row (e.g., check for a row text
or selector unique to 9Q) rather than re-checking visibility; update the
assertion in that test accordingly.
---
Nitpick comments:
In `@src/features/workbooks/services/workbooks.test.ts`:
- Around line 99-197: Add unit tests directly for getWorkbookWithAuthor to
assert it returns null for invalid/missing slugs and correctly resolves for
numeric IDs and valid urlSlugs; specifically add tests that call
getWorkbookWithAuthor with (1) a numeric id when prisma.workBook.findUnique
returns the workbook with user, (2) a valid urlSlug when findUnique returns a
workbook, (3) an invalid/nonexistent slug when findUnique returns null and
assert null is returned, and (4) a case where the workbook exists but the
related user is null and assert authorName becomes 'unknown'. Use the existing
test helpers like mockFindUnique, mockFindMany/asPrismaWorkBookWithUser and
prepareWorkBook to stub prisma responses and mirror how other tests
(createWorkBook/updateWorkBook/deleteWorkBook) assert calls to prisma.
In `@src/features/workbooks/utils/workbooks.ts`:
- Around line 128-133: getTaskResult currently returns
taskResults.get(workbookId) ?? [] which allocates a new empty array on every
cache miss; replace the per-call allocation with a shared immutable fallback
constant (e.g., define a top-level EMPTY_TASK_RESULTS: TaskResults =
Object.freeze([]) or similar) and return taskResults.get(workbookId) ??
EMPTY_TASK_RESULTS so callers get a stable, non-allocating empty value; update
references in getTaskResult and ensure TaskResults typing matches the shared
empty value.
In `@src/routes/workbooks/`+page.server.ts:
- Line 31: Replace the explicit property assignment "workbooks: workbooks" with
the object shorthand "workbooks" in the returned object (wherever the load or
return object is constructed in +page.server.ts), i.e., use the shorthand
property name for the variable workbooks instead of repeating the identifier.
In `@src/routes/workbooks/edit/`[slug]/+page.server.ts:
- Line 105: Remove the debug console.log in the action handler (the line that
reads console.log(`Updated workbook ${workBookId}`))—delete this success-path
console logging; if you need structured logging instead, replace it with the
existing server logger (e.g., call processLogger.info or the module's logger)
and include context like workBookId, otherwise just remove the statement so the
handler no longer emits console output on success.
- Around line 94-99: The action currently converts params.slug and calls
workBooksCrud.getWorkbookWithAuthor without performing the same slug validation
used in load, causing malformed slugs to produce NOT_FOUND; update the action to
run the same slug validation logic as load (reuse the same helper function or
regex check used in load) and if the slug is invalid return error(BAD_REQUEST,
...) instead of proceeding, then only call workBooksCrud.getWorkbookWithAuthor
and return NOT_FOUND if the validated slug is not found.
In `@tests/helpers/auth.ts`:
- Line 21: The selector using page.getByRole('button', { name: 'ログイン'
}).nth(1).click() is brittle because it relies on DOM order; replace it with a
deterministic locator (e.g., a data-testid or a more specific role/attribute) so
the test targets the intended login button. Update the code that performs the
click (the expression using page.getByRole(...).nth(1)) to use a stable selector
such as page.getByTestId('login-button') or
page.locator('button[data-testid="login-button"]') and ensure the app adds that
data-testid to the correct login button; keep the rest of the sign-in flow
unchanged.
In `@tests/workbooks_list.test.ts`:
- Around line 61-69: The test currently skips when no replenishment toggle or
rows are visible (the block using toggleInput.isVisible, toggleInput.isChecked,
toggleLabel.click, and TIMEOUT), which hides regressions; instead ensure
deterministic test setup: seed or stub the workbook data (or navigate to a known
grade/record) in the test setup so a replenishment workbook and visible toggle
always exist, remove the test.skip branch, and assert the toggle behavior
directly; apply the same change to the similar block referencing
toggleInput/toggleLabel around lines 96-103 so both sections rely on seeded
fixtures or a fixed navigation target rather than conditional skipping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab5812b9-c7b5-4cae-bfcb-ff04a74d07ee
📒 Files selected for processing (35)
.claude/rules/coding-style.md.claude/rules/prisma-db.md.claude/rules/svelte-components.md.claude/rules/testing.md.claude/skills/session-close/SKILL.md.claude/skills/session-close/instructions.mdAGENTS.mddocs/guides/claude-code.mdeslint.config.mjssrc/features/workbooks/components/list/CreatedByUserTable.sveltesrc/features/workbooks/components/list/CurriculumTable.sveltesrc/features/workbooks/components/list/CurriculumWorkBookList.sveltesrc/features/workbooks/components/list/EmptyWorkbookList.sveltesrc/features/workbooks/components/list/GradeTableBodyCell.sveltesrc/features/workbooks/components/list/SolutionTable.sveltesrc/features/workbooks/components/list/WorkBookBaseTable.sveltesrc/features/workbooks/components/list/WorkBookList.sveltesrc/features/workbooks/components/list/WorkbookAuthorActionsCell.sveltesrc/features/workbooks/components/list/WorkbookCompletionCell.sveltesrc/features/workbooks/components/list/WorkbookProgressCell.sveltesrc/features/workbooks/services/workbook_tasks.test.tssrc/features/workbooks/services/workbook_tasks.tssrc/features/workbooks/services/workbooks.test.tssrc/features/workbooks/services/workbooks.tssrc/features/workbooks/types/workbook.tssrc/features/workbooks/utils/workbooks.test.tssrc/features/workbooks/utils/workbooks.tssrc/routes/workbooks/+page.server.tssrc/routes/workbooks/+page.sveltesrc/routes/workbooks/[slug]/+page.server.tssrc/routes/workbooks/create/+page.server.tssrc/routes/workbooks/edit/[slug]/+page.server.tstests/helpers/auth.tstests/workbook_order.test.tstests/workbooks_list.test.ts
💤 Files with no reviewable changes (1)
- src/features/workbooks/components/list/WorkBookBaseTable.svelte
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- review.md に詳細なレビューコメントと対応方針を追記 - svelte-docs.md ルールを新規追加 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- テーブルコンポーネントの各行に workbook.id をキーとして追加（svelte/require-each-key 対応） - services/workbooks.ts のログを操作後に移動 - +page.server.ts の console.log を削除またはクリーンアップ - services/workbooks.test.ts にテスト追加 - utils/workbooks.test.ts にテスト追加 - svelte-components.md に each ブロックのキールールを追記 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.claude/skills/session-close/instructions.md (1)
32-36: Add language specification to fenced code block.The code block is missing a language specification, which triggers a markdownlint warning (MD040). Consider adding
textto improve documentation quality.📝 Proposed fix
-``` +```text → Add to `<filename>`: under section `<section>` <code example> <one-sentence rationale>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/session-close/instructions.md around lines 32 - 36, Update the fenced code block in .claude/skills/session-close/instructions.md to include a language specifier (use "text") so the block becomes ```text ... ```, which will resolve the MD040 markdownlint warning; locate the triple-backtick block shown in the diff (the block containing "→ Add to `<filename>`: under section `<section>`" and the code example) and add the language token immediately after the opening backticks.src/features/workbooks/components/list/CurriculumWorkBookList.svelte (2)
91-91: Add key to{#each}for consistency.While
AVAILABLE_GRADESis a static constant tuple and won't cause DOM issues, adding a key satisfies the linter and maintains consistency with other{#each}blocks in the codebase.♻️ Proposed fix
- {`#each` AVAILABLE_GRADES as grade} + {`#each` AVAILABLE_GRADES as grade (grade)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/workbooks/components/list/CurriculumWorkBookList.svelte` at line 91, The {`#each`} block iterating AVAILABLE_GRADES should include a key to satisfy the linter and keep consistency; update the {`#each` AVAILABLE_GRADES as grade} in CurriculumWorkBookList.svelte to use a stable key (e.g., {`#each` AVAILABLE_GRADES as grade (grade)} or (index) if grades are not unique) so each iteration has a unique identifier referencing AVAILABLE_GRADES and the grade loop variable.
59-65: Previous issue resolved: now using reactive$storesyntax.The
$effectcorrectly uses$taskGradesByWorkBookTypeStoreinstead ofget(taskGradesByWorkBookTypeStore), ensuring the effect re-runs when the store updates.Minor: The
if (grade)check on line 62 is redundant since line 60 already provides aTaskGrade.Q10fallback, sogradewill always be truthy.♻️ Optional simplification
$effect(() => { const grade = $taskGradesByWorkBookTypeStore.get(WorkBookType.CURRICULUM) ?? TaskGrade.Q10; - - if (grade) { - selectedGrade = grade; - } + selectedGrade = grade; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/workbooks/components/list/CurriculumWorkBookList.svelte` around lines 59 - 65, The if (grade) check is redundant because grade is already defaulted to TaskGrade.Q10; simplify the $effect by assigning selectedGrade directly from the resolved value: compute grade via $taskGradesByWorkBookTypeStore.get(WorkBookType.CURRICULUM) ?? TaskGrade.Q10 and set selectedGrade = grade (or inline the expression) inside the $effect, removing the conditional; update the block containing $effect, $taskGradesByWorkBookTypeStore, WorkBookType.CURRICULUM, TaskGrade.Q10, and selectedGrade accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/workbooks/`+page.server.ts:
- Around line 20-28: The call to taskResultsCrud.getTaskResultsOnlyResultExists
is being passed loggedInUser?.id as string which unsafely asserts a string when
loggedInUser may be null; either guard the Promise.all invocation so you pass a
real string only when loggedInUser exists (e.g., build the array conditionally
or compute userId = loggedInUser?.id and short-circuit to an empty result when
undefined) or change the getTaskResultsOnlyResultExists signature to accept
userId: string | undefined and handle undefined inside that function; update
references to getTaskResultsOnlyResultExists and the Promise.all call
accordingly to preserve type safety.
---
Nitpick comments:
In @.claude/skills/session-close/instructions.md:
- Around line 32-36: Update the fenced code block in
.claude/skills/session-close/instructions.md to include a language specifier
(use "text") so the block becomes ```text ... ```, which will resolve the MD040
markdownlint warning; locate the triple-backtick block shown in the diff (the
block containing "→ Add to `<filename>`: under section `<section>`" and the code
example) and add the language token immediately after the opening backticks.
In `@src/features/workbooks/components/list/CurriculumWorkBookList.svelte`:
- Line 91: The {`#each`} block iterating AVAILABLE_GRADES should include a key to
satisfy the linter and keep consistency; update the {`#each` AVAILABLE_GRADES as
grade} in CurriculumWorkBookList.svelte to use a stable key (e.g., {`#each`
AVAILABLE_GRADES as grade (grade)} or (index) if grades are not unique) so each
iteration has a unique identifier referencing AVAILABLE_GRADES and the grade
loop variable.
- Around line 59-65: The if (grade) check is redundant because grade is already
defaulted to TaskGrade.Q10; simplify the $effect by assigning selectedGrade
directly from the resolved value: compute grade via
$taskGradesByWorkBookTypeStore.get(WorkBookType.CURRICULUM) ?? TaskGrade.Q10 and
set selectedGrade = grade (or inline the expression) inside the $effect,
removing the conditional; update the block containing $effect,
$taskGradesByWorkBookTypeStore, WorkBookType.CURRICULUM, TaskGrade.Q10, and
selectedGrade accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d7f468d-86f5-4670-91e5-f784615a0ff7
📒 Files selected for processing (16)
.claude/rules/svelte-components.md.claude/rules/svelte-docs.md.claude/skills/session-close/instructions.mddocs/dev-notes/2026-03-19/workbook-list/review.mdsrc/features/workbooks/components/list/CreatedByUserTable.sveltesrc/features/workbooks/components/list/CurriculumTable.sveltesrc/features/workbooks/components/list/CurriculumWorkBookList.sveltesrc/features/workbooks/components/list/SolutionTable.sveltesrc/features/workbooks/services/workbooks.test.tssrc/features/workbooks/services/workbooks.tssrc/features/workbooks/utils/workbooks.test.tssrc/features/workbooks/utils/workbooks.tssrc/routes/workbooks/+page.server.tssrc/routes/workbooks/create/+page.server.tstests/helpers/auth.tstests/workbooks_list.test.ts
💤 Files with no reviewable changes (1)
- src/routes/workbooks/create/+page.server.ts
✅ Files skipped from review due to trivial changes (2)
- .claude/rules/svelte-docs.md
- src/features/workbooks/services/workbooks.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/helpers/auth.ts
- src/features/workbooks/components/list/CurriculumTable.svelte
- src/features/workbooks/components/list/CreatedByUserTable.svelte
- tests/workbooks_list.test.ts
Comment on lines
20
to
+28
| try { | ||
| // 問題集を構成する問題のグレードの最頻値を取得するために使用 | ||
| const tasksMapByIds = await taskCrud.getTasksByTaskId(); | ||
| // ユーザの回答状況を表示するために使用 | ||
| const taskResultsByTaskId = await taskResultsCrud.getTaskResultsOnlyResultExists( | ||
| loggedInUser?.id as string, | ||
| true, | ||
| ); | ||
| // Each query is independent, so we execute them in parallel with Promise.all | ||
| const [workbooks, tasksMapByIds, taskResultsByTaskId] = await Promise.all([ | ||
| workBooksCrud.getWorkBooksWithAuthors(), | ||
| // Used to get the most frequent grade of the tasks that make up the workbook | ||
| taskCrud.getTasksByTaskId(), | ||
| // Used to display the user's answer status | ||
| taskResultsCrud.getTaskResultsOnlyResultExists(loggedInUser?.id as string, true), | ||
| ]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the implementation of getTaskResultsOnlyResultExists
ast-grep --pattern 'async function getTaskResultsOnlyResultExists($$$) {
$$$
}'
# Also search with export keyword
ast-grep --pattern 'export async function getTaskResultsOnlyResultExists($$$) {
$$$
}'Repository: AtCoder-NoviSteps/AtCoderNoviSteps
Length of output: 60
🏁 Script executed:
rg "getTaskResultsOnlyResultExists" --type ts --type js -A 10Repository: AtCoder-NoviSteps/AtCoderNoviSteps
Length of output: 2529
🏁 Script executed:
sed -n '1,50p' src/lib/services/task_results.tsRepository: AtCoder-NoviSteps/AtCoderNoviSteps
Length of output: 1657
🏁 Script executed:
rg "export async function getTaskResultsOnlyResultExists" -A 30 src/lib/services/task_results.tsRepository: AtCoder-NoviSteps/AtCoderNoviSteps
Length of output: 1179
🏁 Script executed:
rg "export async function getAnswers|export.*getAnswers" -A 15 src/lib/services/answers.tsRepository: AtCoder-NoviSteps/AtCoderNoviSteps
Length of output: 1263
Good optimization: parallel query execution.
Using Promise.all for independent queries improves load time. However, the loggedInUser?.id as string cast on line 27 creates a type-safety issue. When loggedInUser is null, this passes undefined to getTaskResultsOnlyResultExists, which accepts string. Although the code handles this gracefully by returning empty results (due to Prisma's behavior with undefined values), the type assertion bypasses TypeScript's checks. Consider either checking loggedInUser before the call or updating the function signature to accept string | undefined.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/workbooks/`+page.server.ts around lines 20 - 28, The call to
taskResultsCrud.getTaskResultsOnlyResultExists is being passed loggedInUser?.id
as string which unsafely asserts a string when loggedInUser may be null; either
guard the Promise.all invocation so you pass a real string only when
loggedInUser exists (e.g., build the array conditionally or compute userId =
loggedInUser?.id and short-circuit to an empty result when undefined) or change
the getTaskResultsOnlyResultExists signature to accept userId: string |
undefined and handle undefined inside that function; update references to
getTaskResultsOnlyResultExists and the Promise.all call accordingly to preserve
type safety.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.claude/rules/testing.md (1)
57-78: Well-structured mock helper guidance with accurate examples.The type aliases and helper functions match the existing patterns in
workbooks.test.ts(context snippet 1). The code examples are syntactically correct, and the guidance to extractmockFindUnique,mockFindMany, andmockCountas a standard trio is clear.Optional enhancement: Consider adding a note that the type alias names (
PrismaWorkBook,PrismaWorkBookRow) should be adapted to match the specific Prisma model being tested (e.g.,PrismaUser,PrismaUserRowfor User model).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/rules/testing.md around lines 57 - 78, Extract the repeated Prisma mock patterns into reusable helpers: define the type aliases (e.g., PrismaWorkBook and PrismaWorkBookRow) once and implement helper functions mockFindUnique, mockFindMany, and mockCount that call vi.mocked(prisma.workBook.findUnique/findMany/count). Where tests also exercise create/transaction/delete flows, add mockCreate, mockTransaction, and mockDelete helpers that mock prisma.workBook.create/prisma.$transaction/prisma.workBook.delete respectively; ensure the type alias names are adapted per model (e.g., PrismaUser/PrismaUserRow) and update tests to call these helpers instead of duplicating vi.mocked(...) calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/rules/testing.md:
- Around line 57-78: Extract the repeated Prisma mock patterns into reusable
helpers: define the type aliases (e.g., PrismaWorkBook and PrismaWorkBookRow)
once and implement helper functions mockFindUnique, mockFindMany, and mockCount
that call vi.mocked(prisma.workBook.findUnique/findMany/count). Where tests also
exercise create/transaction/delete flows, add mockCreate, mockTransaction, and
mockDelete helpers that mock
prisma.workBook.create/prisma.$transaction/prisma.workBook.delete respectively;
ensure the type alias names are adapted per model (e.g.,
PrismaUser/PrismaUserRow) and update tests to call these helpers instead of
duplicating vi.mocked(...) calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cfe179f7-7d5e-4d66-a1ec-aecce8aa11e5
📒 Files selected for processing (3)
.claude/rules/coding-style.md.claude/rules/svelte-components.md.claude/rules/testing.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .claude/rules/coding-style.md
- .claude/rules/svelte-components.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #3280
Summary by CodeRabbit
New Features
Bug Fixes
Performance