fix(gateway): time-gate SSE reconnect history reload (#2404) - #2415
Conversation
…witch flicker (#2404) Every SSE reconnection unconditionally called loadHistory(), which clears the entire chat DOM and re-renders all messages — losing scroll position and causing visible flicker on every browser tab switch. Now tracks when the SSE connection was lost and only reloads history if disconnected for more than 10 seconds. Brief reconnects (tab visibility change, transient network blip) preserve the existing DOM and rely on the "Done without response" safety net for missed events. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request optimizes the SSE reconnection process by only reloading chat history if the disconnection exceeds 10 seconds, reducing UI disruption for brief interruptions. The review feedback focuses on improving code maintainability by suggesting the use of a named constant for the 10-second threshold and consolidating the new _sseDisconnectedAt variable with the existing _connectionLostAt state to avoid redundancy and ensure consistent state management.
| let _connectionLostAt = null; | ||
| let _reconnectAttempts = 0; | ||
| let _lastSseEventId = null; | ||
| let _sseDisconnectedAt = null; |
There was a problem hiding this comment.
The introduction of _sseDisconnectedAt creates redundancy with the existing _connectionLostAt variable (line 224), which is also used to track disconnection timestamps in onerror.
Additionally, the 10-second threshold for history reload is a magic number that should be defined as a constant for better maintainability and consistency with other timeouts in this file (e.g., DONE_WITHOUT_RESPONSE_TIMEOUT_MS).
| let _sseDisconnectedAt = null; | |
| const SSE_RECONNECT_RELOAD_THRESHOLD_MS = 10000; | |
| let _connectionLostAt = null; |
| // the 3rd tab exhausts the browser's per-origin limit. | ||
| document.addEventListener('visibilitychange', () => { | ||
| if (document.hidden) { | ||
| _sseDisconnectedAt = _sseDisconnectedAt || Date.now(); |
There was a problem hiding this comment.
Consider reusing the existing _connectionLostAt variable instead of introducing _sseDisconnectedAt. Reusing the existing state tracker would simplify the logic and avoid having two variables tracking the same event (disconnection start).
| _sseDisconnectedAt = _sseDisconnectedAt || Date.now(); | |
| _connectionLostAt = _connectionLostAt || Date.now(); |
| const disconnectMs = _sseDisconnectedAt ? Date.now() - _sseDisconnectedAt : 0; | ||
| if (disconnectMs > 10000) { | ||
| loadHistory(); | ||
| } |
There was a problem hiding this comment.
Use the suggested constant SSE_RECONNECT_RELOAD_THRESHOLD_MS here to avoid magic numbers. Also, if you consolidate the variables as suggested, use _connectionLostAt here. Calculating the duration dynamically using Date.now() for the active disconnection period is consistent with the repository's approach to tracking durations of ongoing processes.
| const disconnectMs = _sseDisconnectedAt ? Date.now() - _sseDisconnectedAt : 0; | |
| if (disconnectMs > 10000) { | |
| loadHistory(); | |
| } | |
| const disconnectMs = _connectionLostAt ? Date.now() - _connectionLostAt : 0; | |
| if (disconnectMs > SSE_RECONNECT_RELOAD_THRESHOLD_MS) { | |
| loadHistory(); | |
| } |
References
- When calculating durations for ongoing tasks or states, calculate the duration dynamically using Date.now() for the part of the task that is still running.
| loadHistory(); | ||
| } | ||
| } | ||
| _sseDisconnectedAt = null; |
There was a problem hiding this comment.
If consolidating variables, ensure _connectionLostAt is reset here. Note that the existing reset at line 739 is wrapped in an if (lostBanner) block, which means it isn't always cleared on successful reconnection (e.g., for short disconnects where the banner timer never fired). Resetting it here ensures the state is always clean for the next disconnection.
| _sseDisconnectedAt = null; | |
| _connectionLostAt = null; |
| }; | ||
|
|
||
| eventSource.onerror = () => { | ||
| _sseDisconnectedAt = _sseDisconnectedAt || Date.now(); |
henrypark133
left a comment
There was a problem hiding this comment.
Review: SSE reconnect history reload time-gate (Risk: Low)
Clean fix. Tracks _sseDisconnectedAt to only reload full chat history when disconnected >10s. Brief reconnects (tab visibility change, transient blip) rely on SSE catch-up instead of full re-render — preserves scroll position and avoids disrupting the user.
Positives:
- E2E tests updated to simulate the >10s disconnect condition
- Both
visibilitychangeandonerrorpaths set the disconnect timestamp correctly - Reconnect banner logic simplified
LGTM.
henrypark133
left a comment
There was a problem hiding this comment.
No verified findings in the current head.
The reconnect path now only forces a full loadHistory() after a real disconnect window (>10s), which avoids the scroll-destructive reload on brief tab-hide and transient reconnects while still preserving the long-gap recovery path covered by the updated SSE tests.
fba8b59 to
a00af6e
Compare
henrypark133
left a comment
There was a problem hiding this comment.
Review: SSE reconnect history reload time-gate
No verified findings in the current diff.
The reconnect path now only forces a full loadHistory() after a real disconnect window, which preserves scroll position on brief tab-hide or transient reconnects while keeping the long-gap recovery path covered by the updated SSE test.
…itch regression test Address review feedback on #2415: - Remove the duplicate `_connectionLostAt` variable; `_sseDisconnectedAt` now tracks both tab-visibility and onerror disconnects, giving `onopen` a single source of truth and a single `loadHistory()` decision point. - Hoist the 10s threshold to a named `SSE_RELOAD_THRESHOLD_MS` constant. - Add `test_tab_switch_does_not_reload_chat_history` that drives the real `visibilitychange` handler and asserts no `/api/chat/history` call fires and the chat DOM survives — prevents silent reintroduction of #2404. - Pre-seed `_sseDisconnectedAt` in the server-restart test so it reliably exercises the history-reload path on hardware where the restart cycle completes inside the threshold window. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…h-chat-refresh # Conflicts: # crates/ironclaw_gateway/static/app.js
The new-thread helper only waited for `currentThreadId !== assistantThreadId`. When a prior test left the server's `active_thread` pointing at a user-created thread, the initial page load sets `currentThreadId` to that stale thread and the wait passes immediately — before `createNewThread()` resolves. The test then reads messages and sends a chat against the wrong thread, the new thread activates mid-flight and clears the DOM, and the `assistants.length > before` condition never fires. Factor the wait into `_create_new_user_thread`, capture the previous thread id, and require `currentThreadId` to change to a *new* non-assistant value. Apply to both refresh tests that had the pattern. Verified stable over 3 consecutive full-suite runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
A number of flaky tests in Playwright - fixing them here as well, then approving. |
test_refresh_skips_readonly_external_active_thread registers a
`page.route("**/api/chat/threads", ...)` handler that fetches + rewrites
the response. The frontend polls this endpoint periodically (especially
on SSE reconnect), so a request is often in flight inside
`patch_threads_response` when the `page` fixture closes the context.
The cancelled `route.fetch()` surfaces on the *next* test's
`Browser.new_context()` call as a TargetClosedError, flaking
test_multiple_tabs_receive_same_response under full-suite load.
Call `page.unroute_all(behavior="ignoreErrors")` at the end of the test
so in-flight route callbacks are drained before teardown. Verified
stable across 5 consecutive full-suite runs (was ~60% flake rate).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tie the setup and teardown comments together so a future reader adding a similar route intercept understands the full context — why the fetch can be mid-flight at teardown, how the cancellation surfaces on an unrelated later test, and that pairing with unroute_all is mandatory. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…earai#2415) * fix(gateway): time-gate SSE reconnect history reload to prevent tab-switch flicker (nearai#2404) Every SSE reconnection unconditionally called loadHistory(), which clears the entire chat DOM and re-renders all messages — losing scroll position and causing visible flicker on every browser tab switch. Now tracks when the SSE connection was lost and only reloads history if disconnected for more than 10 seconds. Brief reconnects (tab visibility change, transient network blip) preserve the existing DOM and rely on the "Done without response" safety net for missed events. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review findings (iteration 1) Set _sseDisconnectedAt before server restart in E2E test to prevent flaky timeout when the restart completes in <10s. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
loadHistory(), which clears the entire chat DOM and re-renders all messages — losing scroll position and causing visible flicker_sseDisconnectedAt) and only reloads history if disconnected for >10 secondsChanges
crates/ironclaw_gateway/static/app.js: Add_sseDisconnectedAttracking, set it invisibilitychangeandonerrorhandlers, replace unconditionalloadHistory()inonopenwith 10s time-gatetests/e2e/scenarios/test_sse_reconnect.py: Set simulated long-disconnect timestamp before reconnect in the stale-event-ID test so it still exercises the history reload pathTest plan
pytest scenarios/test_sse_reconnect.py— all existing reconnect tests pass🤖 Generated with Claude Code