Restore pane and zoom state on reconnect#57
Conversation
📝 WalkthroughWalkthroughThe PR implements client-side reconnection with persistent session state restoration. It adds per-client tracking of base sessions, active panes, and zoom states via a new reconnect state mechanism. The frontend persists clientIds in localStorage and sends them during authentication. When clients reconnect, the server restores their previous pane and zoom context. Changes span backend session management, frontend storage, protocol types, and test infrastructure. Changes
Sequence DiagramsequenceDiagram
participant Client as Mobile Client
participant Auth as Auth System
participant Server as Server
participant Storage as localStorage
participant Tmux as TMUX
autonumber
rect rgba(100, 150, 200, 0.5)
Note over Client,Tmux: Initial Connection
Client->>Storage: Read persisted clientId (if exists)
Client->>Server: Connect control WebSocket
Client->>Auth: Send auth with clientId
Auth->>Server: Authenticate & normalize clientId
Server->>Tmux: Attach to base session
Server->>Server: Store baseSession in reconnectState[clientId]
Server->>Client: auth_ok with assigned clientId
Client->>Storage: Persist clientId to localStorage
end
rect rgba(100, 200, 150, 0.5)
Note over Client,Tmux: User Actions (Record State)
Client->>Server: select_pane message
Server->>Server: Remember pane in reconnectState[clientId]
Server->>Tmux: Execute pane select
Client->>Server: zoom_pane message
Server->>Server: Update zoomed state in reconnectState[clientId]
Server->>Tmux: Toggle window zoom
end
rect rgba(200, 150, 100, 0.5)
Note over Client,Tmux: Reconnection Flow
Client->>Storage: Read persisted clientId
Client->>Server: Reconnect control WebSocket
Client->>Auth: Send auth with same clientId
Auth->>Server: Reuse clientId, close previous context
Server->>Server: Lookup reconnectState[clientId]
Server->>Tmux: Restore to remembered baseSession
Server->>Server: Attempt restore pane & zoom from state
Server->>Tmux: Attach to remembered pane (if exists)
Server->>Tmux: Apply remembered zoom state
Server->>Client: auth_ok + restored state
Client->>Client: Reflect restored pane/zoom in UI
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
UI Screenshots
Screenshots are available as a build artifact. Files captured: drawer-open.png, main-view.png Download the |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/server.ts (1)
501-506:⚠️ Potential issue | 🟡 MinorPotential race: fire-and-forget shutdown can kill a session the new context just attached to.
On close,
shutdownControlContextis called withoutawait(line 504). This means the mobile session kill runs asynchronously. If a client reconnects before the old shutdown completes, the new context won't find the old one incontrolClients(already deleted on line 503) and may reuse the still-alive mobile session viahasMobileSessioninattachControlToBaseSession. The old fire-and-forgetshutdownControlContextcould then kill that session from under the new context.The window is very narrow (requires reconnection within milliseconds), but it could cause a session-destroyed error on the new connection. Awaiting the shutdown or tracking in-flight shutdowns would close it.
🧹 Nitpick comments (5)
src/backend/server.ts (2)
36-41:reconnectStateByClientIdgrows unboundedly — consider adding eviction.The
reconnectStateByClientIdmap is never pruned. Each uniqueclientIdcreates a permanent entry. While individual entries are small and the server likely serves few concurrent clients, long-running instances could accumulate stale entries. TheupdatedAtfield is already present — a periodic sweep or LRU eviction based on it would prevent unbounded growth.Also applies to: 136-136
361-367: Optimistic zoom toggle may briefly disagree with actual tmux state.The new
zoomedvalue is computed by inverting the currently tracked state rather than querying tmux after the command. If the tracked state is stale (e.g., first interaction before any snapshot), the saved value will be wrong until the nextforcePublish(line 490) corrects it viaupdateReconnectStateFromSnapshot. This is a narrow race, but if a disconnect occurs in that window, the restore will apply the wrong zoom state.Consider querying the actual window zoom state after
zoomPanefor accuracy, or accept this as a known limitation of the best-effort approach.tests/backend/parser.test.ts (1)
14-15: Consider adding a test case forzoomed: true.The test only covers
zoomed: false(input0). Adding a second window line withzoomed: true(input1) would validate both branches of thezoomed === "1"check and guard against regressions.🧪 Proposed addition
- const windows = parseWindows("0\tbash\t1\t0\t2"); - expect(windows[0]).toEqual({ index: 0, name: "bash", active: true, zoomed: false, paneCount: 2 }); + const windows = parseWindows("0\tbash\t1\t0\t2\n1\tvim\t0\t1\t1"); + expect(windows[0]).toEqual({ index: 0, name: "bash", active: true, zoomed: false, paneCount: 2 }); + expect(windows[1]).toEqual({ index: 1, name: "vim", active: false, zoomed: true, paneCount: 1 });tests/integration/server.test.ts (2)
279-281: Fixed-delaysetTimeoutsynchronization is fragile under CI load.Both tests use
await new Promise(r => setTimeout(r, 40))to wait forselect_pane/zoom_paneto be processed and for the socket-close to propagate. On a loaded CI runner this window can be insufficient, making the tests intermittently flaky.Consider awaiting a server-sent acknowledgement for
select_pane/zoom_panecommands, or at minimum awaiting thecloseevent oncontrolFirstinstead of a fixed delay for the disconnect propagation:♻️ Example: event-driven close wait
- controlFirst.close(); - await new Promise((resolve) => setTimeout(resolve, 40)); + await new Promise<void>((resolve) => { + controlFirst.once("close", resolve); + controlFirst.close(); + });Also applies to: 312-314
262-331: Duplicated test setup across both new tests.Lines 263–314 of test 1 and lines 296–314 of test 2 share an identical 20-line block (start server, connect, auth, snapshot, find pane, send select_pane + zoom_pane, disconnect). Extracting a shared helper would reduce duplication and make both tests easier to maintain.
♻️ Sketch of a shared helper
const setupAndDisconnect = async (): Promise<{ firstClientId: string; paneId: string }> => { const controlFirst = await openSocket(`${baseWsUrl}/ws/control`); const firstAuth = await authControl(controlFirst); const snapshot = await buildSnapshot(tmux); const session = snapshot.sessions.find((s) => s.name === firstAuth.attachedSession); const paneId = session?.windowStates[0]?.panes[0]?.id; expect(paneId).toBeDefined(); controlFirst.send(JSON.stringify({ type: "select_pane", paneId })); controlFirst.send(JSON.stringify({ type: "zoom_pane", paneId })); await new Promise<void>((resolve) => { controlFirst.once("close", resolve); controlFirst.close(); }); return { firstClientId: firstAuth.clientId, paneId: paneId as string }; };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/backend/server.tssrc/backend/tmux/cli-executor.tssrc/backend/tmux/parser.tssrc/backend/types/protocol.tssrc/frontend/App.tsxsrc/frontend/types/protocol.tstests/backend/parser.test.tstests/harness/fakeTmux.tstests/integration/server.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/types/protocol.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep protocol types in sync between backend (
src/backend/types/protocol.ts) and frontend (src/frontend/types/protocol.ts)
Files:
src/frontend/types/protocol.tssrc/backend/types/protocol.ts
**/server.ts
📄 CodeRabbit inference engine (AGENTS.md)
Preserve independent auth gating on both
/ws/controland/ws/terminalWebSocket endpoints
Files:
src/backend/server.ts
**/tmux/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use
execFilewith argument arrays in tmux command execution to prevent shell injection attacks
Files:
src/backend/tmux/cli-executor.tssrc/backend/tmux/parser.ts
🧠 Learnings (3)
📚 Learning: 2026-02-14T22:15:23.148Z
Learnt from: CR
Repo: DagsHub/tmux-mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-14T22:15:23.148Z
Learning: Applies to **/server.ts : Preserve independent auth gating on both `/ws/control` and `/ws/terminal` WebSocket endpoints
Applied to files:
src/backend/server.tstests/integration/server.test.tssrc/frontend/App.tsx
📚 Learning: 2026-02-14T22:15:23.148Z
Learnt from: CR
Repo: DagsHub/tmux-mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-14T22:15:23.148Z
Learning: Applies to **/tmux/**/*.ts : Use `execFile` with argument arrays in tmux command execution to prevent shell injection attacks
Applied to files:
src/backend/tmux/cli-executor.ts
📚 Learning: 2026-02-14T22:15:23.148Z
Learnt from: CR
Repo: DagsHub/tmux-mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-14T22:15:23.148Z
Learning: Applies to **/pty/**/*.ts : Shell-quote session names in PTY spawn paths in node-pty-adapter.ts
Applied to files:
src/backend/tmux/cli-executor.ts
🧬 Code graph analysis (3)
tests/backend/parser.test.ts (1)
src/backend/tmux/parser.ts (1)
parseWindows(23-37)
src/backend/server.ts (4)
src/backend/state/state-monitor.ts (1)
TmuxStateMonitor(5-48)src/backend/types/protocol.ts (1)
TmuxStateSnapshot(43-46)src/frontend/types/protocol.ts (1)
TmuxStateSnapshot(29-32)src/backend/util/random.ts (1)
randomToken(3-4)
tests/integration/server.test.ts (2)
tests/harness/ws.ts (2)
openSocket(34-38)waitForMessage(9-32)src/backend/tmux/types.ts (1)
buildSnapshot(27-47)
🔇 Additional comments (8)
src/backend/types/protocol.ts (1)
34-34: LGTM!The
zoomed: booleanaddition toTmuxWindowStateis clean. The field is correctly positioned and aligns with the frontend counterpart insrc/frontend/types/protocol.ts(line 20).src/frontend/types/protocol.ts (1)
20-20: LGTM!The
zoomed: booleanfield is in sync with the backendTmuxWindowStatedefinition. As per coding guidelines, "Keep protocol types in sync between backend (src/backend/types/protocol.ts) and frontend (src/frontend/types/protocol.ts)".src/backend/tmux/cli-executor.ts (1)
10-11: LGTM!The
#{window_zoomed_flag}is correctly inserted between#{window_active}and#{window_panes}, matching the parser's destructuring order[index, name, active, zoomed, panes]. All tmux commands continue to useexecFilewith argument arrays. Based on learnings: "UseexecFilewith argument arrays in tmux command execution to prevent shell injection attacks".src/frontend/App.tsx (1)
25-25: LGTM!The clientId persistence flow is well-structured:
- Stored in
localStoragefor cross-session persistence, read into a ref at startup.- Sent as
undefined(not empty string) when absent via|| undefinedon line 273.- Updated from the server's
auth_okresponse, correctly deferring to the server as the source of truth for the canonical clientId.- Both control and terminal WebSocket endpoints continue to send independent auth messages.
Also applies to: 50-50, 268-275, 290-291
src/backend/tmux/parser.ts (1)
29-34: LGTM!The destructuring now correctly includes
zoomedat the expected position, matching theWINDOW_FMTcolumn order incli-executor.ts. Thezoomed === "1"conversion is consistent with the existingactive === "1"pattern.tests/harness/fakeTmux.ts (1)
232-236: LGTM!The
zoomPanetoggle correctly mimics tmux'sresize-pane -Zbehavior (toggle), andzoomedis properly initialized tofalseinbuildDefaultSessionandnewWindow, propagated inlistWindowsandcreateGroupedSession.tests/integration/server.test.ts (2)
30-47: LGTM —clientIdpropagation inauthControlhelper is correct.
JSON.stringifyomitsundefinedfields, so existing callers that pass noclientIdare unaffected.
286-288: No race condition exists. InattachControlToBaseSession(server.ts lines 272-273), the restore operations (selectPane,zoomPane) are awaited before sending theattachedmessage. The code flow is:
await tryRestoreClientView(context)completes (all tmux calls recorded)sendJson(context.socket, { type: "attached" })sends- Test receives
attachedand asserts ontmux.callsThe
selectPaneandzoomPanecalls are guaranteed to be intmux.callswhen assertions execute. The assertions are correctly written and do not race.Likely an incorrect or invalid review comment.
| const normalizeClientId = (value: unknown): string | undefined => { | ||
| if (typeof value !== "string") { | ||
| return undefined; | ||
| } | ||
| const trimmed = value.trim(); | ||
| if (!trimmed || trimmed.length > 128) { | ||
| return undefined; | ||
| } | ||
| return trimmed; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "buildMobileSessionName" --type=ts -C2
echo "---"
rg -n "MOBILE_SESSION_PREFIX" --type=ts -C2Repository: DagsHub/tmux-mobile
Length of output: 1426
🏁 Script executed:
rg -n "clientId" --type=ts -C2 | head -80Repository: DagsHub/tmux-mobile
Length of output: 5617
🏁 Script executed:
rg -n "normalizeClientId\|context\.clientId\s*=" --type=ts -C3Repository: DagsHub/tmux-mobile
Length of output: 45
🏁 Script executed:
rg -n "normalizeClientId" --type=ts -C3
echo "---"
rg -n "context\.clientId\s*=" --type=ts -C3Repository: DagsHub/tmux-mobile
Length of output: 1861
🏁 Script executed:
rg -n "randomToken" --type=ts -C5Repository: DagsHub/tmux-mobile
Length of output: 4259
🏁 Script executed:
rg -n "mobileSession\|buildMobileSessionName" --type=ts -C5 | head -100Repository: DagsHub/tmux-mobile
Length of output: 45
🏁 Script executed:
rg -n "const mobileSession\|mobileSession\s*=" --type=ts -C5Repository: DagsHub/tmux-mobile
Length of output: 45
🏁 Script executed:
rg -n "mobileSession" --type=ts -C3Repository: DagsHub/tmux-mobile
Length of output: 1651
🏁 Script executed:
cat -n src/backend/server.ts | sed -n '440,475p'Repository: DagsHub/tmux-mobile
Length of output: 1839
🏁 Script executed:
cat -n src/backend/server.ts | sed -n '515,545p'
echo "---"
rg -n "createGroupedSession\|killSession\|attachToSession" --type=ts -C2Repository: DagsHub/tmux-mobile
Length of output: 1295
🏁 Script executed:
fd "tmux" --type=f --extension=ts | grep -E "(tmux|gateway)" | head -20Repository: DagsHub/tmux-mobile
Length of output: 92
🏁 Script executed:
find . -name "*.ts" -type f | xargs grep -l "createGroupedSession\|killSession" | head -10Repository: DagsHub/tmux-mobile
Length of output: 244
🏁 Script executed:
cat -n src/backend/tmux/types.ts | head -50
echo "---"
rg -n "createGroupedSession" src/backend/tmux/cli-executor.ts -C5Repository: DagsHub/tmux-mobile
Length of output: 2530
🏁 Script executed:
web_search
tmux session name character restrictions limitationsRepository: DagsHub/tmux-mobile
Length of output: 156
🌐 Web query:
tmux session name character restrictions . : separators
💡 Result:
In tmux, : and . are not “just characters” in targets—they’re syntax separators:
- A target is parsed as
session:window.pane(colon separates session/window, period separates window/pane). [1] - The man page describes the same parsing:
target-windowissession:window, andtarget-panemay add.pane. [2]
Practical impact on session-name characters
- Avoid
:in session names (and.in window names) if you want to reliably address them by name, because tmux will treat those characters as structural separators when you use-t. [1][2] - If you need arbitrary characters in names (including separators), the robust workaround is to target by IDs instead of names (session IDs look like
$1, window IDs@1, pane IDs%1). [1][2] - Spaces are allowed in names, but then you must quote the target in your shell (otherwise it splits into multiple arguments). (This is shell parsing, not tmux.) [2]
Recommendation
Use session names like proj_api or proj-api (no :), and rely on session:window.pane only for addressing, not naming. [1][2]
Sources: [1] tmux wiki “Advanced Use / Command targets”; [2] tmux(1) manual (target syntax).
normalizeClientId allows characters that are invalid in tmux session targets.
The clientId is used in buildMobileSessionName to form a tmux session name (tmux-mobile-client-<clientId>), which is then passed to tmux commands via the -t (target) flag. Tmux interprets : and . as syntax separators in targets (session:window.pane), so if a clientId contains these characters, tmux will misparse the target, causing commands to fail or target the wrong session/window/pane.
While the server generates clientIds using base64url (safe characters), clients can provide their own clientId in the auth message. Restrict normalizeClientId to [a-zA-Z0-9_-] to prevent unsafe characters.
| } finally { | ||
| controlFirst.close(); | ||
| controlSecond?.close(); | ||
| } |
There was a problem hiding this comment.
controlFirst.close() is called twice — once inside the try block and again in the finally block.
This applies to both new tests (lines 280 + 290, and lines 313 + 328). controlFirst is unconditionally closed in the try block before reconnect, so the finally block should only guard controlSecond.
🔧 Proposed fix
- } finally {
- controlFirst.close();
- controlSecond?.close();
- }
+ } finally {
+ controlSecond?.close();
+ }Apply the same change to both new tests.
Also applies to: 327-330
| const maybeError = await Promise.race([ | ||
| waitForMessage<{ type: string; message?: string }>(controlSecond, (msg) => msg.type === "error"), | ||
| new Promise<null>((resolve) => setTimeout(() => resolve(null), 80)) | ||
| ]); | ||
| expect(maybeError).toBeNull(); |
There was a problem hiding this comment.
Abandoned waitForMessage promise in Promise.race can produce an unhandled rejection ~3 seconds later.
When the 80ms branch wins the race, waitForMessage's internal 3000ms setTimeout is still live. It will later call reject(new Error("Timed out...")) on a promise that has no .catch() handler, triggering an unhandled rejection. In Vitest, this can fail a subsequently running test or emit an error ~3s after this test completes.
Use waitForMessage's built-in timeoutMs parameter and catch the timeout rejection directly — this eliminates the dangling promise entirely:
🔧 Proposed fix
- const maybeError = await Promise.race([
- waitForMessage<{ type: string; message?: string }>(controlSecond, (msg) => msg.type === "error"),
- new Promise<null>((resolve) => setTimeout(() => resolve(null), 80))
- ]);
+ const maybeError = await waitForMessage<{ type: string; message?: string }>(
+ controlSecond,
+ (msg) => msg.type === "error",
+ 80
+ ).catch(() => null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const maybeError = await Promise.race([ | |
| waitForMessage<{ type: string; message?: string }>(controlSecond, (msg) => msg.type === "error"), | |
| new Promise<null>((resolve) => setTimeout(() => resolve(null), 80)) | |
| ]); | |
| expect(maybeError).toBeNull(); | |
| const maybeError = await waitForMessage<{ type: string; message?: string }>( | |
| controlSecond, | |
| (msg) => msg.type === "error", | |
| 80 | |
| ).catch(() => null); | |
| expect(maybeError).toBeNull(); |
1 participant
Summary
Testing
Closes #56
Implementation Details
The reconnection mechanism is built around a per-client
ReconnectStatestructure that persists three pieces of state:Client Identity and Session Reuse
normalizeClientIdfunction sanitizes incoming clientId values from the authentication messageState Tracking and Restoration
select_paneandzoom_panemutations), storing them inrememberReconnectStatebaseSessionis persisted so subsequent reconnects can default to ittryRestoreClientViewfunction applies stored state after attachment, with careful error handling:selectPanewith the remembered paneIdFrontend Client ID Persistence
auth_okresponseTest Harness and Protocol Updates
zoomPaneoperationszoomed: booleanonTmuxWindowStateat both frontend and backend