Conversation
WalkthroughAdds planning docs, a new Prisma model and SQL migration for ContestTaskPair, ERD updates, TypeScript types, and a service module with CRUD operations to map tasks to multiple contests using a composite (contestId, taskId) uniqueness constraint. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Admin / Seeder
participant App as App Service
participant Prisma as Prisma Client
participant DB as Postgres
rect rgb(238,245,255)
note right of App: Create (idempotent) flow
Admin->>App: createContestTaskPair(contestId, taskTableIndex, taskId)
App->>Prisma: findUnique(where: { contestId, taskId })
Prisma->>DB: SELECT
DB-->>Prisma: row?
alt exists
Prisma-->>App: existing row
App-->>Admin: existing ContestTaskPair
else not exists
App->>Prisma: create({ contestId, taskTableIndex, taskId })
Prisma->>DB: INSERT
DB-->>Prisma: row
Prisma-->>App: new ContestTaskPair
App-->>Admin: created ContestTaskPair
end
end
rect rgb(245,238,255)
note right of App: Read (list) flow
Admin->>App: getContestTaskPairs(contestId?)
App->>Prisma: findMany(where: { contestId? })
Prisma->>DB: SELECT ... WHERE contestId=?
DB-->>Prisma: rows
Prisma-->>App: ContestTaskPairs
App-->>Admin: list
end
rect rgb(255,250,238)
note right of App: Update flow
Admin->>App: updateContestTaskPair(contestId, taskId, taskTableIndex)
App->>Prisma: update(where: { contestId_taskId }, data: { taskTableIndex })
Prisma->>DB: UPDATE
DB-->>Prisma: updated row or not found
alt updated
Prisma-->>App: updated ContestTaskPair
App-->>Admin: updated
else not found
Prisma-->>App: P2025 error
App-->>Admin: Not Found error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
prisma/schema.prisma (2)
111-123: Add supporting index for common access pattern by (contestId, taskTableIndex).Fetching tasks within a contest ordered by letter/index is common; add a composite index. Consider a uniqueness guarantee if your domain requires one.
model ContestTaskPair { // ... @@unique([contestId, taskId]) @@index([contestId]) + @@index([contestId, taskTableIndex]) + // If the contest letter must be unique within a contest, enforce it: + // @@unique([contestId, taskTableIndex]) @@map("contesttaskpair") }
113-113: Consider using database UUID type forid(optional).If you’re okay with a slight inconsistency vs. existing tables, prefer
@db.Uuid+@default(uuid())so Postgres stores UUIDs natively.- id String @id @default(uuid()) + id String @id @db.Uuid @default(uuid())prisma/migrations/20250928072152_create_contest_task_pair/migration.sql (1)
13-17: Add helpful indexes for query patterns.Add indexes to support reverse lookups and in‑contest ordering.
CREATE INDEX "contesttaskpair_contestId_idx" ON "public"."contesttaskpair"("contestId"); +CREATE INDEX "contesttaskpair_taskId_idx" ON "public"."contesttaskpair"("taskId"); +CREATE INDEX "contesttaskpair_contestId_taskTableIndex_idx" ON "public"."contesttaskpair"("contestId","taskTableIndex"); -- Optionally enforce uniqueness if required by domain: -- CREATE UNIQUE INDEX "contesttaskpair_contestId_taskTableIndex_key" ON "public"."contesttaskpair"("contestId","taskTableIndex");prisma/ERD.md (1)
129-137: Show relation to Task in ERD (after schema change).Once you add
ContestTaskPair.task -> Task.task_id, re-run ERD generation so the diagram reflects the link.docs/dev-notes/2025-09-17/contest-task-mapping/initial_plan.md (1)
176-189: Fix markdownlint MD040: specify language on code fences.Add a language to the directory tree block (e.g.,
text) to satisfy linting.-``` +```text src/routes/admin/import/ ├── problems/ # 問題データインポート │ ├── +page.svelte │ └── types.ts ├── mapping/ # コンテスト-問題マッピング │ ├── +page.svelte │ ├── bulk/ │ └── selective/ └── shared/ # 共通コンポーネント ├── ImportStatus.svelte └── ValidationResults.svelte</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to data retention organization setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 07586d60698fefbae50a0e250b2eac2b10c73614 and 858761171777872228814448352945cf3c73c8d0. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `docs/dev-notes/2025-09-17/contest-task-mapping/initial_plan.md` (1 hunks) * `docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md` (1 hunks) * `prisma/ERD.md` (2 hunks) * `prisma/migrations/20250928072152_create_contest_task_pair/migration.sql` (1 hunks) * `prisma/schema.prisma` (2 hunks) * `src/lib/types/contest_task_pair.ts` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/dev-notes/2025-09-17/contest-task-mapping/initial_plan.md</summary> 176-176: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: preview </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>prisma/schema.prisma (2)</summary><blockquote> `8-9`: **Confirm binaryTargets match deploy runtime (OpenSSL 3.x).** If your deploy images aren’t RHEL/OpenSSL 3.x, Prisma engines may fail to load. Verify the runtime and adjust if needed. --- `35-39`: **Nice: camelCase for new/modified models.** This follows the note about Lucia v3 and gradual migration. No action needed. </blockquote></details> <details> <summary>src/lib/types/contest_task_pair.ts (1)</summary><blockquote> `1-5`: **Types look good and align with Prisma model.** Lightweight aliases; ready for use in services. If you maintain barrel exports (e.g., `src/lib/types/index.ts`), add these types there for discoverability. </blockquote></details> <details> <summary>docs/dev-notes/2025-09-17/contest-task-mapping/initial_plan.md (1)</summary><blockquote> `121-141`: **Good phased plan and adapter sketch.** The compatibility layer and key format are pragmatic for staged migration. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🔇 Additional comments (1)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md (1)
302-356: Replace assertions with throwing guards inTaskMapAdapter.Line 304 still uses
console.assert, which only logs even when the condition fails. The test plan in this document expectsadapter.get('')andadapter.get('task1', '')to throw, so copying this code will keep the bug we discussed earlier. Please swap the assertions for explicit guards that throwErrors when the inputs are invalid.@@ - console.assert( - typeof taskId === 'string' && taskId.length > 0, - 'taskId must be non-empty string', - ); + if (typeof taskId !== 'string' || taskId.length === 0) { + throw new Error('taskId must be a non-empty string'); + } @@ - console.assert( - typeof contestId === 'string' && contestId.length > 0, - 'contestId must be non-empty string', - ); - console.assert( - typeof taskId === 'string' && taskId.length > 0, - 'taskId must be non-empty string', - ); + if (typeof contestId !== 'string' || contestId.length === 0) { + throw new Error('contestId must be a non-empty string'); + } + if (typeof taskId !== 'string' || taskId.length === 0) { + throw new Error('taskId must be a non-empty string'); + }
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md (5)
308-318: Avoid performance.now portability issues in Nodeperformance may not be in lib.dom for TS; prefer Date.now() or import from node:perf_hooks.
Apply:
- const start = performance.now(); + const start = Date.now(); ... - `TaskMapAdapter.get: enhanced hit (${(performance.now() - start).toFixed(2)}ms)`, + `TaskMapAdapter.get: enhanced hit (${(Date.now() - start).toFixed(2)}ms)`, ... - console.log(`TaskMapAdapter.get: legacy hit (${(performance.now() - start).toFixed(2)}ms)`); + console.log(`TaskMapAdapter.get: legacy hit (${(Date.now() - start).toFixed(2)}ms)`); ... - `TaskMapAdapter.get: enhanced scan hit (${(performance.now() - start).toFixed(2)}ms)`, + `TaskMapAdapter.get: enhanced scan hit (${(Date.now() - start).toFixed(2)}ms)`,Or
import { performance } from 'node:perf_hooks'and keep performance.now.Also applies to: 326-327, 337-338
89-117: Race between existence check and createBetween findUnique and create, concurrent writers can still hit P2002. Prefer upsert (if updating taskTableIndex is acceptable) or catch P2002 explicitly.
Apply (catch P2002, preserving “create-only” semantics):
- // 新規レコード作成 - const contestTaskPair = await db.contestTaskPair.create({ - data: { - contestId, - taskTableIndex, - taskId, - }, - }); + // 新規レコード作成（競合時は穏当に無視） + let contestTaskPair: ContestTaskPair | undefined; + try { + contestTaskPair = await db.contestTaskPair.create({ + data: { contestId, taskTableIndex, taskId }, + }); + } catch (e: unknown) { + if (e && typeof e === 'object' && 'code' in e && (e as any).code === 'P2002') { + console.log(`ContestTaskPair already exists (race): contestId=${contestId}, taskId=${taskId}`); + return; + } + throw e; + }Alternatively, use upsert to update taskTableIndex when the pair exists.
202-204: Unify type aliases for contest-task keyed mapsNames and shapes diverge: TaskResultMapByContestTaskPair vs TaskResultByContestTaskPair. Standardize on one generic alias and use consistently in snippets and imports.
Apply:
-// マップの型（明示的） -type TaskResultMapByContestTaskPair = Map<ContestTaskPairKey, TaskResult>; +// Generic map type keyed by ContestTaskPairKey +export type MapByContestTaskPair<T> = Map<ContestTaskPairKey, T>;-type TaskResultByContestTaskPair<T> = Map<ContestTaskPairKey, T>; +export type TaskResultByContestTaskPair<T> = Map<ContestTaskPairKey, T>;And ensure service usage/imports reference the exported alias:
-import { TaskMapAdapter } from '$lib/utils/task_results'; +import { TaskMapAdapter, TaskResultByContestTaskPair } from '$lib/utils/task_results';Also applies to: 283-286, 440-446
409-418: Remove unused contestTaskPairMap or return/use itcontestTaskPairMap is built but unused. Drop it or include it in the returned object and use downstream.
Apply:
- // contestId:taskId -> Task のマップ（TaskResult 作成で直接参照しやすい） - const contestTaskPairMap = new Map<string, Task>(); - for (const t of tasks) { - const contests = contestTaskPairs.get(t.task_id) ?? [t.contest_id]; - for (const c of contests) { - contestTaskPairMap.set(`${c}:${t.task_id}`, t); - } - } - - return { tasks, contestTaskPairs, contestTaskPairMap }; + return { tasks, contestTaskPairs };
145-149: Update only mutable fields in updateContestTaskPairPassing contestId/taskId in data is redundant and risks accidental key changes. Update taskTableIndex only.
Apply:
data: { - contestId, - taskTableIndex, - taskId, + taskTableIndex, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: preview
🔇 Additional comments (3)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md (3)
425-427: Ensure types are exported/imported consistentlyTaskResultByContestTaskPair is used here but not imported in the snippet. Export it from utils and import alongside TaskMapAdapter.
Apply:
-import { TaskMapAdapter } from '$lib/utils/task_results'; +import { TaskMapAdapter, TaskResultByContestTaskPair } from '$lib/utils/task_results';Please confirm the actual file exports/imports match this.
Also applies to: 440-446
373-381: Syntax error: missing closing brace in createContestTaskPairKeyClose the second if block before returning; otherwise this snippet won’t compile.
Apply:
export function createContestTaskPairKey(contestId: string, taskId: string): ContestTaskPairKey { if (!contestId || contestId.trim() === '') { throw new Error('contestId must be a non-empty string'); } if (!taskId || taskId.trim() === '') { throw new Error('taskId must be a non-empty string'); } + return `${contestId}:${taskId}`; }
302-307: Replace console.assert with throwing guards (aligns with tests)console.assert won’t throw; tests expect errors on invalid inputs. Add explicit validation in get and makeKey (also validate contestId when provided).
Apply:
get(taskId: string, contestId?: string): T | undefined { - console.assert( - typeof taskId === 'string' && taskId.length > 0, - 'taskId must be non-empty string', - ); + if (typeof taskId !== 'string' || taskId.trim().length === 0) { + throw new Error('taskId must be a non-empty string'); + } + if (contestId !== undefined && (typeof contestId !== 'string' || contestId.trim().length === 0)) { + throw new Error('contestId must be a non-empty string when provided'); + }private makeKey(contestId: string, taskId: string): ContestTaskPairKey { - console.assert( - typeof contestId === 'string' && contestId.length > 0, - 'contestId must be non-empty string', - ); - console.assert( - typeof taskId === 'string' && taskId.length > 0, - 'taskId must be non-empty string', - ); + if (typeof contestId !== 'string' || contestId.trim().length === 0) { + throw new Error('contestId must be a non-empty string'); + } + if (typeof taskId !== 'string' || taskId.trim().length === 0) { + throw new Error('taskId must be a non-empty string'); + }Also applies to: 348-356, 241-246
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/lib/types/contest_task_pair.ts (2)
10-18: Refactor Read and Update types to match their semantic purposes.
ContestTaskPairReadandContestTaskPairUpdateare currently aliases ofContestTaskPairCreate, but they should have distinct definitions:
- Read should be an alias of
ContestTaskPair(the full Prisma model withid,createdAt,updatedAt)- Update should typically omit auto-generated fields or be a
Partial<ContestTaskPairCreate>if supporting partial updatesApply this diff:
-export type ContestTaskPairRead = ContestTaskPairCreate; +export type ContestTaskPairRead = ContestTaskPair; -export type ContestTaskPairUpdate = ContestTaskPairCreate; +export type ContestTaskPairUpdate = ContestTaskPairCreate;The Update type might need further refinement depending on whether you support partial updates. If only full updates are supported, the current definition is acceptable but could be renamed to
ContestTaskPairUpdateDatafor clarity.
20-23: Consider adding a key creation helper for type safety.The
ContestTaskPairKeytemplate literal type provides basic compile-time safety, but runtime key creation should be centralized. According to the plan document, there's acreateContestTaskPairKeyhelper in utils.Consider re-exporting the key creation helper from this types file for discoverability:
// For mapping and identification export type ContestTaskPairKey = `${string}:${string}`; // "contest_id:task_id" export type TaskResultMapByContestTaskPair = Map<ContestTaskPairKey, TaskResult>; // Helper function (re-export from utils if defined there) export function createContestTaskPairKey(contestId: string, taskId: string): ContestTaskPairKey { if (!contestId || contestId.trim() === '') { throw new Error('contestId must be a non-empty string'); } if (!taskId || taskId.trim() === '') { throw new Error('taskId must be a non-empty string'); } return `${contestId}:${taskId}`; }This centralizes the key format and validation logic with its type definition.
src/lib/services/contest_task_pairs.ts (6)
22-36: Simplify by returning directly.The function correctly uses the composite key, but the intermediate variable assignment is unnecessary.
Apply this diff:
export async function getContestTaskPair( contestId: string, taskId: string, ): Promise<ContestTaskPair | null> { - const contestTaskPair = await db.contestTaskPair.findUnique({ + return await db.contestTaskPair.findUnique({ where: { contestId_taskId: { contestId, taskId, }, }, }); - - return contestTaskPair; }
60-60: Remove unused variable declaration.The variable
contestTaskPairis declared but immediately assigned from thecreateresult. The declaration withundefinedis unnecessary.Apply this diff:
- let contestTaskPair: ContestTaskPair | undefined; - - contestTaskPair = await db.contestTaskPair.create({ + const contestTaskPair = await db.contestTaskPair.create({ data: { contestId, taskTableIndex, taskId, }, });
52-79: Consider simplifying by removing pre-check and relying on P2002 handling.The current implementation checks for an existing record before creating, but this introduces a time-of-check to time-of-use (TOCTOU) race condition window. Since you already handle the P2002 unique constraint violation error, you could simplify by attempting creation directly and catching the constraint error.
Apply this diff to remove the redundant pre-check:
export async function createContestTaskPair( contestId: string, taskTableIndex: string, taskId: string, ): Promise<void> { try { - const existingRecord = await getContestTaskPair(contestId, taskId); - - if (existingRecord) { - console.log(`ContestTaskPair already exists: contestId=${contestId}, taskId=${taskId}`); - return; - } - - let contestTaskPair: ContestTaskPair | undefined; - - contestTaskPair = await db.contestTaskPair.create({ + const contestTaskPair = await db.contestTaskPair.create({ data: { contestId, taskTableIndex, taskId, }, }); console.log('Created ContestTaskPair:', contestTaskPair); } catch (error) { if (error && typeof error === 'object' && 'code' in error && (error as any).code === 'P2002') { - console.log(`Found ContestTaskPair (race): contestId=${contestId}, taskId=${taskId}`); + console.log(`ContestTaskPair already exists: contestId=${contestId}, taskId=${taskId}`); return; } console.error('Failed to create ContestTaskPair:', error); throw error; } }This approach is more idiomatic (optimistic concurrency) and eliminates the race condition.
72-72: Type-cast error check is fragile.The error code check uses a type-unsafe cast
(error as any).code. Consider using a type guard or Prisma's error types for better type safety.Apply this diff:
+import { Prisma } from '@prisma/client'; + // ... in catch block - if (error && typeof error === 'object' && 'code' in error && (error as any).code === 'P2002') { + if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === 'P2002') { console.log(`Found ContestTaskPair (race): contestId=${contestId}, taskId=${taskId}`); return; }This leverages Prisma's built-in error types for proper type safety.
96-103: Pre-check before update is redundant.Prisma's
updatemethod will throw aP2025error if the record doesn't exist, making the pre-check unnecessary and introducing a TOCTOU race condition window.Apply this diff to remove the redundant check:
export async function updateContestTaskPair( contestId: string, taskTableIndex: string, taskId: string, ): Promise<void> { try { - const existingRecord = await getContestTaskPair(contestId, taskId); - - if (!existingRecord) { - const errorMessage = `Not found ContestTaskPair: contestId=${contestId}, taskId=${taskId}`; - console.log(errorMessage); - throw new Error(errorMessage); - } - const updatedContestTaskPair = await db.contestTaskPair.update({ where: { contestId_taskId: { contestId, taskId, }, }, data: { taskTableIndex, }, }); console.log('Updated ContestTaskPair:', updatedContestTaskPair); } catch (error) { + if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === 'P2025') { + console.error(`Not found ContestTaskPair: contestId=${contestId}, taskId=${taskId}`); + } console.error('Failed to update ContestTaskPair:', error); throw error; } }Handle the
P2025error in the catch block instead.
47-122: Consider returning created/updated records from mutations.Both
createContestTaskPairandupdateContestTaskPaircurrently returnvoid, preventing callers from accessing the created or updated record. Consider changing the return type toPromise<ContestTaskPair>for better developer ergonomics.This would allow callers to:
- Confirm creation/update success
- Access auto-generated fields (id, timestamps)
- Use the returned record without making an additional fetch
Example for
createContestTaskPair:export async function createContestTaskPair( contestId: string, taskTableIndex: string, taskId: string, -): Promise<void> { +): Promise<ContestTaskPair> { try { const contestTaskPair = await db.contestTaskPair.create({ data: { contestId, taskTableIndex, taskId, }, }); console.log('Created ContestTaskPair:', contestTaskPair); + return contestTaskPair; } catch (error) { if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === 'P2002') { console.log(`ContestTaskPair already exists: contestId=${contestId}, taskId=${taskId}`); - return; + // Fetch and return existing record + const existing = await getContestTaskPair(contestId, taskId); + if (!existing) throw new Error('Unexpected: record exists but cannot be fetched'); + return existing; } console.error('Failed to create ContestTaskPair:', error); throw error; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md(1 hunks)src/lib/services/contest_task_pairs.ts(1 hunks)src/lib/types/contest_task_pair.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: preview
🔇 Additional comments (3)
src/lib/types/contest_task_pair.ts (1)
1-8: LGTM! Clean type aliases.The import structure and basic type aliases are well-organized and leverage Prisma's generated types appropriately.
src/lib/services/contest_task_pairs.ts (1)
10-12: LGTM! Simple and correct retrieval.The implementation correctly fetches all ContestTaskPair records. Consider adding pagination support in the future if the dataset grows large.
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md (1)
1-244: Planning document aligns with implementation.The planning document provides comprehensive context for the ContestTaskPair feature implementation. The actual implementation in
src/lib/types/contest_task_pair.tsandsrc/lib/services/contest_task_pairs.tsfollows the outlined structure.Note that past review comments flagged issues in code snippets within this document (around lines 92-113 and elsewhere), and those have been addressed in commits 930f11c to 26676ff per the existing comments. The live implementation files are the authoritative source and take precedence over any draft code in this planning document.
The document serves its purpose as design documentation and historical context for the feature.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md (1)
49-161: Update code examples to align with lessons learned.The CRUD implementation examples show:
createContestTaskPairandupdateContestTaskPairreturningPromise<void>(lines 91, 131)- Pre-checking for existing records before creating/updating (lines 94-99, 134-140)
However, your "教訓と一般化" section (lines 246-253) recommends the opposite:
- Returning created/updated records to avoid additional queries
- Avoiding pre-checks to prevent TOCTOU issues
The actual implementation in
src/lib/services/contest_task_pairs.tscorrectly follows the lessons learned, but readers of this plan document might be confused by the contradiction.Consider updating the code examples to reflect the final approach or adding a note that these are initial examples superseded by the lessons learned section.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md(1 hunks)src/lib/services/contest_task_pairs.ts(1 hunks)src/lib/types/contest_task_pair.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/types/contest_task_pair.ts
🔇 Additional comments (6)
src/lib/services/contest_task_pairs.ts (4)
12-14: LGTM!Simple and correct implementation for fetching all ContestTaskPair records.
24-36: LGTM!Correctly uses the composite unique key
contestId_taskIdto retrieve a single record.
49-81: Solid race-condition handling.The implementation correctly avoids TOCTOU issues by attempting the create operation directly and catching P2002 errors. The fallback fetch (lines 69-73) with the null check provides good defensive programming against edge cases where the record might have been deleted between the error and the fetch.
94-125: LGTM!The update implementation correctly handles the P2025 error case when the record doesn't exist, providing a clear error message.
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md (2)
217-238: LGTM!The execution checklist and decisions are clear and well-organized, providing good guidance for the implementation phases.
241-264: Excellent lessons learned section.The generalized lessons provide valuable guidance, especially:
- Lesson 1: Avoiding TOCTOU with Prisma error handling
- Lesson 2: Returning records to reduce queries
- Lesson 4: Leveraging Prisma error codes for specific handling
These insights will be valuable for future development.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: preview
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: preview
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md (7)
18-29: Prisma schema: consider Int for taskTableIndex, add FKs, and add per‑contest index/uniqueness.
- taskTableIndex looks positional; Int is a better fit.
- Add foreign keys to enforce referential integrity.
- If each contest’s task index is unique, add a unique on (contestId, taskTableIndex). Also consider an index on taskId for reverse lookups.
model ContestTaskPair { - id String @id @default(uuid()) - contestId String - taskTableIndex String - taskId String + id String @id @default(uuid()) + contestId String + taskTableIndex Int + taskId String + // Relations (adjust model/field names to your schema) + contest Contest @relation(fields: [contestId], references: [id], onDelete: Cascade) + task Task @relation(fields: [taskId], references: [id], onDelete: Cascade) createdAt DateTime @default(now()) updatedAt DateTime @updatedAt @@unique([contestId, taskId]) // ペアの重複を防止 + @@unique([contestId, taskTableIndex]) // 同一コンテスト内での並び重複を防止（必要なら） @@index([contestId]) // contestId 検索を高速化 + @@index([taskId]) // taskId からの逆引きを高速化（必要なら） @@map("contesttaskpair") }
53-58: Imported Create/Update types are not used in function signatures.Either use the parameter types in the service functions or remove these imports to avoid drift between docs and code.
82-84: Nit: avoid unnecessaryawaiton return.
return db.contestTaskPair.findMany();is equivalent and slightly cleaner.-export async function getContestTaskPairs(): Promise<ContestTaskPairs> { - return await db.contestTaskPair.findMany(); -} +export async function getContestTaskPairs(): Promise<ContestTaskPairs> { + return db.contestTaskPair.findMany(); +}
89-112: Create: prefer upsert/guard + trim/validate inputs for robustness.
- Use
upsertfor idempotent creation under the composite unique constraint.- Trim and validate inputs; fail fast with clear errors.
export async function createContestTaskPair( contestId: string, - taskTableIndex: string, + taskTableIndex: number, taskId: string, ): Promise<ContestTaskPair> { try { - return await db.contestTaskPair.create({ - data: { - contestId, - taskTableIndex, - taskId, - }, - }); + const cId = contestId.trim(); + const tId = taskId.trim(); + if (!cId) throw new Error('contestId must be non-empty'); + if (!tId) throw new Error('taskId must be non-empty'); + if (!Number.isInteger(taskTableIndex) || taskTableIndex < 0) { + throw new Error('taskTableIndex must be a non-negative integer'); + } + return await db.contestTaskPair.upsert({ + where: { contestId_taskId: { contestId: cId, taskId: tId } }, + create: { contestId: cId, taskTableIndex, taskId: tId }, + update: {}, // idempotent + }); } catch (error) { if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === 'P2002') { const errorMessage = `ContestTaskPair already exists: contestId=${contestId}, taskId=${taskId}`; console.error(errorMessage); throw new Error(errorMessage); } console.error('Failed to create ContestTaskPair:', error); throw error; } }
117-144: Update: validate inputs; consider no‑op updates and clearer where clause type.
- Trim and validate identifiers; validate taskTableIndex if changed.
- If no actual change, allow a no‑op or early return to avoid P2025 surprises in some flows.
export async function updateContestTaskPair( contestId: string, - taskTableIndex: string, + taskTableIndex: number, taskId: string, ): Promise<ContestTaskPair> { try { - return await db.contestTaskPair.update({ + const cId = contestId.trim(); + const tId = taskId.trim(); + if (!cId) throw new Error('contestId must be non-empty'); + if (!tId) throw new Error('taskId must be non-empty'); + if (!Number.isInteger(taskTableIndex) || taskTableIndex < 0) { + throw new Error('taskTableIndex must be a non-negative integer'); + } + return await db.contestTaskPair.update({ where: { - contestId_taskId: { - contestId, - taskId, - }, + contestId_taskId: { contestId: cId, taskId: tId }, }, data: { taskTableIndex, }, }); } catch (error) { if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === 'P2025') { const errorMessage = `Not found ContestTaskPair: contestId=${contestId}, taskId=${taskId}`; console.error(errorMessage); throw new Error(errorMessage); } console.error('Failed to update ContestTaskPair:', error); throw error; } }
168-178: Type shapes: make Update partial; align Create with inferred Prisma args.Update typically allows changing only mutable fields; keep keys separate. If you change taskTableIndex to Int, reflect that here.
-export type ContestTaskPairCreate = { - contestId: string; - taskTableIndex: string; - taskId: string; -}; +export type ContestTaskPairCreate = { + contestId: string; + taskTableIndex: number; + taskId: string; +}; export type ContestTaskPairRead = ContestTaskPairCreate; -export type ContestTaskPairUpdate = ContestTaskPairCreate; +export type ContestTaskPairUpdate = { + contestId: string; + taskId: string; + taskTableIndex?: number; +};
183-193: Key helper: add validation and delimiter safety.Ensure non-empty segments and prevent embedded colons from corrupting the key. Optionally encode segments.
-type ContestTaskPairKey = `${string}:${string}`; // "contest_id:task_id" +type ContestTaskPairKey = `${string}:${string}`; // "contest_id:task_id" // ヘルパー関数 -function createContestTaskPairKey(contestId: string, taskId: string): ContestTaskPairKey { - return `${contestId}:${taskId}`; -} +function createContestTaskPairKey(contestId: string, taskId: string): ContestTaskPairKey { + const c = contestId?.trim(); + const t = taskId?.trim(); + if (!c) throw new Error('contestId must be a non-empty string'); + if (!t) throw new Error('taskId must be a non-empty string'); + if (c.includes(':') || t.includes(':')) { + throw new Error('IDs must not contain ":"'); + } + return `${c}:${t}`; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: preview
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md (8)
26-29: Add an index on taskId for reverse lookups.Frequent queries by taskId (e.g., "which contests include this task?") will benefit from an index.
model ContestTaskPair { @@ @@unique([contestId, taskId]) // ペアの重複を防止 @@index([contestId]) // contestId 検索を高速化 + @@index([taskId]) // taskId からの逆引きを高速化 @@map("contesttaskpair") }
82-84: Allow optional filtering by contestId (scales better).Returning all rows can be heavy; support an optional filter now to avoid future churn.
-export async function getContestTaskPairs(): Promise<ContestTaskPairs> { - return await db.contestTaskPair.findMany(); -} +export async function getContestTaskPairs(contestId?: string): Promise<ContestTaskPairs> { + return await db.contestTaskPair.findMany({ + where: contestId ? { contestId } : undefined, + }); +}
89-93: Align function args with declared DTO types (or drop the unused imports).You import ContestTaskPairCreate/Update but don’t use them. Prefer object DTOs for clarity and future extensibility.
-export async function createContestTaskPair( - contestId: string, - taskTableIndex: string, - taskId: string, -): Promise<ContestTaskPair> { +export async function createContestTaskPair( + params: ContestTaskPairCreate, +): Promise<ContestTaskPair> { try { return await db.contestTaskPair.create({ data: { - contestId, - taskTableIndex, - taskId, + contestId: params.contestId, + taskTableIndex: params.taskTableIndex, + taskId: params.taskId, }, });
117-121: Same: use DTO for update signature.-export async function updateContestTaskPair( - contestId: string, - taskTableIndex: string, - taskId: string, -): Promise<ContestTaskPair> { +export async function updateContestTaskPair( + params: ContestTaskPairUpdate, +): Promise<ContestTaskPair> { try { return await db.contestTaskPair.update({ where: { contestId_taskId: { - contestId, - taskId, + contestId: params.contestId, + taskId: params.taskId, }, }, data: { - taskTableIndex, + taskTableIndex: params.taskTableIndex, }, });
103-107: Preserve original error as cause when rethrowing (diagnostics).- throw new Error(errorMessage); + throw new Error(errorMessage, { cause: error as Error });
135-139: Same for P2025 branch: keep the original error as cause.- throw new Error(errorMessage); + throw new Error(errorMessage, { cause: error as Error });
169-176: Refine Read type to just the lookup keys.Read shouldn’t require taskTableIndex. Keeps call sites clean.
-export type ContestTaskPairRead = ContestTaskPairCreate; +export type ContestTaskPairRead = { + contestId: string; + taskId: string; +};
183-193: Export mapping types/helpers and validate inputs; clarify TaskResult import.Current snippet won’t compile as-is (TaskResult undefined) and types aren’t exported.
-// 型定義 -type ContestTaskPairKey = `${string}:${string}`; // "contest_id:task_id" +// 型定義 +export type ContestTaskPairKey = `${string}:${string}`; // "contest_id:task_id" -// ヘルパー関数 -function createContestTaskPairKey(contestId: string, taskId: string): ContestTaskPairKey { - return `${contestId}:${taskId}`; -} +// ヘルパー関数 +export function createContestTaskPairKey(contestId: string, taskId: string): ContestTaskPairKey { + if (!contestId?.trim()) throw new Error('contestId must be a non-empty string'); + if (!taskId?.trim()) throw new Error('taskId must be a non-empty string'); + return `${contestId}:${taskId}`; +} // マップの型（明示的） -type TaskResultMapByContestTaskPair = Map<ContestTaskPairKey, TaskResult>; +// import type { TaskResult } from '$lib/types/task_result'; // TODO: 正しいパスに置換 +export type TaskResultMapByContestTaskPair = Map<ContestTaskPairKey, TaskResult>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/services/contest_task_pairs.ts (1)
17-19: Consider pagination for future scalability.The current implementation fetches all ContestTaskPair records without pagination. While this works fine initially, it may become a performance concern as the dataset grows.
For future consideration, you could add optional pagination parameters:
export async function getContestTaskPairs( skip?: number, take?: number ): Promise<ContestTaskPairs> { return await db.contestTaskPair.findMany({ skip, take, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md(1 hunks)src/lib/services/contest_task_pairs.ts(1 hunks)src/lib/types/contest_task_pair.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/types/contest_task_pair.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: preview
🔇 Additional comments (7)
src/lib/services/contest_task_pairs.ts (3)
29-41: LGTM! Clean composite key lookup.The function correctly uses the composite unique constraint
contestId_taskIdand properly handles the null case. The implementation is straightforward and type-safe.
52-84: LGTM! Robust error handling for race conditions.The P2002 handling correctly manages concurrent creation attempts by fetching and returning the existing record. The defensive check at lines 74-76 appropriately guards against edge cases where the record exists but can't be retrieved. This implementation aligns well with the PR's goal of avoiding TOCTOU issues through Prisma's error handling.
95-126: LGTM! Proper error handling for missing records.The function correctly handles the P2025 error (record not found) by throwing a descriptive error with the original error as cause. This provides good debugging context while maintaining a clear error message for callers.
docs/dev-notes/2025-09-23/contest-task-pair-mapping/plan.md (4)
13-44: LGTM! Well-designed schema with clear rationale.The Prisma schema is thoughtfully designed:
- The composite unique constraint
@@unique([contestId, taskId])correctly prevents duplicate pairs- The
@@index([contestId])optimizes contest-based queries- The
idfield provides future extensibility as noted in the design rationaleThe schema aligns perfectly with the PR objectives.
46-158: LGTM! Plan examples match actual implementation.The CRUD examples in this planning document align perfectly with the actual implementation in
src/lib/services/contest_task_pairs.ts. The past review concerns (missing Prisma import, incorrect error handling) have been properly addressed. The code demonstrates good practices:
- DRY principle with shared
getContestTaskPair()- Prisma error handling for P2002 and P2025
- TOCTOU-safe approach relying on database constraints
168-215: LGTM! Clear and comprehensive type definitions.The type definitions demonstrate good TypeScript practices:
- Reusing Prisma-generated types ensures type safety and consistency
- Separate CRUD parameter types (
Create,Read,Update) provide clear contracts- The
ContestTaskPairKeytemplate literal type with helper function offers type-safe key generationTaskResultMapByContestTaskPairexplicitly documents the mapping relationship
217-264: LGTM! Comprehensive planning with clear next steps.The execution plan clearly documents:
- Completed items (schema, migration, CRUD, types) matching the PR scope
- Design decisions aligned with PR objectives (naming, adapter location, feature flags, etc.)
- Future tasks appropriately deferred to later phases (migration, adapter implementation, data seeding)
The lessons learned section provides valuable guidance for future development, emphasizing Prisma error handling patterns and type safety practices.
1 participant
WHAT
close #2627
Summary by CodeRabbit
New Features
Documentation
Chores
今後の課題
src/lib/utils/task_results.ts（影響局所化・テスト容易性・削除容易性）test+expect to+ TypeScript strict + assert追加