Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — The production change removes redundant provider probes during WebSocket reconnects while preserving cached snapshots and independent background updates. A focused regression test covers repeated connections and verifies that no refresh is started. Notes:
You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe server subscription no longer triggers provider refreshes. The connection supervisor now allows 45 seconds for establishment, with tests covering slow setup, reconnects, and timeout behavior. ChangesServer configuration subscription
Connection establishment timeout
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The subscription path now serves cached snapshots without triggering provider refreshes, and the longer establishment timeout is covered for slow setup, reconnect, timeout, relay, and wakeup behavior. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Audited That leaves this PR with the If there is still a reproducible case where a healthy connection needs more than 15s after #11811, please post it here with timings and we can talk about the number. Otherwise I'd close this once #11811 merges. |
|
Confirmed your review. Removed the 45-second timeout proposal in 0237d71 and restored the original 15-second deadline and tests. The slow-setup cases only injected delays; they were not evidence of a healthy connection needing more than 15 seconds. I verified that #11811 contains the identical server patch and is merged. A merge preview of this corrected branch against main produces the same tree as main, so this PR is superseded and has nothing further to land. Validation: 58 supervisor/RPC session tests plus 8 server-config tests pass. Client-runtime typecheck, scoped lint, formatting, and Ponytail review pass. |
Each server-config subscription started a full provider refresh, so reconnects and extra tabs repeated provider probes. The server change removes that redundant refresh and keeps cached snapshots and provider-owned live updates.
Julius split this exact server patch into #11811, which merged at 7931227. This PR is now superseded by that merged fix.
Following the review from Julius, commit 0237d71 removes the proposed 45-second setup timeout and restores the existing 15-second timeout and its regression tests. The removed slow-setup tests used artificial delays; they did not establish a real connection that required a longer deadline. Connection readiness follows WebSocket connection, not shell sync.
Verified: 58 connection supervisor/RPC session tests and 8 server-config subscription tests pass. Client-runtime typecheck, scoped lint, formatting, and Ponytail review pass. A merge preview against main at 7931227 produces the exact same tree as main, with no conflicts or new changes.
There is no remaining change to land from this PR. No new PR or stack was created.
Model: GPT-6. Harness: Codex.