core: token events#1179
Conversation
WalkthroughThis PR extends the TokenModel to emit lifecycle events around token persistence operations. The bus event type contract (EventMap) is extended with six new token/* event signatures (before-add, add, before-update, update, before-del, del). TokenModel's add() and update() methods now emit serialized before-* hooks before database operations and parallel hooks after. The del() method is refactored to fetch the token first and return a boolean indicating deletion success, while delByUid() switches from bulk deleteMany to per-document async deletion, returning the count of deleted tokens and emitting hooks for each one. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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/model/token.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/hydrooj/src/service/bus.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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: 3
🧹 Nitpick comments (1)
packages/hydrooj/src/model/token.ts (1)
80-85: ⚡ Quick winInconsistent hook type:
before-delusesparallelinstead ofserial.The
token/before-addandtoken/before-updatehooks usebus.serial(), which allows listeners to run sequentially and potentially block/modify the operation. However,token/before-delusesbus.parallel(), which runs listeners concurrently without ordering guarantees.For consistency with other "before-*" hooks in this file (and to allow potential cancellation/validation logic), consider using
bus.serial:- await bus.parallel('token/before-del', tokenId, tdoc); + await bus.serial('token/before-del', tokenId, tdoc);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/hydrooj/src/model/token.ts` around lines 80 - 85, The deletion hook uses bus.parallel('token/before-del', tokenId, tdoc) which is inconsistent with the other pre-hooks (token/before-add and token/before-update) that use bus.serial; change the before-delete call to use bus.serial('token/before-del', tokenId, tdoc) so listeners run sequentially and can perform validation/cancellation, keeping the subsequent await TokenModel.coll.deleteOne({ _id: tokenId, tokenType }) and bus.parallel('token/del', tokenId, tdoc) behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/hydrooj/src/model/token.ts`:
- Around line 74-75: The code currently emits the 'token/update' event with res
even when findOneAndUpdate returned null; change the logic in the update routine
(the block that calls await bus.parallel('token/update', tokenId, res)) to check
that res is not null before emitting—i.e., only call
bus.parallel('token/update', tokenId, res) when res !== null (or handle the null
case explicitly by emitting a different event or logging), so listeners
expecting a TokenDoc for the 'token/update' event never receive null.
- Around line 114-122: The current delByUid emits bus events even when deleteOne
returns deletedCount === 0; change the flow in TokenModel.delByUid so you await
bus.serial('token/before-del', tdoc._id, tdoc) (for consistency with del()),
then call this.coll.deleteOne({ _id: tdoc._id }) and only if r.deletedCount > 0
emit bus.parallel('token/del', tdoc._id, tdoc) and return true; if deletedCount
is 0 return false (do not emit token/del). Keep the outer Promise.all mapping
over tdocs and the final reduce to count successes.
In `@packages/hydrooj/src/service/bus.ts`:
- Around line 102-107: The event signature for 'token/update' currently requires
a TokenDoc but the emitter in token.ts calls bus.parallel('token/update',
tokenId, res) where res can be TokenDoc | null; update the Bus type entry for
'token/update' to accept (tokenId: string, tdoc: TokenDoc | null) => VoidReturn
(or alternatively add a null-guard in the caller before calling bus.parallel in
token.ts), ensuring references to TokenDoc and the 'token/update' handler match
the actual nullable return from findOneAndUpdate.
---
Nitpick comments:
In `@packages/hydrooj/src/model/token.ts`:
- Around line 80-85: The deletion hook uses bus.parallel('token/before-del',
tokenId, tdoc) which is inconsistent with the other pre-hooks (token/before-add
and token/before-update) that use bus.serial; change the before-delete call to
use bus.serial('token/before-del', tokenId, tdoc) so listeners run sequentially
and can perform validation/cancellation, keeping the subsequent await
TokenModel.coll.deleteOne({ _id: tokenId, tokenType }) and
bus.parallel('token/del', tokenId, tdoc) behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 119bbf46-6463-4194-839c-c5799ee1d713
📒 Files selected for processing (2)
packages/hydrooj/src/model/token.tspackages/hydrooj/src/service/bus.ts
| await bus.parallel('token/update', tokenId, res); | ||
| return res; |
There was a problem hiding this comment.
res may be null; event emits unconditionally.
findOneAndUpdate returns null if no document matches the filter. The token/update event is emitted regardless, potentially passing null to listeners expecting a TokenDoc. Consider guarding:
+ if (res) {
await bus.parallel('token/update', tokenId, res);
+ }
return res;📝 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.
| await bus.parallel('token/update', tokenId, res); | |
| return res; | |
| if (res) { | |
| await bus.parallel('token/update', tokenId, res); | |
| } | |
| return res; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/hydrooj/src/model/token.ts` around lines 74 - 75, The code currently
emits the 'token/update' event with res even when findOneAndUpdate returned
null; change the logic in the update routine (the block that calls await
bus.parallel('token/update', tokenId, res)) to check that res is not null before
emitting—i.e., only call bus.parallel('token/update', tokenId, res) when res !==
null (or handle the null case explicitly by emitting a different event or
logging), so listeners expecting a TokenDoc for the 'token/update' event never
receive null.
| static async delByUid(uid: number) { | ||
| const tdocs = await TokenModel.coll.find({ uid }).toArray(); | ||
| return await Promise.all(tdocs.map(async (tdoc) => { | ||
| await bus.parallel('token/before-del', tdoc._id, tdoc); | ||
| const r = await this.coll.deleteOne({ _id: tdoc._id }); | ||
| await bus.parallel('token/del', tdoc._id, tdoc); | ||
| return !!r.deletedCount; | ||
| })).then((r) => r.reduce((prev, cur) => prev + (cur ? 1 : 0), 0)); | ||
| } |
There was a problem hiding this comment.
Events fire even when deletion fails due to concurrent removal.
If a token is deleted between find() and deleteOne() (e.g., by TTL expiry or another process), token/before-del and token/del events are still emitted even though deletedCount is 0. This could mislead listeners.
Consider emitting token/del only when deletion succeeds:
return await Promise.all(tdocs.map(async (tdoc) => {
await bus.parallel('token/before-del', tdoc._id, tdoc);
const r = await this.coll.deleteOne({ _id: tdoc._id });
- await bus.parallel('token/del', tdoc._id, tdoc);
+ if (r.deletedCount) {
+ await bus.parallel('token/del', tdoc._id, tdoc);
+ }
return !!r.deletedCount;
})).then((r) => r.reduce((prev, cur) => prev + (cur ? 1 : 0), 0));Also, for consistency with del(), consider using bus.serial for token/before-del here.
📝 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.
| static async delByUid(uid: number) { | |
| const tdocs = await TokenModel.coll.find({ uid }).toArray(); | |
| return await Promise.all(tdocs.map(async (tdoc) => { | |
| await bus.parallel('token/before-del', tdoc._id, tdoc); | |
| const r = await this.coll.deleteOne({ _id: tdoc._id }); | |
| await bus.parallel('token/del', tdoc._id, tdoc); | |
| return !!r.deletedCount; | |
| })).then((r) => r.reduce((prev, cur) => prev + (cur ? 1 : 0), 0)); | |
| } | |
| static async delByUid(uid: number) { | |
| const tdocs = await TokenModel.coll.find({ uid }).toArray(); | |
| return await Promise.all(tdocs.map(async (tdoc) => { | |
| await bus.parallel('token/before-del', tdoc._id, tdoc); | |
| const r = await this.coll.deleteOne({ _id: tdoc._id }); | |
| if (r.deletedCount) { | |
| await bus.parallel('token/del', tdoc._id, tdoc); | |
| } | |
| return !!r.deletedCount; | |
| })).then((r) => r.reduce((prev, cur) => prev + (cur ? 1 : 0), 0)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/hydrooj/src/model/token.ts` around lines 114 - 122, The current
delByUid emits bus events even when deleteOne returns deletedCount === 0; change
the flow in TokenModel.delByUid so you await bus.serial('token/before-del',
tdoc._id, tdoc) (for consistency with del()), then call this.coll.deleteOne({
_id: tdoc._id }) and only if r.deletedCount > 0 emit bus.parallel('token/del',
tdoc._id, tdoc) and return true; if deletedCount is 0 return false (do not emit
token/del). Keep the outer Promise.all mapping over tdocs and the final reduce
to count successes.
| 'token/before-add': (payload: any) => VoidReturn; | ||
| 'token/add': (tdoc: TokenDoc) => VoidReturn; | ||
| 'token/before-update': (tokenId: string, payload: any) => VoidReturn; | ||
| 'token/update': (tokenId: string, tdoc: TokenDoc) => VoidReturn; | ||
| 'token/before-del': (tokenId: string, tdoc: TokenDoc) => VoidReturn; | ||
| 'token/del': (tokenId: string, tdoc: TokenDoc) => VoidReturn; |
There was a problem hiding this comment.
Type signature for token/update may not match actual usage.
In token.ts line 74, bus.parallel('token/update', tokenId, res) is called where res is the result of findOneAndUpdate, which can be TokenDoc | null. The event signature here expects TokenDoc, not TokenDoc | null.
Consider updating the signature to handle the nullable case:
- 'token/update': (tokenId: string, tdoc: TokenDoc) => VoidReturn;
+ 'token/update': (tokenId: string, tdoc: TokenDoc | null) => VoidReturn;Alternatively, guard against null in token.ts before emitting the event.
📝 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.
| 'token/before-add': (payload: any) => VoidReturn; | |
| 'token/add': (tdoc: TokenDoc) => VoidReturn; | |
| 'token/before-update': (tokenId: string, payload: any) => VoidReturn; | |
| 'token/update': (tokenId: string, tdoc: TokenDoc) => VoidReturn; | |
| 'token/before-del': (tokenId: string, tdoc: TokenDoc) => VoidReturn; | |
| 'token/del': (tokenId: string, tdoc: TokenDoc) => VoidReturn; | |
| 'token/before-add': (payload: any) => VoidReturn; | |
| 'token/add': (tdoc: TokenDoc) => VoidReturn; | |
| 'token/before-update': (tokenId: string, payload: any) => VoidReturn; | |
| 'token/update': (tokenId: string, tdoc: TokenDoc | null) => VoidReturn; | |
| 'token/before-del': (tokenId: string, tdoc: TokenDoc) => VoidReturn; | |
| 'token/del': (tokenId: string, tdoc: TokenDoc) => VoidReturn; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/hydrooj/src/service/bus.ts` around lines 102 - 107, The event
signature for 'token/update' currently requires a TokenDoc but the emitter in
token.ts calls bus.parallel('token/update', tokenId, res) where res can be
TokenDoc | null; update the Bus type entry for 'token/update' to accept
(tokenId: string, tdoc: TokenDoc | null) => VoidReturn (or alternatively add a
null-guard in the caller before calling bus.parallel in token.ts), ensuring
references to TokenDoc and the 'token/update' handler match the actual nullable
return from findOneAndUpdate.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Changes