Add real-time issue updates across API, live service, and web #7912
Conversation
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughPublishes structured issue events from the API to Redis; the live service exposes a WebSocket controller that subscribes to project channels and forwards Redis messages to clients; the web app adds a resilient useIssueChannel hook with per-issue queues and wires it into the project layout. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant WebApp as Web App (useIssueChannel)
participant LiveWS as Live Service WS (IssueEventsController)
participant Redis as Redis Pub/Sub
participant API as API (issue_activities_task)
participant DB as DB
rect rgba(230,245,255,0.5)
Note over API: Issue activity created
API->>DB: Create IssueActivity records
API->>Redis: PUBLISH issue_events:{projectId} { event payload }
end
rect rgba(235,255,235,0.5)
User->>WebApp: Open project page
WebApp->>LiveWS: WS connect (token, workspaceSlug, projectId)
LiveWS->>Redis: SUBSCRIBE issue_events:{projectId}
Redis-->>LiveWS: Message (event payload)
LiveWS-->>WebApp: WS message (event payload)
end
alt Per-issue processing
WebApp->>WebApp: Enqueue payload by issueId (serialize)
WebApp->>API: GET /issues/{issueId}
API->>DB: Read issue
API-->>WebApp: Issue data or 404
opt Found
WebApp->>WebApp: Update stores/lists
end
opt 404
WebApp->>WebApp: Remove from stores
end
else Errors
LiveWS->>LiveWS: Close with codes (4001-4003, 1011) as applicable
WebApp->>WebApp: Reconnect with backoff
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (5)
apps/api/plane/bgtasks/issue_activities_task.py(2 hunks)apps/live/src/controllers/index.ts(1 hunks)apps/live/src/controllers/issue-events.controller.ts(1 hunks)apps/web/core/hooks/use-issue-channel.ts(1 hunks)apps/web/core/layouts/auth-layout/project-wrapper.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/api/plane/bgtasks/issue_activities_task.py (1)
apps/api/plane/utils/exception_logger.py (1)
log_exception(9-20)
apps/web/core/hooks/use-issue-channel.ts (2)
apps/web/core/lib/store-context.tsx (1)
rootStore(7-7)apps/space/core/hooks/store/use-user.ts (1)
useUser(7-11)
apps/live/src/controllers/issue-events.controller.ts (4)
apps/web/core/hooks/use-issue-channel.ts (1)
closeSocket(278-296)packages/decorators/src/websocket.ts (1)
WebSocket(8-13)apps/live/src/lib/auth.ts (1)
handleAuthentication(64-82)apps/live/src/redis.ts (1)
redisManager(211-211)
apps/web/core/layouts/auth-layout/project-wrapper.tsx (1)
apps/web/core/hooks/use-issue-channel.ts (1)
useIssueChannel(299-323)
| const token = JSON.stringify({ id: this.options.userId, cookie: document.cookie }); | ||
| const wsUrl = buildWsUrl(this.options.workspaceSlug, this.options.projectId, token); | ||
| const socket = new WebSocket(wsUrl); |
There was a problem hiding this comment.
Do not put document.cookie into the WebSocket query string.
buildWsUrl ends up sending the full session cookie inside the token query parameter. Query strings are routinely logged by browsers, proxies, load balancers, etc., so this leaks authentication material and enables session hijacking. Please remove the cookie from the URL entirely—rely on the WebSocket handshake cookies (same-origin requests already transmit them) or mint a short-lived signed token that does not expose the raw session cookie.
🤖 Prompt for AI Agents
In apps/web/core/hooks/use-issue-channel.ts around lines 214 to 216, the code
currently injects document.cookie into the WebSocket URL token which leaks
session cookies via query strings; remove document.cookie from the token payload
so the WebSocket URL never contains raw cookies. Instead send only non-sensitive
identifiers (e.g. userId) or obtain a short-lived signed token from your backend
(or rely on the browser to send cookies during the WS handshake for same-origin
requests) and use that token; update buildWsUrl calls and token creation to omit
document.cookie and wire up a server-minted signed token endpoint if needed.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/core/hooks/use-issue-channel.ts (1)
209-241: Race condition in processIssue can cause concurrent processing.Between line 233 (setting
isProcessing = false) and line 238 (setting it back totrue), a concurrent call toenqueue()may observeisProcessing = falseand spawn a second processing task. This results in two simultaneous calls tosyncIssue()for the same issue, leading to duplicate API requests and potential state corruption.Apply this diff to eliminate the race window:
state.isProcessing = false; - if (!state.latestPayload && !state.pendingGroupedListUpdate) { + const hasWork = state.latestPayload || state.pendingGroupedListUpdate; + if (!hasWork) { this.state.delete(issueId); - } else if (!this.disposed) { + return; + } + + if (!this.disposed) { state.isProcessing = true; enqueueMicrotask(() => this.processIssue(issueId)); }
🧹 Nitpick comments (2)
apps/web/core/hooks/use-issue-channel.ts (2)
58-60: Consider edge case: empty string handling.The function treats empty string
""as"None", which might not always be the desired behavior depending on your data model. Ensure this aligns with your filter semantics, especially if empty string is a valid filter value distinct from null/undefined.
279-291: Consider adding timeout to API fetch.The
getIssuescall at line 280 has no explicit timeout, which could cause the queue to stall indefinitely if the API hangs. Consider adding a timeout or implementing a circuit breaker pattern for resilience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/plane/bgtasks/issue_activities_task.py(2 hunks)apps/live/src/controllers/index.ts(1 hunks)apps/live/src/controllers/issue-events.controller.ts(1 hunks)apps/web/core/hooks/use-issue-channel.ts(1 hunks)apps/web/core/layouts/auth-layout/project-wrapper.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/live/src/controllers/issue-events.controller.ts
- apps/web/core/layouts/auth-layout/project-wrapper.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/core/hooks/use-issue-channel.ts (4)
packages/types/src/view-props.ts (1)
TIssueParams(56-82)packages/types/src/issues/issue.ts (1)
TIssue(83-97)apps/web/core/lib/store-context.tsx (1)
rootStore(7-7)apps/space/core/hooks/store/use-user.ts (1)
useUser(7-11)
apps/api/plane/bgtasks/issue_activities_task.py (1)
apps/api/plane/utils/exception_logger.py (1)
log_exception(9-20)
🔇 Additional comments (7)
apps/live/src/controllers/index.ts (1)
4-6: LGTM!The addition of
IssueEventsControllerto the exported controllers follows the established pattern and integrates cleanly with the existing structure.apps/api/plane/bgtasks/issue_activities_task.py (3)
45-56: LGTM!The defensive normalization logic appropriately handles various payload types and includes proper error handling for JSON parsing.
58-104: LGTM!The broadcast implementation includes appropriate validation, defensive error handling for Redis operations, and safe JSON serialization. The channel naming convention and payload structure are well-designed for the real-time event system.
1643-1652: LGTM!The integration point is well-placed after activity records are persisted but before notifications, ensuring real-time listeners receive updates promptly.
apps/web/core/hooks/use-issue-channel.ts (3)
528-530: LGTM: Proper reconnection handling.The reset of
reconnectAttempton successful connection ensures the backoff strategy starts fresh after reconnection, which is the correct behavior.
587-605: LGTM: Comprehensive socket cleanup.The
closeSocketmethod properly clears all event handlers before closing, preventing memory leaks and ensuring clean disposal.
608-632: LGTM: Well-structured React hook.The hook properly manages lifecycle by creating connection and queue on mount, wiring them together, and cleaning up on unmount. The dependency array correctly includes all external values.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/core/hooks/use-issue-channel.ts (1)
693-695: CRITICAL: Remove session cookies from WebSocket URL.Putting
document.cookiein the WebSocket query string (line 693) exposes session cookies in browser logs, proxy logs, and load balancer logs, enabling session hijacking.Solution: Remove
cookie: document.cookiefrom the token. Options:
Preferred: Rely on automatic cookie transmission during the WebSocket handshake (for same-origin requests, the browser automatically sends cookies).
Alternative: Obtain a short-lived, signed token from your backend that doesn't contain raw cookies.
Apply this diff to remove the cookie:
- const token = JSON.stringify({ id: this.options.userId, cookie: document.cookie }); + const token = JSON.stringify({ id: this.options.userId });If authentication requires more than just the user ID, implement a backend endpoint that mints short-lived WebSocket tokens based on the existing session.
Based on learnings
🧹 Nitpick comments (3)
apps/web/core/layouts/auth-layout/project-wrapper.tsx (1)
64-64: Simplify type coercion.The
.toString()calls are redundant sinceworkspaceSlugandprojectIdare already typed asstringin theIProjectAuthWrapperinterface. The optional chaining (?.) already handles undefined values appropriately.Apply this diff:
- useIssueChannel(workspaceSlug?.toString(), projectId?.toString()); + useIssueChannel(workspaceSlug, projectId);apps/api/plane/bgtasks/issue_activities_task.py (1)
86-86: Consider timestamp precision for real-time events.Converting
epochtointdiscards sub-second precision ifepochis a float. For real-time synchronization, millisecond precision may be needed. Consider usingint(epoch * 1000)to preserve milliseconds or keep the timestamp as a float.If millisecond precision is needed, apply this diff:
- "timestamp": int(epoch) if isinstance(epoch, (int, float)) else None, + "timestamp": int(epoch * 1000) if isinstance(epoch, (int, float)) else None,If second precision is sufficient, you can keep the current implementation but add a comment clarifying the precision:
- "timestamp": int(epoch) if isinstance(epoch, (int, float)) else None, + "timestamp": int(epoch) if isinstance(epoch, (int, float)) else None, # seconds since epochapps/web/core/hooks/use-issue-channel.ts (1)
596-608: Consider caching strategy for unsupported filter types.When project view filters are unsupported, the code falls back to fetching all issues with existing pagination. This could be expensive for large projects. Consider implementing a cache or rate-limiting strategy to minimize repeated full refreshes during bursts of updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/plane/bgtasks/issue_activities_task.py(2 hunks)apps/live/src/controllers/index.ts(1 hunks)apps/live/src/controllers/issue-events.controller.ts(1 hunks)apps/web/core/hooks/use-issue-channel.ts(1 hunks)apps/web/core/layouts/auth-layout/project-wrapper.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/live/src/controllers/index.ts
- apps/live/src/controllers/issue-events.controller.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/api/plane/bgtasks/issue_activities_task.py (1)
apps/api/plane/utils/exception_logger.py (1)
log_exception(9-20)
apps/web/core/hooks/use-issue-channel.ts (5)
packages/types/src/view-props.ts (1)
TIssueParams(56-82)packages/types/src/issues/issue.ts (1)
TIssue(83-97)apps/space/core/store/publish/publish.store.ts (1)
workspaceSlug(93-95)apps/web/core/lib/store-context.tsx (1)
rootStore(7-7)apps/space/core/hooks/store/use-user.ts (1)
useUser(7-11)
apps/web/core/layouts/auth-layout/project-wrapper.tsx (2)
apps/web/core/hooks/use-issue-channel.ts (1)
useIssueChannel(778-802)apps/space/core/store/publish/publish.store.ts (1)
workspaceSlug(93-95)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
apps/api/plane/bgtasks/issue_activities_task.py (2)
45-56: LGTM!The deserialization logic appropriately handles various data types with defensive checks.
1643-1652: LGTM!The broadcast integration is correctly placed after the database write, ensuring consistency. Error handling is appropriate with defensive guards in both the caller and callee.
apps/web/core/hooks/use-issue-channel.ts (3)
17-306: LGTM!The filter evaluation logic is comprehensive and handles various edge cases defensively. The use of symbols for unsupported states and recursive evaluation for complex filter trees is appropriate.
357-429: LGTM!The queue design properly handles concurrent payload arrivals during async processing by re-reading state in each loop iteration and using the
isProcessingflag to prevent duplicate processing for the same issue.
778-802: LGTM!The hook implementation properly guards against missing parameters, creates instances with correct cleanup, and has appropriate effect dependencies.
| if (shouldAttemptProjectViewUpdate && wasInActiveProjectView) { | ||
| stores.projectViewIssues?.removeIssueFromList(issueId); | ||
| } | ||
| stores.issues.removeIssue(issueId); |
There was a problem hiding this comment.
Bug: Grouped List Removal Fails on Undefined Reference
The removeIssueFromList method's updated logic can prevent issues from being removed from grouped lists. When an issue is deleted or not found, callers like handleIssueNotFound don't always provide the issueBeforeUpdate parameter. This results in the method's referenceIssue being undefined, causing it to return early and skip removing the issue from grouped lists.
Additional Locations (1)
| url.searchParams.set("projectId", projectId); | ||
| url.searchParams.set("token", token); | ||
| return url.toString(); | ||
| }; |
There was a problem hiding this comment.
Bug: WebSocket Protocol Override Issue
The WebSocket URL construction in buildWsUrl may incorrectly determine the protocol. The isSecure check considers window.location.protocol, which can override the protocol specified in LIVE_BASE_URL. This might force a wss: connection when LIVE_BASE_URL intended ws:, potentially causing connection failures.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/core/hooks/use-issue-channel.ts (1)
687-695: Critical: Do not includedocument.cookiein the WebSocket URL.Line 693 includes the full session cookie in the
tokenquery parameter, which is then sent in the WebSocket URL. Query strings are logged by browsers, proxies, and load balancers, creating a session hijacking risk.Solutions:
- Preferred: Remove
cookiefrom the token payload entirely. The WebSocket handshake automatically sends cookies for same-origin requests.- Alternative: Request a short-lived, signed token from your backend that doesn't contain the raw session cookie, and use that instead.
Apply this diff to remove the cookie from the token:
- const token = JSON.stringify({ id: this.options.userId, cookie: document.cookie }); + const token = JSON.stringify({ id: this.options.userId });Note: You'll also need to update the server-side authentication logic (in
apps/live/src/controllers/issue-events.controller.ts) to rely on the cookie sent in the WebSocket handshake headers rather than extracting it from the token payload.
🧹 Nitpick comments (1)
apps/web/core/hooks/use-issue-channel.ts (1)
323-327: Consider wrapping the fallback callback in try-catch.The
Promise.resolve().then(callback)fallback can lead to unhandled promise rejections if the callback throws an error. While this is a defensive fallback for browsers withoutqueueMicrotask, consider wrapping the callback in a try-catch to prevent unhandled rejections.Apply this diff to add error handling:
const enqueueMicrotask = typeof queueMicrotask === "function" ? queueMicrotask : (callback: () => void) => { - Promise.resolve().then(callback); + Promise.resolve().then(() => { + try { + callback(); + } catch (error) { + console.error("Microtask callback error", error); + } + }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/api/plane/bgtasks/issue_activities_task.py(2 hunks)apps/live/src/controllers/index.ts(1 hunks)apps/live/src/controllers/issue-events.controller.ts(1 hunks)apps/web/core/hooks/use-issue-channel.ts(1 hunks)apps/web/core/layouts/auth-layout/project-wrapper.tsx(2 hunks)apps/web/core/store/issue/helpers/base-issues.store.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/live/src/controllers/index.ts
🧰 Additional context used
🧬 Code graph analysis (5)
apps/live/src/controllers/issue-events.controller.ts (4)
packages/logger/src/config.ts (1)
logger(14-14)packages/decorators/src/websocket.ts (1)
WebSocket(8-13)apps/live/src/lib/auth.ts (1)
handleAuthentication(64-82)apps/live/src/redis.ts (1)
redisManager(211-211)
apps/api/plane/bgtasks/issue_activities_task.py (1)
apps/api/plane/utils/exception_logger.py (1)
log_exception(9-20)
apps/web/core/layouts/auth-layout/project-wrapper.tsx (2)
apps/web/core/hooks/use-issue-channel.ts (1)
useIssueChannel(778-802)apps/space/core/store/publish/publish.store.ts (1)
workspaceSlug(93-95)
apps/web/core/store/issue/helpers/base-issues.store.ts (1)
packages/types/src/issues/issue.ts (1)
TIssue(83-97)
apps/web/core/hooks/use-issue-channel.ts (4)
packages/types/src/view-props.ts (1)
TIssueParams(56-82)packages/types/src/issues/issue.ts (1)
TIssue(83-97)apps/web/core/lib/store-context.tsx (1)
rootStore(7-7)apps/space/core/hooks/store/use-user.ts (1)
useUser(7-11)
🔇 Additional comments (13)
apps/web/core/layouts/auth-layout/project-wrapper.tsx (2)
30-30: LGTM!The import statement is correct and the hook is properly integrated into the component.
64-65: LGTM!The hook is correctly integrated following React's rules of hooks. The optional chaining and string conversion are appropriate for handling potentially undefined values.
apps/web/core/store/issue/helpers/base-issues.store.ts (2)
88-88: LGTM!The method signature change is backward compatible by making the new parameter optional. This allows callers to provide the pre-update issue state when available, supporting more accurate real-time update reconciliation.
1199-1203: LGTM!The implementation correctly prioritizes the provided
issueBeforeUpdatesnapshot before falling back to the current store state. This ensures grouped-list deletions use the most accurate reference, especially during real-time updates where the store state may have already changed. The guard at line 1201 is appropriate as it prevents errors when attempting to remove an issue that no longer exists in any form.apps/api/plane/bgtasks/issue_activities_task.py (3)
45-56: LGTM!The deserialization helper correctly handles various input types with appropriate fallbacks. The error handling for JSON parsing is defensive and safe.
58-104: LGTM!The broadcast function is well-structured with defensive error handling at each step. The Redis connection errors and publish failures are logged without interrupting the main workflow, which is appropriate. The payload structure includes all necessary fields for downstream consumers.
1643-1652: LGTM!The broadcast integration is correctly placed after persisting activities but before notification processing. This ensures real-time listeners receive updates promptly while maintaining data consistency.
apps/live/src/controllers/issue-events.controller.ts (3)
23-61: LGTM!The helper functions are well-implemented with appropriate error handling and defensive checks. The parameter extraction and token parsing are safe, and the socket closure is properly guarded.
63-85: Verify session validation when no cookie is present.While a previous review indicated this was addressed, the code still returns
trueat line 84 whentoken?.idexists without a cookie. This may allow connections with only a user ID in the token, bypassing proper session validation. Ensure this fallback behavior aligns with your security requirements.Please confirm:
- Is it acceptable to allow WebSocket connections with only a
token.id(no cookie)?- If yes, what prevents an attacker from guessing or enumerating user IDs to open unauthorized connections?
- If no, should line 84 be removed so authentication always requires a valid cookie?
87-178: LGTM!The WebSocket controller is well-structured with robust error handling, proper cleanup, and comprehensive lifecycle management. The Redis subscription pattern correctly matches the broadcast channel format, and all error cases are properly handled with appropriate WebSocket close codes.
apps/web/core/hooks/use-issue-channel.ts (3)
397-429: Queue processing logic is sound.The
processIssuemethod correctly handles the per-issue queue by maintaining a reference to the state object in the map. New payloads enqueued during processing update the same state object, so the while loop will process them on subsequent iterations. The flag-based approach prevents concurrent processing for the same issue.
431-660: LGTM!The sync methods are well-structured with comprehensive handling of different scenarios. The 404 handling correctly removes issues from all relevant stores, and the project view filter evaluation ensures consistency. The conditional module update logic properly handles addition, removal, and updates.
778-802: LGTM!The hook is well-structured with proper React hooks patterns. The effect properly guards against missing dependencies, creates necessary instances, and ensures thorough cleanup on unmount. The dependency array is correct and includes all external values used within the effect.
Description
issue_activityupdates to Redis and broadcast them to per-project channels/live/issueswebsocket endpoint that authenticates users and streams Redis issue eventsuseIssueChannelhook so project views reconcile incoming payloads and refresh module/project lists in real timeType of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Bug Fixes