✨ Add workbook and tasks to seed (#1124) #2180
Conversation
|
Warning Rate limit exceeded@KATO-Hiro has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughThe seeding script was refactored to improve concurrency control and error handling by making all seeding functions async and using PQueue for controlled parallelism. Workbook seeding was added with validation and duplicate checks. A new module provides comprehensive workbook data. Logging was enhanced throughout the seeding process. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main()
participant DB as Database (Prisma)
participant Workbooks as workbooks.ts
Main->>DB: addUsers()
Main->>DB: addTasks()
Main->>DB: addTags()
Main->>DB: addTaskTags()
Main->>DB: addSubmissionStatuses()
Main->>DB: addAnswers()
Main->>Workbooks: import workbooks
Main->>DB: addWorkBooks(workbooks)
DB-->>Main: Logging and error reporting for each step
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
prisma/seed.ts (2)
41-51: Sequential seeding order is clear but not crash-proofIf
addTasks()(≈5 k records) throws, subsequent stages never run and the DB is left half-seeded. Consider wrapping each stage in its own try/catch and continuing, or running the whole pipeline inside a single SQL transaction that can be rolled back atomically.No code change required right now, but worth revisiting.
205-220: Foreign-key risk: workbook tasks are inserted blindly
workBookFactory.create({ workBookTasks: { create: workbook.workBookTasks } })assumes everytaskIdexists.
If a task is missing, the whole workbook creation fails and the catch just logs the error, silently skipping the workbook.Mitigation options:
- Validate task existence beforehand and filter out missing ones.
- Wrap the nested
createinconnectOrCreatewith graceful fallback.- Abort early with an explicit error message listing the missing taskIds.
for (const { taskId } of workbook.workBookTasks) { const exists = await prisma.task.count({ where: { task_id: taskId } }); if (!exists) missing.push(taskId); } if (missing.length) { console.warn('Skip workbook', workbook.title, 'missing tasks:', missing); continue; }
📜 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 (2)
prisma/seed.ts(8 hunks)prisma/workbooks.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
prisma/workbooks.ts (1)
src/lib/types/workbook.ts (2)
WorkBook(15-18)WorkBookTasksBase(56-56)
prisma/seed.ts (2)
prisma/tasks.ts (1)
tasks(3-4831)prisma/workbooks.ts (1)
workbooks(24-624)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (1)
prisma/workbooks.ts (1)
3-18:Details
❌ Incorrect review comment
idis hard-coded to100000– very likely to break the PK constraint on insertion
createWorkBookBase()always returns{ id: 100000, ... }.
BecauseaddWorkBook()never uses/overrides this field, Prisma will still attempt to insert that literal value and the second workbook will violate the unique constraint onworkBook.id.return { - id: 100000, + // Let Prisma auto-increment unless an explicit id is really needed + // id is omitted on purpose. Specify through overrides only when required.
🏁 Script executed:
#!/bin/bash # Search for createWorkBookBase usage and addWorkBook references rg -n 'createWorkBookBase' . rg -n 'addWorkBook' .Length of output: 5959
🏁 Script executed:
#!/bin/bash # Display the implementation of addWorkBook for inspection rg -n -B3 -A15 'async function addWorkBook' prisma/seed.tsLength of output: 1346
Ignore hard-coded
idincreateWorkBookBase– it isn’t used on insert
The seeding logic inprisma/seed.tsbuilds its create call manually (viaworkBookFactory.create) and does not include theidfield, so Prisma’s auto-increment will apply. Theid: 100000increateWorkBookBasenever makes it into the INSERT and won’t cause a PK collision.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
prisma/seed.ts (3)
46-56: Redundant master PQueue
mainQueueis created withconcurrency: 1, yet each step is awaited immediately.
The queue adds no ordering or throttling benefits and just adds overhead.-const mainQueue = new PQueue({ concurrency: 1 }); -await mainQueue.add(() => addUsers()); -… +await addUsers(); +…(Or drop the queue but keep the
try/catchif you need the grouping.)
70-95:Promise.all+ PQueue = needless duplicationAfter enqueuing every user you still collect all returned promises and
await Promise.all(userPromises).
queue.onIdle()is simpler, avoids creating an array the size ofusers, and
keeps the pattern consistent across the file.-const userPromises = users.map((u) => userQueue.add(() => …)); -await Promise.all(userPromises); +for (const u of users) userQueue.add(() => …); +await userQueue.onIdle();(This comment applies to every similar block: tasks, tags, task-tags, submission
statuses, answers.)
196-215: Duplicate check still case-sensitiveWhen
urlSlugis empty you fall back totitle+authorId, but the title
comparison is case-sensitive. Two workbooks whose titles differ only in case
(e.g. “DP-Workbook” vs “dp-workbook”) will bypass the duplicate guard.Consider normalising titles (lower-case + trim) before querying.
📜 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 ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.json(1 hunks)prisma/seed.ts(8 hunks)prisma/workbooks.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- package.json
- prisma/workbooks.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: preview
🔇 Additional comments (2)
prisma/seed.ts (2)
264-280: 👍normalizeUrlSluglooks goodThe helper cleanly handles
'' | null | undefinedand aligns the duplicate
check with the create call.
455-492: FK integrity checks missing for Task & Status
addAnswersguards against missing users but not against missing tasks or
submission statuses. A stale ID will maketaskAnswerFactory.createfail and
abort the worker.Add similar existence checks (or surround the create with
try/catchand log)
to keep seeding resilient.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
prisma/seed.ts (1)
325-333:⚠️ Potential issueIncorrect field when linking a
Taskfrom insideaddTagHere you attempt to connect a
Taskviatag_id, which does not exist on the
Taskmodel. It should connect viatask_id(or another real PK).- connect: { tag_id: tag.id }, + connect: { task_id: tag.id },Without this fix the seed will throw a Prisma validation error at runtime.
♻️ Duplicate comments (1)
prisma/seed.ts (1)
160-167:⚠️ Potential issue
tagconnection still targetstask_idinstead ofidPrevious feedback pointed out the mismatch; the code now does
connect: { task_id: task.id }, but thetagmodel’s primary key isid.
Useconnect: { id: /* tag id */ }to avoid relation errors.- connect: { task_id: task.id }, + connect: { id: task.id },
🧹 Nitpick comments (3)
prisma/seed.ts (3)
46-55: Sequentialawaitnegates the purpose ofmainQueue
mainQueueis configured withconcurrency: 1, yet everyadd()call is awaited immediately.
The result is strictly serial execution with the extra overhead of the queue. Either:
- Keep the queue but enqueue all jobs at once and
await mainQueue.onIdle(), or- Drop the queue and call the seeding functions directly in the desired order.
Both variants achieve the same ordering with less noise.
58-60: Minor wording issue in error log
console.error('Failed to seeding:', e);should read “Failed to seed” or “Seeding failed” for clarity.- console.error('Failed to seeding:', e); + console.error('Failed to seed:', e);
76-77: Hard-coded password will leak into every seeded userAll users receive the same literal password
'Ch0kuda1'.
Even in non-production environments this is brittle and encourages unsafe habits.Consider moving the seed password to an env variable or deriving it from the username (e.g.
"password-" + user.name) so it’s obvious it is dummy data.
📜 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)
prisma/seed.ts(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: preview
🔇 Additional comments (1)
prisma/seed.ts (1)
264-280: Slug normalisation looks solid
normalizeUrlSlugcorrectly converts empty strings andnulltoundefined
and lower-cases meaningful slugs. 👍
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
prisma/seed.ts (4)
46-56: PQueue with concurrency = 1 adds needless complexity
mainQueueis configured withconcurrency: 1, yet every seeding step is already awaited in order.
Using the queue here adds a library hop without any functional gain and slightly obscures stack-traces.-// Create a queue to ensure sequential execution -const mainQueue = new PQueue({ concurrency: 1 }); - -await mainQueue.add(() => addUsers()); -… -await mainQueue.add(() => addAnswers()); +await addUsers(); +await addTasks(); +await addWorkBooks(); +await addTags(); +await addTaskTags(); +await addSubmissionStatuses(); +await addAnswers();Simpler, clearer, and one fewer dependency instance.
70-92: AvoidArray.mapfor side-effects – usefor…oforqueue.addAll
users.mapis only used to enqueue tasks; the produced array is discarded.
for…of(oruserQueue.addAll) signals intent, avoids an intermediate array and scales better for very large datasets.-users.map((user) => - userQueue.add(async () => { +for (const user of users) { + userQueue.add(async () => { … - ), -); - -await userQueue.onIdle(); + }); +} + +await userQueue.onIdle();
172-183: UsefindUnique/findFirstfor single-row checksDuplicate detection issues a
findMany, loads all matches and allocates an array just to testlength.
A single‐row query is cheaper and expresses intent more clearly:-registeredWorkBook = await prisma.workBook.findMany({ - where: { urlSlug: normalizedUrlSlug }, -}); +registeredWorkBook = await prisma.workBook.findFirst({ + where: { urlSlug: normalizedUrlSlug }, +});(Adjust the later
lengthcheck accordingly.)
254-256:toLocaleLowerCaserisks locale surprises – prefertoLowerCaseSlug normalisation should be deterministic regardless of the server’s locale.
toLocaleLowerCase()can change behaviour (e.g., Turkish “İ”).
UsingtoLowerCase()avoids this.-return urlSlug && urlSlug !== '' ? urlSlug.toLocaleLowerCase() : undefined; +return urlSlug && urlSlug !== '' ? urlSlug.toLowerCase() : undefined;
📜 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)
prisma/seed.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
prisma/seed.ts (2)
prisma/tasks.ts (1)
tasks(3-4831)src/lib/utils/contest.ts (1)
classifyContest(5-93)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: preview
🔇 Additional comments (1)
prisma/seed.ts (1)
148-154:contest_typemay getnull– verify schema allows it
classifyContest()returnsnullfor unknown patterns; that value is written tocontest_type.If the
Taskmodel declarescontest_typeas a non-nullable enum, this will throw at seed time.
Please confirm the Prisma schema or add a fallback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
prisma/seed.ts (4)
67-71: Extract concurrency settings into a single constant / env var
new PQueue({ concurrency: 2 })(and 3, 2, … further down) is hard-coded multiple times. Centralising the value(s) makes tuning for different DB sizes trivial and avoids magic numbers scattered through the file.
A tiny helper such asconst CONCURRENCY = Number(process.env.SEED_CONCURRENCY ?? 3);would do.
155-214: Consider parallelising workbook seeding too
addWorkBooksprocesses items sequentially with plainfor … of, unlike the other seeders that use a queue.
If the workbook list grows, this will be the new bottleneck. Re-usingPQueuehere (or at least batching) keeps throughput consistent.
173-179: UsefindUniquefor slug look-ups
urlSlugis presumably declared@uniquein the Prisma schema.findUnique({ where: { urlSlug } })is cheaper and conveys intent better thanfindFirst.
(Same for the title + author composite if there’s a corresponding unique constraint.)
251-253: Trim whitespace when normalising slugsEdge cases like
" union-find "pass the non-empty check yet keep stray spaces.-return urlSlug && urlSlug !== '' ? urlSlug.toLowerCase() : undefined; +return urlSlug && urlSlug.trim() !== '' + ? urlSlug.trim().toLowerCase() + : undefined;
📜 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)
prisma/seed.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: preview
🔇 Additional comments (1)
prisma/seed.ts (1)
169-189: 👍 Duplicate-slug check now normalises before lookupThe earlier bug allowing multiple
NULL/empty slugs is resolved by callingnormalizeUrlSlugahead of the query. Looks good!
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
prisma/seed.ts (1)
96-110: 🛠️ Refactor suggestionMissing parameter types violate
noImplicitAnyFunctions like
addUser,addTask,addWorkBook, … declare parameters without types (user,task,workbook).
WithnoImplicitAnyenabled this is a compile-error; without it you still lose all type safety & IntelliSense.Define explicit interfaces or import the DTO types used in your fixtures:
-async function addUser(user, password: string, userFactory, keyFactory) { +async function addUser( + user: SeedUser, + password: string, + userFactory: ReturnType<typeof defineUserFactory>, + keyFactory: ReturnType<typeof defineKeyFactory> +) {Repeat for the other helpers.
🧹 Nitpick comments (1)
prisma/seed.ts (1)
154-214: Workbook seeding is strictly sequential – consider PQueue for parityAll other entities enjoy controlled parallelism, but workbooks are processed one-by-one.
If the dataset grows (e.g. hundreds of workbooks) this becomes a bottleneck.Re-use the same queued pattern:
const wbQueue = new PQueue({ concurrency: 2 }); for (const wb of workbooks) { wbQueue.add(() => processWorkBook(wb)); } await wbQueue.onIdle();(Where
processWorkBookcontains the current try/catch logic.)
📜 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)
prisma/seed.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: preview
🔇 Additional comments (2)
prisma/seed.ts (2)
251-253:normalizeUrlSluglower-cases the slug – ensure DB index is case-insensitiveYou normalise to
.toLowerCase()beforefindUnique, but the column might be case-sensitive (depending on collation).
Double-check that the DB uses a case-insensitive unique index or store the slug exactly as normalised to avoid “phantom duplicates”.
70-88:Details
❌ Incorrect review comment
Duplicate check + concurrent inserts → race condition
findUniqueis executed before the create runs in another queue worker.
Withconcurrency > 1, two workers can both observe “not found” and then attemptcreate, racing into aP2002(unique-constraint) error.Mitigations:
- Reduce the queue to
concurrency: 1for entities whose uniqueness you probe first, or- Skip the pre-check and catch the
P2002error:try { await userFactory.create(/* … */); } catch (e) { if (e instanceof Prisma.PrismaClientKnownRequestError && e.code === 'P2002') { // duplicate -> ignore } else { throw e; } }
🏁 Script executed:
#!/bin/bash set -e # Find where userQueue is defined or configured rg "userQueue" -n -C3 || echo "No direct references to userQueue found." # Check for queue instantiation to see default concurrency rg "new .*Queue" -n -C3 || echo "No queue instantiation patterns found."Length of output: 3125
🏁 Script executed:
#!/bin/bash set -e # Show import section and initial factory definitions in prisma/seed.ts sed -n '1,80p' prisma/seed.ts # Locate defineUserFactory in seed.ts rg "defineUserFactory" -n -C3 prisma/seed.ts || echo "defineUserFactory not in seed.ts" # Search for addUser definition or import across the repo rg "addUser" -n -C5 .Length of output: 7300
No inherent race on distinct users—concurrency = 2 is safe
Theusersarray contains unique entries and each queue task handles a differentusername. Since you callawait userQueue.onIdle()before moving on, there’s no scenario where two workers race to insert the same user. You can safely keepconcurrency: 2.If you still want to simplify or guard against accidental duplicates, consider one of the following instead of a pre-check:
- Use a single atomic upsert:
- const registeredUser = await prisma.user.findUnique({ - where: { username: user.name }, - }); - if (!registeredUser) { - await addUser(user, password, userFactory, keyFactory); - } + await prisma.user.upsert({ + where: { username: user.name }, + create: { id: user.id, username: user.name, role: user.role /* … */ }, + update: {}, // no-op if already exists + });
- Or catch the unique-constraint error on create:
try { await userFactory.create({ /* … */ }); } catch (e) { if ( e instanceof Prisma.PrismaClientKnownRequestError && e.code === 'P2002' ) { // already exists, ignore } else { throw e; } }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
prisma/seed.ts (1)
25-25:⚠️ Potential issueSingle shared password still present – risk of credential reuse
USER_PASSWORD_FOR_SEEDis injected from./users, but the variable name strongly hints it is still a fixed constant. If the same value is written to every user, the original leakage problem remains.Move the value to an environment variable (or generate a per-user random password) to avoid shipping a credential in source control.
🧹 Nitpick comments (3)
prisma/seed.ts (3)
67-69: Factor queue concurrency into a central constantConcurrency levels (
2,3, …) are hard-coded here and in every other queue.
Extract them into a smallconst QUEUE_CONCURRENCY = { users: 2, tasks: 3, … }at the top of the file (or a config file) so that tuning only requires a one-line change.- const userQueue = new PQueue({ concurrency: 2 }); + const userQueue = new PQueue({ concurrency: QUEUE_CONCURRENCY.users });
156-158:userFactoryis declared but never used
const userFactory = defineUserFactory();is created only to be ignored.
Drop the variable or inline the call to keep the function concise.- const userFactory = defineUserFactory(); - const workBookFactory = defineWorkBookFactory({ defaultData: { user: userFactory } }); + const workBookFactory = defineWorkBookFactory({ + defaultData: { user: defineUserFactory() }, + });
311-327: Run the three independent look-ups in parallel
taskTag,task, andtagqueries are unrelated; execute them throughPromise.allto shave network latency without increasing outer concurrency.- const registeredTaskTag = await prisma.taskTag.findUnique({ ... }); - const registeredTask = await prisma.task.findUnique({ ... }); - const registeredTag = await prisma.tag.findUnique({ ... }); + const [registeredTaskTag, registeredTask, registeredTag] = await Promise.all([ + prisma.taskTag.findUnique({ ... }), + prisma.task.findUnique({ ... }), + prisma.tag.findUnique({ ... }), + ]);
📜 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 (2)
prisma/seed.ts(6 hunks)prisma/users.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- prisma/users.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
|
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. @KATO-Hiro Thank you for requesting the review! I'll provide a comprehensive analysis of your workbook seeding implementation. 🌟 Overall AssessmentThis is an excellent refactoring that significantly improves the seeding infrastructure. The migration to async functions with concurrency control and the addition of workbook seeding represents a substantial enhancement. ✅ Strengths1. Robust Concurrency Management
2. Comprehensive Workbook Validation
3. Enhanced Error Handling & Logging
🔍 Observations & Suggestions1. Database Transaction Considerations // Consider wrapping large seeding operations in transactions
async function addWorkBooks() {
// Current implementation processes workbooks individually
// For better consistency, consider batching in transactions
}2. Memory Efficiency for Large Datasets // Potential improvement for scalability
const BATCH_SIZE = 50;
for (let i = 0; i < workbooks.length; i += BATCH_SIZE) {
const batch = workbooks.slice(i, i + BATCH_SIZE);
await processBatch(batch);
}3. URL Slug Validation function normalizeUrlSlug(urlSlug: string | null | undefined): string | undefined {
if (!urlSlug?.trim()) return undefined;
const normalized = urlSlug.toLowerCase().trim();
// Consider adding format validation (e.g., slug pattern)
return normalized;
}📋 Minor Recommendations1. Configuration Externalization const SEED_CONFIG = {
usersConcurrency: 3,
tasksConcurrency: 2,
workbooksConcurrency: 2
} as const;2. Type Safety Enhancement 3. Performance Monitoring console.log(`✅ Finished adding ${workbooks.length} workbooks in ${Date.now() - startTime}ms`);🎯 Workbook Data QualityThe workbook dataset is impressive! The coverage spans from basic I/O to advanced algorithms (Union-Find, ordered sets, etc.). The task organization with priorities and editorial URLs provides excellent learning progression. 🚀 Ready for ProductionThis implementation is well-architected and production-ready. The combination of:
Makes this a solid foundation for the seeding infrastructure. Great work on this substantial improvement to the codebase! The workbook seeding functionality will be a valuable addition to the platform. |
close #1124
Summary by CodeRabbit