Fix: Clear PartySocket backoff timer and reconnect instantly on network regain - #1782
Merged
Merged
Conversation
|
@revatikadam0607 is attempting to deploy a commit to the pandeysatyam1802-gmailcom's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
🎉 Thank you for contributing to WorkSphere! Please ensure: ✅ Tests pass 💡 Connect & Support:
|
📝 WalkthroughWalkthrough
ChangesPartySocket reconnect recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/partySocketReconnect.ts (1)
165-168: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the resolver when
_connectcancels the wait.A direct
_connect()clears the timer but leavespendingResolvelive. A lateronlineevent can resolve that cancelled, stale_wait()and resume an obsolete reconnect path.Proposed fix
if (pendingTimeoutId) { clearTimeout(pendingTimeoutId); pendingTimeoutId = null; } + pendingResolve = null; originalConnect.call(this);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/partySocketReconnect.ts` around lines 165 - 168, Update the direct _connect() cancellation path to clear pendingResolve alongside pendingTimeoutId, ensuring a later online event cannot resolve the cancelled _wait() or resume the obsolete reconnect flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/lib/partySocketReconnect.test.ts`:
- Around line 140-173: Wrap the mocked-global setup and assertions in the test
around attachJitteredBackoff and _wait with a try/finally block. In the finally
clause, restore window.addEventListener from originalAddEventListener and return
Jest to real timers with jest.useRealTimers(), ensuring cleanup runs even when
the test fails.
---
Outside diff comments:
In `@src/lib/partySocketReconnect.ts`:
- Around line 165-168: Update the direct _connect() cancellation path to clear
pendingResolve alongside pendingTimeoutId, ensuring a later online event cannot
resolve the cancelled _wait() or resume the obsolete reconnect flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24196553-ba24-4660-816d-5d82a5bea862
📒 Files selected for processing (2)
src/__tests__/lib/partySocketReconnect.test.tssrc/lib/partySocketReconnect.ts
Comment on lines
+140
to
+173
| jest.useFakeTimers(); | ||
|
|
||
| let onlineHandler: (() => void) | undefined; | ||
| const originalAddEventListener = window.addEventListener; | ||
| window.addEventListener = jest.fn((event: string, cb: any) => { | ||
| if (event === "online") { | ||
| onlineHandler = cb; | ||
| } | ||
| }) as any; | ||
|
|
||
| const socket = { | ||
| _retryCount: 3, | ||
| _getNextDelay: () => 30_000, | ||
| _connect: jest.fn(), | ||
| _disconnect: jest.fn(), | ||
| _clearTimeouts: jest.fn(), | ||
| addEventListener: jest.fn(), | ||
| } as any; | ||
|
|
||
| attachJitteredBackoff(socket); | ||
|
|
||
| let resolved = false; | ||
| const waitPromise = socket._wait().then(() => { | ||
| resolved = true; | ||
| }); | ||
|
|
||
| // Simulate the browser regaining network connection | ||
| onlineHandler?.(); | ||
| await Promise.resolve(); | ||
| await waitPromise; | ||
|
|
||
| expect(resolved).toBe(true); | ||
|
|
||
| window.addEventListener = originalAddEventListener; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file outline ---'
ast-grep outline src/__tests__/lib/partySocketReconnect.test.ts --view expanded || true
echo '--- top of file with line numbers ---'
sed -n '1,260p' src/__tests__/lib/partySocketReconnect.test.ts | cat -n
echo '--- search for cleanup hooks in this file ---'
rg -n "afterEach|beforeEach|useRealTimers|restoreAllMocks|clearAllTimers|addEventListener =|removeEventListener" src/__tests__/lib/partySocketReconnect.test.ts || trueRepository: SatyamPandey-07/WorkSphere
Length of output: 10734
Restore the mocked globals in finally. A failure before cleanup can leave window.addEventListener mocked and fake timers enabled for later tests, making the suite order-dependent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/lib/partySocketReconnect.test.ts` around lines 140 - 173, Wrap
the mocked-global setup and assertions in the test around attachJitteredBackoff
and _wait with a try/finally block. In the finally clause, restore
window.addEventListener from originalAddEventListener and return Jest to real
timers with jest.useRealTimers(), ensuring cleanup runs even when the test
fails.
SatyamPandey-07
added
good first issue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1435 — PartySocket was still waiting out its exponential backoff delay even after the browser's
onlineevent fired, causing a delayed reconnect instead of an immediate one.Changes
__worksphereForceReconnectmethod inpartySocketReconnect.tsthat clears any pending backoff timeout/promise and resolves it immediately (or calls_connect()directly if no timer is pending).windowonlineevent listener insideattachJitteredBackoffthat calls__worksphereForceReconnectwhen the socket isn't already connected.pendingResolveis cleared alongsidependingTimeoutIdin_clearTimeoutsand_disconnectto avoid stale resolves.onlineevent resolves the pending_wait()immediately instead of waiting for the full backoff delay.Testing
it("clears the pending backoff timer and reconnects immediately on online event", ...)inpartySocketReconnect.test.ts.I've resolved the issue. Kindly review it, and if everything looks good, please consider merging the corresponding PR.
Hi @SatyamPandey-07 , could you please review this PR for a potential bonus label based on the metrics below? Thanks!
Summary by CodeRabbit