Repository navigation
feat(apps/playground): WebSocket sync for Lexical × EG-walker - #831
Merged
Merged
Conversation
Enable Nitro WebSocket relays for document batches and presence, wire the Lexical demo to same-origin WS by default (BroadcastChannel fallback via ?transport=broadcast), and cover cross-browser sync in e2e.
|
Cursor Agent can help with this pull request. Just |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe playground adds selectable WebSocket and BroadcastChannel collaboration transports. It introduces Nitro WebSocket relays, reconnecting document channels, Lexical room integration, connection-state UI, updated navigation, and browser tests for synchronization and persistence. ChangesCollaboration transport
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BrowserA
participant LexicalRoom
participant WebSocketRelays
participant BrowserB
BrowserA->>LexicalRoom: Resolve room transport
LexicalRoom->>WebSocketRelays: Connect document and presence sockets
BrowserA->>WebSocketRelays: Send room event and presence update
WebSocketRelays-->>BrowserB: Relay synchronized data
BrowserB-->>LexicalRoom: Update document and connection state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Member
Author
|
@cursor make sure the type check passes |
|
Fixed. CI failed with TS2742 because Annotated all three Nitro WS handlers as   |
TS2742 failed typecheck because defineWebSocketHandler's inferred default export referenced a non-portable h3 path. Annotate handlers as EventHandler from nitro/h3.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/playground/src/routes/demo/online-collab-editor.tsx (1)
66-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the selected transport in room URLs.
If a user starts with
?transport=broadcast, both history updates remove that parameter. A reload or shared room URL then resolves the default WebSocket transport instead of BroadcastChannel.Update the existing URL and set only its
roomparameter.Proposed fix
- window.history.replaceState( - null, - "", - `${window.location.pathname}?room=${roomId}`, - ); + const url = new URL(window.location.href); + url.searchParams.set("room", roomId); + window.history.replaceState(null, "", url);Apply the same change in
handleJoinRoom.Also applies to: 83-87
🤖 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/playground/src/routes/demo/online-collab-editor.tsx` around lines 66 - 70, Update both history URL writes in the room-joining flow, including handleJoinRoom, to modify the existing URL’s room parameter while preserving any existing transport parameter. Avoid reconstructing the query string from only pathname and roomId so broadcast transport remains encoded across reloads and shared URLs.Source: Coding guidelines
🧹 Nitpick comments (7)
apps/playground/e2e/lexical-eg-walker.spec.ts (1)
283-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSplit this E2E specification.
This file reaches 324 lines. Extract the WebSocket and BroadcastChannel cases into dedicated specification files. Keep shared room helpers in a small utility module.
As per coding guidelines, “Keep modules under 200 lines; split larger modules into smaller ones.”
🤖 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/playground/e2e/lexical-eg-walker.spec.ts` around lines 283 - 324, Split the tests in the “Lexical EG-walker WebSocket cross-browser” describe block into dedicated WebSocket and BroadcastChannel specification files, keeping each under 200 lines. Move shared helpers such as room setup and document assertions into a small utility module, and update both specifications to reuse those helpers without changing test behavior.Source: Coding guidelines
apps/playground/src/modules/collab-transport/websocket-broadcast-channel.ts (2)
95-98: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCompare socket identity in the
closelistener.The listener sets the shared
sockettonullwithout checking which socket fired the event. The current control flow prevents overlapping connections, so this is correct today. If a future change ever callsconnectwhile a socket is still open, a latecloseevent from the old socket clears the new socket and schedules a spurious reconnect.Capture the socket in a local and compare identity.
♻️ Sketch
- socket.addEventListener("close", () => { - socket = null; - scheduleReconnect(); - }); + const current = socket; + current.addEventListener("close", () => { + if (socket !== current) return; + socket = null; + scheduleReconnect(); + });🤖 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/playground/src/modules/collab-transport/websocket-broadcast-channel.ts` around lines 95 - 98, Update the close listener in the socket connection flow to capture the connected socket in a local variable and only clear the shared socket and call scheduleReconnect when the closed socket is still the current socket. Preserve the existing reconnect behavior for the active socket while ignoring late close events from replaced sockets.
46-68: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd reconnect backoff and bound the outbound queue.
Two resilience gaps:
scheduleReconnectalways waits the samereconnectDelayMs, which defaults to 1000 ms. There is no backoff, no jitter, and no attempt cap. If the relay is down, every open tab retries once per second indefinitely.outboundQueuehas no size limit. Line 120 pushes every message sent while the socket is down. A long disconnection in an active editing session grows the array without bound, andflushQueuethen sends the whole backlog in one synchronous loop on reconnect.Apply exponential backoff with jitter, and cap the queue length.
♻️ Sketch
const DEFAULT_RECONNECT_DELAY_MS = 1_000; +const MAX_RECONNECT_DELAY_MS = 30_000; +const MAX_QUEUED_MESSAGES = 500; @@ + let attempt = 0; const scheduleReconnect = (): void => { if (closed || reconnectTimer !== null) return; + const base = Math.min( + reconnectDelayMs * 2 ** attempt, + MAX_RECONNECT_DELAY_MS, + ); + attempt += 1; reconnectTimer = setTimeout(() => { reconnectTimer = null; connect(); - }, reconnectDelayMs); + }, base * (0.5 + Math.random() * 0.5)); };Reset
attempt = 0inside theopenlistener.🤖 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/playground/src/modules/collab-transport/websocket-broadcast-channel.ts` around lines 46 - 68, Update scheduleReconnect to track reconnect attempts, apply capped exponential backoff with jitter, and stop scheduling after a defined maximum attempt count; reset the attempt counter in the socket open listener. Bound outboundQueue when enqueueing messages in the send path, retaining only the permitted number of pending messages, and ensure reconnect behavior remains unchanged after a successful connection.apps/playground/src/modules/collab-transport/urls.test.ts (2)
61-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the reconnect and inbound-message paths.
The suite tests the outbound queue but not the other asynchronous behaviour of
createWebSocketBroadcastChannel:
- Reconnect after a
closeevent, which needsvi.useFakeTimersto advancereconnectDelayMs.- The
messagelistener, including thatonmessagereceives parsed JSON and that a malformed frame is dropped without throwing.- That
channel.close()stops further reconnect attempts.The existing fake socket already collects listeners, so each case needs only a dispatch and an assertion.
As per path instructions "Test async operations thoroughly using Vitest or Playwright".
🤖 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/playground/src/modules/collab-transport/urls.test.ts` around lines 61 - 104, Extend the websocket broadcast channel tests around createWebSocketBroadcastChannel to cover reconnect scheduling after a close event using vi.useFakeTimers and reconnectDelayMs, inbound message handling that passes parsed JSON to onmessage, and malformed frames being ignored without throwing. Also verify channel.close() prevents subsequent reconnect attempts, reusing the existing fake socket listener registry and dispatching events directly; restore fake timers after each test.Source: Coding guidelines
15-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a
wss:input case to the origin test.
toWebSocketOriginis tested only withhttp:andhttps:inputs. It downgrades awss:input tows:, as noted in the comment onurls.tslines 20-27. Add the case so the fix is locked in.💚 Proposed addition
expect(toWebSocketOrigin("https://playground.example/")).toBe( "wss://playground.example", ); + expect(toWebSocketOrigin("wss://playground.example")).toBe( + "wss://playground.example", + );🤖 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/playground/src/modules/collab-transport/urls.test.ts` around lines 15 - 22, Add a `wss:` input assertion to the existing “maps http(s) origins to ws(s)” test for `toWebSocketOrigin`, verifying that a secure WebSocket origin remains `wss:` with its host and path preserved.apps/playground/src/modules/collab-transport/urls.ts (1)
87-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the
nulloverride semantics and the broadcast placeholder origin.Three points:
- Line 100-102 uses
??, so an explicitnullfordocWsUrl,presenceWsUrl, orsyncWsUrlfalls back to the same-origin default. The option types declarestring | null, which suggestsnullmeans "no endpoint". Narrow the option types tostring | undefined, or handlenullas an explicit disable.- Line 88 passes the literal
"http://localhost"intobuildSameOriginEndpointsfor broadcast mode. The broadcast branch returns all-null endpoints and never reads the origin. The argument is dead and misleading. Return the broadcast endpoints directly.- Line 94 hardcodes the SSR fallback port
3000. The PR makes the Playwright port configurable. Keep the two in sync, or derive this value from the same source.🤖 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/playground/src/modules/collab-transport/urls.ts` around lines 87 - 103, Update the URL option types and fallback handling for docWsUrl, presenceWsUrl, and syncWsUrl so null is not silently converted to a default—either narrow them to string | undefined or preserve null as an explicit disabled endpoint. In the broadcast branch, return the broadcast endpoints directly without passing the unused localhost origin to buildSameOriginEndpoints. Replace the SSR localhost:3000 fallback with the shared configurable Playwright port source.apps/playground/src/env.ts (1)
19-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate the WebSocket override values as URLs.
z.string().min(1)accepts invalid input such as"not-a=url". These values are passed tonew URL(baseUrl, ...), which throws at WebSocket construction time. Usez.url()so malformed WebSocket overrides fail during environment validation, and keep them limited to WebSocket schemes if needed.🤖 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/playground/src/env.ts` around lines 19 - 24, Update the VITE_COLLAB_DOC_WS_URL, VITE_COLLAB_PRESENCE_WS_URL, and VITE_COLLAB_SYNC_WS_URL schemas in the environment validation to use z.url() instead of only z.string().min(1). Restrict accepted values to WebSocket schemes (ws or wss) if supported by the existing validation conventions.
🤖 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/playground/playwright.config.ts`:
- Around line 45-46: Ensure the Playwright web server and Vite use a strict E2E
port by updating the Vite server configuration in vite.config.ts to enable
strictPort, or by adding the equivalent --strictPort flag to the command in the
Playwright configuration. Preserve the existing port and baseURL values.
In `@apps/playground/server/api/collab-doc.ts`:
- Line 80: Replace each direct default export of defineWebSocketHandler with a
typed const assignment followed by exporting that const. Apply the explicit
handler annotation in apps/playground/server/api/collab-doc.ts lines 80-80,
apps/playground/server/api/presence.ts lines 77-77, and
apps/playground/server/api/collab-sync.ts line 62, using the existing WebSocket
handler type so no internal h3 path appears in the inferred export type.
- Around line 38-46: Implement room lifecycle limits across all relays: in
apps/playground/server/api/collab-doc.ts:38-46, cap each RoomState.batches map
and remove the rooms entry when the final peer disconnects; in
apps/playground/server/api/collab-sync.ts:27-43, cap RoomState.events, delete
rooms on the last disconnect, and remove the unused peers field or maintain it
in the close hook; in apps/playground/server/api/presence.ts:177-193, delete the
rooms entry when the room’s user map becomes empty.
- Around line 51-65: Update parseDocMessage to validate knownBatchIds before
casting and returning the parsed message: require it to be an array containing
only non-empty strings, or apply equivalent non-array validation when the field
is represented as a Set. Preserve null for malformed messages so downstream new
Set(parsed.knownBatchIds ?? []) cannot receive numeric or object values.
In `@apps/playground/src/modules/collab-transport/urls.ts`:
- Around line 20-27: Update toWebSocketOrigin to preserve existing WebSocket
schemes: keep wss: as wss:, map https: to wss:, and map http: to ws: without
downgrading secure inputs. Remove the unused trimTrailingSlash helper and return
the URL origin directly.
---
Outside diff comments:
In `@apps/playground/src/routes/demo/online-collab-editor.tsx`:
- Around line 66-70: Update both history URL writes in the room-joining flow,
including handleJoinRoom, to modify the existing URL’s room parameter while
preserving any existing transport parameter. Avoid reconstructing the query
string from only pathname and roomId so broadcast transport remains encoded
across reloads and shared URLs.
---
Nitpick comments:
In `@apps/playground/e2e/lexical-eg-walker.spec.ts`:
- Around line 283-324: Split the tests in the “Lexical EG-walker WebSocket
cross-browser” describe block into dedicated WebSocket and BroadcastChannel
specification files, keeping each under 200 lines. Move shared helpers such as
room setup and document assertions into a small utility module, and update both
specifications to reuse those helpers without changing test behavior.
In `@apps/playground/src/env.ts`:
- Around line 19-24: Update the VITE_COLLAB_DOC_WS_URL,
VITE_COLLAB_PRESENCE_WS_URL, and VITE_COLLAB_SYNC_WS_URL schemas in the
environment validation to use z.url() instead of only z.string().min(1).
Restrict accepted values to WebSocket schemes (ws or wss) if supported by the
existing validation conventions.
In `@apps/playground/src/modules/collab-transport/urls.test.ts`:
- Around line 61-104: Extend the websocket broadcast channel tests around
createWebSocketBroadcastChannel to cover reconnect scheduling after a close
event using vi.useFakeTimers and reconnectDelayMs, inbound message handling that
passes parsed JSON to onmessage, and malformed frames being ignored without
throwing. Also verify channel.close() prevents subsequent reconnect attempts,
reusing the existing fake socket listener registry and dispatching events
directly; restore fake timers after each test.
- Around line 15-22: Add a `wss:` input assertion to the existing “maps http(s)
origins to ws(s)” test for `toWebSocketOrigin`, verifying that a secure
WebSocket origin remains `wss:` with its host and path preserved.
In `@apps/playground/src/modules/collab-transport/urls.ts`:
- Around line 87-103: Update the URL option types and fallback handling for
docWsUrl, presenceWsUrl, and syncWsUrl so null is not silently converted to a
default—either narrow them to string | undefined or preserve null as an explicit
disabled endpoint. In the broadcast branch, return the broadcast endpoints
directly without passing the unused localhost origin to
buildSameOriginEndpoints. Replace the SSR localhost:3000 fallback with the
shared configurable Playwright port source.
In `@apps/playground/src/modules/collab-transport/websocket-broadcast-channel.ts`:
- Around line 95-98: Update the close listener in the socket connection flow to
capture the connected socket in a local variable and only clear the shared
socket and call scheduleReconnect when the closed socket is still the current
socket. Preserve the existing reconnect behavior for the active socket while
ignoring late close events from replaced sockets.
- Around line 46-68: Update scheduleReconnect to track reconnect attempts, apply
capped exponential backoff with jitter, and stop scheduling after a defined
maximum attempt count; reset the attempt counter in the socket open listener.
Bound outboundQueue when enqueueing messages in the send path, retaining only
the permitted number of pending messages, and ensure reconnect behavior remains
unchanged after a successful connection.
🪄 Autofix
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 Plus
Run ID: 7c7329e4-095b-4429-8a87-29af238e0138
📒 Files selected for processing (20)
apps/playground/e2e/lexical-eg-walker.spec.tsapps/playground/playwright.config.tsapps/playground/server/README.mdapps/playground/server/api/collab-doc.tsapps/playground/server/api/collab-sync.tsapps/playground/server/api/presence.tsapps/playground/src/env.tsapps/playground/src/modules/collab-transport/urls.test.tsapps/playground/src/modules/collab-transport/urls.tsapps/playground/src/modules/collab-transport/websocket-broadcast-channel.tsapps/playground/src/modules/lexical-eg-walker/LexicalEgWalkerDemo.tsxapps/playground/src/modules/lexical-eg-walker/StatusRail.tsxapps/playground/src/modules/lexical-eg-walker/presence.test.tsapps/playground/src/modules/lexical-eg-walker/presence.tsapps/playground/src/modules/lexical-eg-walker/transport.tsapps/playground/src/modules/lexical-eg-walker/useLexicalRoom.tsapps/playground/src/routes/demo/online-collab-editor.tsxapps/playground/src/routes/index.tsxapps/playground/vite.config.tsdocs/design/surface-bindings.md
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/playground/server/api/collab-sync.ts (1)
81-88: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winBroken Authentication (CWE-287): Improper Authentication
Reachability: External
Keep
userIdadvisory or bind it before the relay uses it.
collab-syncacceptsuserIdonly from the parsed message and uses it forroom.peers, so clients can spoof another user. Either reject spoofed IDs against an authenticated peer identity or document thatuserIdis advisory only.🤖 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/playground/server/api/collab-sync.ts` around lines 81 - 88, Update the join/leave handling around the parsed userId usage so clients cannot alter room.peers membership by spoofing another identity: validate parsed.userId against the authenticated peer identity and reject mismatches before mutating or publishing, or explicitly mark and treat userId as advisory throughout the relay. Preserve the existing add/delete and publish behavior for valid messages.
🤖 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.
Outside diff comments:
In `@apps/playground/server/api/collab-sync.ts`:
- Around line 81-88: Update the join/leave handling around the parsed userId
usage so clients cannot alter room.peers membership by spoofing another
identity: validate parsed.userId against the authenticated peer identity and
reject mismatches before mutating or publishing, or explicitly mark and treat
userId as advisory throughout the relay. Preserve the existing add/delete and
publish behavior for valid messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abe0b29c-618b-40c1-bdfc-18be9d000ec7
📒 Files selected for processing (3)
apps/playground/server/api/collab-doc.tsapps/playground/server/api/collab-sync.tsapps/playground/server/api/presence.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/playground/server/api/presence.ts
Harden room lifecycle caps/cleanup, knownBatchIds validation, WS URL resolution, reconnect backoff, and Playwright strictPort; split e2e transport specs and extend unit coverage.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/playground/playground-port.ts (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename utility modules to camelCase.
Both new utility module paths use kebab case.
apps/playground/playground-port.ts#L1-L2: Rename the module toplaygroundPort.tsand update its imports.apps/playground/e2e/helpers/lexical-eg-walker.ts#L1-L7: Rename the module tolexicalEgWalker.tsand update spec imports.As per coding guidelines, use camelCase filenames for utility modules.
🤖 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/playground/playground-port.ts` around lines 1 - 2, Rename apps/playground/playground-port.ts to playgroundPort.ts and update every import referencing it; rename apps/playground/e2e/helpers/lexical-eg-walker.ts to lexicalEgWalker.ts and update all spec imports accordingly. Preserve the existing exports and behavior.Source: Coding guidelines
🤖 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 `@apps/playground/playground-port.ts`:
- Around line 1-2: Rename apps/playground/playground-port.ts to
playgroundPort.ts and update every import referencing it; rename
apps/playground/e2e/helpers/lexical-eg-walker.ts to lexicalEgWalker.ts and
update all spec imports accordingly. Preserve the existing exports and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 41d5d97b-553e-468d-a36a-61ff227426fd
📒 Files selected for processing (14)
apps/playground/e2e/helpers/lexical-eg-walker.tsapps/playground/e2e/lexical-eg-walker-broadcast.spec.tsapps/playground/e2e/lexical-eg-walker-websocket.spec.tsapps/playground/e2e/lexical-eg-walker.spec.tsapps/playground/playground-port.tsapps/playground/playwright.config.tsapps/playground/server/api/collab-doc.tsapps/playground/server/api/collab-sync.tsapps/playground/server/api/presence.tsapps/playground/src/env.tsapps/playground/src/modules/collab-transport/urls.test.tsapps/playground/src/modules/collab-transport/urls.tsapps/playground/src/modules/collab-transport/websocket-broadcast-channel.tsapps/playground/src/routes/demo/online-collab-editor.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/playground/playwright.config.ts
- apps/playground/server/api/collab-doc.ts
- apps/playground/src/env.ts
- apps/playground/src/modules/collab-transport/websocket-broadcast-channel.ts
- apps/playground/src/modules/collab-transport/urls.ts
- apps/playground/src/modules/collab-transport/urls.test.ts
Lead with Lexical × EG-walker WebSocket collab, drop TanStack marketing cards, and update home e2e coverage for the new demo grid.
…e UI Request batch repair when the document WebSocket reopens, publish presence reconnecting/error states, and show degraded connection status in the Lexical StatusRail (including sync paused hint).
|
Important UI Tests need review – Review now🟡 UI Tests: softmaple_packages/awareness: 38 visual and accessibility changes must be accepted as baselines |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/playground/src/modules/collab-transport/websocket-broadcast-channel.ts (2)
40-202: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSplit this module below 200 lines.
Extract the reconnect or channel-wrapper logic into a focused module. The current file has 215 lines.
As per coding guidelines: “Keep modules under 200 lines; split larger modules into smaller ones.”
🤖 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/playground/src/modules/collab-transport/websocket-broadcast-channel.ts` around lines 40 - 202, Split createWebSocketBroadcastChannel into focused modules so the current file is under 200 lines. Extract either the reconnect lifecycle helpers around connect, scheduleReconnect, and clearReconnect or the returned channel wrapper around postMessage and close, while preserving the existing connection, queue, and event-handler behavior.Source: Coding guidelines
40-42: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse a camelCase utility filename.
Rename
websocket-broadcast-channel.tsto a camelCase filename such aswebsocketBroadcastChannel.ts. Update its direct imports.As per coding guidelines: “Use camelCase filenames for utility modules.”
🤖 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/playground/src/modules/collab-transport/websocket-broadcast-channel.ts` around lines 40 - 42, Rename the utility module file from websocket-broadcast-channel.ts to websocketBroadcastChannel.ts and update every direct import or reference to the new camelCase filename, keeping createWebSocketBroadcastChannel unchanged.Source: Coding guidelines
🤖 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/playground/src/modules/lexical-eg-walker/StatusRail.tsx`:
- Around line 196-204: Update the reconnect warning condition in StatusRail so
it is not rendered while connectionState is connecting; restrict it to
reconnecting, disconnected, or error states while preserving the existing
websocket requirement and message.
In `@packages/awareness/src/adapters/websocket/websocket.ts`:
- Around line 239-241: Update handleClose to call
clearConnectionTimeout(internal) after stopHeartbeat and before beginReconnect,
ensuring the closed socket’s pending connection timeout cannot affect the
replacement connection.
---
Outside diff comments:
In `@apps/playground/src/modules/collab-transport/websocket-broadcast-channel.ts`:
- Around line 40-202: Split createWebSocketBroadcastChannel into focused modules
so the current file is under 200 lines. Extract either the reconnect lifecycle
helpers around connect, scheduleReconnect, and clearReconnect or the returned
channel wrapper around postMessage and close, while preserving the existing
connection, queue, and event-handler behavior.
- Around line 40-42: Rename the utility module file from
websocket-broadcast-channel.ts to websocketBroadcastChannel.ts and update every
direct import or reference to the new camelCase filename, keeping
createWebSocketBroadcastChannel unchanged.
🪄 Autofix
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 Plus
Run ID: ef928776-eed4-4c7e-88ce-d1d038cd0aac
📒 Files selected for processing (13)
apps/playground/e2e/home.spec.tsapps/playground/src/modules/collab-transport/urls.test.tsapps/playground/src/modules/collab-transport/websocket-broadcast-channel.tsapps/playground/src/modules/lexical-eg-walker/LexicalEgWalkerDemo.tsxapps/playground/src/modules/lexical-eg-walker/StatusRail.tsxapps/playground/src/modules/lexical-eg-walker/persistence/channel.test.tsapps/playground/src/modules/lexical-eg-walker/persistence/channel.tsapps/playground/src/modules/lexical-eg-walker/persistence/coordinator.test.tsapps/playground/src/modules/lexical-eg-walker/persistence/coordinator.tsapps/playground/src/modules/lexical-eg-walker/useLexicalRoom.test.tsapps/playground/src/routes/index.tsxpackages/awareness/src/adapters/websocket/websocket.test.tspackages/awareness/src/adapters/websocket/websocket.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/playground/src/modules/lexical-eg-walker/LexicalEgWalkerDemo.tsx
Hide the sync-paused badge during initial connecting, clear presence connection timeouts on close, and rename/split the WebSocket broadcast channel helpers into camelCase modules under 200 lines.
Coverage Report for packages/awareness
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/playground/src/modules/collab-transport/websocketBroadcastChannelLifecycle.ts (1)
78-94: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the previous socket when
connectruns again.
connectis part of the publicWebSocketLifecycleinterface. It replacessocketwithout closing the previous socket. The current caller inwebsocketBroadcastChannel.tscallsconnect()once, and the reconnect timer only fires after acloseevent, so no leak occurs today. If a future caller callsconnect()while a socket is open, the old socket stays open and its listeners return early because of thesocket !== currentSocketguard, so the leak is silent.♻️ Proposed guard
const connect = (): void => { if (options.isClosed()) return; clearReconnect(); + const previous = socket; + socket = null; + previous?.close(); if (options.getConnectionState() !== "reconnecting") { options.setConnectionState("connecting"); }🤖 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/playground/src/modules/collab-transport/websocketBroadcastChannelLifecycle.ts` around lines 78 - 94, Update the public connect function to close the existing socket before replacing it with a newly created WebSocket. Preserve the current socket state and reconnect behavior, and ensure the previous socket is closed only when one is already assigned.
🤖 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/playground/src/modules/collab-transport/websocketBroadcastChannelLifecycle.ts`:
- Around line 96-127: Add a connection-timeout timer in connect for sockets that
remain in the connecting state, using the existing connectionTimeoutMs
configuration. Clear the timer when the socket opens or closes; on expiry,
verify the socket is still current and active, close it, and invoke
scheduleReconnect() so queued messages and connection state recover.
---
Nitpick comments:
In
`@apps/playground/src/modules/collab-transport/websocketBroadcastChannelLifecycle.ts`:
- Around line 78-94: Update the public connect function to close the existing
socket before replacing it with a newly created WebSocket. Preserve the current
socket state and reconnect behavior, and ensure the previous socket is closed
only when one is already assigned.
🪄 Autofix
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 Plus
Run ID: 254cd607-6c50-4854-ac7f-ecee93317026
📒 Files selected for processing (8)
apps/playground/src/modules/collab-transport/urls.test.tsapps/playground/src/modules/collab-transport/websocketBroadcastChannel.tsapps/playground/src/modules/collab-transport/websocketBroadcastChannelLifecycle.tsapps/playground/src/modules/lexical-eg-walker/StatusRail.tsxapps/playground/src/modules/lexical-eg-walker/persistence/channel.test.tsapps/playground/src/modules/lexical-eg-walker/transport.tspackages/awareness/src/adapters/message-edge-cases.test.tspackages/awareness/src/adapters/websocket/websocket.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/awareness/src/adapters/websocket/websocket.ts
- apps/playground/src/modules/collab-transport/urls.test.ts
- apps/playground/src/modules/lexical-eg-walker/persistence/channel.test.ts
- apps/playground/src/modules/lexical-eg-walker/StatusRail.tsx
Arm a connectionTimeoutMs timer while sockets stay connecting, clear it on open/close, and close any existing socket before connect() replaces it.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/playground/src/modules/collab-transport/urls.test.ts`:
- Around line 316-320: Update the lifecycle.connect test around webSocketFactory
and sockets to record the factory invocation and existing socket close calls,
then assert the first socket’s close occurs before the second webSocketFactory
invocation. Preserve the existing socket-count and replacement assertions.
🪄 Autofix
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 Plus
Run ID: d579cd8b-c8ce-4dae-b909-d4e40db5912c
📒 Files selected for processing (8)
apps/playground/src/modules/collab-transport/urls.test.tsapps/playground/src/modules/collab-transport/websocketBroadcastChannel.tsapps/playground/src/modules/collab-transport/websocketBroadcastChannelLifecycle.tsapps/playground/src/modules/lexical-eg-walker/StatusRail.tsxapps/playground/src/modules/lexical-eg-walker/persistence/channel.test.tsapps/playground/src/modules/lexical-eg-walker/transport.tspackages/awareness/src/adapters/message-edge-cases.test.tspackages/awareness/src/adapters/websocket/websocket.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/playground/src/modules/lexical-eg-walker/persistence/channel.test.ts
- packages/awareness/src/adapters/websocket/websocket.ts
- apps/playground/src/modules/lexical-eg-walker/StatusRail.tsx
- apps/playground/src/modules/collab-transport/websocketBroadcastChannelLifecycle.ts
- apps/playground/src/modules/lexical-eg-walker/transport.ts
- packages/awareness/src/adapters/message-edge-cases.test.ts
- apps/playground/src/modules/collab-transport/websocketBroadcastChannel.ts
Record lifecycle connect ordering so the first socket close is verified to happen before the second webSocketFactory invocation.
This was referenced Aug 6, 2026
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.
Summary
Adds real WebSocket transport for Lexical + EG-walker collaboration (document batches + presence), with a playground demo you can verify across browsers.
What changed
/api/collab-doc,/api/presence,/api/collab-syncreconnecting/error; StatusRail shows offline UIconnectionTimeoutMsfor hung connecting sockets; close prior socket beforeconnect()replaces it; camelCase modules under 200 linesHow to verify
pnpm --filter @softmaple/playground typecheck pnpm --filter @softmaple/playground test pnpm --filter @softmaple/awareness typecheck pnpm --filter @softmaple/awareness test:coverageSummary by CodeRabbit