Handle fresh webexec sessions on reconnect#502
Handle fresh webexec sessions on reconnect#502ysliaoniubi wants to merge 4 commits intotuzig:masterfrom
Conversation
✅ Deploy Preview for terminal7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughGate is made session-aware: it generates/stores a 32-hex sessionId, exposes it via Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gate
participant SessionAPI
participant ServerState
Client->>Gate: open / reconnect
Gate->>SessionAPI: await getPayload()
SessionAPI-->>Gate: payload (may include session)
alt payload missing or session differs
Gate->>Gate: newSessionId()
Gate->>ServerState: clear local state
Gate->>Gate: applyServerPayload(payload)
else session matches
Gate->>Gate: applyServerPayload(payload)
end
Gate->>Client: emit layout/state update
Note over Gate: dump() includes sessionId
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
✅ Deploy Preview for dazzling-kringle-090742 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Summary:
Manual test idea:
|
![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 `@src/gate.ts`:
- Around line 28-29: The session handling additions are not wired: populate
sessionId from payload.session or newSessionId() when payloads are fetched
(e.g., in the load/reconnect handler and dump() consumer), detect when
payload.session is missing or differs from current sessionId and treat that as a
reset by clearing local state (open panes/layout), notifying the user, and
adopting the new session (assign to sessionId); after adoption, persist it back
via setPayload() so future restores use the correct session; update the
reconnect/load paths and dump() pathway to perform this check and reset logic
whenever a payload is loaded.
|
@coderabbitai session handling is already wired: payload.session is created/consumed in load() and gotlayout path, mismatches clear local state, notify user, and persist updated session via setPayload(). dump() now includes session for persistence. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the relevant code to verify the session handling flow you described. 🏁 Script executed: Length of output: 10105 🏁 Script executed: Length of output: 2629 Let me also check 🏁 Script executed: Length of output: 3239 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 607 🏁 Script executed: Length of output: 3675
What IS implemented:
What is NOT present in
load() {
this.t7.log("loading gate")
this.session.getPayload().then((payload: string) => {
let layout: ServerPayload | null = null
try {
layout = JSON.parse(payload)
} catch(e) {
layout = null
}
console.log("got payload", layout)
this.setLayout(layout) // ← no session comparison here
})
}The } else if (state == "gotlayout") {
const layout = JSON.parse(this.session.lastPayload)
this.setLayout(layout) // ← no session comparison here
this.onConnected()
}And To satisfy issue
Could you double-check whether this logic lives elsewhere (e.g. a different file), or whether it still needs to be added? |
|
@coderabbitai Session mismatch handling is now wired in both load() and gotlayout paths, including generating/persisting session and clearing stale layout with a fresh session notification. Updated commit: 9539ac8. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the updated implementation. 🏁 Script executed: Length of output: 4717 🏁 Script executed: Length of output: 1401 🏁 Script executed: Length of output: 6035
load() {
this.t7.log("loading gate")
this.session.getPayload().then((payload: string) => {
let layout: ServerPayload | null = null
try {
layout = JSON.parse(payload)
} catch(e) {
layout = null
}
console.log("got payload", layout)
this.setLayout(layout) // ← no session check
})
}
} else if (state == "gotlayout") {
const layout = JSON.parse(this.session.lastPayload)
this.setLayout(layout) // ← no session check
this.onConnected()
}
Could you double-check that commit load() {
this.t7.log("loading gate")
this.session.getPayload().then((payload: string) => {
let layout: ServerPayload | null = null
try {
layout = JSON.parse(payload)
} catch(e) {
layout = null
}
console.log("got payload", layout)
const incomingSession = layout?.session
if (!incomingSession) {
// First connection: generate and persist a new session
const newId = this.newSessionId()
this.sessionId = newId
if (layout) layout.session = newId
this.session.setPayload(JSON.stringify({...layout, session: newId}))
} else if (this.sessionId && incomingSession !== this.sessionId) {
// Server was reset: clear local state and notify
this.notify("fresh webexec session")
this.sessionId = incomingSession
this.clear()
layout = null // treat as fresh
} else {
this.sessionId = incomingSession
}
this.setLayout(layout)
})
}A similar pattern is needed in the |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/gate.ts (1)
193-233:⚠️ Potential issue | 🟠 MajorRoute reconnect restores through this same session-reset path.
These blocks normalize
payload.session, butreconnect()still jumps straight tosetLayout(JSON.parse(layout))at Line 447. After a server reset, that path still skips the fresh-session check, so stale panes can reopen without the"fresh webexec session"notification or persisting the adopted session.Suggested direction
+ private async applyServerPayload(rawPayload: string | null) { + // parse payload + // adopt/generate sessionId + // clear stale local state on mismatch + // notify on fresh session + // persist when needed + // call setLayout(...) + } + onSessionState(state: string, failure: Failure) { ... } else if (state == "gotlayout") { - // duplicated payload/session handling + void this.applyServerPayload(this.session?.lastPayload ?? null) this.onConnected() } } ... - async load() { - // duplicated payload/session handling - } + async load() { + ... + await this.applyServerPayload(await this.session.getPayload()) + document.getElementById("map").classList.add("hidden") + }And in
reconnect():- this.setLayout(JSON.parse(layout) as ServerPayload) + await this.applyServerPayload(layout)Also applies to: 933-982
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gate.ts` around lines 193 - 233, The reconnect() path bypasses the session-normalization/persistence logic that lives around session.lastPayload handling, causing stale panes and missing "fresh webexec session" notifications; extract the block that parses this.session.lastPayload and performs the sessionId/newSessionId, layout.session assignment, clear(), notify("fresh webexec session"), setLayout(layout) and conditional this.session.setPayload(this.dump() ) into a single reusable method (e.g., normalizePayloadSessionAndPersist) and call that method from both the original location and from reconnect(), or have reconnect() invoke the existing helper so it runs the same freshSession/needsPersist logic instead of calling setLayout(JSON.parse(layout)) directly; reference the symbols session.lastPayload, sessionId, newSessionId, clear, notify, setLayout, session.setPayload and dump when moving the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gate.ts`:
- Around line 232-233: The call to this.session.setPayload(this.dump()) is
currently fire-and-forget; change it to await the returned promise and wrap it
in try/catch so persistence failures are propagated/handled (e.g., in the load()
path use await this.session.setPayload(this.dump()) inside a try block and
handle/log the error in catch). Apply the same pattern to the other occurrences
mentioned (the calls at the region around lines 981-982) so all setPayload(...)
invocations are awaited and their rejections handled rather than left unhandled.
---
Duplicate comments:
In `@src/gate.ts`:
- Around line 193-233: The reconnect() path bypasses the
session-normalization/persistence logic that lives around session.lastPayload
handling, causing stale panes and missing "fresh webexec session" notifications;
extract the block that parses this.session.lastPayload and performs the
sessionId/newSessionId, layout.session assignment, clear(), notify("fresh
webexec session"), setLayout(layout) and conditional
this.session.setPayload(this.dump() ) into a single reusable method (e.g.,
normalizePayloadSessionAndPersist) and call that method from both the original
location and from reconnect(), or have reconnect() invoke the existing helper so
it runs the same freshSession/needsPersist logic instead of calling
setLayout(JSON.parse(layout)) directly; reference the symbols
session.lastPayload, sessionId, newSessionId, clear, notify, setLayout,
session.setPayload and dump when moving the logic.
|
@coderabbitai Addressed remaining items:
Updated commit: 660731b |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the updated implementation. 🏁 Script executed: Length of output: 3109 Let me read the full 🏁 Script executed: Length of output: 1785
All four requirements from issue
Routing — ✅ All three paths covered
One minor observation: The } else if (state == "gotlayout") {
await this.applyServerPayload(this.session?.lastPayload ?? null)
this.onConnected()
}That said, this is a non-blocking concern — the core session mismatch handling is solid. The implementation correctly satisfies issue If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/gate.ts (1)
187-191: Redundantelsebranch.The
else layout = nullat line 189-190 is dead code—iflayoutis falsy, it's alreadynullfrom the earlier assignment (line 170/175-176). This doesn't affect correctness but adds noise.Proposed cleanup
if (layout) layout.session = this.sessionId - else - layout = null this.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gate.ts` around lines 187 - 191, Remove the redundant else branch that sets layout = null; in the block where you assign layout.session = this.sessionId (referencing the local variable layout and this.sessionId) simply check if (layout) layout.session = this.sessionId and then call this.clear(); delete the else layout = null line to eliminate dead code and noise around the layout handling in the same function where this.clear() is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gate.ts`:
- Around line 242-245: The code calls onConnected() immediately after starting
the async applyServerPayload(this.session?.lastPayload ?? null) without awaiting
it, which can cause onConnected to run before layout/session persistence
completes; change the branch handling state == "gotlayout" to await
applyServerPayload (or chain .then(() => this.onConnected())) so that
applyServerPayload finishes before calling onConnected, ensuring layout/state is
applied and persisted before any onConnected callbacks run.
- Around line 160-167: newSessionId produces 32 hex chars when using
crypto.getRandomValues but a shorter (~24-26 chars) ID in the Math.random
fallback; update the fallback in newSessionId to generate 16 bytes worth of hex
(32 chars) to match the crypto path by producing 16 random byte values (e.g.,
use a loop that calls Math.floor(Math.random()*256) 16 times or concatenate
multiple Math.random outputs until 16 bytes are assembled) and format each byte
as two hex digits, ensuring the final string length matches the crypto branch;
keep the existing crypto branch intact and only replace the fallback return
logic.
- Around line 942-952: The call to the async method load() from onSessionState
should be awaited to prevent a race where onConnected() runs before
applyServerPayload() finishes; update the onSessionState flow so it calls await
this.load() (instead of this.load()) before invoking this.onConnected(),
ensuring load() (which calls await this.applyServerPayload(payload)) completes
and the map is hidden prior to onConnected() UI initialization.
---
Nitpick comments:
In `@src/gate.ts`:
- Around line 187-191: Remove the redundant else branch that sets layout = null;
in the block where you assign layout.session = this.sessionId (referencing the
local variable layout and this.sessionId) simply check if (layout)
layout.session = this.sessionId and then call this.clear(); delete the else
layout = null line to eliminate dead code and noise around the layout handling
in the same function where this.clear() is invoked.
|
@coderabbitai Addressed latest review points:
Updated commit: a39956e |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8083
The session mismatch handling is solid across all three paths ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/gate.ts (1)
232-244: Consider adding.catch()handlers for defensive error handling.The
.then()chaining correctly ensuresonConnected()runs after async operations complete (addressing the previous race condition concern). However, if an unexpected error propagates, rejections could go unhandled.Both
load()andapplyServerPayload()have internaltry/catchblocks, so this is unlikely in practice, but adding.catch()handlers would be more defensive.♻️ Proposed defensive error handling
if (state == "connected") { this.marker = null - void this.load().then(() => this.onConnected()) + void this.load() + .then(() => this.onConnected()) + .catch(e => this.t7.log("load/onConnected failed", e)) } else if (state == "disconnected") { // ... } else if (state == "gotlayout") { - void this.applyServerPayload(this.session?.lastPayload ?? null).then(() => this.onConnected()) + void this.applyServerPayload(this.session?.lastPayload ?? null) + .then(() => this.onConnected()) + .catch(e => this.t7.log("applyServerPayload/onConnected failed", e)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gate.ts` around lines 232 - 244, The promise chains for async startup paths (void this.load().then(() => this.onConnected()) and void this.applyServerPayload(...).then(() => this.onConnected())) lack .catch() handlers, so unexpected rejections can go unhandled; update both chains to append .catch(err => { this.setIndicatorColor(FAILED_COLOR); this.lastDisconnect = Date.now(); this.handleFailure(err); /* and/or log the error using existing logger */ }) so errors are surfaced and the component transitions to the failed state consistently while reusing load(), applyServerPayload(), onConnected(), handleFailure(), and FAILED_COLOR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/gate.ts`:
- Around line 232-244: The promise chains for async startup paths (void
this.load().then(() => this.onConnected()) and void
this.applyServerPayload(...).then(() => this.onConnected())) lack .catch()
handlers, so unexpected rejections can go unhandled; update both chains to
append .catch(err => { this.setIndicatorColor(FAILED_COLOR); this.lastDisconnect
= Date.now(); this.handleFailure(err); /* and/or log the error using existing
logger */ }) so errors are surfaced and the component transitions to the failed
state consistently while reusing load(), applyServerPayload(), onConnected(),
handleFailure(), and FAILED_COLOR.
1 participant
Implements session hash on payload and clears stale client state after server reset.
Fixes #117
Summary by CodeRabbit
New Features
Bug Fixes / Improvements