core: auth/login event#1181
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughTwo new typed bus events are added to the Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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/handler/user.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. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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.
🧹 Nitpick comments (1)
packages/hydrooj/src/handler/user.ts (1)
34-49: 💤 Low valueConsider whether
auth/loginsubscribers need fresh login metadata.The
udocpassed to both events is the same object fetched before theuser.setByIdcall at line 36. Subscribers toauth/loginwon't see the updatedloginat/loginipvalues inudocsince those are persisted to the database but not reflected back into the object.If subscribers are expected to access fresh login metadata, consider either:
- Re-fetching
udocbefore emittingauth/login, or- Documenting that
udocreflects pre-persistence stateIf this is intentional (event represents user at authentication time, not after DB update), the current implementation is fine.
🤖 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/handler/user.ts` around lines 34 - 49, The udoc object passed to the auth/login event at line 48 contains stale data because it was not updated after the user.setById call at line 36 persisted the loginat and loginip values to the database. If subscribers to the auth/login event need access to the fresh login metadata (loginat and loginip), re-fetch the udoc using user.getById or similar before passing it to the ctx.serial('auth/login', udoc) call. If the current behavior is intentional (meaning the event should receive the user state at authentication time before database persistence), add a comment documenting this design decision to clarify for future maintainers.
🤖 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.
Nitpick comments:
In `@packages/hydrooj/src/handler/user.ts`:
- Around line 34-49: The udoc object passed to the auth/login event at line 48
contains stale data because it was not updated after the user.setById call at
line 36 persisted the loginat and loginip values to the database. If subscribers
to the auth/login event need access to the fresh login metadata (loginat and
loginip), re-fetch the udoc using user.getById or similar before passing it to
the ctx.serial('auth/login', udoc) call. If the current behavior is intentional
(meaning the event should receive the user state at authentication time before
database persistence), add a comment documenting this design decision to clarify
for future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9314f40b-44ca-4c8c-b81d-bf42c50ca12f
📒 Files selected for processing (2)
packages/hydrooj/src/handler/user.tspackages/hydrooj/src/service/bus.ts
| async function successfulAuth(this: Handler, udoc: User) { | ||
| if (udoc._id !== 0) await user.setById(udoc._id, { loginat: new Date(), loginip: this.request.ip }); | ||
| if (udoc._id !== 0) { | ||
| await this.ctx.serial('auth/before-login', udoc); |
There was a problem hiding this comment.
Also attach handler context might be better. (In case of redirecting user to specific page, showing notificatons, etc)
2 participants
Summary by CodeRabbit
Release Notes