Skip to content

Conversation

@ThomasK33
Copy link
Member

@ThomasK33 ThomasK33 commented Dec 10, 2025

Problem

When running make dev-server and the backend restarts via nodemon, the frontend incorrectly shows "Authentication Required" modal instead of automatically reconnecting.

Root Cause

  • The error handler set auth_required even when no token existed
  • The close handler only handled auth codes (1008/4401), ignoring normal disconnection codes (1001, 1006)
  • No reconnection logic existed

Solution

Implement auto-reconnect with exponential backoff:

  • New reconnecting status - allows the app to stay visible while reconnecting
  • Track connection history - distinguish "first connect failed" vs "was connected, lost connection"
  • Exponential backoff - 100ms base delay, doubling up to 10s max, 10 attempts before giving up
  • Smart error handling:
    • Auth error codes (1008, 4401) → show auth modal
    • Normal disconnects after being connected → reconnect silently
    • First connection failure → show auth modal (server might require auth)

Testing

Added 5 unit tests covering:

  • Reconnects on close without showing auth_required when previously connected
  • Shows auth_required on close with auth error codes (4401, 1008)
  • Shows auth_required on first connection error without token
  • Reconnects on error when previously connected

📋 Implementation Plan

Plan: Fix WebSocket Reconnection on Server Restart

Problem Statement

When running make dev-server and 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:

  1. error handler (lines 152-160) - Sets auth_required even when no token exists:

    } else {
      setState({ status: "auth_required" });  // Wrong: assumes auth needed
    }
  2. close handler (lines 162-168) - Only handles auth codes 1008/4401, ignores normal disconnection codes (1001, 1006)

  3. No reconnection logic - Frontend never attempts to reconnect after disconnection

Solution: Auto-reconnect with exponential backoff

Changes to src/browser/contexts/API.tsx

1. Add new connection state for reconnecting

export type APIState =
  | { status: "connecting"; api: null; error: null }
  | { status: "connected"; api: APIClient; error: null }
  | { status: "reconnecting"; api: null; error: null; attempt: number }  // NEW
  | { status: "auth_required"; api: null; error: string | null }
  | { status: "error"; api: null; error: string };

2. Track "has ever connected" state

Add a ref to track whether we've successfully connected at least once:

const hasConnectedRef = useRef(false);

Set to true when connection succeeds; used to distinguish "first connect failed" vs "reconnect needed".

3. Add reconnection logic with exponential backoff

const reconnectAttemptRef = useRef(0);
const reconnectTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const MAX_RECONNECT_ATTEMPTS = 10;
const BASE_DELAY_MS = 100;
const MAX_DELAY_MS = 10000;

const scheduleReconnect = useCallback(() => {
  const attempt = reconnectAttemptRef.current;
  if (attempt >= MAX_RECONNECT_ATTEMPTS) {
    setState({ status: "error", error: "Connection lost. Please refresh the page." });
    return;
  }

  const delay = Math.min(BASE_DELAY_MS * Math.pow(2, attempt), MAX_DELAY_MS);
  reconnectAttemptRef.current = attempt + 1;
  setState({ status: "reconnecting", api: null, error: null, attempt: attempt + 1 });

  reconnectTimeoutRef.current = setTimeout(() => {
    connect(authToken);
  }, delay);
}, [authToken, connect]);

4. Update error handler

ws.addEventListener("error", () => {
  cleanup();
  if (hasConnectedRef.current) {
    // Was connected before - try to reconnect
    scheduleReconnect();
  } else if (token) {
    // First connection failed with token - might be invalid
    clearStoredAuthToken();
    setState({ status: "auth_required", error: "Connection failed - invalid token?" });
  } else {
    // First connection failed without token - server might require auth
    setState({ status: "auth_required" });
  }
});

5. Update close handler

ws.addEventListener("close", (event) => {
  // Auth-specific close codes
  if (event.code === 1008 || event.code === 4401) {
    cleanup();
    clearStoredAuthToken();
    hasConnectedRef.current = false;  // Reset - need fresh auth
    setState({ status: "auth_required", error: "Authentication required" });
    return;
  }

  // Normal disconnection (server restart, network issue)
  if (hasConnectedRef.current) {
    cleanup();
    scheduleReconnect();
  }
});

6. Reset reconnect state on successful connection

In the open handler's success path:

.then(() => {
  hasConnectedRef.current = true;
  reconnectAttemptRef.current = 0;  // Reset backoff
  // ... existing code
})

7. Cleanup on unmount

useEffect(() => {
  connect(authToken);
  return () => {
    cleanupRef.current?.();
    if (reconnectTimeoutRef.current) {
      clearTimeout(reconnectTimeoutRef.current);
    }
  };
}, []);

Changes to src/browser/App.tsx

Update the status check to handle reconnecting state - no modal, just let the app show while reconnecting:

// Show auth modal if authentication is required
if (status === "auth_required") {
  return <AuthTokenModal isOpen={true} onSubmit={authenticate} error={error} />;
}

// Reconnecting is handled gracefully - app stays visible
// Optional: could add a subtle toast/indicator later

Files to Modify

  1. src/browser/contexts/API.tsx - Main changes (reconnection logic)
  2. src/browser/App.tsx - Handle reconnecting state (minor, just update type handling)

Testing

Unit Test: src/browser/contexts/API.test.tsx

Test the reconnection behavior without showing auth modal on disconnect:

