Repository navigation
fix(realtime): broadcast drive role mutations and wire RolesManager socket listener - #1328
Conversation
…ocket listener Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds best-effort WebSocket broadcasts to role mutation endpoints (create, update, delete, reorder) that resolve drive recipient user IDs and emit a ChangesDrive Role Real-Time Synchronization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/src/components/settings/RolesManager.tsx (1)
10-10: ⚡ Quick winUse a type-only import for
DriveEventPayload.This symbol is only used in type position (parameter type annotation on line 71);
import typeavoids accidental runtime coupling and improves tree-shaking.Suggested change
-import { DriveEventPayload } from '@/lib/websocket'; +import type { DriveEventPayload } from '@/lib/websocket';🤖 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 `@apps/web/src/components/settings/RolesManager.tsx` at line 10, Change the import of DriveEventPayload to a type-only import to avoid runtime coupling: replace the current import of DriveEventPayload used only as a parameter type (see usage in RolesManager component/function where DriveEventPayload is annotated on the event handler parameter) with an "import type" form so it is erased at runtime and improves tree-shaking.
🤖 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 `@apps/web/src/app/api/drives/`[driveId]/roles/[roleId]/route.ts:
- Around line 118-121: The broadcast side-effect (calls to
getDriveRecipientUserIds and broadcastDriveEvent using createDriveEventPayload)
must not cause a 500 when the underlying mutation already succeeded; wrap the
broadcast logic in a try/catch in the route handler so any errors are caught,
logged (with context and the error), and ignored for the HTTP response, ensuring
the handler still returns success for the update/delete; apply this same change
to both broadcast call sites in this file that invoke
getDriveRecipientUserIds/createDriveEventPayload/broadcastDriveEvent.
In `@apps/web/src/app/api/drives/`[driveId]/roles/reorder/route.ts:
- Around line 54-57: The broadcast call is causing API failures when it errors
even though the role reorder has already been persisted; update the code around
broadcastDriveEvent (and its preparation with
getDriveRecipientUserIds/createDriveEventPayload) to treat broadcasting as
best-effort by wrapping the broadcast in a try/catch (or performing a non-await
fire-and-forget) and swallowing/logging errors instead of allowing them to
propagate to the route handler so the HTTP response remains a success after
persistence.
In `@apps/web/src/app/api/drives/`[driveId]/roles/route.ts:
- Around line 92-95: The websocket broadcast is a non-critical side effect
causing the route to return 500 if it fails: wrap the broadcast logic (calls to
getDriveRecipientUserIds, createDriveEventPayload, and broadcastDriveEvent) in a
try/catch (or use Promise.allSettled if broadcasting multiple targets) inside
the roles route handler so any exceptions are caught, logged, and suppressed
instead of rethrown; ensure the route still returns the successful create
response even when broadcastDriveEvent fails and include a clear log
message/error context referencing driveId and the payload for debugging.
---
Nitpick comments:
In `@apps/web/src/components/settings/RolesManager.tsx`:
- Line 10: Change the import of DriveEventPayload to a type-only import to avoid
runtime coupling: replace the current import of DriveEventPayload used only as a
parameter type (see usage in RolesManager component/function where
DriveEventPayload is annotated on the event handler parameter) with an "import
type" form so it is erased at runtime and improves tree-shaking.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec0115a9-c757-4901-8368-55c1b1484a00
📒 Files selected for processing (4)
apps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.tsapps/web/src/components/settings/RolesManager.tsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70f4c65c26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const recipientUserIds = await getDriveRecipientUserIds(driveId); | ||
| await broadcastDriveEvent(createDriveEventPayload(driveId, 'updated', {}), recipientUserIds); |
There was a problem hiding this comment.
After createDriveRole succeeds, the new getDriveRecipientUserIds call runs inside the main request try block; if that lookup throws (e.g., transient DB/read failure), this handler returns 500 even though the role was already committed. That creates a false failure signal and can trigger client retries that then hit duplicate-name conflicts. Make the realtime recipient lookup/broadcast path best-effort (isolated try/catch) so post-commit notification issues don’t change API success semantics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7acfe49 — same fix as the CodeRabbit thread above.
- Broadcast errors no longer cause 500 responses when the underlying DB mutation already succeeded - DriveEventPayload import changed to type-only import Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ocket listener (#1328) * fix(realtime): broadcast drive role mutations and wire RolesManager socket listener Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(realtime): wrap broadcasts in try/catch; use import type - Broadcast errors no longer cause 500 responses when the underlying DB mutation already succeeded - DriveEventPayload import changed to type-only import Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
broadcastDriveEventcalls to all four drive role mutation routes (create, update, delete, reorder) so peer admins receive adrive:updatedsocket event immediatelydrive:updatedsocket listener inRolesManager.tsxthat callsfetchRoles()when an event arrives for the current drive, keeping the roles list live for all admins viewing the panel simultaneouslyTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit