refactor: decouple useCollaborationStatus from lint WebSocket endpoint#154
Open
R-Hart80 wants to merge 9 commits intoCatholicOS:devfrom
Open
refactor: decouple useCollaborationStatus from lint WebSocket endpoint#154R-Hart80 wants to merge 9 commits intoCatholicOS:devfrom
R-Hart80 wants to merge 9 commits intoCatholicOS:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDecouples lint WebSocket handling from collaboration presence: Changes
Sequence Diagram(s)sequenceDiagram
participant EditorPage
participant useCollab as useCollaborationStatus
participant DevLayout as DeveloperEditorLayout
participant HealthPanel as HealthCheckPanel
participant LintWS as Lint WS
EditorPage->>useCollab: request collaboration status (projectId)
useCollab-->>EditorPage: status="disabled", endpoint="/api/v1/collab/ws"
EditorPage->>DevLayout: render with showHealthCheck/onClose/onLintWsStatusChange
DevLayout->>HealthPanel: mount when showHealthCheck=true (pass onWsStatusChange)
HealthPanel->>LintWS: open WS when mounted
LintWS-->>HealthPanel: open / message / close / error events
HealthPanel-->>DevLayout: onWsStatusChange("connecting"/"connected"/"disconnected")
DevLayout-->>EditorPage: propagate lint WS status via callback/state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/projects/`[id]/editor/page.tsx:
- Around line 911-917: The ConnectionStatus lint indicator is currently gated by
showHealthCheck but HealthCheckPanel may not be mounted, causing a misleading
disconnected icon; change the render condition to the actual panel
mount/visibility flag used for HealthCheckPanel (e.g., isHealthCheckPanelOpen,
healthCheckPanelMounted, or whatever state/prop controls mounting of
HealthCheckPanel) so ConnectionStatus is only rendered when the panel (and its
WS) exists; keep the same props (state={lintWsStatus} and endpoint using
projectId) but replace showHealthCheck with the true panel visibility/mount
variable referenced where HealthCheckPanel is rendered.
🪄 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: 984e75c7-b4ae-43f1-a1b9-45946bf8e8fc
📒 Files selected for processing (7)
__tests__/lib/hooks/useProjectViewer.test.tsapp/projects/[id]/editor/page.tsxapp/projects/[id]/page.tsxcomponents/editor/HealthCheckPanel.tsxcomponents/editor/developer/DeveloperEditorLayout.tsxlib/hooks/useCollaborationStatus.tslib/hooks/useProjectViewer.ts
💤 Files with no reviewable changes (3)
- tests/lib/hooks/useProjectViewer.test.ts
- app/projects/[id]/page.tsx
- lib/hooks/useProjectViewer.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/projects/`[id]/editor/page.tsx:
- Line 93: The lintWsStatus state can persist across panel unmounts causing
stale “connected” UI; add logic to reset it to "disconnected" whenever the
health-check panel is hidden or developer mode is turned off: update the
component containing lintWsStatus and setLintWsStatus to include a useEffect
that watches showHealthCheck and developerMode (or whatever prop/state controls
developer mode) and calls setLintWsStatus("disconnected") when showHealthCheck
becomes false or developerMode is false; if there is a dedicated health-check
panel component with a useEffect for mount/unmount (or a cleanup callback in the
WS connection handler), ensure that on unmount you also call
setLintWsStatus("disconnected") to clear state.
🪄 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: 94340e4c-b12b-4cf0-9e0e-1b01fb8a9c20
📒 Files selected for processing (1)
app/projects/[id]/editor/page.tsx
60 tasks
Closes CatholicOS#140. Previously useCollaborationStatus connected to /lint/ws as a placeholder, creating a duplicate WebSocket connection alongside HealthCheckPanel. Changes: - useCollaborationStatus: removed WebSocket logic, returns "disabled" state until the real /collab/ws endpoint is available - HealthCheckPanel: added onWsStatusChange prop to expose lint WS status - DeveloperEditorLayout: threads onLintWsStatusChange down to HealthCheckPanel - Editor toolbar: collab icon uses useCollaborationStatus; lint icon uses HealthCheckPanel WS status and only renders when the panel is open - Viewer toolbar: removed lint WS icon (no HealthCheckPanel in viewer) - Removed connectionStatus/wsEndpoint/wsPurpose from useProjectViewer return - Removed stale "exposes collaboration status" test from useProjectViewer tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…unt condition CodeRabbit noted that showHealthCheck alone is insufficient — HealthCheckPanel is only mounted inside DeveloperEditorLayout (developer mode). Added editorMode === "developer" to the lint ConnectionStatus condition so the icon only appears when the panel is actually mounted and the WebSocket is active. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CodeRabbit noted that lintWsStatus could remain stale when the panel closes before the WebSocket cleanup callback fires. Added a useEffect that resets the state to "disconnected" whenever showHealthCheck is false or editorMode is not "developer", ensuring the UI is never stuck in a connected state after the panel and its WebSocket are gone. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
R-Hart80
force-pushed
the
refactor/decouple-collaboration-status
branch
from
1545871 to
7c223ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/projects/`[id]/editor/page.tsx:
- Around line 1066-1069: The page currently mounts a second HealthCheckPanel
causing duplicate WebSocket connections; update the page-level HealthCheckPanel
usage so it is not mounted in developer mode (guard its render with editorMode
!== "developer") or remove it entirely and rely on DeveloperEditorLayout's
HealthCheckPanel which already reports status via
onLintWsStatusChange/setLintWsStatus; target the HealthCheckPanel render and
props (showHealthCheck, onCloseHealthCheck, onLintWsStatusChange,
onUpdateProperty) and ensure only DeveloperEditorLayout keeps the WS-handling
HealthCheckPanel to avoid parallel connections.
🪄 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: 2b2b50bb-0372-46f5-890e-3d9241b0a8a0
📒 Files selected for processing (7)
__tests__/lib/hooks/useProjectViewer.test.tsapp/projects/[id]/editor/page.tsxapp/projects/[id]/page.tsxcomponents/editor/HealthCheckPanel.tsxcomponents/editor/developer/DeveloperEditorLayout.tsxlib/hooks/useCollaborationStatus.tslib/hooks/useProjectViewer.ts
💤 Files with no reviewable changes (2)
- tests/lib/hooks/useProjectViewer.test.ts
- lib/hooks/useProjectViewer.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/projects/[id]/page.tsx
- components/editor/developer/DeveloperEditorLayout.tsx
- components/editor/HealthCheckPanel.tsx
In developer mode, DeveloperEditorLayout already mounts HealthCheckPanel and reports its WS status via onLintWsStatusChange. Mounting a second panel in the page caused duplicate WebSocket connections. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/projects/[id]/editor/page.tsx (1)
69-75:⚠️ Potential issue | 🟡 MinorRemove the unused
enableWebSocketparameter—it's never referenced in the hook implementation.The parameter is defined in the interface and destructured in the function signature, but never used in the function body or return statement. Since WebSocket connections for lint status and quality checks are managed independently by
HealthCheckPaneland other components (not byuseProjectViewer), this parameter is dead code and should be removed along with its JSDoc comment. The decoupling from websocket lifecycle is already complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/projects/`[id]/editor/page.tsx around lines 69 - 75, Remove the dead enableWebSocket option from the useProjectViewer API: delete the enableWebSocket property from the hook's options interface and JSDoc, remove it from the useProjectViewer function parameter destructuring/signature and any internal references, and update all call sites (e.g., the call in page.tsx that passes enableWebSocket: true) to stop passing that option; leave WebSocket lifecycle to HealthCheckPanel and other components as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/projects/`[id]/editor/page.tsx:
- Around line 916-927: The toolbar currently shows the lint ConnectionStatus
only when showHealthCheck && editorMode === "developer"; instead, wire the
page-level HealthCheckPanel to publish lint WebSocket status by passing the
setter (setLintWsStatus) into that component (the HealthCheckPanel instance that
currently mounts elsewhere) and change the toolbar branch to render the lint
ConnectionStatus whenever showHealthCheck is true (remove the editorMode ===
"developer" guard) using state={lintWsStatus}, purpose="Real-time lint status
updates" and endpoint={`/api/v1/projects/${projectId}/lint/ws`} so the icon
follows the open HealthCheckPanel across both modes.
---
Outside diff comments:
In `@app/projects/`[id]/editor/page.tsx:
- Around line 69-75: Remove the dead enableWebSocket option from the
useProjectViewer API: delete the enableWebSocket property from the hook's
options interface and JSDoc, remove it from the useProjectViewer function
parameter destructuring/signature and any internal references, and update all
call sites (e.g., the call in page.tsx that passes enableWebSocket: true) to
stop passing that option; leave WebSocket lifecycle to HealthCheckPanel and
other components as-is.
🪄 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: ee9ed195-97f8-4f67-869c-3bb0b8f35d9b
📒 Files selected for processing (1)
app/projects/[id]/editor/page.tsx
- Add missing ConnectionStatus import to page.tsx (viewer) - Add missing HealthCheckPanel import to DeveloperEditorLayout - Add canManage prop to DeveloperEditorLayoutProps and pass it from editor page - Make showHealthCheck/onCloseHealthCheck optional (viewer and tests don't use them) - Remove enableWebSocket from UseProjectViewerOptions (no longer used after stub) - Remove enableWebSocket: true call-site in editor page Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Update useCollaborationStatus tests to match stub behavior - Add addEventListener to createLintWebSocket mock (beforeEach and initial) - Remove fetchData from WS effect deps (fetchDataRef already used inside) - Pass setLintWsStatus to page-level HealthCheckPanel (standard mode) - Show lint ConnectionStatus whenever panel is open, not just in developer mode Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The mockImplementation at the lint_started/lint_complete test was returning a WebSocket stub without addEventListener, causing an unhandled error when the component called ws.addEventListener(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds unit tests for the onWsStatusChange status transitions (disconnected/connecting/connected/cleanup) in HealthCheckPanel and verifies that DeveloperEditorLayout renders HealthCheckPanel only when showHealthCheck is true, closing the Codecov patch gap. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TypeScript 5.x narrows let variables assigned inside closures to never.
Using a { fn } object holder avoids this — object property access is not
subject to closure-based control flow narrowing.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #140
Summary
useCollaborationStatus: removed all WebSocket logic — returns"disabled"state until the real/collab/wsendpoint is implementedHealthCheckPanel: addedonWsStatusChangeprop to expose the lint WS connection state to the parentDeveloperEditorLayout: threadsonLintWsStatusChangeprop down toHealthCheckPaneluseCollaborationStatus; lint icon usesHealthCheckPanelWS status and only renders when the panel is open (eliminating the duplicate connection)HealthCheckPanelconnectionStatus/wsEndpoint/wsPurposefromuseProjectViewerreturn value"exposes collaboration status"test fromuseProjectViewertestsBefore / After
Before:
After:
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
UI
Tests