core&ui: contest balloon notice - #1071
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughMessage delivery now supports multi-recipient messages: Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/hydrooj/src/model/contest.ts (4)
1111-1128: Harden event handler: add error handling and null-guards; avoid unhandled rejections
- IIFE has no catch; failures (e.g., DB/network) will produce unhandled rejections.
- team/tdoc/pdoc may be null; access to team.avatar/pdoc.title would throw.
Apply this patch:
@@ - ctx.on('contest/balloon', (domainId, tid, bdoc) => { + ctx.on('contest/balloon', (domainId, tid, bdoc) => { if (!bdoc.first) return; - (async () => { + (async () => { const tsdocs = await getMultiStatus(domainId, { docId: tid, subscribe: 1 }).toArray(); const uids = Array.from<number>(new Set(tsdocs.map((tsdoc) => tsdoc.uid))); - const [team, tdoc, pdoc] = await Promise.all([ + const [team, tdoc, pdoc] = await Promise.all([ UserModel.getById(domainId, bdoc.uid), get(domainId, tid), ProblemModel.get(domainId, bdoc.pid), ]); - await MessageModel.send(1, uids, JSON.stringify({ + if (!team || !tdoc || !pdoc || !uids.length) return; + await MessageModel.send(1, uids, JSON.stringify({ message: 'Team {0} is the first to solve problem {1} ({2})', - avatar: avatar(team.avatar), + avatar: avatar(team.avatar), params: [team.uname, getAlphabeticId(tdoc.pids.indexOf(bdoc.pid)), pdoc.title], }), MessageModel.FLAG_I18N | MessageModel.FLAG_RICHTEXT); - })(); + })().catch((err) => ctx.logger?.error?.('contest/balloon notify failed: %o', err)); });
1123-1127: Message content/i18n keyingYou set FLAG_I18N with a literal English template. If your i18n expects stable keys, prefer a namespaced key (e.g., 'contest.balloon.first') and move the English to locale files.
1125-1126: Problem label fallbackgetAlphabeticId(tdoc.pids.indexOf(bdoc.pid)) returns '?' if pid not found. If that’s possible (e.g., rejudge outside pids), consider guarding to avoid confusing output.
1129-1133: LGTM: indexes “basic” and partial “first” are the right constraints for data integrity. Consider adding a read index for { domainId:1, tid:1, pid:1, _id:1 } if you foresee frequent range scans by pid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/hydrooj/src/handler/home.ts(2 hunks)packages/hydrooj/src/handler/user.ts(1 hunks)packages/hydrooj/src/interface.ts(1 hunks)packages/hydrooj/src/model/contest.ts(7 hunks)packages/hydrooj/src/model/message.ts(1 hunks)packages/hydrooj/src/service/db.ts(2 hunks)packages/ui-default/components/message/index.page.ts(4 hunks)packages/ui-default/components/message/worker.ts(2 hunks)packages/ui-default/pages/contest_balloon.page.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/hydrooj/src/service/db.ts (4)
packages/hydrooj/src/model/document.ts (1)
coll(19-19)packages/hydrooj/src/model/user.ts (1)
coll(22-22)packages/hydrooj/src/model/discussion.ts (1)
coll(42-42)packages/hydrooj/src/model/oplog.ts (1)
coll(8-8)
packages/hydrooj/src/handler/user.ts (1)
framework/framework/error.ts (1)
ForbiddenError(50-50)
packages/ui-default/components/message/index.page.ts (3)
packages/ui-default/constant/message.js (8)
FLAG_I18N(5-5)FLAG_I18N(5-5)FLAG_ALERT(2-2)FLAG_ALERT(2-2)FLAG_INFO(4-4)FLAG_INFO(4-4)FLAG_RICHTEXT(3-3)FLAG_RICHTEXT(3-3)packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/notification/index.ts (1)
Notification(26-83)
packages/ui-default/pages/contest_balloon.page.tsx (1)
packages/ui-default/utils/pjax.js (1)
pjax(10-10)
packages/hydrooj/src/model/contest.ts (6)
framework/utils/lib/common.ts (2)
sleep(130-134)getAlphabeticId(240-247)packages/hydrooj/src/model/record.ts (1)
RecordModel(20-283)packages/hydrooj/src/model/message.ts (1)
apply(78-84)packages/hydrooj/src/model/document.ts (2)
apply(412-447)getMultiStatus(284-288)packages/hydrooj/src/context.ts (2)
Context(31-41)Context(68-88)packages/hydrooj/src/model/problem.ts (2)
getMultiStatus(228-230)ProblemModel(78-706)
⏰ 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). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (23)
packages/hydrooj/src/service/db.ts (1)
123-127: Verify that silently continuing after index creation failures is acceptable.Index creation is now wrapped in try/catch blocks that log errors but continue execution. While this prevents startup failures, missing indexes can cause:
- Significant query performance degradation
- Unexpected query timeouts or failures
- Difficult-to-diagnose production issues
Consider whether index creation failures should halt initialization or if there's a monitoring/alerting mechanism to surface these errors beyond logs.
Also applies to: 144-148
packages/ui-default/components/message/worker.ts (2)
43-44: LGTM! Improved notification handling with inter-port messaging.Moving notification creation from the worker context to the main thread via
postMessageis the correct approach, as workers have limited access to browser APIs. The timeout adjustment from 5s to 3s provides quicker user feedback.
90-91: LGTM! Added proper port lifecycle cleanup.The 'close' and 'error' event handlers ensure ports are removed from the set when they become invalid, preventing memory leaks and message delivery to dead ports.
packages/hydrooj/src/handler/user.ts (1)
177-177: LGTM! More specific error messaging.Adding the 'no-challenge' message to
ForbiddenErrorimproves debuggability for WebAuthn authentication failures.packages/hydrooj/src/model/message.ts (1)
24-28: LGTM! Efficient multi-recipient message storage.The shift from inserting one document per recipient (
insertMany) to a single document with multiple recipients (insertOne) is more storage-efficient while maintaining broadcast functionality. Thetofield normalization to an array (line 24) ensures consistent handling of both single and multiple recipients.packages/hydrooj/src/interface.ts (1)
314-314: LGTM! Interface change enables multi-recipient messaging.Updating
tofromnumbertonumber | number[]is the foundation for the multi-recipient message support implemented across this PR. Consumers of this interface now need to handle both single and array recipients appropriately.packages/hydrooj/src/handler/home.ts (2)
569-587: LGTM! Proper multi-recipient message handling.The updated logic correctly handles the new
tofield as either a number or array:
- Line 571: Flattens potential arrays when collecting UIDs
- Lines 577-586: Creates parsed entries for each target user, ensuring both sent and received messages are properly organized
This aligns well with the
MessageDoc.tointerface change.
607-607: Verify the impact of restricting deletion to senders only.Message deletion is now limited to senders (
msg.from === this.user._id). Previously, recipients could also delete messages. This is more secure but represents a breaking behavioral change that may affect user expectations.Consider whether this aligns with your intended UX - users may expect to be able to delete messages from their inbox.
packages/ui-default/components/message/index.page.ts (4)
12-23: LGTM! Avatar preservation for i18n messages.Line 18 correctly preserves the avatar from i18n content into
msg.mdoc.avatarfor consistent rendering. This ensures avatars are available regardless of whether the message uses i18n formatting.
24-44: LGTM! Dual notification system with proper gating.The
systemNotificationparameter creates two distinct notification paths:
- In-app notifications (default): Shows alerts/info via dialogs and VjNotification components (lines 24-44)
- Browser notifications (systemNotification=true): Uses the native Notification API with distinct handling for system messages (uid === 1) vs user messages (lines 46-61)
The early returns (lines 34, 43, 60) prevent unintended fallthrough between notification types.
Also applies to: 46-61
63-78: LGTM! Avatar support in message rendering.Line 69 conditionally includes the avatar in VjNotification when available for system messages, enhancing the visual presentation of notifications.
94-95: LGTM! Worker notification routing.The worker correctly routes 'notification' type messages to
onmessagewithsystemNotification=true, ensuring browser notifications are triggered from the worker context.packages/ui-default/pages/contest_balloon.page.tsx (1)
88-89: No changes needed—selector spelling is correct and intentional.The verification confirms that
constest_balloon-tbodyis not a typo. Both the selector in the code and the actual DOM element in the template use the identical spelling consistently. The selector will match successfully, and the PJAX logic works as intended.Likely an incorrect or invalid review comment.
packages/hydrooj/src/model/contest.ts (10)
4-7: Imports look fine; adding sleep/Context is consistent with new usage.
15-15: avatar import acknowledged.
22-25: New model imports (MessageModel, ProblemModel, RecordModel, UserModel) align with refactor.
238-241: LGTM: switching to UserModel.getListForRender for ACM scoreboard is correct and keeps projection logic centralized.
412-415: LGTM: same refactor for OI scoreboard; consistent with ACM path.
780-784: LGTM: same refactor for Homework scoreboard; consistent use across rules.
882-882: Confirm bus.emit reaches ctx.on('contest/balloon') listeners across workersPrevious patterns use bus.parallel for cross-module events. Ensure emit here propagates to the same bus ctx.on is attached to; otherwise, first-solve notifications may not fire in clustered deployments.
1111-1116: Audience selection: confirm subscribe: 1 semanticsYou filter tsdocs by subscribe: 1. Ensure this field actually represents “wants contest notifications”; otherwise, some participants may miss the balloon notice. Consider fallback to participants of the contest if the flag is absent.
1136-1138: LGTM: exporting apply on the global surface is consistent with other models.
1123-1126: MessageModel.send correctly supports array recipients and indexes are adequateThe send method signature already accepts
to: number | number[](line 19 of message.ts), normalizes arrays consistently, and stores each message as a single document withtoas an array field. The message collection index{ to: 1, _id: -1 }works correctly with array values through MongoDB's automatic multikey indexing—queries likegetByUser(uid)that match on thetofield will use this index properly. The contest.ts usage at line 1122 passinguidsarray is supported as designed.
| const balloon = await collBalloon.find({ domainId, tid, pid }).project({ uid: 1 }).toArray(); | ||
| if (balloon.find((i) => i.uid === uid)) return null; | ||
| let isFirst = !balloon.length; | ||
| if (isFirst) { | ||
| let pending: RecordDoc[] = []; | ||
| do { | ||
| if (pending.length) await sleep(500); // eslint-disable-line no-await-in-loop | ||
| pending = await RecordModel.getMulti(domainId, { // eslint-disable-line no-await-in-loop | ||
| pid, contest: tid, _id: { $lt: rid }, status: { | ||
| $in: [ | ||
| STATUS.STATUS_WAITING, STATUS.STATUS_COMPILING, | ||
| STATUS.STATUS_JUDGING, STATUS.STATUS_FETCHED, | ||
| STATUS.STATUS_ACCEPTED, | ||
| ], | ||
| }, | ||
| }).limit(1).toArray(); | ||
| } while (pending.length && !pending.some((i) => i.status === STATUS.STATUS_ACCEPTED)); | ||
| if (pending.some((i) => i.status === STATUS.STATUS_ACCEPTED)) isFirst = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unbounded wait and missing duplicate-key handling in first-solve detection can stall updates and throw on races
- The do/while loop can wait indefinitely if a prior record stays in WAITING/COMPILING/JUDGING/FETCHED for long; updateStatus awaits addBalloon, blocking judge flow.
- On concurrent inserts, partial unique index ('first')/basic unique collisions can throw E11000; no retry/downgrade path.
Recommend: cap wait time and handle duplicate-key by retrying without first or returning gracefully.
Apply this patch:
@@
- let isFirst = !balloon.length;
+ let isFirst = !balloon.length;
if (isFirst) {
- let pending: RecordDoc[] = [];
- do {
- if (pending.length) await sleep(500); // eslint-disable-line no-await-in-loop
- pending = await RecordModel.getMulti(domainId, { // eslint-disable-line no-await-in-loop
+ let pending: RecordDoc[] = [];
+ const waitDeadline = Date.now() + 60_000; // cap wait to 60s to avoid stalling judge path
+ do {
+ if (pending.length) await sleep(500); // eslint-disable-line no-await-in-loop
+ if (Date.now() > waitDeadline) break;
+ pending = await RecordModel.getMulti(domainId, { // eslint-disable-line no-await-in-loop
pid, contest: tid, _id: { $lt: rid }, status: {
$in: [
STATUS.STATUS_WAITING, STATUS.STATUS_COMPILING,
STATUS.STATUS_JUDGING, STATUS.STATUS_FETCHED,
STATUS.STATUS_ACCEPTED,
],
},
}).limit(1).toArray();
- } while (pending.length && !pending.some((i) => i.status === STATUS.STATUS_ACCEPTED));
+ } while (pending.length && !pending.some((i) => i.status === STATUS.STATUS_ACCEPTED));
if (pending.some((i) => i.status === STATUS.STATUS_ACCEPTED)) isFirst = false;
}
@@
- await collBalloon.insertOne(newBdoc);
+ try {
+ await collBalloon.insertOne(newBdoc);
+ } catch (e: any) {
+ if (e?.code === 11000) {
+ // Re-check: if user's balloon already exists, treat as no-op
+ const exists = await collBalloon.findOne({ domainId, tid, pid, uid });
+ if (exists) return null;
+ // If 'first' is already taken, retry insert without first flag
+ if (isFirst) {
+ const { first, ...rest } = newBdoc as any;
+ await collBalloon.insertOne(rest);
+ } else {
+ throw e;
+ }
+ } else {
+ throw e;
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui-default/components/message/index.page.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui-default/components/message/index.page.ts (3)
packages/ui-default/constant/message.js (8)
FLAG_I18N(5-5)FLAG_I18N(5-5)FLAG_ALERT(2-2)FLAG_ALERT(2-2)FLAG_INFO(4-4)FLAG_INFO(4-4)FLAG_RICHTEXT(3-3)FLAG_RICHTEXT(3-3)packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/notification/index.ts (1)
Notification(26-83)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/hydrooj/src/model/contest.ts (1)
858-876: Critical issues remain from previous review: unbounded wait and missing duplicate-key handling.The issues flagged in the previous review comment are still present:
Lines 862-874: The
do-whileloop can wait indefinitely if a prior record remains inWAITING/COMPILING/JUDGING/FETCHEDstatus, blocking the judge flow sinceupdateStatusawaitsaddBalloon.Line 880:
insertOnecan throwE11000on concurrent inserts due to the unique indexes (basic and partial 'first' index). No duplicate-key handling or retry logic is present.The previous review provided a detailed fix including a 60-second timeout cap and E11000 handling with graceful retry. Please refer to that suggestion.
Also applies to: 880-880
🧹 Nitpick comments (3)
packages/ui-default/components/message/index.page.ts (2)
13-13: Remove or guard debug console.log.This debug statement should either be removed for production or wrapped in an environment check like
if (process.env.NODE_ENV !== 'production')to avoid console noise.- console.log('Received message', msg, systemNotification); + if (process.env.NODE_ENV !== 'production') console.log('Received message', msg, systemNotification);
49-49: Extract system user ID to a constant.The magic number
1(system user ID) appears multiple times. Consider extracting it to a named constant likeSYSTEM_USER_IDfor better maintainability and clarity.Define the constant at the top of the file:
const SYSTEM_USER_ID = 1;Then update the comparisons:
- msg.udoc._id === 1 ? msg.mdoc.content.split('\n')[0] : msg.udoc.uname || 'Hydro Notification', + msg.udoc._id === SYSTEM_USER_ID ? msg.mdoc.content.split('\n')[0] : msg.udoc.uname || 'Hydro Notification',- ...(msg.udoc._id === 1) + ...(msg.udoc._id === SYSTEM_USER_ID)Also applies to: 66-66
packages/ui-default/components/message/worker.ts (1)
43-45: Good delegation pattern, but consider cleanup.Delegating notification creation to the page context (via
ports[0]?.postMessage) is the correct approach since the Notification API must be called from a window context, not a worker. The reduced timeout (3000ms) also improves responsiveness.Note: The debug console.log on line 43 should be removed or guarded with an environment check.
- console.log('Sending as system notification'); + if (process.env.NODE_ENV !== 'production') console.log('Sending as system notification');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/hydrooj/src/model/contest.ts(7 hunks)packages/ui-default/components/message/index.page.ts(3 hunks)packages/ui-default/components/message/worker.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/ui-default/components/message/index.page.ts (3)
packages/ui-default/constant/message.js (4)
FLAG_I18N(5-5)FLAG_I18N(5-5)FLAG_RICHTEXT(3-3)FLAG_RICHTEXT(3-3)packages/ui-default/components/notification/index.ts (1)
Notification(26-83)packages/ui-default/utils/base.ts (1)
i18n(14-17)
packages/hydrooj/src/model/contest.ts (6)
framework/utils/lib/common.ts (2)
sleep(130-134)getAlphabeticId(240-247)packages/hydrooj/src/model/record.ts (1)
RecordModel(20-283)packages/hydrooj/src/model/message.ts (1)
apply(78-84)packages/hydrooj/src/model/document.ts (1)
apply(412-447)packages/hydrooj/src/context.ts (2)
Context(31-41)Context(68-88)packages/hydrooj/src/model/problem.ts (1)
ProblemModel(78-706)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (5)
packages/hydrooj/src/model/contest.ts (3)
4-6: LGTM! Import additions support the new functionality.The new imports are necessary for the first-balloon detection logic (
sleep), event subscription (Context), user avatar formatting (avatar), and model-based data access (MessageModel,ProblemModel,RecordModel,UserModel).Also applies to: 15-15, 22-25
240-240: LGTM! Consistent refactoring to use UserModel wrapper.The migration from direct user data access to
UserModel.getListForRenderimproves code organization and maintains consistency across ACM, OI, and homework scoreboard implementations.Also applies to: 414-414, 783-783
1136-1138: LGTM! Properly exports the new apply function.The
applyfunction is correctly added to the global model registry, following the standard pattern for model initialization hooks.packages/ui-default/components/message/index.page.ts (2)
95-96: LGTM: Notification delegation to page context.Routing worker-originated notifications through
onmessage(payload, true)correctly delegates browser Notification creation to the page context where the Notification API is available and can respect the document visibility state.
45-61: The type safety concern is not substantiated; the code is correct.After tracing the complete message flow:
Flag processing (lines 14-23): If
FLAG_I18Nis set, content is parsed from JSON and converted back to a string viai18n(). If not set, it remains a plain string from the backend.Early returns (lines 25-37): Messages with
FLAG_ALERTorFLAG_INFOexit before reaching thesystemNotificationblock, so those paths don't apply.System notification source: System notifications are created via
MessageModel.sendNotification()(backendmessage.tsline 73), which sends withFLAG_RICHTEXT(notFLAG_I18N) and contains a plain string.By the time the
systemNotificationblock executes (line 45),msg.mdoc.contentis guaranteed to be a string, making the.split('\n')calls safe.
| export async function apply(ctx: Context) { | ||
| ctx.on('contest/balloon', (domainId, tid, bdoc) => { | ||
| if (!bdoc.first) return; | ||
| (async () => { | ||
| const tsdocs = await getMultiStatus(domainId, { docId: tid, subscribe: 1 }).toArray(); | ||
| const uids = Array.from<number>(new Set(tsdocs.map((tsdoc) => tsdoc.uid))); | ||
| const [team, tdoc, pdoc] = await Promise.all([ | ||
| UserModel.getById(domainId, bdoc.uid), | ||
| get(domainId, tid), | ||
| ProblemModel.get(domainId, bdoc.pid), | ||
| ]); | ||
| await MessageModel.send(1, uids, JSON.stringify({ | ||
| message: 'Team {0} is the first to solve problem {1} ({2})', | ||
| avatar: avatar(team.avatar), | ||
| params: [team.uname, getAlphabeticId(tdoc.pids.indexOf(bdoc.pid)), pdoc.title], | ||
| }), MessageModel.FLAG_I18N); | ||
| })(); | ||
| }); | ||
| await ctx.db.ensureIndexes( | ||
| collBalloon, | ||
| { key: { domainId: 1, tid: 1, pid: 1, uid: 1 }, unique: true, name: 'basic' }, | ||
| { key: { domainId: 1, tid: 1, pid: 1 }, unique: true, name: 'first', partialFilterExpression: { first: true } }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Add error handling to async IIFE and verify method signatures.
-
Line 1114: The async IIFE
(async () => { ... })();has no error handling. If any operation inside throws (e.g., database queries,MessageModel.send), it will result in an unhandled promise rejection. -
Lines 1129-1133: Index creation is not wrapped in try-catch despite the AI summary claiming it should be. If
ctx.db.ensureIndexesfails, the entireapplyfunction will reject. -
Line 1132: The partial unique index on
firstcan throwE11000during concurrent first-solve attempts, which relates to the race condition inaddBalloon(line 880).
Apply this diff to add error handling:
ctx.on('contest/balloon', (domainId, tid, bdoc) => {
if (!bdoc.first) return;
(async () => {
const tsdocs = await getMultiStatus(domainId, { docId: tid, subscribe: 1 }).toArray();
const uids = Array.from<number>(new Set(tsdocs.map((tsdoc) => tsdoc.uid)));
const [team, tdoc, pdoc] = await Promise.all([
UserModel.getById(domainId, bdoc.uid),
get(domainId, tid),
ProblemModel.get(domainId, bdoc.pid),
]);
await MessageModel.send(1, uids, JSON.stringify({
message: 'Team {0} is the first to solve problem {1} ({2})',
avatar: avatar(team.avatar),
params: [team.uname, getAlphabeticId(tdoc.pids.indexOf(bdoc.pid)), pdoc.title],
}), MessageModel.FLAG_I18N);
- })();
+ })().catch((err) => {
+ ctx.logger.error('Failed to send first balloon notification:', err);
+ });
});
- await ctx.db.ensureIndexes(
+ try {
+ await ctx.db.ensureIndexes(
- collBalloon,
- { key: { domainId: 1, tid: 1, pid: 1, uid: 1 }, unique: true, name: 'basic' },
- { key: { domainId: 1, tid: 1, pid: 1 }, unique: true, name: 'first', partialFilterExpression: { first: true } },
- );
+ collBalloon,
+ { key: { domainId: 1, tid: 1, pid: 1, uid: 1 }, unique: true, name: 'basic' },
+ { key: { domainId: 1, tid: 1, pid: 1 }, unique: true, name: 'first', partialFilterExpression: { first: true } },
+ );
+ } catch (err) {
+ ctx.logger.warn('Failed to create contest balloon indexes:', err);
+ }Run the following script to verify that the methods used exist:
🏁 Script executed:
#!/bin/bash
# Verify UserModel.getById, MessageModel.send, and MessageModel.FLAG_I18N exist
echo "=== Checking UserModel.getById ==="
ast-grep --pattern $'class UserModel {
$$$
getById($$$) {
$$$
}
$$$
}'
echo "=== Checking MessageModel.send signature ==="
ast-grep --pattern $'class MessageModel {
$$$
send($$$) {
$$$
}
$$$
}'
echo "=== Checking MessageModel.FLAG_I18N ==="
rg -n "FLAG_I18N" --type=ts -g "!node_modules"Length of output: 5982
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for UserModel.getById with more flexible patterns
echo "=== Searching for UserModel.getById (flexible) ==="
rg -n "UserModel\.getById" --type=ts -C3
echo "=== Checking if ctx.logger exists in apply function context ==="
rg -n "ctx\.logger" packages/hydrooj/src/model/contest.ts -C2
echo "=== Checking async IIFE error handling patterns in contest.ts ==="
rg -n "\.catch\(" packages/hydrooj/src/model/contest.ts -B2 -A2
Length of output: 12345
🏁 Script executed:
#!/bin/bash
# Verify MessageModel.sendInfo signature (alternative pattern)
echo "=== Checking MessageModel.sendInfo implementation ==="
rg -n "static async sendInfo" packages/hydrooj/src/model/message.ts -A10Length of output: 438
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for Context type definition and properties
echo "=== Searching for Context interface definition ==="
rg -n "interface Context" --type=ts -A15
echo "=== Checking error handling in other apply functions in contest.ts ==="
rg -n "export async function apply" packages/hydrooj/src/model/contest.ts -A30
echo "=== Looking for logger usage patterns in contest.ts ==="
rg -n "logger\|console\.\|throw\|Error" packages/hydrooj/src/model/contest.ts | head -20
Length of output: 20664
🏁 Script executed:
#!/bin/bash
# Check how other event handlers in contest.ts handle errors
echo "=== Event handlers in contest.ts that might show error patterns ==="
rg -n "ctx\.on\(" packages/hydrooj/src/model/contest.ts -A10 | head -50Length of output: 704
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify ctx.logger is available in Cordis-based Context
echo "=== Checking ctx.logger usage in hydrooj package ==="
rg -n "ctx\.logger\." packages/hydrooj/src --type=ts | head -15
echo "=== Checking how event handlers with async IIFE are handled elsewhere ==="
rg -n "ctx\.on\(" packages/hydrooj/src --type=ts -A8 | grep -E "(ctx\.on|catch|then)" | head -20
Length of output: 3201
Add error handling to async IIFE and wrap ensureIndexes in try-catch.
The async IIFE at line 1114-1127 fires without error handling, risking unhandled promise rejections if database queries or MessageModel.send fail. The ensureIndexes call at line 1129 is awaited but unprotected—if it fails, the entire module initialization will fail.
Apply this diff:
ctx.on('contest/balloon', (domainId, tid, bdoc) => {
if (!bdoc.first) return;
(async () => {
const tsdocs = await getMultiStatus(domainId, { docId: tid, subscribe: 1 }).toArray();
const uids = Array.from<number>(new Set(tsdocs.map((tsdoc) => tsdoc.uid)));
const [team, tdoc, pdoc] = await Promise.all([
UserModel.getById(domainId, bdoc.uid),
get(domainId, tid),
ProblemModel.get(domainId, bdoc.pid),
]);
await MessageModel.send(1, uids, JSON.stringify({
message: 'Team {0} is the first to solve problem {1} ({2})',
avatar: avatar(team.avatar),
params: [team.uname, getAlphabeticId(tdoc.pids.indexOf(bdoc.pid)), pdoc.title],
}), MessageModel.FLAG_I18N);
- })();
+ })().catch((err) => {
+ ctx.logger.error('Failed to send first balloon notification:', err);
+ });
});
- await ctx.db.ensureIndexes(
+ try {
+ await ctx.db.ensureIndexes(
- collBalloon,
- { key: { domainId: 1, tid: 1, pid: 1, uid: 1 }, unique: true, name: 'basic' },
- { key: { domainId: 1, tid: 1, pid: 1 }, unique: true, name: 'first', partialFilterExpression: { first: true } },
- );
+ collBalloon,
+ { key: { domainId: 1, tid: 1, pid: 1, uid: 1 }, unique: true, name: 'basic' },
+ { key: { domainId: 1, tid: 1, pid: 1 }, unique: true, name: 'first', partialFilterExpression: { first: true } },
+ );
+ } catch (err) {
+ ctx.logger.warn('Failed to create contest balloon indexes:', err);
+ }🤖 Prompt for AI Agents
In packages/hydrooj/src/model/contest.ts around lines 1111-1134, wrap the async
IIFE invoked in the 'contest/balloon' handler in a try/catch so any errors from
getMultiStatus, UserModel/ProblemModel/get, or MessageModel.send are caught and
logged (use ctx.logger.error if available, otherwise console.error) to avoid
unhandled promise rejections; also wrap the awaited ctx.db.ensureIndexes call in
its own try/catch and log any failure (and decide whether to swallow or rethrow
based on desired startup semantics) so index creation errors are handled instead
of crashing module initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui-default/components/message/index.page.ts(2 hunks)packages/ui-default/components/message/worker.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui-default/components/message/index.page.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui-default/components/message/worker.ts (2)
packages/ui-default/constant/message.js (2)
FLAG_I18N(5-5)FLAG_I18N(5-5)packages/ui-default/utils/base.ts (1)
i18n(14-17)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ui-default/components/message/worker.ts (2)
17-21: Consider stronger typing for the ack payload.The
payload?: anyfield loses type safety. Since this is used to pass translated message data back from i18n processing (line 37), consider defining a more specific type that ensures structural compatibility with the original message object.For example:
+interface MessagePayload { + udoc: { _id: number; uname?: string; avatarUrl?: string }; + mdoc: { _id: string; content: string; flag: number; avatar?: string }; +} + interface RequestAckPayload { type: 'ack'; id: string; - payload?: any; + payload?: MessagePayload; }
32-38: Good solution for the i18n issue, but needs documentation.The refactored approach successfully resolves the previous critical issue where
i18nwould crash in the SharedWorker context. By delegating translation to the main thread via the'i18n'broadcast (line 38) and receiving the translated payload through the ack callback (line 37), you've created a worker-safe i18n mechanism.However, this two-part ack system (
_idfor acknowledgment,_id-i18nfor translation) is non-obvious and would benefit from comments explaining:
- The expected round-trip flow
- What structure the i18n ack payload should have
- Why both ack mechanisms are needed
Consider adding a brief comment block:
+ // Set up ack handlers: one for user acknowledgment, one for i18n translation payload ack[message.mdoc._id] = () => { acked = true; }; ack[`${message.mdoc._id}-i18n`] = (v) => { payload = v; }; + // Request i18n translation from main thread if needed if (message.mdoc.flag & FLAG_I18N) broadcastMsg({ type: 'i18n', payload: message });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui-default/components/message/index.page.ts(3 hunks)packages/ui-default/components/message/worker.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui-default/components/message/index.page.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui-default/components/message/worker.ts (1)
packages/ui-default/constant/message.js (4)
FLAG_I18N(5-5)FLAG_I18N(5-5)FLAG_INFO(4-4)FLAG_INFO(4-4)
⏰ 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). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (4)
packages/ui-default/components/message/worker.ts (4)
4-4: LGTM!The FLAG_I18N import is correctly added and used throughout the file.
103-107: LGTM!The ack handler correctly passes the payload to the callback (line 105), enabling the i18n mechanism. The optional chaining safely handles cases where the ack arrives after the timeout cleanup (lines 40-41).
50-61: Avatar property usage is intentional and correct—no changes needed.The code appropriately uses different avatar properties based on message type:
- System notifications (udoc._id === 1) use
mdoc.avatarfrom the message document, which is populated from i18n message content- User messages use
udoc.avatarUrl, which is computed fromudoc.avatarin the handlerThis reflects the different data structures: system messages have avatar in the message document, while user messages have avatar information in the user document. The property names are semantically distinct by design.
39-47: Timing concern is valid but likely manageable; consider monitoring in production.The 3000ms timeout provides a reasonable margin for synchronous i18n translation plus inter-frame messaging. However, the risk is real if the main thread stalls during this window:
- Translation itself is synchronous (fast, <10ms typical)
- Risk window: If main thread blocks before the ack callback executes, the i18n ack arrives after the timeout
- Graceful degradation: The optional chaining on line 105 ensures late acks are safely ignored, and untranslated messages are shown instead of breaking
The code handles this correctly. The recommendation to test on lower-end devices and add telemetry to measure actual i18n ack latency is sound—consider tracking
postMessageround-trip timing to validate the 3000ms assumption holds in practice. If telemetry shows consistent late acks (>2000ms), the timeout should be increased.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Other