-
Notifications
You must be signed in to change notification settings - Fork 30
🤖 fix: auto-reconnect WebSocket on server restart #1071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fe161c4 to
1e1bbdb
Compare
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2217ea8 to
4a713b5
Compare
Previously, when the dev server restarted (nodemon), the frontend would incorrectly show the 'Authentication Required' modal instead of automatically reconnecting. Changes: - Add 'reconnecting' status to APIState for graceful reconnection UX - Track whether we've ever connected successfully (hasConnectedRef) - Implement exponential backoff reconnection (100ms → 10s, max 10 attempts) - Only show auth_required on first connection failure or auth error codes - Normal disconnects (1001, 1006) trigger reconnection if previously connected - Auth error codes (1008, 4401) still show auth modal _Generated with mux_ Change-Id: I8cf8431181a879c5faae6cff48660212f8b68e95 Signed-off-by: Thomas Kosiewski <tk@coder.com>
The CI environment may have window.api defined, causing the APIProvider to use the Electron client path instead of the WebSocket path, resulting in no MockWebSocket instances being created. Change-Id: Ic971f4e0fe8dd6efc997b04a42181eaae626b2b8 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ling Browser fires 'error' before 'close' for WebSocket failures. Previously both handlers could schedule reconnection, leading to double attempts. Now: - 'error' handler is a no-op (close will follow) - 'close' handler handles all cleanup and reconnection logic - Consolidated first-connection-failure logic into close handler Change-Id: If1575bee81ad4fd43e29f186a722e0c60646bcc4 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Instead of manipulating global state in tests, add a createWebSocket prop to APIProvider that allows injecting a mock WebSocket factory. This eliminates: - Global console.error suppression - Global WebSocket replacement - window.api deletion hacks - Complex GlobalWindow setup with Object.assign Tests now simply pass createMockWebSocket prop and the minimal DOM setup required by @testing-library/react. Change-Id: I341ddef8db208081180b989e92615c673aceed31 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Other test files (ProjectContext.test.tsx, WorkspaceContext.test.tsx) mock @/browser/contexts/API with a fake APIProvider. Due to bun's known issue with module mock isolation (oven-sh/bun#12823), these mocks leak between test files. This caused API.test.tsx to get the mocked APIProvider instead of the real one, resulting in MockWebSocket never being created because the mocked APIProvider just returns children without calling connect(). Fix: Import the real module first, then re-mock @/browser/contexts/API with the real exports. This ensures subsequent tests that import from that path get our real implementation, not another test's mock. Change-Id: Ie4552acdea69e1078b364e5b8f97e084bf1ec788 Signed-off-by: Thomas Kosiewski <tk@coder.com>
4a713b5 to
8257b0e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Problem
When running
make dev-serverand the backend restarts via nodemon, the frontend incorrectly shows "Authentication Required" modal instead of automatically reconnecting.Root Cause
errorhandler setauth_requiredeven when no token existedclosehandler only handled auth codes (1008/4401), ignoring normal disconnection codes (1001, 1006)Solution
Implement auto-reconnect with exponential backoff:
reconnectingstatus - allows the app to stay visible while reconnectingTesting
Added 5 unit tests covering:
📋 Implementation Plan
Plan: Fix WebSocket Reconnection on Server Restart
Problem Statement
When running
make dev-serverand the backend restarts via nodemon, the frontend incorrectly shows "Authentication Required" modal instead of automatically reconnecting.Root Cause Analysis
In
src/browser/contexts/API.tsx:errorhandler (lines 152-160) - Setsauth_requiredeven when no token exists:closehandler (lines 162-168) - Only handles auth codes 1008/4401, ignores normal disconnection codes (1001, 1006)No reconnection logic - Frontend never attempts to reconnect after disconnection
Solution: Auto-reconnect with exponential backoff
Changes to
src/browser/contexts/API.tsx1. Add new connection state for reconnecting
2. Track "has ever connected" state
Add a ref to track whether we've successfully connected at least once:
Set to
truewhen connection succeeds; used to distinguish "first connect failed" vs "reconnect needed".3. Add reconnection logic with exponential backoff
4. Update error handler
5. Update close handler
6. Reset reconnect state on successful connection
In the
openhandler's success path:7. Cleanup on unmount
Changes to
src/browser/App.tsxUpdate the status check to handle
reconnectingstate - no modal, just let the app show while reconnecting:Files to Modify
src/browser/contexts/API.tsx- Main changes (reconnection logic)src/browser/App.tsx- Handlereconnectingstate (minor, just update type handling)Testing
Unit Test:
src/browser/contexts/API.test.tsxTest the reconnection behavior without showing auth modal on disconnect:
Key Test Cases
reconnectingreconnectingauth_requiredauth_requiredauth_requiredauth_requirederrorManual Testing
make dev-serverGenerated with
mux