fix(ui): stop a tab that the device reports taken over on the signaling socket - #1661
Merged
Merged
Conversation
…ng socket A device can report a takeover on the signaling WebSocket, with an "other-session-connected" message or close code 4001, instead of the otherSessionConnected RPC event. The hook reconnected after every close, so the old tab took the session back and two tabs traded it forever. Mark the session superseded on either signal: close the peer, open /other-session, and keep the socket closed until Use Here, which reopens it for fresh metadata and a new peer. The RPC path is unchanged.
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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
1 participant
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.
Problem
The device allows one WebRTC session, so when a second browser tab connects, the first tab (the old tab) must stop and show "Another session is active" with a Use Here button. The JetKVM reports this takeover with the
otherSessionConnectedJSON-RPC event on the old session's RPC data channel. The JetKVM Mini, a smaller JetKVM with its own firmware that serves this UI, sends{"type":"other-session-connected"}on the signaling WebSocket instead, which also reaches a tab that has no RPC channel yet, and then closes the socket with code 4001. On a JetKVM Mini the old tab does not stop, and the two tabs take the session from each other without end.Cause
In
useWebSocketSignaling.ts, the signaling message handler ignoresother-session-connected, andshouldReconnectreturns true for every close, including code 4001. The old tab therefore reconnects and takes the session back, and the same then happens to the new tab:sequenceDiagram participant Old as Old tab participant Mini as JetKVM Mini participant New as New tab New->>Mini: open signaling socket, send offer Mini->>Old: other-session-connected, close (4001) Note over Old: message ignored, shouldReconnect returns true Old->>Mini: open signaling socket, send new offer Mini->>New: other-session-connected, close (4001) Note over New: message ignored, shouldReconnect returns trueFix
All code changes are in
useWebSocketSignaling.ts. The hook marks the session as superseded on another-session-connectedmessage or on a close with code 4001, which covers a socket that closes before the message is read. The new functionstopForOtherSessionthen closes the peer connection, sets the connection state toclosedand opens/other-session, which shows Use Here.While the session is superseded,
shouldReconnectreturns false, and the hook passes!sessionSupersededas theuseWebSocketargument that decides whether the socket is open, so the library does not reopen it. The flag is kept in a ref forshouldReconnectand in React state for that argument.Use Here calls the hook's
connect, now a wrapper aroundsetupPeerConnection. After a signaling takeover it clears the flag and sets the state tonew, so the socket opens again and the existingdevice-metadatahandler sets up a new peer; otherwise it callssetupPeerConnectionas before. The RPCotherSessionConnectedpath is unchanged, so a device that never sends the message or code 4001, such as the JetKVM today, behaves as before.Testing
The new spec
ui/e2e/session-takeover.spec.tshas one test, "a live session is taken over and handed back". After the second tab connects, it samples both tabs once per second for 30 s: the first tab must show Use Here, open no signaling socket and create noRTCPeerConnection, and the second tab must stay connected without a reconnect and answer an RPC. The first tab then selects Use Here and the roles swap. The old tab has 10 s to lose its connection, because the JetKVM closes the old peer 1 s after the takeover and Chrome took 6 to 7 s to report it closed.devdevThe JetKVM UI was installed with
dev_deploy.sh --install, and the two Mini firmware builds differed only in this file. On the Mini without the change, each tab opened signaling and created a new peer 27 to 28 times per 30 s window, every close had code 4001, the first tab never showed Use Here, and the second tab was disconnected in 13 to 15 of 30 samples. With the change, after one 4001 close the first tab opened no socket, created no peer and showed Use Here in all 30 samples, in both directions.tsc(app and e2e),oxlintandoxfmtpass.Not in this PR
On the JetKVM, a tab that is taken over before its RPC channel opens still never shows Use Here, because the JetKVM sends only the RPC event. The spec does not test that case, and a fix needs a device change.