fix(client): make reconnect work on Server-Sent Events, and say where timeout applies - #2457
Conversation
…re `timeout` applies
Two options were each inert on one transport.
`reconnect` was overridden to `Infinity` over Server-Sent Events, so asking
for a bounded number of attempts on the default transport did nothing. It
is honoured now. Unset still means keep trying for as long as the page is
open: a dev server is expected to come back, and a tab left open across a
restart has to find it again.
`timeout` was handed to a WebSocket client whose constructor is
`constructor(url)` and takes no options at all. The fix there is not to
implement the watchdog, which was my first instinct and is wrong — the two
transports send different kinds of heartbeat:
EventSourceServer client.write("data: 💓\n\n") data, visible to JS
WebSocketServer client.ping(() => {}) a protocol ping
The browser answers a ping in its network stack and never shows it to
JavaScript, so a silence watchdog over a WebSocket would fire on a
connection that is healthy and merely idle. The half-open case it would
have caught is already handled where the pong is visible: the server sees
the missing one and terminates the socket, so the browser gets a real
`close` and reconnects. So `timeout` is Server-Sent Events only, it is
documented as such, and it is no longer passed to a client that drops it.
Both decisions moved into `client-src/utils/socket-options.js`, which is a
pure function and therefore testable — eight cases, one per transport per
option, including `reconnect: 0` meaning none rather than unset.
The README already said `"sse"` ignored `reconnect`; the schema did not.
Both say what happens now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
🦋 Changeset detectedLatest commit: 7ad7dad The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review. WalkthroughThe client now selects socket retry and timeout settings by transport. Server-Sent Events default to unlimited retries and use the configured timeout for retry delay and client options. WebSocket settings use the configured retry count without a retry delay or client timeout option. Tests cover both transports, and the README and option documentation describe these behaviors. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to SSE reconnect limits are enforced, the normal client retains its timeout default, and the README limits timeout to SSE. No actionable merge-blocking issue remains. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change makes configured SSE retry limits effective without changing connection destinations or credential handling. The reviewed failure and cleanup paths preserve bounded retries and explicit shutdown; no material security risk was identified in this change. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 2fe58dd6-fa7a-4195-9460-053c0f2f65d2
📒 Files selected for processing (10)
.changeset/reconnect-and-timeout-per-transport.mdREADME.mdclient-src/index.jsclient-src/utils/socket-options.jssrc/hot.jssrc/options.check.jssrc/options.jsontest/client-socket-options.test.jstypes/client/utils/socket-options.d.tstypes/hot.d.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2457 +/- ##
==========================================
+ Coverage 96.27% 96.36% +0.08%
==========================================
Files 22 23 +1
Lines 2445 2449 +4
==========================================
+ Hits 2354 2360 +6
+ Misses 91 89 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The README quoted `10`, which is `createSocket`'s fallback and therefore
only what a WebSocket does. Unset over Server-Sent Events the retry count
is `Infinity` — the figure was wrong for the default transport, and had
been while the option was ignored there, so making it work is what made the
documentation wrong rather than merely incomplete.
unset, "sse" -> Infinity (this module's default)
unset, "ws" -> 10 (createSocket's, reached by passing nothing)
set -> both
A test pins both, reading `createSocket`'s number out of its source rather
than restating it, since that is the half the docs do not own.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
1 participant
What
Two client options were each inert on one transport, which came out of looking at whether
timeout,reconnectandautoConnectcould be grouped into one option. The grouping is cosmetic; this is the part that was actually broken.reconnectdid nothing over Server-Sent Events — the default transport. It was overridden toInfinity, so asking for a bounded number of attempts was ignored. It is honoured now. Unset still means keep trying for as long as the page is open: a dev server is expected to come back, and a tab left open across a restart has to find it again.timeoutdid nothing over a WebSocket. It was handed to a client whose constructor isconstructor(url)and takes no options at all.Why
timeoutis documented rather than implementedMy first instinct was to give the WebSocket client the same silence watchdog the
EventSourceone has. That is wrong, and the reason is in the servers:The browser answers a ping in its network stack and never shows it to JavaScript. A silence watchdog over a WebSocket would therefore fire on a connection that is healthy and merely idle.
And it is not needed. The half-open case the watchdog exists for is already handled where the pong is visible — the server sees the missing one and terminates the socket, so the browser gets a real
closeand reconnects:So
timeoutis Server-Sent Events only, it says so in the schema, the typedef and the README, and it is no longer passed to a client that drops it.Both decisions in one testable place
They moved into
client-src/utils/socket-options.js, a pure function — which is what makes them testable at all, sinceclient-src/index.jsconnects on import. Eight cases: one per transport per option, plusreconnect: 0meaning none rather than unset.A correction
The README already said
"sse"ignoredreconnect. I had said neither doc mentioned it; that was true of the schema only. Both describe the new behaviour now.Verified
npm run lint— clean (eslint, prettier, cspell,tsc, client types, schema-check)Next, not here
connect: false | { retries, timeout }would fold these three into one option. Worth doing once they behave, not before — grouping them while two were inert would have madeconnect: { retries: 10 }look even more like it worked on the default transport. Noted in #2454.🤖 Generated with Claude Code
https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Generated by Claude Code
Summary by CodeRabbit