fix(client): read autoConnect off the query like every other boolean - #2456
Conversation
It tested `=== "true"` where the rest of them test `!== "false"`, so a
value it did not recognise turned the client off:
?autoConnect=1 -> was off, is now left on
?hmr=1 -> left on, as it always was
?autoConnect=false -> off, unchanged
The default is already on, so turning it off is the only thing anyone
writes this option for — which makes the odd reading a trap rather than a
stricter policy.
The existing coverage could not catch it: it only used `autoConnect=false`,
which both readings treat the same way. Two tests now, one for each half,
and the first was checked against the old reading to be sure it fails
there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
🦋 Changeset detectedLatest commit: a763e12 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 (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughThe client now treats only the exact Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains: the default behavior is preserved, and the query cases are covered. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2456 +/- ##
=======================================
Coverage 96.27% 96.27%
=======================================
Files 22 22
Lines 2445 2445
=======================================
Hits 2354 2354
Misses 91 91 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What
autoConnectwas the one boolean on the client's query read with=== "true". Every other one is read with!== "false".?autoConnect=1?autoConnect=false?hot=1The default is already on, so turning it off is the only thing anyone writes this option for — which makes the odd reading a trap rather than a stricter policy: a value it did not recognise silently disconnected the client.
Why the existing test could not catch it
There was already a
?autoConnect=falsecase, and both readings treat that identically. The difference lives entirely in the values neither recognises, so the bug sat behind passing coverage.Two tests now, one per half. I checked the first against the old reading before trusting it — reverted the parser, rebuilt the client, and it failed on the
=1case while the=falsecase kept passing. Without that it would be decoration.Severity, stated plainly
autoConnectarrived after 8.3.0, so it is unreleased: nobody has hit this in a published version. A patch to unreleased code rather than a fix for anyone in the field.Verified
npm run lint— clean (eslint, prettier, cspell,tsc, client types, schema-check)Worth knowing for anyone reviewing a client change:
client/is a build output compiled fromclient-src/bybabel client-src -d client, and the browser tests bundleclient/.pretestonly lints, so a directjest test/e2eafter editingclient-srcruns the previous client untilnpm run build:client.🤖 Generated with Claude Code
https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Generated by Claude Code
Summary by CodeRabbit
autoConnectquery parameter now disables auto-connection only when set tofalse. Values such as1enable it.