import { act, cleanup, render, waitFor } from "@testing-library/react";
import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
import { GlobalWindow } from "happy-dom";
import { APIProvider, useAPI } from "./API";

// Mock WebSocket that we can control
class MockWebSocket {
  static instances: MockWebSocket[] = [];
  url: string;
  readyState = 0; // CONNECTING
  onopen: (() => void) | null = null;
  onclose: ((event: { code: number }) => void) | null = null;
  onerror: (() => void) | null = null;
  eventListeners: Map<string, Function[]> = new Map();

  constructor(url: string) {
    this.url = url;
    MockWebSocket.instances.push(this);
  }

  addEventListener(event: string, handler: Function) {
    const handlers = this.eventListeners.get(event) || [];
    handlers.push(handler);
    this.eventListeners.set(event, handlers);
  }

  close() {
    this.readyState = 3; // CLOSED
  }

  // Test helpers
  simulateOpen() {
    this.readyState = 1; // OPEN
    this.eventListeners.get("open")?.forEach((h) => h());
  }

  simulateClose(code: number) {
    this.readyState = 3;
    this.eventListeners.get("close")?.forEach((h) => h({ code }));
  }

  simulateError() {
    this.eventListeners.get("error")?.forEach((h) => h());
  }

  static reset() {
    MockWebSocket.instances = [];
  }

  static lastInstance() {
    return MockWebSocket.instances[MockWebSocket.instances.length - 1];
  }
}

// Test component to observe API state
function APIStateObserver({ onState }: { onState: (state: ReturnType<typeof useAPI>) => void }) {
  const apiState = useAPI();
  onState(apiState);
  return null;
}

describe("API reconnection", () => {
  beforeEach(() => {
    const window = new GlobalWindow();
    globalThis.window = window as unknown as Window & typeof globalThis;
    globalThis.document = window.document;
    globalThis.WebSocket = MockWebSocket as unknown as typeof WebSocket;
    MockWebSocket.reset();
  });

  afterEach(() => {
    cleanup();
    MockWebSocket.reset();
  });

  test("reconnects on close without showing auth_required when previously connected", async () => {
    const states: string[] = [];
    
    render(
      <APIProvider>
        <APIStateObserver onState={(s) => states.push(s.status)} />
      </APIProvider>
    );

    // Initial connection
    const ws1 = MockWebSocket.lastInstance();
    expect(ws1).toBeDefined();
    
    // Simulate successful connection
    await act(async () => {
      ws1.simulateOpen();
      // Mock the ping succeeding (need to mock orpc client)
    });

    // Simulate server restart (close code 1006 = abnormal closure)
    await act(async () => {
      ws1.simulateClose(1006);
    });

    // Should be "reconnecting", NOT "auth_required"
    await waitFor(() => {
      expect(states).toContain("reconnecting");
      expect(states.filter((s) => s === "auth_required")).toHaveLength(0);
    });

    // New WebSocket should be created for reconnect attempt
    expect(MockWebSocket.instances.length).toBeGreaterThan(1);
  });

  test("shows auth_required on close with auth error codes (1008, 4401)", async () => {
    const states: string[] = [];
    
    render(
      <APIProvider>
        <APIStateObserver onState={(s) => states.push(s.status)} />
      </APIProvider>
    );

    const ws1 = MockWebSocket.lastInstance();
    
    // Simulate successful connection then auth rejection
    await act(async () => {
      ws1.simulateOpen();
      ws1.simulateClose(4401); // Auth required code
    });

    await waitFor(() => {
      expect(states).toContain("auth_required");
    });
  });

  test("shows error after max reconnect attempts exhausted", async () => {
    // Fast-forward timers for this test
    const states: string[] = [];
    
    render(
      <APIProvider>
        <APIStateObserver onState={(s) => states.push(s.status)} />
      </APIProvider>
    );

    // Simulate 10 failed reconnection attempts
    for (let i = 0; i < 11; i++) {
      const ws = MockWebSocket.lastInstance();
      await act(async () => {
        ws.simulateError();
      });
      // Advance timers for backoff delay
    }

    await waitFor(() => {
      expect(states).toContain("error");
    });
  });
});

Key Test Cases

Scenario Expected Status Auth Modal?
Server restart (close code 1006) after connected reconnecting No
Server restart (close code 1001) after connected reconnecting No
Auth rejection (close code 4401) auth_required Yes
Auth rejection (close code 1008) auth_required Yes
First connection fails, no token auth_required Yes
First connection fails, with token auth_required Yes
Max reconnect attempts (10) exhausted error No

Manual Testing

  1. Run make dev-server
  2. Open frontend in browser
  3. Make a code change to trigger nodemon restart
  4. Verify frontend reconnects automatically without showing auth modal
  5. Test with auth token to ensure auth errors still show modal appropriately

Generated with mux

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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".

@ThomasK33
Copy link
Member Author

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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".

@ThomasK33 ThomasK33 force-pushed the fix-auth-display branch 10 times, most recently from 2217ea8 to 4a713b5 Compare December 10, 2025 12:04
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>
@ThomasK33
Copy link
Member Author

@codex review

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. Keep them coming!

ℹ️ 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".

@ThomasK33 ThomasK33 added this pull request to the merge queue Dec 10, 2025
Merged via the queue into main with commit 92fd55f Dec 10, 2025
20 checks passed
@ThomasK33 ThomasK33 deleted the fix-auth-display branch December 10, 2025 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant