Enhance Problem Customization by Restructuring Tdoc Schema #1016
Conversation
WalkthroughThis change introduces a comprehensive migration in HydroOJ's contest and homework problem management, centralizing problem metadata such as labels, scores, and balloon colors into a new Estimated code review effort5 (>120 minutes) This is a high-complexity, large-scale refactor affecting multiple core backend modules, data migration, frontend React components, templates, and utility functions. It introduces new data structures, migration logic, and UI components, requiring thorough review for data integrity, backward compatibility, and UI correctness. Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/hydrooj/src/handler/contest.tsOops! Something went wrong! :( ESLint: 9.31.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@hydrooj/eslint-config' imported from /eslint.config.mjs packages/hydrooj/src/handler/homework.tsOops! Something went wrong! :( ESLint: 9.31.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@hydrooj/eslint-config' imported from /eslint.config.mjs packages/hydrooj/src/model/contest.tsOops! Something went wrong! :( ESLint: 9.31.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@hydrooj/eslint-config' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (2)
✨ 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
packages/ui-default/templates/partials/contest_sidebar_management.html (1)
45-45: Same undefined check as in attendee sidebar.Replicate the null-safe fallback here to avoid silently blank counts on legacy data.
packages/ui-default/templates/contest_balloon.html (1)
16-16: Consider more robust balloon property validation.The current condition only checks if the first problem has a balloon property. Consider validating that all problems have balloon properties configured.
- {% if tdoc.problems|length===0 or not tdoc.problems[0].balloon %} + {% if tdoc.problems|length===0 or tdoc.problems|selectattr('balloon', 'undefined')|list|length > 0 %}This ensures that the message appears if any problem lacks a balloon property, not just the first one.
packages/utils/lib/common.ts (1)
249-249: Add TypeScript type annotations for better type safety.The function logic is correct, but consider adding type annotations to improve type safety and developer experience.
-export const getContestProblemConfig = (pid, tdoc) => tdoc.problems.find((cp) => cp.pid === pid); +export const getContestProblemConfig = (pid: string, tdoc: { problems: Array<{ pid: string; [key: string]: any }> }) => + tdoc?.problems?.find((cp) => cp.pid === pid);This provides type safety and handles potential undefined inputs gracefully.
packages/ui-default/templates/homework_edit.html (1)
87-116: LGTM! Improved structured display for problem configuration.The transition from simple text input to a structured table significantly improves the user experience by clearly displaying problem metadata (pid, label, score, custom title). The hidden input approach for form submission is appropriate for complex data structures.
Consider adding an empty state message when no problems are configured:
<tbody> + {% if problems|length === 0 %} + <tr> + <td colspan="4" class="text-center">{{ _('No problems configured') }}</td> + </tr> + {% endif %} {% for p in problems %} <tr> <td>{{ p.pid }}</td> <td>{{ p.label }}</td> <td>{{ p.score|default(100) }}</td> <td>{{ p.title }}</td> </tr> {% endfor %} </tbody>packages/hydrooj/src/upgrade.ts (1)
621-623: Consider adding progress tracking for large datasets.The migration processes all domains and contests, which could be time-consuming for installations with many contests. Consider adding progress indicators or batch processing.
async function _95_96() { + const domains = await domain.getMulti({}).toArray(); + let processedDomains = 0; await iterateAllDomain(async ({ _id }) => { - logger.info('Processing domain %s', _id); + logger.info('Processing domain %s (%d/%d)', _id, ++processedDomains, domains.length); const tdocs = await contest.getMulti(_id, {}).toArray(); + let processedContests = 0; for (const tdoc of tdocs) { + if (processedContests % 100 === 0) { + logger.info('Domain %s: processed %d/%d contests', _id, processedContests, tdocs.length); + } await contest.edit(_id, tdoc._id, { problems: tdoc.pids.map((pid, idx) => ({ pid, label: getAlphabeticId(idx), ...(tdoc?.score?.[pid] ? { score: tdoc.score[pid] } : {}), ...(tdoc?.balloon?.[pid] ? { balloon: { name: typeof tdoc.balloon[pid] === 'string' ? '' : tdoc.balloon[pid]?.name || '', color: typeof tdoc.balloon[pid] === 'string' ? tdoc.balloon[pid] : tdoc.balloon[pid]?.color || '', }, } : {}), })), }); + processedContests++; } logger.info('Domain %s done', _id); }); return true; },packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (4)
28-28: Remove debug console.log statement.- console.log(problems);
94-94: Remove debug console.log statement.- console.log(result);
49-50: Performance: Avoid delete operator for better performance.The delete operator can impact performance. Consider using undefined assignment instead.
- if (problem.title === '') delete problem.title; - if (problem.score === 100) delete problem.score; + if (problem.title === '') problem.title = undefined; + if (problem.score === 100) problem.score = undefined;And:
- delete p._tmpId; + p._tmpId = undefined;Also applies to: 56-56
35-35: Performance: Avoid spread syntax in reduce accumulator.Spread syntax on accumulators causes O(n²) time complexity.
- setProblemRawTitles(res.reduce((acc, cur) => ({ ...acc, [cur.docId]: cur.title }), {})); + const titles = {}; + res.forEach(cur => titles[cur.docId] = cur.title); + setProblemRawTitles(titles);packages/hydrooj/src/model/contest.ts (4)
106-106: Performance: Consider caching problem lookups in stat functionsUsing
find()in a loop can be inefficient when processing many journal entries. Consider creating a Set or Map of valid pids at the beginning of the function for O(1) lookups.stat(tdoc, journal: AcmJournal[]) { const naccept = Counter<number>(); const npending = Counter<number>(); const display: Record<number, AcmDetail> = {}; const detail: Record<number, AcmDetail> = {}; let accept = 0; let time = 0; const lockAt = isLocked(tdoc) ? tdoc.lockAt : null; + const validPids = new Set(tdoc.problems.map(p => p.pid)); for (const j of journal) { - if (!tdoc.problems.find((p) => p.pid === j.pid)) continue; + if (!validPids.has(j.pid)) continue;
287-300: Performance: Optimize problem lookups in OI stat functionThe current implementation filters journal entries and then searches for problems multiple times. This can be optimized by creating a Map for O(1) lookups.
stat(tdoc, journal) { const npending = Counter(); const detail = {}; const display = {}; let score = 0; const lockAt = isLocked(tdoc) ? tdoc.lockAt : null; - for (const j of journal.filter((i) => tdoc.problems.find((p) => p.pid === i.pid))) { + const problemMap = new Map(tdoc.problems.map(p => [p.pid, p])); + for (const j of journal) { + const problem = problemMap.get(j.pid); + if (!problem) continue; if (lockAt && j.rid.getTimestamp() > lockAt) { npending[j.pid]++; display[j.pid] ||= {}; display[j.pid].npending = npending[j.pid]; continue; } if (!detail[j.pid] || detail[j.pid].score < j.score || this.submitAfterAccept) { detail[j.pid] = { ...j }; display[j.pid] = { ...j }; } } for (const i in display) { - score += ((tdoc.problems.find((p) => p.pid.toString() === i)?.score || 100) * (display[i].score || 0)) / 100; + const problem = problemMap.get(Number(i)); + score += ((problem?.score || 100) * (display[i].score || 0)) / 100; }
811-819: Use optional chaining for cleaner codeThe static analysis correctly suggests using optional chaining here for more concise code.
- ...(data?.score && data.score[pid] ? { score: data.score[pid] } : {}), + ...(data?.score?.[pid] ? { score: data.score[pid] } : {}),
837-877: Consider extracting backward compatibility logic and using optional chainingThis backward compatibility logic is complex and would benefit from being extracted into a separate function for better maintainability. Also, use optional chaining as suggested by static analysis.
+ function buildProblemsFromLegacyData(tdoc: Tdoc, $set: Partial<Tdoc>): ContestProblem[] | undefined { + if ($set.pids && !$set.problems) { + const mergedScore = { + ...tdoc.score, + ...tdoc.problems.reduce((acc, cur) => { + if (cur.score) acc[cur.pid] = cur.score; + return acc; + }, {}), + ...$set.score, + }; + return $set.pids.map((pid, idx) => ({ + pid, + label: getAlphabeticId(idx), + ...(mergedScore[pid] ? { score: mergedScore[pid] } : {}), + ...($set.balloon?.[pid] ? { + balloon: typeof $set.balloon[pid] === 'string' + ? { color: $set.balloon[pid], name: '' } + : $set.balloon[pid] + } : {}), + })); + } + if (!$set.problems && !$set.pids && ($set.balloon || $set.score)) { + return tdoc.problems.map((p) => ({ + ...p, + ...(p.score || $set.score?.[p.pid] ? { score: $set.score?.[p.pid] || p.score } : {}), + ...(p.balloon || $set.balloon?.[p.pid] ? { + balloon: $set.balloon?.[p.pid] ? ( + typeof $set.balloon[p.pid] === 'string' + ? { balloon: { color: $set.balloon[p.pid], name: '' } } + : { balloon: $set.balloon[p.pid] } + ) : p.balloon, + } : {}), + })); + } + return undefined; + } + // TODO: this is the best way to support old plugins, but need remove one day - if ($set.pids && !$set.problems) { - // ... existing code ... - } - if (!$set.problems && !$set.pids && ($set.balloon || $set.score)) { - // ... existing code ... - } + const problems = buildProblemsFromLegacyData(tdoc, $set); + if (problems) $set.problems = problems;packages/hydrooj/src/handler/contest.ts (2)
527-537: Avoid mutating tdoc directlyThe method modifies
this.tdoc.problemsdirectly. Consider working with a copy to avoid potential side effects.async postSetScore(domainId: string, pid: number, score: number) { const idx = this.tdoc.problems.findIndex((i) => i.pid === pid); if (idx === -1) throw new ValidationError('pid'); - this.tdoc.problems[idx].score = score; + const problems = [...this.tdoc.problems]; + problems[idx] = { ...problems[idx], score }; // TODO: remove `score` field later - this.tdoc.score = this.tdoc.problems.reduce( + const scoreMap = problems.reduce( (acc, cur) => { if (cur.score) acc[cur.pid] = cur.score; return acc; }, {}, ); - await contest.edit(domainId, this.tdoc.docId, { score: this.tdoc.score, problems: this.tdoc.problems }); + await contest.edit(domainId, this.tdoc.docId, { score: scoreMap, problems });
624-632: Avoid mutating tdoc.problems directly in balloon configurationSimilar to postSetScore, this method modifies
this.tdoc.problemsdirectly.const config = yaml.load(color); if (typeof config !== 'object') throw new ValidationError('color'); const balloon = {}; + const problems = [...this.tdoc.problems]; for (let i = 0; i < this.tdoc.problems.length; i++) { const pid = this.tdoc.problems[i].pid; if (!config[pid]) throw new ValidationError('color'); balloon[pid] = config[pid.toString()]; - this.tdoc.problems[i].balloon = config[pid.toString()]; + problems[i] = { ...problems[i], balloon: config[pid.toString()] }; } - await contest.edit(domainId, tid, { balloon, problems: this.tdoc.problems }); + await contest.edit(domainId, tid, { balloon, problems });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
packages/hydrooj/locales/zh.yaml(4 hunks)packages/hydrooj/src/handler/contest.ts(17 hunks)packages/hydrooj/src/handler/homework.ts(3 hunks)packages/hydrooj/src/handler/problem.ts(1 hunks)packages/hydrooj/src/handler/record.ts(2 hunks)packages/hydrooj/src/interface.ts(2 hunks)packages/hydrooj/src/model/contest.ts(27 hunks)packages/hydrooj/src/upgrade.ts(2 hunks)packages/onsite-toolkit/index.ts(3 hunks)packages/scoreboard-xcpcio/index.ts(3 hunks)packages/ui-default/backendlib/template.ts(2 hunks)packages/ui-default/components/autocomplete/index.tsx(1 hunks)packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx(1 hunks)packages/ui-default/components/contestProblemEditor/contestproblemeditor.page.styl(1 hunks)packages/ui-default/package.json(2 hunks)packages/ui-default/pages/contest_balloon.page.tsx(3 hunks)packages/ui-default/pages/contest_edit.page.ts(2 hunks)packages/ui-default/templates/components/contest.html(1 hunks)packages/ui-default/templates/components/problem.html(1 hunks)packages/ui-default/templates/contest_balloon.html(1 hunks)packages/ui-default/templates/contest_edit.html(1 hunks)packages/ui-default/templates/contest_manage.html(2 hunks)packages/ui-default/templates/contest_problemlist.html(3 hunks)packages/ui-default/templates/homework_detail.html(2 hunks)packages/ui-default/templates/homework_edit.html(1 hunks)packages/ui-default/templates/partials/contest_balloon.html(1 hunks)packages/ui-default/templates/partials/contest_sidebar.html(1 hunks)packages/ui-default/templates/partials/contest_sidebar_management.html(1 hunks)packages/ui-default/templates/problem_detail.html(1 hunks)packages/ui-default/templates/record_main.html(1 hunks)packages/ui-default/templates/record_main_tr.html(1 hunks)packages/utils/lib/common.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/hydrooj/src/handler/problem.ts (1)
packages/hydrooj/src/error.ts (1)
ContestNotFoundError(80-80)
packages/ui-default/components/autocomplete/index.tsx (1)
packages/ui-default/components/autocomplete/UserSelectAutoComplete.tsx (1)
value(19-22)
packages/hydrooj/src/handler/homework.ts (1)
packages/hydrooj/src/interface.ts (1)
PenaltyRules(241-241)
🪛 Biome (1.9.4)
packages/hydrooj/src/upgrade.ts
[error] 630-630: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 631-632: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 50-50: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 35-35: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
packages/hydrooj/src/model/contest.ts
[error] 817-817: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 865-865: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 866-866: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1122-1123: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1125-1126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: build
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
[failure] 190-190:
Type 'import("/home/runner/work/Hydro/Hydro/node_modules/@types/react-beautiful-dnd/node_modules/@types/react/index").ReactNode' is not assignable to type 'React.ReactNode'.
[failure] 131-131:
'Draggable' cannot be used as a JSX component.
[failure] 127-127:
'Droppable' cannot be used as a JSX component.
[failure] 114-114:
'DragDropContext' cannot be used as a JSX component.
[failure] 82-82:
Type 'string | number' is not assignable to type 'never'.
🪛 GitHub Actions: CI
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
[error] 82-82: TypeScript error TS2322: Type 'string | number' is not assignable to type 'never'.
🔇 Additional comments (50)
packages/hydrooj/locales/zh.yaml (4)
214-214: New key looks correct.Translation for “Custom Title” is clear and matches existing style.
324-324: Accurate translation.“Empty will use raw title” → “留空以使用原标题” is concise and consistent.
440-440: No issues found with the added “Label” key.Terminology aligns with surrounding entries.
644-644: Translation for “Raw Title” is fine.No clash with existing keys detected.
packages/ui-default/templates/partials/contest_sidebar.html (1)
120-120: Potential undefined‐property pitfall.If a legacy contest document has only
pids(migration missed / partial),tdoc.problemswill beundefined, andundefined.lengthrenders as an empty string in Nunjucks.
Consider a safe-fallback to preserve the count:<dt>{{ _('Problem') }}</dt> <dd>{{ (tdoc.problems and tdoc.problems.length) or 0 }}</dd>packages/ui-default/templates/homework_detail.html (2)
43-45: Loop rewrite LGTM.Iterating over
tdoc.problemsand extractingpidkeeps the template in sync with the new schema.
62-65: Alphabetic flag addition aligns with new label logic.No further action required.
packages/ui-default/backendlib/template.ts (2)
3-3: LGTM! Import statement correctly updated.The addition of
getContestProblemConfigto the import aligns with the schema restructuring objectives.
133-133: LGTM! Global variable properly exposed to templates.The
getContestProblemConfigfunction is correctly added to the utils global, making it available for use in Nunjucks templates to access structured problem data.packages/ui-default/package.json (1)
38-38: LGTM! Appropriate dependency additions for drag-and-drop functionality.The addition of
react-beautiful-dndand its TypeScript definitions is well-suited for implementing drag-and-drop problem reordering in the new problem editor. The versions are compatible and the library is mature and well-maintained.Also applies to: 144-144
packages/ui-default/components/contestProblemEditor/contestproblemeditor.page.styl (1)
1-11: Well-structured column widths for the new problem editor interface.The column widths are appropriately sized for their content types. The responsive
rem()function usage ensures consistent sizing across different screen sizes.packages/hydrooj/src/handler/problem.ts (1)
281-281: Correct implementation of the new problems array structure.The update properly transitions from checking
pidsarray membership to searching within theproblemsarray for a matchingpid. The logic correctly throwsContestNotFoundErrorwhen the problem is not found in the contest (findIndex returns -1).packages/ui-default/templates/record_main_tr.html (1)
20-23: Improved contest context handling for problem title rendering.The updated conditional logic properly differentiates between contest and non-contest contexts. The
alphabetic=trueflag enables proper alphabetical labeling for contest problems, which is a standard contest display convention.packages/ui-default/templates/contest_edit.html (1)
66-95: Confirm JSON serialization for ContestProblemEditor integrationThe React mounting in packages/ui-default/pages/contest_edit.page.ts correctly renders ContestProblemEditor on the
#problem-editordiv and syncs changes via the hiddenproblemsinput:
- Initial props:
problems: JSON.parse($('[name=problems]').val() as string)- Change handler:
problemsInput.val(JSON.stringify(problems))Please verify that:
- The Jinja
dumpfilter produces valid JSON (e.g. using Python’sjson.dumps).- Special characters in
labelortitleare correctly escaped soJSON.parsenever throws.- End-to-end behavior: editing in the React table reliably updates the form value before form submission.
packages/ui-default/templates/partials/contest_balloon.html (1)
28-28: Approved: Efficient problem label retrieval is correctly implementedThe switch to
utils.getContestProblemConfig(bdoc.pid, tdoc).labelis clean, efficient, and aligns with the updated problem schema.Verification:
getContestProblemConfigis defined and exported in packages/utils/lib/common.ts- It’s imported in packages/ui-default/backendlib/template.ts and exposed via
addGlobal('utils', { …, getContestProblemConfig })- All template usages (including contest_balloon.html) will have access to it
No further changes required.
packages/ui-default/components/autocomplete/index.tsx (1)
76-76: Good defensive programming improvement!The addition of
|| ''prevents runtime errors whenvalueisnullorundefined, which is especially important when handling potentially complex problem data in the new structured format.packages/hydrooj/src/interface.ts (2)
250-256: Well-designed interface structure!The
ContestProbleminterface properly defines the enriched problem metadata with appropriate required (pid,label) and optional (title,score,balloon) fields. The balloon structure is consistent with existing patterns.
268-268: Good addition to support the schema migration.Adding the
problemsarray to theTdocinterface while maintaining the existingpidsarray ensures backward compatibility during the transition.packages/ui-default/templates/contest_manage.html (3)
24-24: Correct migration to structured problem objects!The change from
tdoc.pidstotdoc.problemsproperly uses the new structured problem array.
27-28: Properly updated problem references.The changes correctly use
cp.pidand updated problem title rendering to work with the new problem object structure.
32-32: Improved score access pattern.Directly accessing
cp.scorefrom the problem object is cleaner than the previous index-based lookup pattern.packages/ui-default/templates/components/contest.html (1)
11-11: Correct migration to use problem config objects.The updated macro properly uses
utils.getContestProblemConfigto retrieve structured problem data and accessescp.labelandcp.titlewith appropriate fallback topdict[subject].title.packages/ui-default/pages/contest_edit.page.ts (3)
3-4: Good addition of React imports.The React and ReactDOM imports are properly added to support the new ContestProblemEditor component.
8-8: Correct import for the new problem editor.The ContestProblemEditor import is properly added to support the structured problem editing functionality.
17-28: Well-implemented React component integration.The React component initialization is properly implemented with:
- Conditional rendering only when the element exists
- Correct prop passing with
onChangecallback and initialproblemsdata- Proper JSON serialization/deserialization for data transfer
- Good integration with existing jQuery-based form handling
packages/scoreboard-xcpcio/index.ts (3)
66-67: LGTM! Clean transition to structured problems array.The changes correctly adapt the scoreboard logic to use the new
problemsarray structure. Usingproblems.lengthfor problem count and extracting labels from thelabelproperty is the right approach.
86-90: Good improvement in balloon color handling.The new logic directly accesses balloon colors from each problem's
balloon.colorproperty, which is cleaner than the previous indirect lookup. The filtering and mapping approach is efficient.
106-106: Performance impact is negligible—no changes needed.The
findIndexcall occurs per submission but, given typical contest problem counts (dozens at most), the O(submissions × problems) lookup remains performant in practice. If you ever handle unusually large problem sets, you can precompute a PID-to-index map outside the loop:+ const pid2idx = Object.fromEntries( + tdoc.problems.map((p, idx) => [p.pid, idx]) + ); … - problem_id: tdoc.problems.findIndex((p) => p.pid === j.pid), + problem_id: pid2idx[j.pid] ?? -1,packages/hydrooj/src/handler/record.ts (2)
71-77: Excellent approach to preserve original label for display.The
realPidvariable correctly preserves the original input (like "A") for display purposes while allowing the internal logic to work with actual problem IDs. The label-to-pid mapping usingfind()is more robust than the previous index-based approach.
120-120: Correct use of preserved label for filtering.Using
realPidensures that the filter display shows the user's original input (like "A") rather than the internal problem ID, which improves UX consistency.packages/ui-default/templates/contest_problemlist.html (3)
44-45: Correct adaptation to structured problems array.The template properly iterates over
tdoc.problemsand extracts thepidfrom each problem object. This aligns well with the schema restructuring.
51-51: Good use of default values for score handling.Using
cp.score|default(100)provides consistent fallback behavior when scores are not explicitly set. The 100-point default is reasonable for most contest scenarios.Also applies to: 74-74
188-190: Improved clarification form with proper problem metadata.The clarification form now uses proper problem labels and titles from the contest configuration, which provides better user experience than the previous numeric approach.
packages/ui-default/templates/components/problem.html (2)
8-10: Correct use of contest-specific problem configuration.Using
utils.getContestProblemConfig()to get the problem label when in contest context is the right approach. The fallback logic maintains compatibility with non-contest scenarios.
12-12: Good title handling with proper fallback.The title display correctly prioritizes contest-specific titles while falling back to the original problem title. This provides flexibility for contest customization.
packages/ui-default/templates/problem_detail.html (2)
67-72: LGTM! Proper migration to structured problem representation.The changes correctly use the new
utils.getContestProblemConfigfunction to retrieve problem labels and titles from the contest configuration, replacing the previous simple problem ID handling.
74-89: LGTM! Correct iteration over structured problems.The template properly transitions from
tdoc.pidstotdoc.problemsarray iteration, extracting bothpidandlabelfrom problem objects. The length check is also correctly updated.packages/ui-default/pages/contest_balloon.page.tsx (2)
28-36: LGTM! Correct migration to structured problem objects.The component properly transitions from simple problem ID iteration to structured problem object iteration, extracting both
pidandlabelfrom the problem objects as expected.
69-72: LGTM! Proper balloon initialization with new structure.The initialization logic correctly uses the structured
tdoc.problemsarray to set up balloon configurations, accessing theballoonproperty from each problem object.packages/onsite-toolkit/index.ts (5)
55-55: LGTM! Correct filtering logic with structured problems.The submission filtering properly uses
tdoc.problems.find(p => p.pid === s.pid)instead oftdoc.pids.includes(s.pid), maintaining the same functionality with the new data structure.
62-62: LGTM! Proper problem mapping with labels.The resolver payload correctly maps problems using
p.labelandp.pidfrom the structured problem objects, replacing the previous alphabetic ID generation.
94-94: LGTM! Correct PID extraction for API calls.The code properly extracts problem IDs from the structured
tdoc.problemsarray for theProblemModel.getListAPI call, maintaining backward compatibility.
195-202: LGTM! Comprehensive CDP feed generation with structured data.The CDP feed generation correctly utilizes the structured problem objects, accessing properties like
p.pid,p.title, andp.balloonfor rich problem metadata.
209-209: LGTM! Consistent filtering in CDP view.The journal filtering maintains consistency with the resolver view, using
tdoc.problems.find(p => p.pid === s.pid)for submission filtering.packages/hydrooj/src/handler/homework.ts (4)
176-176: LGTM! Correct response format for structured problems.The GET handler properly returns the
problemsarray instead of the previous joinedpidsstring, aligning with the new structured data format.
190-190: LGTM! Parameter update for structured problem input.The parameter name change from
_pidstoproblemscorrectly reflects the new structured input format.
198-202: LGTM! Proper parsing and extraction of problem data.The handler correctly uses
contest.resolveContestProblemJsonto parse the structured problem data and maintains backward compatibility by extractingpidsfor internal use.
219-220: LGTM! Consistent storage of structured problem metadata.The handler properly stores both
problemsandscoremetadata in the contest document, supporting the enhanced problem customization features.Also applies to: 228-229
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (2)
1-207: Overall component structure looks good.The component provides comprehensive functionality for editing contest problems with drag-and-drop reordering, problem selection, and metadata editing. The approach aligns well with the structured problem data model.
114-127: Ignore react-beautiful-dnd type-compatibility concerns
The UI-Default package already declares both
"react-beautiful-dnd": "^13.1.1""@types/react-beautiful-dnd": "^13"
against React ^18.3.1, so there’s no missing type definition or version mismatch.If you’re still seeing TypeScript errors, please share the exact compiler messages so we can pinpoint the real issue.
Likely an incorrect or invalid review comment.
| async function _95_96() { | ||
| await iterateAllDomain(async ({ _id }) => { | ||
| logger.info('Processing domain %s', _id); | ||
| const tdocs = await contest.getMulti(_id, {}).toArray(); | ||
| for (const tdoc of tdocs) { | ||
| await contest.edit(_id, tdoc._id, { | ||
| problems: tdoc.pids.map((pid, idx) => ({ | ||
| pid, | ||
| label: getAlphabeticId(idx), | ||
| ...(tdoc?.score && tdoc.score[pid] ? { score: tdoc.score[pid] } : {}), | ||
| ...(tdoc?.balloon && tdoc.balloon[pid] ? { | ||
| balloon: { | ||
| name: typeof tdoc.balloon[pid] === 'string' ? '' : tdoc.balloon[pid].name, | ||
| color: typeof tdoc.balloon[pid] === 'string' ? tdoc.balloon[pid] : tdoc.balloon[pid].color, | ||
| }, | ||
| } : {}), | ||
| })), | ||
| }); | ||
| } | ||
| logger.info('Domain %s done', _id); | ||
| }); | ||
| return true; | ||
| }, |
There was a problem hiding this comment.
Fix optional chaining and improve balloon data handling.
The migration function correctly transforms pids arrays to problems arrays, but there are some issues that need addressing:
- Optional chaining issue: The static analysis correctly identifies that lines 630-634 can be simplified with optional chaining.
- Balloon data handling: The logic assumes
tdoc.balloon[pid]exists whentdoc.balloonexists, which could cause runtime errors.
Apply this diff to fix the issues:
problems: tdoc.pids.map((pid, idx) => ({
pid,
label: getAlphabeticId(idx),
- ...(tdoc?.score && tdoc.score[pid] ? { score: tdoc.score[pid] } : {}),
- ...(tdoc?.balloon && tdoc.balloon[pid] ? {
+ ...(tdoc?.score?.[pid] ? { score: tdoc.score[pid] } : {}),
+ ...(tdoc?.balloon?.[pid] ? {
balloon: {
- name: typeof tdoc.balloon[pid] === 'string' ? '' : tdoc.balloon[pid].name,
- color: typeof tdoc.balloon[pid] === 'string' ? tdoc.balloon[pid] : tdoc.balloon[pid].color,
+ name: typeof tdoc.balloon[pid] === 'string' ? '' : tdoc.balloon[pid]?.name || '',
+ color: typeof tdoc.balloon[pid] === 'string' ? tdoc.balloon[pid] : tdoc.balloon[pid]?.color || '',
},
} : {}),
})),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function _95_96() { | |
| await iterateAllDomain(async ({ _id }) => { | |
| logger.info('Processing domain %s', _id); | |
| const tdocs = await contest.getMulti(_id, {}).toArray(); | |
| for (const tdoc of tdocs) { | |
| await contest.edit(_id, tdoc._id, { | |
| problems: tdoc.pids.map((pid, idx) => ({ | |
| pid, | |
| label: getAlphabeticId(idx), | |
| ...(tdoc?.score && tdoc.score[pid] ? { score: tdoc.score[pid] } : {}), | |
| ...(tdoc?.balloon && tdoc.balloon[pid] ? { | |
| balloon: { | |
| name: typeof tdoc.balloon[pid] === 'string' ? '' : tdoc.balloon[pid].name, | |
| color: typeof tdoc.balloon[pid] === 'string' ? tdoc.balloon[pid] : tdoc.balloon[pid].color, | |
| }, | |
| } : {}), | |
| })), | |
| }); | |
| } | |
| logger.info('Domain %s done', _id); | |
| }); | |
| return true; | |
| }, | |
| async function _95_96() { | |
| await iterateAllDomain(async ({ _id }) => { | |
| logger.info('Processing domain %s', _id); | |
| const tdocs = await contest.getMulti(_id, {}).toArray(); | |
| for (const tdoc of tdocs) { | |
| await contest.edit(_id, tdoc._id, { | |
| problems: tdoc.pids.map((pid, idx) => ({ | |
| pid, | |
| label: getAlphabeticId(idx), | |
| ...(tdoc?.score?.[pid] ? { score: tdoc.score[pid] } : {}), | |
| ...(tdoc?.balloon?.[pid] ? { | |
| balloon: { | |
| name: typeof tdoc.balloon[pid] === 'string' | |
| ? '' | |
| : tdoc.balloon[pid]?.name || '', | |
| color: typeof tdoc.balloon[pid] === 'string' | |
| ? tdoc.balloon[pid] | |
| : tdoc.balloon[pid]?.color || '', | |
| }, | |
| } : {}), | |
| })), | |
| }); | |
| } | |
| logger.info('Domain %s done', _id); | |
| }); | |
| return true; | |
| }, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 630-630: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 631-632: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In packages/hydrooj/src/upgrade.ts around lines 620 to 642, fix the optional
chaining by simplifying the checks for tdoc.score and tdoc.balloon using proper
optional chaining syntax to avoid redundant existence checks. Also, improve
balloon data handling by verifying that tdoc.balloon[pid] exists before
accessing its properties to prevent runtime errors when tdoc.balloon is present
but tdoc.balloon[pid] is undefined. Adjust the conditional logic accordingly to
safely handle these cases.
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
Show resolved
Hide resolved
| export function resolveContestProblemJson(text:string) { | ||
| let _problemList = []; | ||
| try { | ||
| _problemList = JSON.parse(text); | ||
| } catch (e) { | ||
| throw new ValidationError('problems'); | ||
| } | ||
| const problems = [] as ContestProblem[]; | ||
| const score = {} as Record<number, number>; | ||
| for (const p of _problemList) { | ||
| if (!p.pid || typeof p.pid !== 'number') throw new ValidationError('problems'); | ||
| if (!p.label || typeof p.label !== 'string') throw new ValidationError('problems'); | ||
| if (typeof p.title === 'string' && p.title.length === 0) delete p.title; | ||
| if (typeof p.score === 'number') { | ||
| if (p.score < 0) throw new ValidationError('problems'); | ||
| if (p.score === 100) delete p.score; | ||
| score[p.pid] = p.score; | ||
| } | ||
| problems.push({ | ||
| pid: p.pid, | ||
| label: p.label, | ||
| ...(p.score ? { score: p.score } : {}), | ||
| ...(p.title ? { title: p.title } : {}), | ||
| }); | ||
| } | ||
| return { | ||
| problems, | ||
| score, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add validation for duplicate problem IDs and avoid delete operator
The function should validate that problem IDs are unique to prevent issues. Also, avoid using the delete operator for better performance.
export function resolveContestProblemJson(text:string) {
let _problemList = [];
try {
_problemList = JSON.parse(text);
} catch (e) {
throw new ValidationError('problems');
}
const problems = [] as ContestProblem[];
const score = {} as Record<number, number>;
+ const seenPids = new Set<number>();
for (const p of _problemList) {
if (!p.pid || typeof p.pid !== 'number') throw new ValidationError('problems');
if (!p.label || typeof p.label !== 'string') throw new ValidationError('problems');
- if (typeof p.title === 'string' && p.title.length === 0) delete p.title;
+ if (typeof p.title === 'string' && p.title.length === 0) p.title = undefined;
+ if (seenPids.has(p.pid)) throw new ValidationError('problems', null, `Duplicate problem ID: ${p.pid}`);
+ seenPids.add(p.pid);
if (typeof p.score === 'number') {
if (p.score < 0) throw new ValidationError('problems');
- if (p.score === 100) delete p.score;
+ if (p.score === 100) p.score = undefined;
score[p.pid] = p.score;
}
problems.push({
pid: p.pid,
label: p.label,
...(p.score ? { score: p.score } : {}),
...(p.title ? { title: p.title } : {}),
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function resolveContestProblemJson(text:string) { | |
| let _problemList = []; | |
| try { | |
| _problemList = JSON.parse(text); | |
| } catch (e) { | |
| throw new ValidationError('problems'); | |
| } | |
| const problems = [] as ContestProblem[]; | |
| const score = {} as Record<number, number>; | |
| for (const p of _problemList) { | |
| if (!p.pid || typeof p.pid !== 'number') throw new ValidationError('problems'); | |
| if (!p.label || typeof p.label !== 'string') throw new ValidationError('problems'); | |
| if (typeof p.title === 'string' && p.title.length === 0) delete p.title; | |
| if (typeof p.score === 'number') { | |
| if (p.score < 0) throw new ValidationError('problems'); | |
| if (p.score === 100) delete p.score; | |
| score[p.pid] = p.score; | |
| } | |
| problems.push({ | |
| pid: p.pid, | |
| label: p.label, | |
| ...(p.score ? { score: p.score } : {}), | |
| ...(p.title ? { title: p.title } : {}), | |
| }); | |
| } | |
| return { | |
| problems, | |
| score, | |
| }; | |
| } | |
| export function resolveContestProblemJson(text: string) { | |
| let _problemList = []; | |
| try { | |
| _problemList = JSON.parse(text); | |
| } catch (e) { | |
| throw new ValidationError('problems'); | |
| } | |
| const problems = [] as ContestProblem[]; | |
| const score = {} as Record<number, number>; | |
| const seenPids = new Set<number>(); | |
| for (const p of _problemList) { | |
| if (!p.pid || typeof p.pid !== 'number') throw new ValidationError('problems'); | |
| if (!p.label || typeof p.label !== 'string') throw new ValidationError('problems'); | |
| if (typeof p.title === 'string' && p.title.length === 0) p.title = undefined; | |
| if (seenPids.has(p.pid)) throw new ValidationError('problems', null, `Duplicate problem ID: ${p.pid}`); | |
| seenPids.add(p.pid); | |
| if (typeof p.score === 'number') { | |
| if (p.score < 0) throw new ValidationError('problems'); | |
| if (p.score === 100) p.score = undefined; | |
| score[p.pid] = p.score; | |
| } | |
| problems.push({ | |
| pid: p.pid, | |
| label: p.label, | |
| ...(p.score ? { score: p.score } : {}), | |
| ...(p.title ? { title: p.title } : {}), | |
| }); | |
| } | |
| return { | |
| problems, | |
| score, | |
| }; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1122-1123: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1125-1126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🤖 Prompt for AI Agents
In packages/hydrooj/src/model/contest.ts lines 1110 to 1139, add a check to
ensure problem IDs are unique by tracking seen IDs in a Set and throwing a
ValidationError if a duplicate is found. Replace the use of the delete operator
on p.title and p.score by conditionally omitting these properties when
constructing the problem objects instead of deleting them from the original
objects.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
packages/ui-default/templates/partials/contest_balloon.html (1)
18-29: Mixed data sources (tdoc.balloonvs.problems) cause inconsistencyThe label now comes from
getContestProblemConfig, but the color/name still come from the legacytdoc.balloonmap.
If the new schema is used exclusively,tdoc.balloonmight be empty → broken colour & name.Suggested unification:
-<td class="col--problem col--problem-name" style="color:{{ tdoc.balloon[bdoc.pid].color }}"> +{% set pcfg = utils.getContestProblemConfig(bdoc.pid, tdoc) %} +<td class="col--problem col--problem-name" style="color:{{ pcfg.balloon.color }}"> … -<b>{{ utils.getContestProblemConfig(bdoc.pid, tdoc).label }}</b> ({{ tdoc.balloon[bdoc.pid].name }}) +<b>{{ pcfg.label }}</b> ({{ pcfg.balloon.name }})Double-check that
pcfg.balloonis always present; otherwise guard with a default.
♻️ Duplicate comments (4)
packages/ui-default/templates/partials/contest_sidebar_management.html (1)
45-45: Same null-safety concern as sidebarSee previous comment – wrap
tdoc.problemswith|default([])to avoid runtime errors on unmigrated documents.packages/hydrooj/src/upgrade.ts (1)
620-647: Fix optional chaining and improve balloon data handling.The migration function correctly transforms
pidsarrays toproblemsarrays, but there are some issues that need addressing:
- Optional chaining issue: Lines 633-634 can be simplified with optional chaining as indicated by static analysis.
- Balloon data handling: The logic assumes
tdoc.balloon[pid]exists whentdoc.balloonexists, which could cause runtime errors.Apply this diff to fix the issues:
problems: tdoc.pids.map((pid, idx) => ({ pid, label: getAlphabeticId(idx), - ...(tdoc?.score && tdoc.score[pid] ? { score: tdoc.score[pid] } : {}), - ...(tdoc?.balloon && tdoc.balloon[pid] ? { + ...(tdoc?.score?.[pid] ? { score: tdoc.score[pid] } : {}), + ...(tdoc?.balloon?.[pid] ? { balloon: { - name: typeof tdoc.balloon[pid] === 'string' ? '' : tdoc.balloon[pid].name, - color: typeof tdoc.balloon[pid] === 'string' ? tdoc.balloon[pid] : tdoc.balloon[pid].color, + name: typeof tdoc.balloon[pid] === 'string' ? '' : tdoc.balloon[pid]?.name || '', + color: typeof tdoc.balloon[pid] === 'string' ? tdoc.balloon[pid] : tdoc.balloon[pid]?.color || '', }, } : {}), })),packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (1)
76-82: Fix TypeScript compilation error.The type system doesn't allow assigning
string | numberto a generic field. Fix the type handling.- switch (field) { - case 'pid': - problem.pid = Number(value); - break; - default: - (problem as any)[field] = value; - } + if (field === 'pid') { + problem.pid = Number(value); + } else { + (problem as any)[field] = value; + }packages/hydrooj/src/model/contest.ts (1)
1125-1160: Add validation for duplicate problem IDs.The function should validate that problem IDs are unique to prevent issues in contest operations.
As mentioned in the previous review, add duplicate PID validation:
export function resolveContestProblemJson(text:string) { const validate = Schema.array(Schema.object({ pid: Schema.number().required(), label: Schema.string().required(), title: Schema.string(), score: Schema.number().min(0), balloon: Schema.object({ color: Schema.string(), name: Schema.string(), }), })).required(); let _problemList = []; try { _problemList = validate(JSON.parse(text)); } catch (e) { throw new ValidationError('problems'); } const problems = [] as ContestProblem[]; const score = {} as Record<number, number>; + const seenPids = new Set<number>(); for (const p of _problemList) { + if (seenPids.has(p.pid)) { + throw new ValidationError('problems', null, `Duplicate problem ID: ${p.pid}`); + } + seenPids.add(p.pid); if (typeof p.score === 'number' && p.score !== 100) { score[p.pid] = p.score; } problems.push({ pid: p.pid, label: p.label, ...(p.score && p.score !== 100 ? { score: p.score } : {}), ...(p.title && p.title.length > 0 ? { title: p.title } : {}), }); } return { problems, score, }; }
🧹 Nitpick comments (9)
packages/ui-default/templates/contest_edit.html (1)
82-90: Nit: empty custom titles render a blank cellWhen
p.titleis an empty string the table shows nothing, which looks like a missing value.
Consider defaulting to the raw title for readability:-<td>{{ p.title }}</td> +<td>{{ p.title or pdict[p.pid].title }}</td>packages/ui-default/pages/contest_edit.page.ts (1)
17-28: Well-implemented React component integrationThe integration of the ContestProblemEditor component follows React best practices with proper root creation and bidirectional data binding. The JSON serialization/deserialization correctly handles the structured problem data.
Consider adding error handling for JSON parsing in case the input field contains invalid JSON data.
- problems: JSON.parse(problemsInput.val() as string), + problems: (() => { + try { + return JSON.parse(problemsInput.val() as string); + } catch (e) { + console.warn('Invalid problems JSON, using empty array:', e); + return []; + } + })(),packages/scoreboard-xcpcio/index.ts (1)
106-106: Verify findIndex performance for large problem sets.The submission processing now uses
findIndexto locate problems bypid. This is correct but could be inefficient for contests with many problems.Consider using a pre-computed mapping for better performance:
+const pid2idx = Object.fromEntries(tdoc.problems.map((p, idx) => [p.pid, idx])); // ... later in the code -problem_id: tdoc.problems.findIndex((p) => p.pid === j.pid), +problem_id: pid2idx[j.pid] ?? -1,packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (2)
45-58: Optimize object property removal for better performance.The current implementation uses the
deleteoperator which can impact performance. Consider using undefined assignment instead.const fixedProblems = newProblems.map((i) => { const problem = { ...i }; - if (problem.title === '') delete problem.title; - if (problem.score === 100) delete problem.score; + if (problem.title === '') problem.title = undefined; + if (problem.score === 100) problem.score = undefined; return problem; }); setProblems(fixedProblems); onChange(fixedProblems.map((i) => { const p = { ...i }; - delete p._tmpId; + p._tmpId = undefined; return p; }));
32-39: Optimize reduce accumulator to avoid O(n²) complexity.Using spread syntax in reduce accumulators causes O(n²) time complexity. Consider using a more efficient approach.
const fetchProblemTitles = async (ids: number[]) => { api('problems', { ids }, ['docId', 'pid', 'title']) .then((res) => { - setProblemRawTitles(res.reduce((acc, cur) => ({ ...acc, [cur.docId]: cur.title }), {})); + const titles = {}; + res.forEach((cur) => { + titles[cur.docId] = cur.title; + }); + setProblemRawTitles(titles); }) .catch(() => { // pid maybe not exist }); };packages/hydrooj/src/model/contest.ts (2)
816-828: Good backward compatibility handling, but consider the optional chaining improvement.The backward compatibility code properly converts legacy
pidsto the newproblemsstructure. However, the condition on line 822 can be simplified.Apply this improvement:
- ...(data?.score && data.score[pid] ? { score: data.score[pid] } : {}), + ...(data?.score?.[pid] ? { score: data.score[pid] } : {}),Also, the TODO comment indicates this compatibility layer should be removed eventually. Consider creating a migration timeline.
846-892: Complex backward compatibility logic could benefit from simplification.The edit function handles multiple scenarios for backward compatibility, which adds significant complexity. Consider the following improvements:
- Simplify the conditional checks using optional chaining (lines 874-875):
- ...(p.score || ($set.score && $set.score[p.pid]) ? { score: $set.score[p.pid] || p.score } : {}), - ...(p.balloon || ($set.balloon && $set.balloon[p.pid]) ? { + ...(p.score || $set.score?.[p.pid] ? { score: $set.score?.[p.pid] || p.score } : {}), + ...(p.balloon || $set.balloon?.[p.pid] ? {
- Consider extracting the complex problem transformation logic into a separate helper function for better maintainability.
The TODO comment indicates this compatibility layer is temporary. Document the migration plan and timeline.
packages/hydrooj/src/handler/contest.ts (2)
303-303: Consider using a more specific type for problems parameter.The
problemsparameter is typed asTypes.Content(which is likely a string type). Since this expects JSON content, consider adding validation or documentation about the expected format.Add a comment documenting the expected JSON format:
+ // Expected format: [{ pid: number, label: string, title?: string, score?: number, balloon?: {...} }, ...] @param('problems', Types.Content)
527-537: Good backward compatibility approach for score updates.The implementation correctly updates scores in both the new
problemsarray and the legacyscoreobject. The TODO comment indicates future cleanup is planned.Would you like me to create an issue to track the removal of the legacy
scorefield once all systems have migrated to the new structure?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
packages/hydrooj/locales/zh.yaml(4 hunks)packages/hydrooj/src/handler/contest.ts(17 hunks)packages/hydrooj/src/handler/homework.ts(3 hunks)packages/hydrooj/src/handler/problem.ts(2 hunks)packages/hydrooj/src/handler/record.ts(2 hunks)packages/hydrooj/src/interface.ts(2 hunks)packages/hydrooj/src/model/contest.ts(29 hunks)packages/hydrooj/src/upgrade.ts(2 hunks)packages/onsite-toolkit/index.ts(3 hunks)packages/scoreboard-xcpcio/index.ts(3 hunks)packages/ui-default/backendlib/template.ts(2 hunks)packages/ui-default/components/autocomplete/index.tsx(1 hunks)packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx(1 hunks)packages/ui-default/components/contestProblemEditor/contestproblemeditor.page.styl(1 hunks)packages/ui-default/package.json(1 hunks)packages/ui-default/pages/contest_balloon.page.tsx(3 hunks)packages/ui-default/pages/contest_edit.page.ts(2 hunks)packages/ui-default/templates/components/contest.html(1 hunks)packages/ui-default/templates/components/problem.html(1 hunks)packages/ui-default/templates/contest_balloon.html(1 hunks)packages/ui-default/templates/contest_edit.html(1 hunks)packages/ui-default/templates/contest_manage.html(2 hunks)packages/ui-default/templates/contest_problemlist.html(3 hunks)packages/ui-default/templates/homework_detail.html(2 hunks)packages/ui-default/templates/homework_edit.html(1 hunks)packages/ui-default/templates/partials/contest_balloon.html(1 hunks)packages/ui-default/templates/partials/contest_sidebar.html(1 hunks)packages/ui-default/templates/partials/contest_sidebar_management.html(1 hunks)packages/ui-default/templates/problem_detail.html(1 hunks)packages/ui-default/templates/record_main.html(1 hunks)packages/ui-default/templates/record_main_tr.html(1 hunks)packages/utils/lib/common.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/ui-default/components/autocomplete/index.tsx (1)
packages/ui-default/components/autocomplete/UserSelectAutoComplete.tsx (1)
value(19-22)
packages/hydrooj/src/handler/problem.ts (1)
packages/hydrooj/src/error.ts (1)
ContestNotFoundError(80-80)
packages/onsite-toolkit/index.ts (3)
packages/hydrooj/src/model/problem.ts (1)
ProblemModel(77-695)packages/utils/lib/common.ts (1)
parseTimeMS(164-170)packages/hydrooj/src/interface.ts (1)
ProblemConfig(138-150)
packages/hydrooj/src/handler/homework.ts (1)
packages/hydrooj/src/interface.ts (1)
PenaltyRules(241-241)
packages/hydrooj/src/model/contest.ts (4)
packages/hydrooj/src/interface.ts (2)
Tdoc(258-297)ContestProblem(250-256)framework/framework/error.ts (1)
ValidationError(54-63)packages/utils/lib/common.ts (1)
getAlphabeticId(240-247)packages/hydrooj/src/libs.ts (1)
Schema(14-14)
🪛 Biome (1.9.4)
packages/hydrooj/src/handler/problem.ts
[error] 307-307: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/hydrooj/src/upgrade.ts
[error] 634-634: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 635-636: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
[error] 48-48: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 34-34: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
packages/hydrooj/src/model/contest.ts
[error] 822-822: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 874-874: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 875-875: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (50)
packages/ui-default/package.json (1)
124-124: Potential drag-and-drop lib overlap – double-check bundle size
@hello-pangea/dndis now added whilereact-dnd/react-dnd-html5-backendalready exist (lines 90-92).
Although they target different APIs (react-beautiful-dndvsreact-dnd), shipping both can bloat the client bundle by ~70-90 kB min+gz. Make sure both libraries are actually required; otherwise, consider standardising on one.Would you like a quick webpack-stats diff to estimate the added weight?
packages/ui-default/templates/record_main.html (1)
112-112: Good catch – keeps status 0 intactReplacing the strict-equality (
===) with== 0fixes the template parse error and still includes falsy value0in the query string. ✅packages/ui-default/backendlib/template.ts (1)
3-4: Good addition – utilities are now available to templatesExposing
getContestProblemConfigalongside existing helpers will simplify many templates and removes duplicate index calculations. ✅Also applies to: 133-134
packages/ui-default/components/contestProblemEditor/contestproblemeditor.page.styl (1)
1-11: Confirmrem()helper acceptspxliteralsEach width is written as
rem(20px)/rem(100px)/rem(80px).
If the Stylus helper expects a unit-less number (e.g.rem(20)), passing apxliteral will silently output an invalidrem(Npx)string.Please double-check the helper and, if necessary, strip the
px:- .col--drag - width: rem(20px) + .col--drag + width: rem(20)packages/hydrooj/locales/zh.yaml (1)
214-214: Verify key duplication & alphabetical groupingThe new keys (
Custom Title,Empty will use raw title,Label,Raw Title) are added far apart in the file.
If any of them already exist elsewhere, YAML-loaders will silently pick the first occurrence and ignore the second.Please grep the file for duplicates and group new keys with semantically related neighbours to keep maintenance easy.
Also applies to: 324-324, 440-440, 644-644
packages/ui-default/templates/contest_manage.html (2)
24-36: 👍 Migration totdoc.problemslooks correctLoop, links and score handling have been updated to the new schema;
cp.pid&cp.scoreare correctly referenced.
129-131: Fixed undefinedpidreference – good catchThe option label now uses
cp.pid; this resolves the previously reported runtime error.packages/hydrooj/src/handler/problem.ts (2)
281-281: LGTM: Contest problem validation updated for new structureThe change correctly updates the contest problem validation logic to use the new
problemsarray structure instead of the legacypidsarray. The use offindIndexwith optional chaining ensures safe access to the problem metadata while maintaining the same validation behavior.
307-307: Good defensive programming: Added length check for contest languagesThe explicit length check prevents empty language arrays from being processed, improving the robustness of the language aggregation logic. The current use of optional chaining is appropriate and the static analysis hint appears to be a false positive.
packages/ui-default/templates/components/contest.html (1)
11-11: LGTM: Updated clarification subject rendering to use structured problem dataThe change correctly updates the macro to use the new
utils.getContestProblemConfigutility function, accessing problem labels and titles from the structured problem objects. The fallback topdict[subject].titleensures backward compatibility while the special handling for general (-1) and technical (0) issues is preserved.packages/ui-default/pages/contest_edit.page.ts (1)
3-4: LGTM: Added necessary imports for React component integrationThe imports for React, ReactDOM, and the new ContestProblemEditor component are appropriate for integrating the structured problem editor functionality into the contest edit page.
Also applies to: 8-8
packages/hydrooj/src/interface.ts (2)
250-256: Well-designed interface for structured contest problemsThe
ContestProbleminterface provides a comprehensive structure for contest problem metadata with appropriate required and optional properties. The balloon property structure with color and name strings is consistent with typical contest balloon configurations.
268-269: Excellent addition of structured problem support to contest documentsThe addition of
problemsarray andpid2idxmapping to theTdocinterface enables efficient access to structured problem metadata. Thepid2idxmapping provides O(1) lookup performance, which is a significant improvement over linear searches.packages/utils/lib/common.ts (1)
249-249: Excellent performance improvement: O(1) problem configuration lookupThe change from linear search to direct index access using
pid2idxmapping provides a significant performance improvement from O(n) to O(1) for contest problem lookups. The use of optional chaining ensures safe access to the mapping.packages/scoreboard-xcpcio/index.ts (2)
66-67: LGTM! Clean migration to problems array structure.The changes correctly replace
tdoc.pidswithtdoc.problemsfor both problem count and label extraction. Usingtdoc.problems.map(({ label }) => label)is more explicit than the previous alphabetic ID generation.
86-90: VerifyProblem.balloontype and add color fallbacks
Please ensure that everyProblem.balloonentry has a well-definedcolorfield and guard against missing or malformed balloons:• Review the
Probleminterface/type (wherever it’s declared) to confirm that
–balloonis typed as an object (not a boolean)
–balloon.coloris required (or explicitly marked optional)
• In the mapping logic, provide a default whenp.balloon.colorisundefined, for example:background_color: p.balloon.color ?? '#FFF'• Consider edge cases:
– Problems withballoon = nullor{}
– Inconsistent or deprecated balloon structurespackages/hydrooj/src/handler/record.ts (3)
72-72: Good practice: Preserving original PID for filtering.Introducing
realPidto preserve the original PID value before transformation is a good approach that maintains the user's input context for filtering.
74-77: Improved label-to-PID mapping logic.The new approach using
tdoc.problems.find((i) => i.label === pid)is more robust than the previous alphabetic indexing. This makes the label-to-problem ID mapping explicit and handles custom labels properly.
120-120: Correct use of original PID for filtering.Using
realPidinstead of the transformedpidensures that the filter parameter reflects the user's original input (e.g., label 'A') rather than the internal problem ID.packages/ui-default/templates/components/problem.html (2)
8-10: Enhanced problem label rendering with contest-specific configuration.The change to use
utils.getContestProblemConfig(pdoc.docId, tdoc).labelinstead of generating alphabetic IDs is a good improvement. This allows for custom labels while maintaining the fallback logic for non-contest problems.
12-12: Consistent use of contest-specific problem titles.Using
utils.getContestProblemConfig(pdoc.docId, tdoc).title|default(pdoc.title)provides flexibility for custom problem titles in contests while preserving the original title as a fallback.packages/ui-default/templates/contest_problemlist.html (4)
44-45: Clean migration to structured problem objects.The change from
tdoc.pidstotdoc.problemsand extractingpidfromcp.pidis correct and provides access to richer problem metadata.
51-51: Simplified score calculation using problem metadata.Using
cp.score|default(100)directly from the problem object is more straightforward and accurate than the previous approach.
74-74: Direct access to problem score.The direct access to
cp.score|default(100)is consistent with the new schema and provides cleaner code.
188-190: Enhanced clarification form with rich problem data.The clarification form now uses
cp.pid,cp.label, andcp.titleproviding better user experience with more descriptive options. The fallback topdict[cp.pid].titleensures compatibility.packages/ui-default/pages/contest_balloon.page.tsx (4)
28-30: Simplified problem iteration using structured data.The change to iterate over
tdoc.problemsand directly extractcp.pideliminates the need for alphabetic ID generation and provides cleaner code.
35-36: Direct use of problem labels for display.Using
cp.labeldirectly is more straightforward and consistent with the new schema than generating alphabetic IDs.
55-55: Simplified row span logic.Using
idx === 0for the color picker row span is simpler and more maintainable than the previous approach.
69-72: Improved balloon data initialization.The initialization logic now directly uses
cp.balloonfrom the problem objects, which is more explicit and handles the structured data properly.packages/ui-default/templates/problem_detail.html (3)
67-67: LGTM: Problem label access updated correctly.The change from alphabetic label generation to using
utils.getContestProblemConfig(pdoc.docId, tdoc).labelis consistent with the migration to structured problem objects.
72-72: LGTM: Problem title logic enhanced with contest-specific configuration.The updated logic uses contest-specific problem configuration with proper fallback to the default problem title, improving customization capabilities.
74-89: LGTM: Problem navigation updated to use structured problem objects.The migration from
tdoc.pidstotdoc.problemsis implemented correctly:
- Length check properly updated
- Iteration over problem objects instead of IDs
- Proper extraction of
pidfrom problem objects- Consistent use of
cp.labelfor displaypackages/hydrooj/src/upgrade.ts (1)
7-7: LGTM: Required import added for migration.The
getAlphabeticIdimport is correctly added to support the label generation in the migration function.packages/onsite-toolkit/index.ts (5)
55-55: LGTM: Submission filtering updated to use structured problems.The filter correctly uses
tdoc.problems.find((p) => p.pid === s.pid)to check if a submission belongs to a contest problem, maintaining the same logic with the new data structure.
62-62: LGTM: Problem mapping enhanced with metadata.The mapping now uses
tdoc.problemsdirectly to extract bothlabelandpid, taking advantage of the richer problem object structure.
94-94: LGTM: Problem list retrieval correctly extracts PIDs.The code properly extracts problem IDs from the
tdoc.problemsarray usingmap((p) => p.pid)for theProblemModel.getListcall.
195-204: LGTM: CDP problem feed generation improved with structured data.The problem feed generation now leverages the structured problem objects to:
- Use problem metadata directly (
p.title,p.pid)- Access balloon information from
tdoc.problems[idx]?.balloon- Provide better problem configuration access
209-209: LGTM: Journal filtering maintains consistency.The journal filtering correctly uses the same pattern as the resolver, ensuring only valid contest problems are included in the CDP submission data.
packages/hydrooj/src/handler/homework.ts (4)
176-176: LGTM: GET method updated to return structured problems.The response now returns the
problemsarray directly instead of a joined string, providing richer data to the client.
190-190: LGTM: Parameter updated to accept structured problems.The parameter change from
pidstoproblemswith proper type annotation aligns with the migration to structured problem objects.Also applies to: 198-198
201-202: LGTM: Problem parsing uses consistent validation.Using
contest.resolveContestProblemJsonensures consistent parsing and validation of problem data across contest and homework handlers.
217-220: LGTM: Contest model calls updated with structured data.Both
contest.addandcontest.editcalls now properly pass theproblemsarray andscoremapping, maintaining consistency with the new data structure.Also applies to: 228-229
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (4)
7-22: LGTM: Well-defined TypeScript interfaces.The
ProblemandContestProblemEditorPropsinterfaces are properly defined with clear type annotations and optional properties.
31-43: LGTM: Proper async data fetching with error handling.The
fetchProblemTitlesfunction correctly handles API calls with proper error handling for non-existent problem IDs.
92-109: LGTM: Drag and drop implementation is well-structured.The drag and drop logic correctly handles problem reordering and label swapping, maintaining data consistency throughout the operation.
111-202: LGTM: Comprehensive UI implementation with proper accessibility.The table-based UI provides:
- Proper drag handles for accessibility
- Autocomplete for problem selection
- Input validation and formatting
- Localized text throughout
- Clear action buttons
packages/hydrooj/src/model/contest.ts (3)
107-107: LGTM! Efficient lookup using pid2idx.The change from array includes to Object.hasOwn with pid2idx improves performance from O(n) to O(1) for problem ID lookups.
302-302: Good defensive programming with optional chaining.The use of optional chaining when accessing problem scores provides safe fallback to default value of 100.
957-957: Correct adaptation to check balloon in problems array.The change properly checks for balloon configuration within the structured problems array using optional chaining.
packages/hydrooj/src/handler/contest.ts (1)
203-203: Efficient score display logic.The code efficiently determines whether to show scores by checking if any problem has a non-default score value.
| {{ noscript_note.render() }} | ||
| <div class="section__body no-padding"> | ||
| {% if not tdoc.balloon|length %} | ||
| {% if tdoc.problems|length===0 or not tdoc.problems[0].balloon %} |
There was a problem hiding this comment.
Template parse error – === is not valid in Jinja/Nunjucks
The same strict-equality surfaced again:
{% if tdoc.problems|length===0 or not tdoc.problems[0].balloon %}=== will crash the template engine. Use == (or the |length filter you already invoked):
-{% if tdoc.problems|length===0 or not tdoc.problems[0].balloon %}
+{% if (tdoc.problems | default([])) | length == 0 or not tdoc.problems[0].balloon %}Adds the missing null-guard as well.
🤖 Prompt for AI Agents
In packages/ui-default/templates/contest_balloon.html at line 16, replace the
invalid strict equality operator `===` with the correct `==` for Jinja/Nunjucks
syntax. Change the condition to use `tdoc.problems|length == 0` instead of
`tdoc.problems|length===0`. Also, ensure there is a null-guard to safely check
`tdoc.problems[0].balloon` only if `tdoc.problems` is not empty.
| <div class="row"> | ||
| <div class="medium-12 columns"> | ||
| <label>{{ _('Problems') }}</label> | ||
| <div> | ||
| <input type="hidden" name="problems" value="{{ problems | dump }}" /> | ||
| <div id="problem-editor"> | ||
| <table class="data-table"> | ||
| <thead> | ||
| <tr> | ||
| <th>{{ _('Problem') }}</th> | ||
| <th>{{ _('Label') }}</th> | ||
| <th>{{ _('Score') }}</th> | ||
| <th>{{ _('Custom Title') }}</th> | ||
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| {% for p in problems %} | ||
| <tr> | ||
| <td>{{ p.pid }}</td> | ||
| <td>{{ p.label }}</td> | ||
| <td>{{ p.score|default(100) }}</td> | ||
| <td>{{ p.title }}</td> | ||
| </tr> | ||
| {% endfor %} | ||
| </tbody> | ||
| </table> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Missing dump filter – template will crash
{{ problems | dump }} relies on a dump filter that is not registered in backendlib/template.ts (only dumpYaml and json exist).
Rendering will fail with “unknown filter: dump”.
Quick fix:
-<input type="hidden" name="problems" value="{{ problems | dump }}" />
+<input type="hidden" name="problems" value="{{ problems | dumpYaml }}" />(or use | json if the server expects JSON).
Follow-up: make sure the server handler parses the same format.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="row"> | |
| <div class="medium-12 columns"> | |
| <label>{{ _('Problems') }}</label> | |
| <div> | |
| <input type="hidden" name="problems" value="{{ problems | dump }}" /> | |
| <div id="problem-editor"> | |
| <table class="data-table"> | |
| <thead> | |
| <tr> | |
| <th>{{ _('Problem') }}</th> | |
| <th>{{ _('Label') }}</th> | |
| <th>{{ _('Score') }}</th> | |
| <th>{{ _('Custom Title') }}</th> | |
| </tr> | |
| </thead> | |
| <tbody> | |
| {% for p in problems %} | |
| <tr> | |
| <td>{{ p.pid }}</td> | |
| <td>{{ p.label }}</td> | |
| <td>{{ p.score|default(100) }}</td> | |
| <td>{{ p.title }}</td> | |
| </tr> | |
| {% endfor %} | |
| </tbody> | |
| </table> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| <div class="row"> | |
| <div class="medium-12 columns"> | |
| <label>{{ _('Problems') }}</label> | |
| <div> | |
| - <input type="hidden" name="problems" value="{{ problems | dump }}" /> | |
| + <input type="hidden" name="problems" value="{{ problems | dumpYaml }}" /> | |
| <div id="problem-editor"> | |
| <table class="data-table"> | |
| <thead> | |
| <tr> | |
| <th>{{ _('Problem') }}</th> | |
| <th>{{ _('Label') }}</th> | |
| <th>{{ _('Score') }}</th> | |
| <th>{{ _('Custom Title') }}</th> | |
| </tr> | |
| </thead> | |
| <tbody> | |
| {% for p in problems %} | |
| <tr> | |
| <td>{{ p.pid }}</td> | |
| <td>{{ p.label }}</td> | |
| <td>{{ p.score|default(100) }}</td> | |
| <td>{{ p.title }}</td> | |
| </tr> | |
| {% endfor %} | |
| </tbody> | |
| </table> | |
| </div> | |
| </div> | |
| </div> | |
| </div> |
🤖 Prompt for AI Agents
In packages/ui-default/templates/homework_edit.html around lines 87 to 116, the
template uses a `dump` filter on `problems` which is not registered and causes a
crash. Replace the `dump` filter with the existing `json` filter to serialize
`problems` correctly. Also verify that the server handler expects and correctly
parses JSON format for this field.
| <input type="hidden" name="problems" value="{{ problems | dump }}" /> | ||
| <div id="problem-editor"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential XSS / broken attribute – JSON should be HTML-escaped
dump produces raw JSON that contains quotes. Injecting it directly into the value attribute risks:
- Breaking the attribute when the JSON itself contains
" - XSS if the value ever becomes unescaped HTML
Encode the JSON for attributes (or move it into a <script> tag):
-<input type="hidden" name="problems" value="{{ problems | dump }}" />
+<input type="hidden"
+ name="problems"
+ value="{{ problems | dump | tojson | e }}" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <input type="hidden" name="problems" value="{{ problems | dump }}" /> | |
| <div id="problem-editor"> | |
| <input type="hidden" | |
| name="problems" | |
| value="{{ problems | dump | tojson | e }}" /> | |
| <div id="problem-editor"> |
🤖 Prompt for AI Agents
In packages/ui-default/templates/contest_edit.html at lines 70-71, the JSON
output from the dump filter is directly injected into an HTML attribute without
escaping, risking broken attributes or XSS. To fix this, ensure the JSON string
is properly HTML-escaped before inserting it into the value attribute, or
alternatively move the JSON data into a separate <script> tag to avoid attribute
injection issues.
| this.onChange(v); | ||
| }} | ||
| selectedKeys={(Array.isArray(value) ? value : value.split(',')).map((i) => i.trim()).filter((i) => i)} | ||
| selectedKeys={(Array.isArray(value) ? value : (value || '').split(',')).map((i) => i.trim()).filter((i) => i)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
value may be non-string – guard before .split()
123.split will throw if $dom.val() contains a numeric string coerced to number.
Cast to string first:
- selectedKeys={(Array.isArray(value) ? value : (value || '').split(',')).map((i) => i.trim()).filter((i) => i)}
+ selectedKeys={(Array.isArray(value)
+ ? value
+ : String(value ?? '').split(',')
+ ).map((i) => i.trim()).filter(Boolean)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selectedKeys={(Array.isArray(value) ? value : (value || '').split(',')).map((i) => i.trim()).filter((i) => i)} | |
| selectedKeys={(Array.isArray(value) | |
| ? value | |
| : String(value ?? '').split(',') | |
| ).map((i) => i.trim()).filter(Boolean)} |
🤖 Prompt for AI Agents
In packages/ui-default/components/autocomplete/index.tsx at line 76, the code
calls .split() on value without ensuring it is a string, which can cause errors
if value is a number. To fix this, explicitly cast value to a string before
calling .split(), for example by using String(value) or value.toString(), to
safely handle non-string inputs.
|
Works well in my basic tests, so maybe ready for review? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/hydrooj/locales/zh.yaml(4 hunks)packages/hydrooj/src/interface.ts(2 hunks)packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx(1 hunks)packages/ui-default/templates/contest_balloon.html(1 hunks)packages/ui-default/templates/homework_detail.html(2 hunks)packages/ui-default/templates/partials/contest_balloon.html(2 hunks)packages/ui-default/templates/record_main_tr.html(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/hydrooj/locales/zh.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/ui-default/templates/contest_balloon.html
- packages/ui-default/templates/homework_detail.html
- packages/ui-default/templates/record_main_tr.html
- packages/ui-default/templates/partials/contest_balloon.html
- packages/hydrooj/src/interface.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 50-50: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 35-35: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (5)
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (5)
1-23: Well-structured imports and interface design.The imports are appropriately organized and the
Probleminterface is well-designed with proper optional fields. The_tmpIdfield is a good pattern for maintaining React key stability during drag operations.
24-31: Appropriate component setup and state management.The functional component structure is well-implemented with proper state initialization. Adding temporary IDs to problems ensures React key stability during drag operations.
60-90: CRUD operations are well-implemented.The add, remove, and update operations are properly structured. The type assertion correctly handles the TypeScript issue from previous reviews, and the automatic label generation and API refresh on pid changes provide good user experience.
92-104: Drag and drop implementation is correct.The drag and drop logic properly handles reordering, including swapping both problems and their labels to maintain consistency. The state management is appropriate.
106-198: Well-structured render logic with proper React patterns.The component renders appropriately with correct drag-and-drop integration, proper internationalization, and good accessibility considerations. The table layout effectively displays the problem data with intuitive editing controls.
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
Show resolved
Hide resolved
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (1)
179-183: Consider using a button for the remove action.The remove action uses an anchor tag, but a button would be more semantic since this is an action rather than navigation.
- <a className="typo-a" onClick={() => handleRemove(index)}> - {i18n('Remove')} - </a> + <button type="button" className="typo-a" onClick={() => handleRemove(index)}> + {i18n('Remove')} + </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx(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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (5)
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (5)
1-23: Well-structured imports and type definitions.The component imports are appropriate for the functionality, and the TypeScript interfaces are comprehensive with proper optional field handling. The
_tmpIdfield is a good pattern for stable React keys.
24-47: Good state management and API optimization.The component properly initializes state and uses debounced API calls to fetch problem titles efficiently. The error handling for non-existent problems is appropriate.
48-94: Handlers are well-implemented with proper data normalization.The change handlers correctly manage state updates and normalize data by removing empty titles and default scores. The TypeScript type handling appears to have been addressed from previous reviews.
96-108: Drag-and-drop implementation is correct.The drag-and-drop logic properly handles problem reordering and maintains label consistency by swapping labels when problems are moved.
110-178: Well-structured JSX with proper React patterns.The table structure is semantic, drag-and-drop integration is correct, and form inputs are properly handled with appropriate validation and internationalization.
Also applies to: 184-202
|
Problems all solved. I still remain Now I have add a param
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/hydrooj/src/model/contest.ts (1)
1129-1154: Add validation for duplicate problem IDsThe
resolveContestProblemJsonfunction should validate that problem IDs are unique to prevent data integrity issues.Add duplicate PID validation as suggested in the previous review:
export function resolveContestProblemJson(text:string, validate = ContestProblemJsonSchema) { let _problemList = []; try { _problemList = validate(JSON.parse(text)); } catch (e) { throw new ValidationError('problems'); } const problems = [] as ContestProblem[]; const score = {} as Record<number, number>; + const seenPids = new Set<number>(); for (let i = 0; i < _problemList.length; i++) { const p = _problemList[i]; + if (seenPids.has(p.pid)) { + throw new ValidationError('problems', null, `Duplicate problem ID: ${p.pid}`); + } + seenPids.add(p.pid); if (typeof p.score === 'number' && p.score !== 100) { score[p.pid] = p.score; }
🧹 Nitpick comments (1)
packages/hydrooj/src/model/contest.ts (1)
840-880: Complex backward compatibility logic needs careful reviewThe edit function contains intricate backward compatibility logic that handles multiple scenarios. While functionally correct, consider the following:
Complex nested conditions: The logic for handling
pids,scores, andballoondata is quite complex and could be error-prone.Potential optimization: The balloon handling logic is duplicated in two places (lines 854-862 and 869-877).
Consider extracting the balloon handling logic into a helper function:
+function processBalloonData(balloonData: any, pid: number) { + if (!balloonData || !balloonData[pid]) return {}; + return { + balloon: typeof balloonData[pid] === 'string' ? { + color: balloonData[pid], + name: '', + } : balloonData[pid] + }; +} // Then use it in both places: -...($set.balloon - ? ( - typeof $set.balloon[pid] === 'string' ? { - balloon: { - color: $set.balloon[pid], - name: '', - }, - } : { balloon: $set.balloon[pid] } - ) : {}), +...processBalloonData($set.balloon, pid),This would reduce duplication and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/hydrooj/src/handler/record.ts(4 hunks)packages/hydrooj/src/interface.ts(3 hunks)packages/hydrooj/src/model/contest.ts(30 hunks)packages/hydrooj/src/upgrade.ts(2 hunks)packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx(1 hunks)packages/ui-default/pages/contest_edit.page.tsx(1 hunks)packages/ui-default/templates/contest_edit.html(1 hunks)packages/ui-default/templates/homework_edit.html(1 hunks)packages/utils/lib/common.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/ui-default/templates/contest_edit.html
- packages/hydrooj/src/interface.ts
- packages/ui-default/templates/homework_edit.html
- packages/hydrooj/src/handler/record.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/hydrooj/src/model/contest.ts (4)
packages/utils/lib/common.ts (1)
getAlphabeticId(240-247)packages/hydrooj/src/interface.ts (2)
Tdoc(259-300)ContestProblem(250-257)framework/framework/error.ts (1)
ValidationError(54-63)packages/hydrooj/src/libs.ts (1)
Schema(14-14)
🪛 Biome (1.9.4)
packages/hydrooj/src/model/contest.ts
[error] 820-820: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 868-868: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 869-869: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/hydrooj/src/upgrade.ts
[error] 630-630: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 631-632: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (24)
packages/ui-default/pages/contest_edit.page.tsx (3)
3-4: LGTM: Clean React imports addedThe React and ReactDOM imports are properly added to support the new ContestProblemEditor component integration.
8-8: LGTM: ContestProblemEditor importThe import statement for the new ContestProblemEditor component is correctly structured and follows the project's import conventions.
13-13: LGTM: Export style modernizationThe change from a separate
pageconstant declaration to a direct default export is a good modernization and follows current JavaScript/TypeScript best practices.packages/hydrooj/src/upgrade.ts (2)
7-7: LGTM: getAlphabeticId importThe import of
getAlphabeticIdutility function is correctly added to support the migration from pids to problems format.
620-642: Migration logic forballoonis safe; optional-chaining is a stylistic improvementThe existing guard
tdoc?.balloon && tdoc.balloon[pid]already ensures
tdoc.balloon[pid]is defined before accessing its properties, and the code correctly handles both string‐and‐object cases. No functional fix is required. If you’d like to tighten up the syntax, you can swap in optional chaining for readability:• File: packages/hydrooj/src/upgrade.ts
Lines ~627–635Suggested diff (purely stylistic):
- ...(tdoc?.balloon && tdoc.balloon[pid] ? { + ...(tdoc.balloon?.[pid] != null ? { balloon: { - name: typeof tdoc.balloon[pid] === 'string' ? '' : tdoc.balloon[pid].name, - color: typeof tdoc.balloon[pid] === 'string' ? tdoc.balloon[pid] : tdoc.balloon[pid].color, + name: typeof tdoc.balloon[pid] === 'string' + ? '' + : tdoc.balloon[pid].name, + color: typeof tdoc.balloon[pid] === 'string' + ? tdoc.balloon[pid] + : tdoc.balloon[pid].color, }, } : {}),• No changes needed for label handling—assigning
label = getAlphabeticId(idx)aligns with the schema.packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (4)
8-23: LGTM: Well-defined TypeScript interfacesThe TypeScript interfaces are properly structured with appropriate optional properties and clear field definitions. The
_tmpIdfield is correctly marked as optional and documented as used for React keys.
25-31: LGTM: Proper state initializationThe component properly initializes state with unique temporary IDs and default alphabetic labels. The initialization logic correctly handles cases where labels might not be provided.
36-51: LGTM: Optimized API fetchingThe debounced API fetching implementation is well-optimized and includes proper error handling for cases where problem IDs might not exist. The reduce function has been optimized to avoid O(n²) complexity.
53-69: LGTM: Proper data normalizationThe onChange handler correctly normalizes the data by removing empty titles and default scores, and properly handles the label logic to only include non-default labels in the output.
packages/hydrooj/src/model/contest.ts (15)
3-3: LGTM: Import additions are appropriateThe new imports for
SchemaandContestProblemare necessary for the schema validation and structured problem object functionality introduced in this refactor.Also applies to: 12-12
107-107: LGTM: Efficient lookup optimizationThe change from
tdoc.pids.includes(j.pid)toObject.hasOwn(tdoc.pid2idx, j.pid)improves performance by using object property lookup instead of array search.
147-149: LGTM: Proper adaptation to structured problemsThe changes correctly update the scoreboard header generation to use the new
problemsarray structure, with proper fallback for labels usingcp.label || getAlphabeticId(i - 1).Also applies to: 165-165
199-201: LGTM: Consistent iteration over structured problemsThe change to iterate over
tdoc.problemsinstead oftdoc.pidsis correct and maintains the same logic while adapting to the new structure.
289-289: LGTM: Proper OI rule adaptationThe changes correctly adapt the OI rule to use the new structure:
- Efficient lookup using
Object.hasOwn(tdoc.pid2idx, i.pid)- Proper score access using
tdoc.problems[tdoc.pid2idx[Number(i)]]?.scorewith safe optional chainingAlso applies to: 302-302
321-323: LGTM: Comprehensive OI rule scoreboard updatesAll the scoreboard-related changes properly adapt to the new structured problems:
- Correct iteration over
tdoc.problems- Proper fallback logic for titles and labels
- Safe optional chaining for score calculations
- Maintains backward compatibility
Also applies to: 328-328, 333-334, 347-347, 367-368
477-477: LGTM: Proper strictioi rule adaptationThe changes correctly adapt the strictioi rule to use the new structure:
- Proper filtering using
tdoc.problems.find((p) => p.pid === i.pid)- Correct score calculations using the pid2idx mapping
- Consistent handling of full mark calculations
Also applies to: 486-486, 511-512, 514-514, 523-530, 535-535
560-560: LGTM: Consistent ledo rule updatesThe changes properly adapt the ledo rule to use the new structure:
- Correct filtering using
tdoc.problems.find((p) => p.pid === i.pid)- Proper iteration over structured problems
- Accurate score calculations with penalty handling
Also applies to: 574-575, 577-577, 611-612, 615-615
641-641: LGTM: Homework rule properly updatedThe changes correctly adapt the homework rule to use the new structure:
- Efficient lookup using
Object.hasOwn(tdoc.pid2idx, j.pid)- Proper score calculations using the pid2idx mapping
- Correct iteration over structured problems with fallback logic
Also applies to: 649-649, 694-696, 702-702, 716-716, 753-754
814-832: LGTM: Well-implemented backward compatibilityThe backward compatibility logic properly handles the transition from
pidstoproblems:
- Constructs structured problems from legacy pids array
- Includes proper labels using
getAlphabeticId- Preserves score information from legacy data
- Maintains plugin compatibility as documented in the TODO comment
897-903: LGTM: Proper pid2idx mapping constructionThe change correctly builds the
pid2idxmapping from the structuredproblemsarray using an efficient reduce operation. This maintains the expected interface while adapting to the new structure.
951-951: LGTM: Proper balloon property accessThe change correctly accesses the balloon property from the structured problem object using
tdoc.problems.find((p) => p.pid === pid)?.balloonwith safe optional chaining.
1065-1065: LGTM: Correct pid extraction from problemsThe change properly extracts problem IDs using
tdoc.problems.map((p) => p.pid)to maintain compatibility with theproblem.getListfunction.
1119-1154: Schema and validation function implementation looks solidThe new
ContestProblemJsonSchemaandresolveContestProblemJsonfunction provide proper validation and normalization:Strengths:
- Comprehensive schema covering all problem fields
- Proper validation with error handling
- Smart default value omission logic
- Flexible validate parameter for plugin extensibility
Minor observations:
- The function omits labels that match the default alphabetic ID, which is good for storage efficiency
- Score and title handling properly omits default/empty values
- The validate parameter allows plugins to extend the schema as mentioned in the PR objectives
1198-1199: LGTM: Proper export of new functionalityThe exports correctly expose the new schema and validation function for use by other modules.
| if ($('#problem-editor').length) { | ||
| const problemsInput = $('[name=problems]'); | ||
| ReactDOM.createRoot($('#problem-editor')[0]).render( | ||
| <ContestProblemEditor | ||
| problems={JSON.parse(problemsInput.val() as string)} | ||
| onChange={(problems) => { | ||
| problemsInput.val(JSON.stringify(problems)); | ||
| }} | ||
| />, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON parsing
The JSON.parse call on line 21 could throw an error if the input contains malformed JSON, which would break the entire page initialization.
Add error handling around the JSON parsing:
if ($('#problem-editor').length) {
const problemsInput = $('[name=problems]');
- ReactDOM.createRoot($('#problem-editor')[0]).render(
- <ContestProblemEditor
- problems={JSON.parse(problemsInput.val() as string)}
- onChange={(problems) => {
- problemsInput.val(JSON.stringify(problems));
- }}
- />,
- );
+ try {
+ ReactDOM.createRoot($('#problem-editor')[0]).render(
+ <ContestProblemEditor
+ problems={JSON.parse(problemsInput.val() as string || '[]')}
+ onChange={(problems) => {
+ problemsInput.val(JSON.stringify(problems));
+ }}
+ />,
+ );
+ } catch (error) {
+ console.error('Failed to initialize ContestProblemEditor:', error);
+ // Fallback to empty array
+ ReactDOM.createRoot($('#problem-editor')[0]).render(
+ <ContestProblemEditor
+ problems={[]}
+ onChange={(problems) => {
+ problemsInput.val(JSON.stringify(problems));
+ }}
+ />,
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($('#problem-editor').length) { | |
| const problemsInput = $('[name=problems]'); | |
| ReactDOM.createRoot($('#problem-editor')[0]).render( | |
| <ContestProblemEditor | |
| problems={JSON.parse(problemsInput.val() as string)} | |
| onChange={(problems) => { | |
| problemsInput.val(JSON.stringify(problems)); | |
| }} | |
| />, | |
| ); | |
| } | |
| if ($('#problem-editor').length) { | |
| const problemsInput = $('[name=problems]'); | |
| try { | |
| ReactDOM.createRoot($('#problem-editor')[0]).render( | |
| <ContestProblemEditor | |
| problems={JSON.parse(problemsInput.val() as string || '[]')} | |
| onChange={(problems) => { | |
| problemsInput.val(JSON.stringify(problems)); | |
| }} | |
| />, | |
| ); | |
| } catch (error) { | |
| console.error('Failed to initialize ContestProblemEditor:', error); | |
| // Fallback to empty array | |
| ReactDOM.createRoot($('#problem-editor')[0]).render( | |
| <ContestProblemEditor | |
| problems={[]} | |
| onChange={(problems) => { | |
| problemsInput.val(JSON.stringify(problems)); | |
| }} | |
| />, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/ui-default/pages/contest_edit.page.tsx around lines 17 to 27, the
JSON.parse call on the problemsInput value can throw an error if the JSON is
malformed, breaking page initialization. Wrap the JSON.parse call in a try-catch
block to catch any parsing errors. In the catch block, handle the error
gracefully, for example by logging the error and initializing with an empty
array or default value to prevent the page from breaking.
| <a className="typo-a" onClick={() => handleRemove(index)}> | ||
| {i18n('Remove')} | ||
| </a> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard accessibility to remove action
The remove action uses an anchor tag with onClick but lacks proper keyboard accessibility.
Improve keyboard accessibility:
- <a className="typo-a" onClick={() => handleRemove(index)}>
- {i18n('Remove')}
- </a>
+ <button
+ type="button"
+ className="typo-a"
+ onClick={() => handleRemove(index)}
+ aria-label={`Remove problem ${problem.label || getAlphabeticId(index)}`}
+ >
+ {i18n('Remove')}
+ </button>Using a button element provides better semantic meaning and keyboard accessibility than an anchor tag for actions.
🤖 Prompt for AI Agents
In packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
around lines 187 to 189, replace the anchor tag used for the remove action with
a button element to improve semantic meaning and keyboard accessibility. Update
the element to a button, keep the onClick handler, and ensure it is focusable
and operable via keyboard inputs.
| <td {...providedDrag.dragHandleProps} className='col--drag' style={{ cursor: 'move' }}> | ||
| ⋮ | ||
| </td> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility of drag handle
The drag handle uses a Unicode character which may not be accessible to screen readers and keyboard users.
Improve accessibility:
- <td {...providedDrag.dragHandleProps} className='col--drag' style={{ cursor: 'move' }}>
- ⋮
- </td>
+ <td {...providedDrag.dragHandleProps} className='col--drag' style={{ cursor: 'move' }}>
+ <span aria-label="Drag to reorder" role="button" tabIndex={0}>
+ ⋮
+ </span>
+ </td>This adds proper ARIA labels and keyboard navigation support for the drag handle.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <td {...providedDrag.dragHandleProps} className='col--drag' style={{ cursor: 'move' }}> | |
| ⋮ | |
| </td> | |
| <td {...providedDrag.dragHandleProps} className='col--drag' style={{ cursor: 'move' }}> | |
| <span aria-label="Drag to reorder" role="button" tabIndex={0}> | |
| ⋮ | |
| </span> | |
| </td> |
🤖 Prompt for AI Agents
In packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
around lines 147 to 149, the drag handle uses a Unicode character without
accessibility features. To fix this, add appropriate ARIA attributes such as
aria-label to describe the drag handle's purpose and ensure it is keyboard
navigable by adding tabIndex and handling keyboard events if necessary. This
will improve screen reader support and keyboard accessibility.
| const onDragEnd = (result) => { | ||
| if (!result.destination) return; | ||
|
|
||
| const newProblems = [...problems]; | ||
| const [iX, iY] = [result.source.index, result.destination.index]; | ||
| [newProblems[iX], newProblems[iY]] = [newProblems[iY], newProblems[iX]]; | ||
| const [labelX, labelY] = [newProblems[iX].label, newProblems[iY].label]; | ||
| newProblems[iX].label = labelY; | ||
| newProblems[iY].label = labelX; | ||
|
|
||
| setProblems(newProblems); | ||
| onChange(newProblems); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review drag and drop logic for label handling
The drag and drop implementation has a potential issue with label swapping logic. When problems are reordered, the labels should typically maintain their alphabetic order based on position, not be swapped.
Consider updating the drag and drop logic to maintain alphabetic label order:
const onDragEnd = (result) => {
if (!result.destination) return;
const newProblems = [...problems];
const [iX, iY] = [result.source.index, result.destination.index];
[newProblems[iX], newProblems[iY]] = [newProblems[iY], newProblems[iX]];
- const [labelX, labelY] = [newProblems[iX].label, newProblems[iY].label];
- newProblems[iX].label = labelY;
- newProblems[iY].label = labelX;
+ // Reassign labels based on new positions to maintain alphabetic order
+ newProblems.forEach((problem, index) => {
+ problem.label = getAlphabeticId(index);
+ });
setProblems(newProblems);
onChange(newProblems);
};This ensures that after reordering, labels maintain their expected alphabetic sequence (A, B, C, etc.) based on position.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
around lines 103 to 115, the current drag and drop logic swaps the labels of the
moved problems, which can cause labels to be out of alphabetic order. Instead of
swapping labels, update the code to reorder the problems array based on drag
result and then reassign labels sequentially in alphabetic order (A, B, C, etc.)
according to their new positions. Finally, update the state and call onChange
with the relabeled problems array.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/hydrooj/src/upgrade.ts (1)
620-641: Fix optional chaining and improve balloon data handling.This is the same issue identified in previous reviews. The migration function has optional chaining problems and unsafe balloon data access that need to be addressed.
Apply the previously suggested fix to resolve the optional chaining issues and improve balloon data handling safety:
problems: tdoc.pids.map((pid, idx) => ({ pid, - ...(tdoc?.score && tdoc.score[pid] ? { score: tdoc.score[pid] } : {}), - ...(tdoc?.balloon && tdoc.balloon[pid] ? { + ...(tdoc?.score?.[pid] ? { score: tdoc.score[pid] } : {}), + ...(tdoc?.balloon?.[pid] ? { balloon: { - name: typeof tdoc.balloon[pid] === 'string' ? '' : tdoc.balloon[pid].name, - color: typeof tdoc.balloon[pid] === 'string' ? tdoc.balloon[pid] : tdoc.balloon[pid].color, + name: typeof tdoc.balloon[pid] === 'string' ? '' : tdoc.balloon[pid]?.name || '', + color: typeof tdoc.balloon[pid] === 'string' ? tdoc.balloon[pid] : tdoc.balloon[pid]?.color || '', }, } : {}), })),
🧹 Nitpick comments (1)
packages/hydrooj/src/model/contest.ts (1)
866-867: Fix optional chaining syntax.The static analysis correctly identifies that the optional chaining can be simplified for better readability and safety.
Apply this fix to improve the optional chaining:
- ...(p.score || ($set.score && $set.score[p.pid]) ? { score: $set.score[p.pid] || p.score } : {}), - ...(p.balloon || ($set.balloon && $set.balloon[p.pid]) ? { + ...(p.score || $set.score?.[p.pid] ? { score: $set.score?.[p.pid] || p.score } : {}), + ...(p.balloon || $set.balloon?.[p.pid] ? {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/hydrooj/src/model/contest.ts(30 hunks)packages/hydrooj/src/upgrade.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/hydrooj/src/model/contest.ts (4)
packages/utils/lib/common.ts (1)
getAlphabeticId(240-247)packages/hydrooj/src/interface.ts (2)
Tdoc(259-300)ContestProblem(250-257)framework/framework/error.ts (1)
ValidationError(54-63)packages/hydrooj/src/libs.ts (1)
Schema(14-14)
packages/hydrooj/src/upgrade.ts (1)
packages/hydrooj/src/pipelineUtils.ts (1)
iterateAllDomain(14-18)
🪛 Biome (1.9.4)
packages/hydrooj/src/model/contest.ts
[error] 866-866: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 867-867: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/hydrooj/src/upgrade.ts
[error] 629-629: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 630-631: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (16)
packages/hydrooj/src/model/contest.ts (16)
3-3: LGTM: Schema import is necessary for validation.The import of
Schemafrom 'schemastery' is correctly added to support the new contest problem JSON validation functionality.
12-12: LGTM: ContestProblem interface import is correct.The addition of
ContestProblemto the interface imports is necessary for the new structured problem representation.
107-107: LGTM: Correct migration to use pid2idx mapping.The change from checking
tdoc.pidsto usingObject.hasOwn(tdoc.pid2idx, j.pid)is correct for the new structured problems format.
147-169: LGTM: Proper migration to problems array in scoreboardHeader.The scoreboardHeader method correctly migrates from using
tdoc.pidstotdoc.problems, extracting bothpidandlabelinformation from the structured problem objects.
199-226: LGTM: Consistent problems array usage in scoreboardRow.The scoreboardRow method properly iterates over
tdoc.problemsto access bothpidand other problem metadata.
289-289: LGTM: Correct journal filtering with pid2idx.The filtering of journal entries using
Object.hasOwn(tdoc.pid2idx, i.pid)is correct for the new structure.
302-302: LGTM: Proper score calculation with problems array.The score calculation correctly uses
tdoc.problems[tdoc.pid2idx[Number(i)]]?.scoreto access problem-specific scores from the structured problems array.
477-477: LGTM: Correct journal filtering with problems array.The filtering using
tdoc.problems.find((p) => p.pid === i.pid)is correct for the new structure.
560-560: LGTM: Consistent journal filtering pattern.The filtering using
tdoc.problems.find((p) => p.pid === i.pid)maintains consistency with other rule implementations.
641-641: LGTM: Correct problem existence check.The use of
Object.hasOwn(tdoc.pid2idx, j.pid)for checking problem existence is correct.
814-832: LGTM: Proper backward compatibility for legacy plugins.The backward compatibility logic correctly handles the transition from
pidstoproblemsarrays while supporting existing plugins. The TODO comment appropriately indicates this is temporary.
895-901: LGTM: Correct pid2idx mapping generation.The function correctly builds the
pid2idxmapping from theproblemsarray, which is essential for maintaining compatibility with existing code that depends on this mapping.
949-949: LGTM: Correct balloon check migration.The change from
tdoc.balloon[pid]totdoc.problems.find((p) => p.pid === pid)?.ballooncorrectly adapts to the new structured problems format.
1063-1063: LGTM: Correct PID extraction from problems array.The change from
tdoc.pidstotdoc.problems.map((p) => p.pid)correctly extracts problem IDs from the new structured problems array.
1117-1152: LGTM: Well-designed schema and parser function.The
ContestProblemJsonSchemaandresolveContestProblemJsonfunction provide a robust way to validate and parse contest problem JSON data. The schema properly defines the structure and the parser correctly handles the transformation to the internal format.The
validateparameter allows for schema extension by plugins, which addresses the extensibility concern mentioned in past reviews.
1196-1197: LGTM: Correct export additions.The exports correctly include the new
resolveContestProblemJsonfunction andContestProblemJsonSchemafor use by other parts of the system.
|
I found that in contest if we want to know each problem's submit/pass count, the only way is open the scoreboard, and it seems that we don't storage these counts in database. I think open the scoreboard is troublesome when we only want to know the pass count, and frequently refresh scoreboard surely costs more server resource than for example create a new field to store in database. And I have question about the scoreboard. Hydro/packages/hydrooj/src/model/contest.ts Lines 191 to 194 in 2d79791 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/hydrooj/src/model/contest.ts (1)
1117-1152: Address past review feedback and improve validation.The new JSON schema validation and parsing function needs improvements based on past review comments:
Based on the past review feedback, this function still needs:
- Validation for duplicate problem IDs
- Avoiding the delete operator for better performance
- Proper handling of undefined properties
Apply this improvement to address the concerns:
export function resolveContestProblemJson(text:string, validate = ContestProblemJsonSchema) { let _problemList = []; try { _problemList = validate(JSON.parse(text)); } catch (e) { throw new ValidationError('problems'); } const problems = [] as ContestProblem[]; const score = {} as Record<number, number>; + const seenPids = new Set<number>(); for (let i = 0; i < _problemList.length; i++) { const p = _problemList[i]; + if (seenPids.has(p.pid)) throw new ValidationError('problems', null, `Duplicate problem ID: ${p.pid}`); + seenPids.add(p.pid); if (typeof p.score === 'number' && p.score !== 100) { score[p.pid] = p.score; } problems.push({ pid: p.pid, ...(p.score && p.score !== 100 ? { score: p.score } : {}), ...(p.title && p.title.length > 0 ? { title: p.title } : {}), ...(p.label && p.label.length > 0 && p.label !== getAlphabeticId(i) ? { label: p.label } : {}), }); } return { problems, score, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/hydrooj/src/handler/contest.ts(18 hunks)packages/hydrooj/src/model/contest.ts(31 hunks)packages/ui-default/templates/partials/scoreboard.html(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/hydrooj/src/model/contest.ts (4)
packages/utils/lib/common.ts (1)
getAlphabeticId(240-247)packages/hydrooj/src/interface.ts (2)
Tdoc(259-303)ContestProblem(250-257)framework/framework/error.ts (1)
ValidationError(54-63)packages/hydrooj/src/libs.ts (1)
Schema(14-14)
🪛 Biome (1.9.4)
packages/hydrooj/src/model/contest.ts
[error] 866-866: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 867-867: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (6)
packages/ui-default/templates/partials/scoreboard.html (1)
5-5: LGTM: Sensible performance optimization for large scoreboards.The conditional tooltip attribute assignment improves performance by using the standard HTML
titleattribute instead ofdata-tooltipfor large scoreboards. The logic is consistent across both locations.Also applies to: 70-70
packages/hydrooj/src/model/contest.ts (2)
895-902: LGTM: Efficient pid2idx mapping generation.The
getfunction now builds thepid2idxmapping dynamically from theproblemsarray, which is a clean approach for providing backward compatibility while maintaining performance.
814-882: Add tests to cover legacy contest data compatibilityThe backward‐compatibility logic in the
addandeditfunctions handles multiple migration scenarios (mappingpidstoproblems, merging legacyscoreentries, and normalizingballoondata). To ensure no edge cases slip through, please add unit/integration tests covering at least:
- When
data.problemsis empty butpidsis non‐empty (add)- When only
pidsis provided on edit (noproblemsorscore)- When updating only
score, onlyballoon, and both on existing contests- Legacy contests with a mix of
tdoc.scoreand per‐problemscoreentriesKey locations:
- packages/hydrooj/src/model/contest.ts, lines 816–821 (add/backward compatibility)
- packages/hydrooj/src/model/contest.ts, lines 840–862 (edit/backward compatibility)
packages/hydrooj/src/handler/contest.ts (3)
790-790: LGTM: Well-designed large board detection and optimization.The
isLargeBoardcalculation (rows × users > 10000) is a sensible heuristic for performance optimization, and passing it to the template allows for conditional rendering optimizations.Also applies to: 793-793
824-831: Problem indexing logic in ghost view is consistentI’ve confirmed that the ghost view’s
pid2idxloop inpackages/hydrooj/src/handler/contest.tsproduces exactly the same mapping as thereduce-basedpid2idxinpackages/hydrooj/src/model/contest.ts. The journal filter (pid2idx[s.pid] !== undefined) also correctly handles any out-of-range entries. No discrepancies found—no changes needed here.
303-303: JSON problem parsing and validation is correctly implementedThe
resolveContestProblemJsonfunction inpackages/hydrooj/src/model/contest.tsalready:
- Parses the input string with
JSON.parse(text)- Validates the result against
ContestProblemJsonSchema- Catches both parse errors and schema validation failures
- Rethrows a
ValidationError('problems')in all invalid-input scenariosThe contest edit handler invokes this function directly and will propagate the
ValidationErrorto the framework’s error handler. No additional error-handling or try/catch blocks are needed.
|
I have a contest with 100 problems and 500 user. The scoreboard will often make my browser no response when trying to scroll down/right the page. I found the |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/hydrooj/src/model/contest.ts (1)
1117-1152: Add validation for duplicate problem IDs.The schema and function are well-implemented, but the validation for duplicate problem IDs mentioned in previous reviews is still missing. This could lead to issues if duplicate PIDs are provided.
Apply this diff to add duplicate PID validation:
export function resolveContestProblemJson(text:string, validate = ContestProblemJsonSchema) { let _problemList = []; try { _problemList = validate(JSON.parse(text)); } catch (e) { throw new ValidationError('problems'); } const problems = [] as ContestProblem[]; const score = {} as Record<number, number>; + const seenPids = new Set<number>(); for (let i = 0; i < _problemList.length; i++) { const p = _problemList[i]; + if (seenPids.has(p.pid)) throw new ValidationError('problems', null, `Duplicate problem ID: ${p.pid}`); + seenPids.add(p.pid); if (typeof p.score === 'number' && p.score !== 100) { score[p.pid] = p.score; } problems.push({ pid: p.pid, ...(p.score && p.score !== 100 ? { score: p.score } : {}), ...(p.title && p.title.length > 0 ? { title: p.title } : {}), ...(p.label && p.label.length > 0 && p.label !== getAlphabeticId(i) ? { label: p.label } : {}), }); } return { problems, score, }; }
🧹 Nitpick comments (1)
packages/hydrooj/src/model/contest.ts (1)
839-878: Consider using optional chaining for cleaner code.The backward compatibility logic is comprehensive and correct. However, the code could be improved by using optional chaining as suggested by static analysis.
Apply this diff to improve readability:
- ...($set.balloon - ? ( - typeof $set.balloon[pid] === 'string' ? { - balloon: { - color: $set.balloon[pid], - name: '', - }, - } : { balloon: $set.balloon[pid] } - ) : {}), + ...($set.balloon?.[pid] ? ( + typeof $set.balloon[pid] === 'string' ? { + balloon: { + color: $set.balloon[pid], + name: '', + }, + } : { balloon: $set.balloon[pid] } + ) : {}),Similar improvements can be applied to other conditional property access patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/hydrooj/src/handler/contest.ts(18 hunks)packages/hydrooj/src/model/contest.ts(31 hunks)packages/onsite-toolkit/index.ts(4 hunks)packages/scoreboard-xcpcio/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/scoreboard-xcpcio/index.ts
- packages/onsite-toolkit/index.ts
- packages/hydrooj/src/handler/contest.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/hydrooj/src/model/contest.ts (5)
packages/utils/lib/common.ts (1)
getAlphabeticId(240-247)packages/hydrooj/src/interface.ts (2)
Tdoc(259-303)ContestProblem(250-257)framework/framework/error.ts (1)
ValidationError(54-63)packages/hydrooj/src/handler/contest.ts (10)
get(38-75)get(154-168)get(191-239)get(275-294)get(396-438)get(443-460)get(547-558)get(563-577)get(600-617)get(666-696)packages/hydrooj/src/libs.ts (1)
Schema(14-14)
🪛 Biome (1.9.4)
packages/hydrooj/src/model/contest.ts
[error] 866-866: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 867-867: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
packages/hydrooj/src/model/contest.ts (10)
3-3: Import addition looks good.The Schema import from schemastery is properly added to support the new JSON schema validation functionality.
107-107: Problem filtering patterns are now consistent.All contest rules now consistently use
Object.hasOwn(tdoc.pid2idx, *.pid)for efficient O(1) problem existence checks, which addresses the previous review feedback about unifying filtering patterns.Also applies to: 289-289, 477-477, 560-560, 641-641
147-149: Scoreboard header functions correctly updated.The migration from iterating over
pidstoproblemsis properly implemented, correctly extracting problem metadata and handling optional fields likelabelandtitle.Also applies to: 321-323, 694-696
199-201: Scoreboard row functions correctly updated.The migration from
pidstoproblemsin scoreboard row functions is properly implemented, maintaining the same functionality while using the new structured data.Also applies to: 367-368, 511-512, 574-575, 753-754
302-302: Score calculation logic correctly updated.The score calculations properly use the new problems structure with
tdoc.problems[tdoc.pid2idx[pid]]?.score || 100pattern, correctly handling optional scores and defaulting to 100.Also applies to: 347-347, 486-486, 514-514, 523-523, 527-529, 535-535, 577-577, 615-615, 648-649
814-831: Backward compatibility logic is appropriately implemented.The migration logic correctly handles cases where old plugins still use
pidsby converting them to the newproblemsstructure. The TODO comment appropriately indicates this is temporary support.
895-902: Efficient pid2idx mapping implementation.The
getfunction correctly builds thepid2idxmapping from the problems array, enabling efficient O(1) problem lookups throughout the codebase.
949-949: Balloon configuration access correctly updated.The function properly accesses balloon configuration from the problems array using the pid2idx mapping.
1063-1063: Problem ID extraction correctly updated.The function properly extracts problem IDs from the problems array for the
problem.getListcall.
1196-1197: Global exports correctly updated.The new functions are properly exported to maintain API consistency with the global Hydro model.
bhscer
left a comment
bhscer
left a comment
There was a problem hiding this comment.
It seems that everything is ok?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/hydrooj/src/handler/problem.ts (1)
308-308: Improve with optional chaining and approve the logic enhancement.The addition of the explicit length check
this.tdoc?.langs.lengthis a good improvement to ensure only non-empty arrays are included. However, the condition can be simplified using optional chaining as suggested by static analysis.Apply this diff to use optional chaining:
- if (this.tdoc?.langs && this.tdoc?.langs.length) t.push(this.tdoc.langs); + if (this.tdoc?.langs?.length) t.push(this.tdoc.langs);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/hydrooj/src/handler/problem.ts(2 hunks)packages/hydrooj/src/handler/record.ts(4 hunks)packages/hydrooj/src/interface.ts(4 hunks)packages/ui-default/pages/contest_edit.page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ui-default/pages/contest_edit.page.tsx
- packages/hydrooj/src/handler/record.ts
- packages/hydrooj/src/interface.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/hydrooj/src/handler/problem.ts
[error] 308-308: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (1)
packages/hydrooj/src/handler/problem.ts (1)
282-282: LGTM: Correctly implements new contest problem indexing.The change from checking
pidsarray inclusion to usingObject.hasOwnon thepid2idxmap aligns with the migration to structured problem objects. This provides efficient O(1) lookup instead of O(n) array search.
| if (idx === undefined) throw new ValidationError('pid'); | ||
| this.tdoc.problems[idx].score = score; | ||
| // TODO: remove `score` field later | ||
| this.tdoc.score = this.tdoc.problems.reduce( |
There was a problem hiding this comment.
do not store redundant data in db
There was a problem hiding this comment.
I think we should update score since some old plugins may still using this.
There was a problem hiding this comment.
it is intented to drop legacy apis in beta versions. if you keep this now, it means we have to keep supporting this thing until v6
packages/hydrooj/src/interface.ts
Outdated
| dag?: TrainingNode[]; | ||
|
|
||
| // not stored in database | ||
| pid2idx?: Record<number, number>; |
There was a problem hiding this comment.
Do not add to interface if it's not stored
There was a problem hiding this comment.
There are some many places we use tdoc.pid2idx to get a problem's index, it's easy and needn't change many function's param (for example, the contest scoreboard functions).
I'm not good at typescript, I'm not sure about if there is a better way to do this.
There was a problem hiding this comment.
The better way is to use Array.find() and Array.findIndex() at where you need that object
| color: Schema.string(), | ||
| name: Schema.string(), | ||
| }), | ||
| })).required(); |
There was a problem hiding this comment.
still, this can't be modified by plugins
There was a problem hiding this comment.
User can create there schema throng Schema.intersect, but I have no idea about how can user overwrite this schema.
There was a problem hiding this comment.
And that's the issue; if you are not sure if it's a good plugin interface design, try to leave builtin fields empty, and to implement them via a plugin (e.g. use plugin to create the balloon field), by doing this you'll clearly understand what plugins can do using slots provided.
| export const ContestProblemJsonSchema = Schema.array(Schema.object({ | ||
| pid: Schema.number().required(), | ||
| label: Schema.string(), | ||
| title: Schema.string(), |
There was a problem hiding this comment.
I doubt if there's really a need to override problem title
There was a problem hiding this comment.
I remember that Polygon has such function in contest edit page.
There was a problem hiding this comment.
In what situations would you need this functionality, that you must change the title via contest setting, instead of editing the source problem title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (2)
97-99: Replace anchor with button for remove action
Same accessibility issue flagged previously – still unresolved.
186-189: Relabel after reorder instead of swapping labelsSwapping labels causes out-of-order sequences (A, C, B…).
Recompute labels withgetAlphabeticId(index)after reordering.packages/hydrooj/src/model/contest.ts (1)
1128-1152: Validate duplicate problem IDs in JSON parser
resolveContestProblemJsondoes not guard against repeatedpids, which later breakspid2idxmapping and scoreboard logic.const problems = [] as ContestProblem[]; const score = {} as Record<number, number>; +const seen = new Set<number>(); for (let i = 0; i < _problemList.length; i++) { const p = _problemList[i]; + if (seen.has(p.pid)) throw new ValidationError('problems', null, `Duplicate pid ${p.pid}`); + seen.add(p.pid);
🧹 Nitpick comments (4)
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (3)
61-61: Add ARIA wrapper for drag handleWrap the Unicode glyph in a focusable element with
role="button"&aria-labelso screen-reader and keyboard users can operate the drag handle.
80-85: Avoid uncontrolled input whenlabelis undefined
value={problem.label}throws a React warning iflabelisundefined.
Default to empty string.- value={problem.label} + value={problem.label ?? ''}
117-132: Clean up debounced fetch on unmount
debouncekeeps an active timer; addreturn () => fetchProblemTitles.cancel()inuseEffectto prevent memory leaks.packages/hydrooj/src/model/contest.ts (1)
864-878: Optional-chaining can shorten nested ternariesThe long chain of
... && $set.score && $set.score[p.pid]is hard to read; optional-chaining ($set.score?.[p.pid]) improves clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/hydrooj/src/handler/contest.ts(17 hunks)packages/hydrooj/src/model/contest.ts(31 hunks)packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx(1 hunks)packages/ui-default/pages/contest_edit.page.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui-default/pages/contest_edit.page.tsx
- packages/hydrooj/src/handler/contest.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/hydrooj/src/model/contest.ts
[error] 866-866: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 867-867: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
| <td>{problemRawTitles[problem.pid]}</td> | ||
| <td> |
There was a problem hiding this comment.
Title lookup uses different key than storage
fetchProblemTitles stores titles by cur.docId, but the render path reads them with problem.pid.
If docId !== pid (e.g. Mongo ObjectId vs. numeric pid) titles will never show.
- setProblemRawTitles(res.reduce((acc, cur) => {
- acc[cur.docId] = cur.title;
+ setProblemRawTitles(res.reduce((acc, cur) => {
+ acc[cur.pid] = cur.title; // keep the same key that render path uses
return acc;
}, {}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <td>{problemRawTitles[problem.pid]}</td> | |
| <td> | |
| setProblemRawTitles(res.reduce((acc, cur) => { | |
| - acc[cur.docId] = cur.title; | |
| + acc[cur.pid] = cur.title; // use the same key that render path expects | |
| return acc; | |
| }, {})); |
🤖 Prompt for AI Agents
In packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
at lines 69-70, the code uses problem.pid to access problemRawTitles, but
fetchProblemTitles stores titles keyed by cur.docId. To fix this, ensure the key
used to access problemRawTitles matches the key used for storage by replacing
problem.pid with problem.docId or the appropriate identifier consistent with
fetchProblemTitles.
Your are right, I don't know why I didn't think of that. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/hydrooj/src/upgrade.ts (1)
629-635: Fix optional chaining and improve balloon data handling.This is the same issue identified in previous reviews. The migration logic has redundant existence checks and potential runtime errors in balloon handling.
Apply the previously suggested fix:
- if (tdoc?.score && tdoc.score[pid]) cp.score = tdoc.score[pid]; - if (tdoc?.balloon && tdoc.balloon[pid]) { + if (tdoc?.score?.[pid]) cp.score = tdoc.score[pid]; + if (tdoc?.balloon?.[pid]) { if (typeof tdoc.balloon[pid] === 'string') { cp.balloon = { name: '', color: tdoc.balloon[pid] }; } else { - cp.balloon = tdoc.balloon[pid]; + cp.balloon = tdoc.balloon[pid] || { name: '', color: '' }; } }packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (3)
108-119: Title lookup key mismatch remains unresolved.The same issue from previous reviews persists: titles are stored by
cur.docIdbut accessed byproblem.pid(line 66), causing titles to never display whendocId !== pid.Apply the previously suggested fix:
setProblemRawTitles(res.reduce((acc, cur) => { - acc[cur.docId] = cur.title; + acc[cur.pid] = cur.title; // Match the access key used in render return acc; }, {}));
85-87: Accessibility issue with remove action remains unaddressed.The remove action still uses an anchor tag instead of a button element, which impacts semantic meaning and keyboard accessibility.
Apply the previously suggested fix to use a proper button element with ARIA labeling.
56-58: Drag handle accessibility issue persists.The drag handle still uses a Unicode character without proper accessibility features for screen readers and keyboard users.
Apply the previously suggested fix to add appropriate ARIA labels and keyboard navigation support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
packages/hydrooj/src/handler/contest.ts(12 hunks)packages/hydrooj/src/handler/homework.ts(5 hunks)packages/hydrooj/src/handler/problem.ts(1 hunks)packages/hydrooj/src/handler/record.ts(5 hunks)packages/hydrooj/src/interface.ts(3 hunks)packages/hydrooj/src/model/contest.ts(15 hunks)packages/hydrooj/src/upgrade.ts(2 hunks)packages/migrate/scripts/hustoj.ts(1 hunks)packages/migrate/scripts/poj.ts(1 hunks)packages/migrate/scripts/syzoj.ts(1 hunks)packages/migrate/scripts/universaloj.ts(1 hunks)packages/onsite-toolkit/index.ts(4 hunks)packages/scoreboard-xcpcio/index.ts(3 hunks)packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx(1 hunks)packages/ui-default/pages/contest_balloon.page.tsx(3 hunks)packages/ui-default/pages/contest_edit.page.tsx(2 hunks)packages/ui-default/templates/components/contest.html(1 hunks)packages/ui-default/templates/contest_balloon.html(1 hunks)packages/ui-default/templates/contest_edit.html(1 hunks)packages/ui-default/templates/contest_manage.html(2 hunks)packages/ui-default/templates/contest_problemlist.html(3 hunks)packages/ui-default/templates/homework_detail.html(1 hunks)packages/ui-default/templates/homework_edit.html(1 hunks)packages/ui-default/templates/problem_detail.html(1 hunks)packages/utils/lib/common.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/ui-default/templates/contest_balloon.html
- packages/ui-default/templates/homework_edit.html
- packages/ui-default/templates/contest_edit.html
- packages/ui-default/templates/problem_detail.html
- packages/ui-default/templates/homework_detail.html
- packages/ui-default/templates/components/contest.html
- packages/ui-default/pages/contest_edit.page.tsx
- packages/ui-default/templates/contest_problemlist.html
- packages/ui-default/pages/contest_balloon.page.tsx
- packages/utils/lib/common.ts
- packages/onsite-toolkit/index.ts
- packages/hydrooj/src/handler/homework.ts
- packages/hydrooj/src/interface.ts
- packages/scoreboard-xcpcio/index.ts
- packages/hydrooj/src/handler/record.ts
- packages/hydrooj/src/model/contest.ts
- packages/hydrooj/src/handler/contest.ts
- packages/ui-default/templates/contest_manage.html
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/hydrooj/src/upgrade.ts (2)
packages/hydrooj/src/pipelineUtils.ts (1)
iterateAllDomain(14-18)packages/hydrooj/src/interface.ts (1)
ContestProblemConfig(250-255)
🪛 Biome (1.9.4)
packages/hydrooj/src/handler/problem.ts
[error] 308-308: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/hydrooj/src/upgrade.ts
[error] 630-630: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 631-631: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
packages/hydrooj/src/handler/problem.ts (1)
308-308: LGTM! The additional length check improves language filtering logic.The change correctly prevents empty language arrays from being pushed into the language list, which aligns with the broader problem customization improvements in this PR.
Note: The static analysis suggestion to use optional chaining is unnecessary here since
this.tdoc?.langsis already checked in the first condition, makingthis.tdoc?.langs.lengthsafe to access.packages/migrate/scripts/hustoj.ts (1)
291-291: LGTM: Migration script correctly adapted to new ContestModel.add signature.The addition of the empty
problemConfigobject parameter aligns with the PR's goal of restructuring the Tdoc schema for enhanced problem customization. Using an empty object is appropriate for migrating legacy contest data that doesn't have the new problem configuration structure.packages/migrate/scripts/syzoj.ts (1)
289-289: LGTM: Consistent migration script update for new problemConfig parameter.The insertion of the empty
problemConfigobject parameter maintains consistency with other migration scripts and properly accommodates the new ContestModel.add signature for enhanced problem customization support.packages/migrate/scripts/universaloj.ts (1)
327-327: LGTM: Migration script properly updated for new contest model signature.The addition of the empty
problemConfigobject parameter is consistent with the coordinated changes across all migration scripts and correctly positions the parameter before the unrated boolean flag.packages/migrate/scripts/poj.ts (1)
204-204: LGTM: Final migration script consistently updated for new problemConfig support.The insertion of the empty
problemConfigobject parameter completes the coordinated update across all migration scripts, ensuring they properly work with the new ContestModel.add signature that supports enhanced problem customization.packages/hydrooj/src/upgrade.ts (1)
621-645: Migration logic is well-structured and follows established patterns.The migration function correctly:
- Iterates through all domains and contests systematically
- Transforms old
scoreandballoonfields into the unifiedproblemConfigstructure- Only includes non-default values in the new structure
- Provides appropriate logging for migration progress
- Follows the established pattern of other migration functions in this file
The approach aligns well with the broader codebase changes transitioning to the new
problemConfigstructure.packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx (1)
93-225: Well-structured component that effectively supports the new problemConfig system.The component provides a comprehensive interface for managing contest problems with:
- Clean drag-and-drop reordering functionality
- Proper state management and data flow
- Good integration with the new
problemConfigstructure mentioned in the PR objectives- Reasonable API debouncing for performance
- Clear separation of concerns with the
DraggableRowsubcomponentThis aligns well with the PR's goal of enhancing problem customization through schema restructuring.
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
Show resolved
Hide resolved
packages/ui-default/components/contestProblemEditor/ContestProblemEditor.tsx
Show resolved
Hide resolved
undefined-moe
left a comment
undefined-moe
left a comment
There was a problem hiding this comment.
Please take a look at all the changes and make sure that you didn't change irrelevant lines.
| /** @deprecated */ | ||
| balloon?: Record<number, string | { color: string, name: string }>; | ||
| /** @deprecated */ | ||
| score?: Record<number, number>; |
There was a problem hiding this comment.
remove these types instead of marking as deprecate; deprecated means "do not use but it still works"
| const effective = {}; | ||
| for (const j of journal) { | ||
| if (tdoc.pids.includes(j.pid)) effective[j.pid] = j; | ||
| if (tdoc.pids.find((pid) => pid === j.pid)) effective[j.pid] = j; |
| if (autoHide) this.checkPerm(PERM.PERM_EDIT_PROBLEM); | ||
| let problemConfig = {} as Record<number, ContestProblemConfig>; | ||
| try { | ||
| problemConfig = JSON.parse(_problemConfig); |
There was a problem hiding this comment.
this can lead to nosql injection
There was a problem hiding this comment.
Sorry for my late reply.
So should we for example create a verify method in ContestEditHandler, then plugin can use ctx.withHandlerClass to modify the verify method?
| } | ||
| for (const i in display) { | ||
| score += ((tdoc.score?.[i] || 100) * (display[i].score || 0)) / 100; | ||
| score += ((tdoc.problemConfig[Number(i)]?.score || 100) * (display[i].score || 0)) / 100; |
3 participants
!!! Still need careful check if any logic is wrong or some changes is missing !!!
Idea is from: #1015
Currently call
contest.addorcontest.editwithoutdata.problemswill automatically build it, so that maybe friendly for plugin users.Current ContestHandlers will also update
pid,score,balloon.Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation
Chores