Conversation
- Split AojApiClient into AojCoursesApiClient and AojChallengesApiClient - Reorganize AOJ fixtures into source-specific directories (courses/challenges) - Add ContestTaskFetcher to unify per-source fetch logic - Replace TaskListForEdit with TaskSearchBox and TaskTableForImport components - Update task import page to support source selection and pagination Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n vite config Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…PaginationNav for numbered pagination
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rder 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>
…ementation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… flow - Add trim() to contest/task titles from AtCoder and AOJ API clients - Return fail() with proper HTTP status in import and update actions - Log caught errors instead of silently swallowing them - Surface import failure to user via error message in TaskTableForImport Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Delete task-import-by-source review.md (findings addressed in prior commit) - Expand inline object literals in atcoder_problems.test.ts for readability Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAOJクライアント実装を基底クラス中心の設計から、汎用キャッシュ取得ユーティリティベースの設計に移行。クライアント取得元を ChangesAOJ Client Refactoring & Import Flow
Sequence Diagram(s)以下の条件を確認：
sequenceDiagram
participant Admin as Admin User
participant Page as +page.svelte
participant Server as +page.server.ts
participant Fetcher as fetchContests/Tasks
participant HttpClient as HttpRequestClient
participant DB as Database
Admin->>Page: source選択・Fetch実行
Page->>Server: ?/fetch (source)
Server->>Fetcher: fetchContests(source)
Server->>HttpClient: API取得
HttpClient->>Fetcher: レスポンス
Server->>DB: getTasks()
DB->>Server: 既存タスク一覧
Fetcher->>Server: 外部コンテスト/タスク
Server->>Page: importContests (未登録含む)
Page->>Page: 検索フィルタ・ページング表示
Admin->>Page: タスク選択・インポート
Page->>Server: ?/create (tasks JSON)
Server->>DB: createTask (各タスク)
DB->>Page: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/clients/fixtures/record_requests.ts`:
- Around line 53-60: The fixtures are sampled non-deterministically and can
produce tasks that reference contests not present in the sampled contests;
change the sampling to deterministic (e.g., use a seeded RNG or sort+slice) and
ensure referential integrity by filtering the sampled tasks to only include
tasks whose contest_id exists in the sampled contests before writing; update the
calls around getRandomElementsFromArray(contests, 100) and
getRandomElementsFromArray(tasks, 100) (and the other similar occurrences noted)
so you first deterministically select contests into a variable (e.g.,
sampledContests), then deterministically select or sort tasks and filter them
with sampledContests' IDs (e.g., sampledTasks = tasks.filter(t =>
sampledContestsIds.has(t.contest_id)) then slice to desired count) and pass
those filtered arrays to toJson so the output is reproducible and consistent.
In `@src/routes/`(admin)/tasks/_utils/contests.ts:
- Around line 4-6: Treat strings that are only whitespace as empty: change the
check and normalization to use trim(). Replace the current hasNoQuery and
lowerQuery logic so hasNoQuery becomes "!query || query.trim() === ''" and
lowerQuery is computed from "query.trim().toLowerCase()" (ensuring you guard for
undefined/null query before calling trim()). Update the assignments where
"hasNoQuery" and "lowerQuery" are defined (the current lines with hasNoQuery and
lowerQuery) accordingly.
In `@src/routes/`(admin)/tasks/[task_id]/+page.server.ts:
- Around line 54-80: Replace the manual formData parsing in the actions.update
handler with Superforms + Zod: create a Zod schema (e.g., TaskUpdateSchema) that
validates task_id as a nonempty string and task_grade as an optional
enum/nullable matching your TaskGrade values (or a string refined to TaskGrade),
then call superValidate(request, TaskUpdateSchema) inside update and branch on
validation result instead of ad-hoc checks; extract validated values from
result.data, map/convert task_grade to your TaskGrade type (or let Zod produce
it) and call taskService.updateTask(task_id, task_grade), and on validation
failure return the Superforms-consistent error response so errors from
actions.update follow the same Superforms + Zod pattern used elsewhere (leave
validateAdminAccess and taskService.updateTask calls intact).
- Around line 55-73: The update handler currently trusts the form field task_id
(taskId) allowing tampering; change it to use the route parameter instead: read
the task id from the handler's params (use params.task_id) and remove/ignore the
form-derived task_id, then pass that params-derived id into
taskService.updateTask; keep existing validation for task_grade (getTaskGrade)
and the bad-request/fail branches but ensure any checks reference params.task_id
(and return fail if it's missing) rather than the form value.
In `@src/routes/`(admin)/tasks/+page.server.ts:
- Around line 70-96: The handler currently trusts the client-provided tasks JSON
(tasksJson → tasks) and uses client-supplied fields (task.title,
task.contest_id, task.source) when calling taskService.createTask; instead,
validate/trust only an identifier from the client (e.g., task.id or task.source)
and re-fetch the canonical task data on the server (via your service/DB) before
creating records. Change the import loop that maps over TaskForImport so it
discards client title/contest_id and instead: lookup the authoritative task
payload using the submitted task.id/source, derive the server-side values
(server_contest_id, server_title, server_problem_index), compute the id with
sha256(contest_id + server_title) and call taskService.createTask with those
server-validated fields; ensure any missing or invalid lookups return
BAD_REQUEST or a clear failure path.
- Around line 21-104: The handlers fetch and create currently parse and validate
request.formData() manually; replace them with Superforms + Zod: define Zod
schemas (e.g. ContestImportQuerySchema with source for fetch, and
ContestImportCreateSchema with source:string, contest_id:string, tasks:string
(or structured TasksForImport) for create), use superValidate(request, schema)
in both handlers and return the validated form (or validation errors) so
Superforms handles client errors; keep the admin check
(validateAdminAccess(locals)), then in fetch use data.source from the validated
form to call fetchContests/fetchTasks and the existing
prepareTaskMap/filterUnregisteredTasks/mergeContestsAndUnregisteredTasks flow,
and in create use data.contest_id and the parsed tasks (parse JSON after
validation or use a Zod preprocessor to parse tasks into TasksForImport) and
then run the existing sha256 + taskService.createTask loop; ensure all
validation failures return the superform validation result instead of manual
fail responses and preserve try/catch for server errors with the same
INTERNAL_SERVER_ERROR handling.
- Around line 85-94: The current ID generation uses sha256(contest_id +
task.title) which risks collisions when multiple tasks share the same title;
change the seed to use the original external identifier by hashing contest_id +
task.id instead. Locate the tasks.map block where TaskForImport is processed,
replace usage of task.title in the sha256 call with task.id so the generated id
passed into taskService.createTask is derived from contest_id + task.id.
In `@src/routes/`(admin)/tasks/+page.svelte:
- Around line 94-97: 選択している取得元を切り替えたときに古い結果が残って誤インポートを誘発しているので、onchange ハンドラ内で
currentPage = 1 に加えて importContests を空配列にリセットし、インポート失敗の表示を消すためのエラー状態（例:
importError または importErrors を使っている箇所）を null/空にクリアしてください。該当箇所は +page.svelte の
onchange コールバックなので、そのブロックに importContests = [] と importError = null
を追加して一覧とエラー状態を同時に初期化してください。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1ec287c-2d74-4277-a94d-231211adee9d
📒 Files selected for processing (30)
.claude/rules/coding-style.mdsrc/lib/clients/aizu_online_judge/clients.test.tssrc/lib/clients/aizu_online_judge/clients.tssrc/lib/clients/aizu_online_judge/utils.test.tssrc/lib/clients/aizu_online_judge/utils.tssrc/lib/clients/atcoder/atcoder_problems.test.tssrc/lib/clients/atcoder/atcoder_problems.tssrc/lib/clients/contest_task_fetcher.tssrc/lib/clients/fixtures/aizu_online_judge/challenges/jag_prelim/contests.jsonsrc/lib/clients/fixtures/aizu_online_judge/challenges/jag_regional/contests.jsonsrc/lib/clients/fixtures/aizu_online_judge/challenges/pck_final/contests.jsonsrc/lib/clients/fixtures/aizu_online_judge/challenges/pck_prelim/contests.jsonsrc/lib/clients/fixtures/aizu_online_judge/contests.jsonsrc/lib/clients/fixtures/aizu_online_judge/courses/contests.jsonsrc/lib/clients/fixtures/aizu_online_judge/courses/tasks.jsonsrc/lib/clients/fixtures/aizu_online_judge/tasks.jsonsrc/lib/clients/fixtures/atcoder_problems/contests.jsonsrc/lib/clients/fixtures/atcoder_problems/tasks.jsonsrc/lib/clients/fixtures/record_requests.tssrc/lib/clients/index.tssrc/lib/components/TaskForm.sveltesrc/lib/components/TaskListForEdit.sveltesrc/routes/(admin)/tasks/+page.server.tssrc/routes/(admin)/tasks/+page.sveltesrc/routes/(admin)/tasks/[task_id]/+page.server.tssrc/routes/(admin)/tasks/_components/TaskSearchBox.sveltesrc/routes/(admin)/tasks/_components/TaskTableForImport.sveltesrc/routes/(admin)/tasks/_utils/contests.test.tssrc/routes/(admin)/tasks/_utils/contests.tsvite.config.ts
💤 Files with no reviewable changes (3)
- src/lib/clients/fixtures/aizu_online_judge/tasks.json
- src/lib/components/TaskListForEdit.svelte
- src/lib/clients/fixtures/aizu_online_judge/contests.json
Comment on lines
+53
to
+60
| await toJson( | ||
| path.join(TEST_DATA_BASE_DIR, 'atcoder_problems', 'contests.json'), | ||
| getRandomElementsFromArray(contests, 100), | ||
| ); | ||
| await toJson( | ||
| path.join(TEST_DATA_BASE_DIR, 'atcoder_problems', 'tasks.json'), | ||
| getRandomElementsFromArray(tasks, 100), | ||
| ); |
Contributor
There was a problem hiding this comment.
fixtureサンプリングが非決定かつ参照不整合を作り得ます
現状だと実行ごとに内容が変わり、さらに atcoder_problems/tasks.json に「存在しない contest を参照する task」が混入し得ます。テスト再現性とデータ整合性に影響が大きいです。
修正案（決定的サンプリング + contest/task の整合確保）
async function saveAtCoder(): Promise<void> {
const contests = await atCoderClient.getContests();
const tasks = await atCoderClient.getTasks();
+ const sampledContests = getDeterministicElementsFromArray(contests, 100);
+ const contestIds = new Set(sampledContests.map((contest) => contest.id));
+ const linkedTasks = tasks.filter((task) => contestIds.has(task.contest_id));
+ const sampledTasks = getDeterministicElementsFromArray(linkedTasks, 100);
await toJson(
path.join(TEST_DATA_BASE_DIR, 'atcoder_problems', 'contests.json'),
- getRandomElementsFromArray(contests, 100),
+ sampledContests,
);
await toJson(
path.join(TEST_DATA_BASE_DIR, 'atcoder_problems', 'tasks.json'),
- getRandomElementsFromArray(tasks, 100),
+ sampledTasks,
);
}
@@
- const sampled = getRandomElementsFromArray(raw, 100);
+ const sampled = getDeterministicElementsFromArray(raw, 100);
@@
- const sampled = { ...raw, contests: getRandomElementsFromArray(raw.contests, 100) };
+ const sampled = {
+ ...raw,
+ contests: getDeterministicElementsFromArray(raw.contests, 100),
+ };
@@
-function getRandomElementsFromArray<T>(array: T[], count: number): T[] {
+function getDeterministicElementsFromArray<T>(array: T[], count: number): T[] {
if (!Array.isArray(array)) {
throw new Error('Input must be an array');
}
if (count < 0) {
throw new Error('Count must be non-negative');
}
-
- count = Math.min(count, array.length);
- const results = [];
- const selectedIndices = new Set<number>();
-
- while (results.length < count) {
- const index = Math.floor(Math.random() * array.length);
-
- if (selectedIndices.has(index)) {
- continue;
- }
-
- selectedIndices.add(index);
- results.push(array[index]);
- }
-
- return results;
+ const normalizedCount = Math.min(count, array.length);
+ return [...array]
+ .map((value, index) => ({ value, index, key: JSON.stringify(value) }))
+ .sort((a, b) => (a.key === b.key ? a.index - b.index : a.key.localeCompare(b.key)))
+ .slice(0, normalizedCount)
+ .map((entry) => entry.value);
}Also applies to: 76-77, 93-93, 107-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/clients/fixtures/record_requests.ts` around lines 53 - 60, The
fixtures are sampled non-deterministically and can produce tasks that reference
contests not present in the sampled contests; change the sampling to
deterministic (e.g., use a seeded RNG or sort+slice) and ensure referential
integrity by filtering the sampled tasks to only include tasks whose contest_id
exists in the sampled contests before writing; update the calls around
getRandomElementsFromArray(contests, 100) and getRandomElementsFromArray(tasks,
100) (and the other similar occurrences noted) so you first deterministically
select contests into a variable (e.g., sampledContests), then deterministically
select or sort tasks and filter them with sampledContests' IDs (e.g.,
sampledTasks = tasks.filter(t => sampledContestsIds.has(t.contest_id)) then
slice to desired count) and pass those filtered arrays to toJson so the output
is reproducible and consistent.
Comment on lines
+4
to
+6
| const hasNoQuery = !query; | ||
| const lowerQuery = query.toLowerCase(); | ||
|
|
Contributor
There was a problem hiding this comment.
空白のみクエリは未入力として扱う方が安全です。
' ' が全件非表示になり得るため、trim() して判定してください。
diff案
- const hasNoQuery = !query;
- const lowerQuery = query.toLowerCase();
+ const normalizedQuery = query.trim().toLowerCase();
+ const hasNoQuery = normalizedQuery.length === 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(admin)/tasks/_utils/contests.ts around lines 4 - 6, Treat
strings that are only whitespace as empty: change the check and normalization to
use trim(). Replace the current hasNoQuery and lowerQuery logic so hasNoQuery
becomes "!query || query.trim() === ''" and lowerQuery is computed from
"query.trim().toLowerCase()" (ensuring you guard for undefined/null query before
calling trim()). Update the assignments where "hasNoQuery" and "lowerQuery" are
defined (the current lines with hasNoQuery and lowerQuery) accordingly.
Comment on lines
+54
to
+80
| export const actions: Actions = { | ||
| update: async ({ request, locals }) => { | ||
| await validateAdminAccess(locals); | ||
|
|
||
| const formData = await request.formData(); | ||
| const taskId = formData.get('task_id')?.toString(); | ||
| const taskGradeStr = formData.get('task_grade')?.toString() ?? ''; | ||
|
|
||
| if (taskGradeStr === '') { | ||
| return { success: true }; | ||
| } | ||
|
|
||
| const task_grade: TaskGrade | undefined = getTaskGrade(taskGradeStr); | ||
|
|
||
| if (!taskId || task_grade === undefined) { | ||
| return fail(BAD_REQUEST, { success: false }); | ||
| } | ||
|
|
||
| const updateResult = await taskService.updateTask(taskId, task_grade); | ||
|
|
||
| if (updateResult === null) { | ||
| return fail(INTERNAL_SERVER_ERROR, { success: false }); | ||
| } | ||
|
|
||
| return { success: true }; | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
このフォームも Superforms + Zod へ寄せてください。
手動バリデーションのままだと、同ルート内の検証/エラー返却の一貫性が維持しづらいです。
As per coding guidelines, "src/routes/**/*.{server.ts,svelte}: Use Superforms + Zod validation for forms."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(admin)/tasks/[task_id]/+page.server.ts around lines 54 - 80,
Replace the manual formData parsing in the actions.update handler with
Superforms + Zod: create a Zod schema (e.g., TaskUpdateSchema) that validates
task_id as a nonempty string and task_grade as an optional enum/nullable
matching your TaskGrade values (or a string refined to TaskGrade), then call
superValidate(request, TaskUpdateSchema) inside update and branch on validation
result instead of ad-hoc checks; extract validated values from result.data,
map/convert task_grade to your TaskGrade type (or let Zod produce it) and call
taskService.updateTask(task_id, task_grade), and on validation failure return
the Superforms-consistent error response so errors from actions.update follow
the same Superforms + Zod pattern used elsewhere (leave validateAdminAccess and
taskService.updateTask calls intact).
Comment on lines
+55
to
+73
| update: async ({ request, locals }) => { | ||
| await validateAdminAccess(locals); | ||
|
|
||
| const formData = await request.formData(); | ||
| const taskId = formData.get('task_id')?.toString(); | ||
| const taskGradeStr = formData.get('task_grade')?.toString() ?? ''; | ||
|
|
||
| if (taskGradeStr === '') { | ||
| return { success: true }; | ||
| } | ||
|
|
||
| const task_grade: TaskGrade | undefined = getTaskGrade(taskGradeStr); | ||
|
|
||
| if (!taskId || task_grade === undefined) { | ||
| return fail(BAD_REQUEST, { success: false }); | ||
| } | ||
|
|
||
| const updateResult = await taskService.updateTask(taskId, task_grade); | ||
|
|
Contributor
There was a problem hiding this comment.
更新対象の task_id をフォーム値に依存しないでください。
ルートパラメータではなく form の task_id を採用しているため、リクエスト改変で別タスク更新が可能です。params.task_id を唯一の更新対象として使ってください。
diff案
-export const actions: Actions = {
- update: async ({ request, locals }) => {
+export const actions: Actions = {
+ update: async ({ request, locals, params }) => {
await validateAdminAccess(locals);
const formData = await request.formData();
- const taskId = formData.get('task_id')?.toString();
+ const taskId = params.task_id;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(admin)/tasks/[task_id]/+page.server.ts around lines 55 - 73, The
update handler currently trusts the form field task_id (taskId) allowing
tampering; change it to use the route parameter instead: read the task id from
the handler's params (use params.task_id) and remove/ignore the form-derived
task_id, then pass that params-derived id into taskService.updateTask; keep
existing validation for task_grade (getTaskGrade) and the bad-request/fail
branches but ensure any checks reference params.task_id (and return fail if it's
missing) rather than the form value.
Comment on lines
+21
to
+104
| fetch: async ({ request, locals }) => { | ||
| await validateAdminAccess(locals); | ||
|
|
||
| const tasksFromDB = await taskService.getTasks(); | ||
| const registeredTaskMap = prepareTaskMap(tasksFromDB); | ||
| const formData = await request.formData(); | ||
| const source = formData.get('source'); | ||
|
|
||
| const unregisteredTasks = filterUnregisteredTasks( | ||
| contestsForImport, | ||
| tasksForImport, | ||
| registeredTaskMap, | ||
| ); | ||
| const contestsWithUnregisteredTasks: Contests = mergeContestsAndUnregisteredTasks( | ||
| contestsForImport, | ||
| unregisteredTasks, | ||
| ); | ||
| if (!isContestTaskImportSource(source)) { | ||
| return fail(BAD_REQUEST, { message: 'コンテストサイト・種別が不正です。' }); | ||
| } | ||
|
|
||
| return { | ||
| importContests: contestsWithUnregisteredTasks, | ||
| }; | ||
| } | ||
| try { | ||
| const [contestsForImport, tasksForImport, tasksFromDB] = await Promise.all([ | ||
| fetchContests(source), | ||
| fetchTasks(source), | ||
| taskService.getTasks(), | ||
| ]); | ||
|
|
||
| const registeredTaskMap = prepareTaskMap(tasksFromDB); | ||
| const unregisteredTasks = filterUnregisteredTasks( | ||
| contestsForImport, | ||
| tasksForImport, | ||
| registeredTaskMap, | ||
| ); | ||
|
|
||
| async function fetchContestsAndTasksFromAPI(): Promise<{ | ||
| contestsForImport: ContestsForImport; | ||
| tasksForImport: TasksForImport; | ||
| }> { | ||
| const contestsForImport = await apiClient.getContests(); | ||
| const tasksForImport = await apiClient.getTasks(); | ||
| return { | ||
| importContests: mergeContestsAndUnregisteredTasks(contestsForImport, unregisteredTasks), | ||
| }; | ||
| } catch (error) { | ||
| console.error('Failed to fetch contests/tasks:', error); | ||
| return fail(INTERNAL_SERVER_ERROR, { message: 'データ取得に失敗しました。' }); | ||
| } | ||
| }, | ||
|
|
||
| return { contestsForImport, tasksForImport }; | ||
| } | ||
| create: async ({ request, locals }) => { | ||
| await validateAdminAccess(locals); | ||
|
|
||
| const formData = await request.formData(); | ||
| const source = formData.get('source'); | ||
|
|
||
| if (!isContestTaskImportSource(source)) { | ||
| return fail(BAD_REQUEST, { message: 'コンテストサイト・種別が不正です。' }); | ||
| } | ||
|
|
||
| const contest_id = formData.get('contest_id'); | ||
|
|
||
| if (typeof contest_id !== 'string' || !contest_id) { | ||
| return fail(BAD_REQUEST, { message: 'コンテストIDが指定されていません。' }); | ||
| } | ||
|
|
||
| const tasksJson = formData.get('tasks'); | ||
|
|
||
| if (typeof tasksJson !== 'string') { | ||
| return fail(BAD_REQUEST, { message: '問題データが不正です。' }); | ||
| } | ||
|
|
||
| let tasks: TasksForImport; | ||
|
|
||
| try { | ||
| tasks = JSON.parse(tasksJson) as TasksForImport; | ||
| } catch { | ||
| return fail(BAD_REQUEST, { message: '問題データの解析に失敗しました。' }); | ||
| } | ||
|
|
||
| try { | ||
| await Promise.all( | ||
| tasks.map(async (task: TaskForImport) => { | ||
| const id = (await sha256(contest_id + task.title)) as string; | ||
| await taskService.createTask( | ||
| id, | ||
| task.id, | ||
| task.contest_id, | ||
| task.problem_index, | ||
| task.title, | ||
| ); | ||
| }), | ||
| ); | ||
| } catch (error) { | ||
| console.error('Failed to create tasks:', error); | ||
| return fail(INTERNAL_SERVER_ERROR, { success: false }); | ||
| } | ||
|
|
||
| return { success: true }; | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
フォーム処理を Superforms + Zod に統一してください。
このルートの fetch/create は手動パース実装になっており、バリデーションとエラー整形の一貫性が崩れています。
As per coding guidelines, "src/routes/**/*.{server.ts,svelte}: Use Superforms + Zod validation for forms."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(admin)/tasks/+page.server.ts around lines 21 - 104, The handlers
fetch and create currently parse and validate request.formData() manually;
replace them with Superforms + Zod: define Zod schemas (e.g.
ContestImportQuerySchema with source for fetch, and ContestImportCreateSchema
with source:string, contest_id:string, tasks:string (or structured
TasksForImport) for create), use superValidate(request, schema) in both handlers
and return the validated form (or validation errors) so Superforms handles
client errors; keep the admin check (validateAdminAccess(locals)), then in fetch
use data.source from the validated form to call fetchContests/fetchTasks and the
existing
prepareTaskMap/filterUnregisteredTasks/mergeContestsAndUnregisteredTasks flow,
and in create use data.contest_id and the parsed tasks (parse JSON after
validation or use a Zod preprocessor to parse tasks into TasksForImport) and
then run the existing sha256 + taskService.createTask loop; ensure all
validation failures return the superform validation result instead of manual
fail responses and preserve try/catch for server errors with the same
INTERNAL_SERVER_ERROR handling.
Comment on lines
+70
to
+96
| const tasksJson = formData.get('tasks'); | ||
|
|
||
| if (typeof tasksJson !== 'string') { | ||
| return fail(BAD_REQUEST, { message: '問題データが不正です。' }); | ||
| } | ||
|
|
||
| let tasks: TasksForImport; | ||
|
|
||
| try { | ||
| tasks = JSON.parse(tasksJson) as TasksForImport; | ||
| } catch { | ||
| return fail(BAD_REQUEST, { message: '問題データの解析に失敗しました。' }); | ||
| } | ||
|
|
||
| try { | ||
| await Promise.all( | ||
| tasks.map(async (task: TaskForImport) => { | ||
| const id = (await sha256(contest_id + task.title)) as string; | ||
| await taskService.createTask( | ||
| id, | ||
| task.id, | ||
| task.contest_id, | ||
| task.problem_index, | ||
| task.title, | ||
| ); | ||
| }), | ||
| ); |
Contributor
There was a problem hiding this comment.
クライアント送信の tasks JSON を信頼しないでください。
現在は hidden input を改変すると任意の task payload を登録できます。source/contest_id を受けてサーバ側で再取得した正データのみを登録対象にしてください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(admin)/tasks/+page.server.ts around lines 70 - 96, The handler
currently trusts the client-provided tasks JSON (tasksJson → tasks) and uses
client-supplied fields (task.title, task.contest_id, task.source) when calling
taskService.createTask; instead, validate/trust only an identifier from the
client (e.g., task.id or task.source) and re-fetch the canonical task data on
the server (via your service/DB) before creating records. Change the import loop
that maps over TaskForImport so it discards client title/contest_id and instead:
lookup the authoritative task payload using the submitted task.id/source, derive
the server-side values (server_contest_id, server_title, server_problem_index),
compute the id with sha256(contest_id + server_title) and call
taskService.createTask with those server-validated fields; ensure any missing or
invalid lookups return BAD_REQUEST or a clear failure path.
Comment on lines
+85
to
+94
| await Promise.all( | ||
| tasks.map(async (task: TaskForImport) => { | ||
| const id = (await sha256(contest_id + task.title)) as string; | ||
| await taskService.createTask( | ||
| id, | ||
| task.id, | ||
| task.contest_id, | ||
| task.problem_index, | ||
| task.title, | ||
| ); |
Contributor
There was a problem hiding this comment.
タスクID生成に title を使うと衝突リスクがあります。
同一コンテスト内で同名タイトルがあるとハッシュ衝突し、登録失敗や上書き競合の原因になります。task.id ベースに変更してください。
diff案
- const id = (await sha256(contest_id + task.title)) as string;
+ const id = (await sha256(contest_id + task.id)) as string;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(admin)/tasks/+page.server.ts around lines 85 - 94, The current
ID generation uses sha256(contest_id + task.title) which risks collisions when
multiple tasks share the same title; change the seed to use the original
external identifier by hashing contest_id + task.id instead. Locate the
tasks.map block where TaskForImport is processed, replace usage of task.title in
the sha256 call with task.id so the generated id passed into
taskService.createTask is derived from contest_id + task.id.
Comment on lines
+94
to
+97
| onchange={() => { | ||
| currentPage = 1; | ||
| }} | ||
| /> |
Contributor
There was a problem hiding this comment.
取得元切替時に一覧をクリアしてください。
source を変えても旧結果が残るため、別ソースのデータに見えて誤インポートしやすいです。切替時に importContests とエラー状態をリセットしてください。
diff案
onchange={() => {
currentPage = 1;
+ importContests = [];
+ fetchError = null;
+ importError = null;
}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(admin)/tasks/+page.svelte around lines 94 - 97,
選択している取得元を切り替えたときに古い結果が残って誤インポートを誘発しているので、onchange ハンドラ内で currentPage = 1 に加えて
importContests を空配列にリセットし、インポート失敗の表示を消すためのエラー状態（例: importError または importErrors
を使っている箇所）を null/空にクリアしてください。該当箇所は +page.svelte の onchange コールバックなので、そのブロックに
importContests = [] と importError = null を追加して一覧とエラー状態を同時に初期化してください。
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 #3525
Summary by CodeRabbit
リリースノート
新機能
バグ修正
その他