fix: preserve WebSocket connections on SPA route changes (#25)#47
fix: preserve WebSocket connections on SPA route changes (#25)#47emavitta wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/background/background.js">
<violation number="1" location="src/background/background.js:291">
P2: Stale-connection regression: connection clearing now depends exclusively on content-script `page-loaded` messages, which never arrive on non-HTTP/HTTPS navigations (chrome://, file://, etc.) since the content script only matches `http://*/*` and `https://*/*`. Unlike the removed `chrome.tabs.onUpdated` listener, there is no fallback cleanup for tabs that navigate to pages outside the content-script match patterns.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Thanks for the review. Valid point — addressed in 535a7f9. After removing the 535a7f9 re-adds a chrome.tabs.onUpdated.addListener((tabId, changeInfo) => {
if (!changeInfo.url) return;
if (/^https?:\/\//i.test(changeInfo.url)) return; // handled by content script
clearTabConnections(tabId, changeInfo.url);
});This covers the gap the content script can't, and it can't reintroduce #25: an SPA route change always stays on http/https, so it never reaches |
@emavitta Looks good — this addresses the gap I flagged. The two-path approach is clean:
The shared Commit 535a7f9 resolves the concern from my previous review. |
…ot#25) The background script detected "real navigation" via a chrome.tabs.onUpdated listener that compared URL pathnames. That heuristic is wrong for SPAs: a client-side route change (history.pushState/replaceState) changes the pathname without reloading the document, so the listener wiped still-alive connections from the panel and stopped showing their subsequent messages. Real document loads are now detected by the content script itself, which is re-injected on every full page load/reload but never on an SPA route change. On load it sends a "page-loaded" message (top frame only) and the background clears that tab's connections in response. No new manifest permissions.
Addresses review feedback (identified by cubic): after removing the chrome.tabs.onUpdated pathname heuristic, cleanup relied solely on the content script's page-loaded message, which never arrives when a tab navigates to a URL the content script cannot run on (chrome://, file://, extension pages), leaving stale connections for that tab. Re-add a chrome.tabs.onUpdated listener scoped strictly to non-http(s) destinations. It cannot fire for SPA route changes (those always stay on http/https), so it does not reintroduce issue law-chain-hot#25. Clearing logic is factored into a shared clearTabConnections() helper.
535a7f9 to
01caba4
Compare
Problem
In a single-page app, changing route makes the connection disappear from the DevTools panel even though the WebSocket is still open — and subsequent messages on that socket are no longer shown. This matches issue #25.
Root cause
background.jsdetected "real navigation" with achrome.tabs.onUpdatedlistener that compared URL pathnames:That heuristic is wrong for SPAs. A client-side route change via
history.pushState/replaceStatechanges the pathname without reloading the document. The injected proxy keeps theProxiedWebSocketalive (correct), but the background treated the pathname change as a full reload, clearedwebsocketData.connectionsfor the tab, and sentpage-refreshto the panel — which calledhandleClearConnections().After that, incoming
messageevents can't restore the connection either: the panel only updatesconnectionsMapentries that already exist, so the wiped connection never reappears.Fix
Detect real document loads from the content script instead of from URL diffing. A content script is re-injected on every full page load/reload but never on an SPA route change — so it's the reliable signal.
content.jssends apage-loadedmessage on injection (top frame only, so an iframe reload can't wipe the parent's connections).background.jsclears that tab's connections + notifies the panel in response topage-loaded, and the oldchrome.tabs.onUpdated/tabUrlslogic is removed.No new manifest permissions. Real reloads still clear correctly; SPA navigations now preserve the live connection and its message stream.
Testing
Built the extension and loaded it unpacked. Verified on a path-routed SPA: navigating between routes keeps the connection and its messages in the panel, while a full page reload still resets state as expected.
Summary by cubic
Preserves WebSocket connections during SPA route changes and keeps the message stream alive. Connections now clear only on real page loads or when navigating to pages where the content script can’t run.
content.js: sendspage-loadedfrom the top frame on injection.background.js, useclearTabConnections()to reset onpage-loadedand on non-http(s) navigations via a scopedchrome.tabs.onUpdatedfallback; notify the panel withpage-refresh.Written for commit 01caba4. Summary will update on new commits.