Do an imitation of protecting our butts (race conditions in message loading, websocket defensiveness, optimistic UI update rollback handling

2026-08-08 17:53:10 +02:00 · 2026-01-19 11:47:20 -08:00
parent bf03d76c33
commit 0138233743
12 changed files with 553 additions and 91 deletions
@@ -13,7 +13,7 @@
    <link rel="shortcut icon" href="/favicon.ico" />
    <link rel="apple-touch-icon" sizes="180x180" href="/apple-touch-icon.png" />
    <link rel="manifest" href="/site.webmanifest" />
-    <script type="module" crossorigin src="/assets/index-BGhVMB5J.js"></script>
+    <script type="module" crossorigin src="/assets/index-BeiXDnDV.js"></script>
    <link rel="stylesheet" crossorigin href="/assets/index-CnRBRJ10.css">
  </head>
  <body>
@@ -637,22 +637,26 @@ export function App() {
  );

  // Handle sort order change via API with optimistic update
-  const handleSortOrderChange = useCallback(async (order: 'recent' | 'alpha') => {
-    // Optimistic update for responsive UI
-    setAppSettings((prev) => (prev ? { ...prev, sidebar_sort_order: order } : prev));
+  const handleSortOrderChange = useCallback(
+    async (order: 'recent' | 'alpha') => {
+      // Capture previous value for rollback on error
+      const previousOrder = appSettings?.sidebar_sort_order ?? 'recent';

-    try {
-      const updatedSettings = await api.updateSettings({ sidebar_sort_order: order });
-      setAppSettings(updatedSettings);
-    } catch (err) {
-      console.error('Failed to update sort order:', err);
-      // Revert on error
-      setAppSettings((prev) =>
-        prev ? { ...prev, sidebar_sort_order: order === 'recent' ? 'alpha' : 'recent' } : prev
-      );
-      toast.error('Failed to save sort preference');
-    }
-  }, []);
+      // Optimistic update for responsive UI
+      setAppSettings((prev) => (prev ? { ...prev, sidebar_sort_order: order } : prev));
+
+      try {
+        const updatedSettings = await api.updateSettings({ sidebar_sort_order: order });
+        setAppSettings(updatedSettings);
+      } catch (err) {
+        console.error('Failed to update sort order:', err);
+        // Revert to previous value on error (not inverting the new value)
+        setAppSettings((prev) => (prev ? { ...prev, sidebar_sort_order: previousOrder } : prev));
+        toast.error('Failed to save sort preference');
+      }
+    },
+    [appSettings?.sidebar_sort_order]
+  );

  // Sidebar content (shared between desktop and mobile)
  const sidebarContent = (
@@ -45,6 +45,16 @@ async function fetchJson<T>(url: string, options?: RequestInit): Promise<T> {
  return res.json();
 }

+/** Check if an error is an AbortError (request was cancelled) */
+export function isAbortError(err: unknown): boolean {
+  // DOMException is thrown by fetch when aborted, and it's not an Error subclass
+  if (err instanceof DOMException && err.name === 'AbortError') {
+    return true;
+  }
+  // Also check for Error with AbortError name (for compatibility)
+  return err instanceof Error && err.name === 'AbortError';
+}
+
 interface DecryptResult {
  started: boolean;
  total_packets: number;
@@ -134,19 +144,22 @@ export const api = {
    }),

  // Messages
-  getMessages: (params?: {
-    limit?: number;
-    offset?: number;
-    type?: 'PRIV' | 'CHAN';
-    conversation_key?: string;
-  }) => {
+  getMessages: (
+    params?: {
+      limit?: number;
+      offset?: number;
+      type?: 'PRIV' | 'CHAN';
+      conversation_key?: string;
+    },
+    signal?: AbortSignal
+  ) => {
    const searchParams = new URLSearchParams();
    if (params?.limit) searchParams.set('limit', params.limit.toString());
    if (params?.offset) searchParams.set('offset', params.offset.toString());
    if (params?.type) searchParams.set('type', params.type);
    if (params?.conversation_key) searchParams.set('conversation_key', params.conversation_key);
    const query = searchParams.toString();
-    return fetchJson<Message[]>(`/messages${query ? `?${query}` : ''}`);
+    return fetchJson<Message[]>(`/messages${query ? `?${query}` : ''}`, { signal });
  },
  getMessagesBulk: (
    conversations: Array<{ type: 'PRIV' | 'CHAN'; conversation_key: string }>,
@@ -1,6 +1,6 @@
 import { useState, useCallback, useEffect, useRef } from 'react';
 import { toast } from '../components/ui/sonner';
-import { api } from '../api';
+import { api, isAbortError } from '../api';
 import type { Conversation, Message, MessagePath } from '../types';

 const MESSAGE_PAGE_SIZE = 200;
@@ -33,26 +33,49 @@ export function useConversationMessages(
  // Track seen message content for deduplication
  const seenMessageContent = useRef<Set<string>>(new Set());

+  // AbortController for cancelling in-flight requests on conversation change
+  const abortControllerRef = useRef<AbortController | null>(null);
+
+  // Ref to track the conversation ID being fetched to prevent stale responses
+  const fetchingConversationIdRef = useRef<string | null>(null);
+
  // Fetch messages for active conversation
+  // Note: This is called manually and from the useEffect. The useEffect handles
+  // cancellation via AbortController; manual calls (e.g., after sending a message)
+  // don't need cancellation.
  const fetchMessages = useCallback(
-    async (showLoading = false) => {
+    async (showLoading = false, signal?: AbortSignal) => {
      if (!activeConversation || activeConversation.type === 'raw') {
        setMessages([]);
        setHasOlderMessages(false);
        return;
      }

+      // Track which conversation we're fetching for
+      const conversationId = activeConversation.id;
+
      if (showLoading) {
        setMessagesLoading(true);
        // Clear messages first so MessageList resets scroll state for new conversation
        setMessages([]);
      }
      try {
-        const data = await api.getMessages({
-          type: activeConversation.type === 'channel' ? 'CHAN' : 'PRIV',
-          conversation_key: activeConversation.id,
-          limit: MESSAGE_PAGE_SIZE,
-        });
+        const data = await api.getMessages(
+          {
+            type: activeConversation.type === 'channel' ? 'CHAN' : 'PRIV',
+            conversation_key: activeConversation.id,
+            limit: MESSAGE_PAGE_SIZE,
+          },
+          signal
+        );
+
+        // Check if this response is still for the current conversation
+        // This handles the race where the conversation changed while awaiting
+        if (fetchingConversationIdRef.current !== conversationId) {
+          // Stale response - conversation changed while we were fetching
+          return;
+        }
+
        setMessages(data);
        // Track seen content for new messages
        seenMessageContent.current.clear();
@@ -62,6 +85,10 @@ export function useConversationMessages(
        // If we got a full page, there might be more
        setHasOlderMessages(data.length >= MESSAGE_PAGE_SIZE);
      } catch (err) {
+        // Don't show error toast for aborted requests (user switched conversations)
+        if (isAbortError(err)) {
+          return;
+        }
        console.error('Failed to fetch messages:', err);
        toast.error('Failed to load messages', {
          description: err instanceof Error ? err.message : 'Check your connection',
@@ -114,10 +141,40 @@ export function useConversationMessages(
    }
  }, [activeConversation, loadingOlder, hasOlderMessages, messages.length]);

-  // Fetch messages when conversation changes
+  // Fetch messages when conversation changes, with proper cancellation
  useEffect(() => {
-    fetchMessages(true);
-  }, [fetchMessages]);
+    // Abort any previous in-flight request
+    if (abortControllerRef.current) {
+      abortControllerRef.current.abort();
+    }
+
+    // Track which conversation we're now fetching
+    fetchingConversationIdRef.current = activeConversation?.id ?? null;
+
+    // Clear state for new conversation
+    if (!activeConversation || activeConversation.type === 'raw') {
+      setMessages([]);
+      setHasOlderMessages(false);
+      return;
+    }
+
+    // Create new AbortController for this fetch
+    const controller = new AbortController();
+    abortControllerRef.current = controller;
+
+    // Fetch messages with the abort signal
+    fetchMessages(true, controller.signal);
+
+    // Cleanup: abort request if conversation changes or component unmounts
+    return () => {
+      controller.abort();
+    };
+    // NOTE: Intentionally omitting fetchMessages and activeConversation from deps:
+    // - fetchMessages is recreated when activeConversation changes, which would cause infinite loops
+    // - activeConversation object identity changes on every render; we only care about id/type
+    // - We use fetchingConversationIdRef and AbortController to handle stale responses safely
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [activeConversation?.id, activeConversation?.type]);

  // Add a message if it's new (deduplication)
  // Returns true if the message was added, false if it was a duplicate
@@ -0,0 +1,62 @@
+/**
+ * Tests for API utilities.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { isAbortError } from '../api';
+
+describe('isAbortError', () => {
+  it('returns true for AbortError', () => {
+    const controller = new AbortController();
+    controller.abort();
+
+    // Create an error that mimics fetch abort
+    const error = new DOMException('The operation was aborted', 'AbortError');
+
+    expect(isAbortError(error)).toBe(true);
+  });
+
+  it('returns true for Error with name AbortError', () => {
+    const error = new Error('Request cancelled');
+    error.name = 'AbortError';
+
+    expect(isAbortError(error)).toBe(true);
+  });
+
+  it('returns false for regular Error', () => {
+    const error = new Error('Something went wrong');
+
+    expect(isAbortError(error)).toBe(false);
+  });
+
+  it('returns false for TypeError', () => {
+    const error = new TypeError('Network failure');
+
+    expect(isAbortError(error)).toBe(false);
+  });
+
+  it('returns false for null', () => {
+    expect(isAbortError(null)).toBe(false);
+  });
+
+  it('returns false for undefined', () => {
+    expect(isAbortError(undefined)).toBe(false);
+  });
+
+  it('returns false for non-Error objects', () => {
+    expect(isAbortError({ message: 'error' })).toBe(false);
+    expect(isAbortError('error string')).toBe(false);
+    expect(isAbortError(42)).toBe(false);
+  });
+
+  it('returns false for Error subclasses with different names', () => {
+    class CustomError extends Error {
+      constructor() {
+        super('Custom error');
+        this.name = 'CustomError';
+      }
+    }
+
+    expect(isAbortError(new CustomError())).toBe(false);
+  });
+});
@@ -193,3 +193,80 @@ describe('parseWebSocketMessage', () => {
    expect(onRawPacket).toHaveBeenCalledWith(packetData);
  });
 });
+
+describe('useWebSocket ref-based handler pattern', () => {
+  /**
+   * These tests verify the pattern used in useWebSocket to avoid stale closures.
+   * The hook stores handlers in a ref and accesses them through the ref in callbacks.
+   * This ensures that when handlers are updated, the WebSocket still calls the latest version.
+   */
+
+  it('demonstrates ref pattern prevents stale closure', () => {
+    // Simulate the ref pattern used in useWebSocket
+    interface Handlers {
+      onMessage?: (msg: string) => void;
+    }
+
+    // This simulates what the hook does: store handlers in a ref
+    const handlersRef: { current: Handlers } = { current: {} };
+
+    // First handler version
+    const firstHandler = vi.fn();
+    handlersRef.current = { onMessage: firstHandler };
+
+    // Simulate what onmessage does: access handlers through ref
+    const processMessage = (data: string) => {
+      // This is the pattern: access through ref.current, not closed-over variable
+      handlersRef.current.onMessage?.(data);
+    };
+
+    // Send first message
+    processMessage('message1');
+    expect(firstHandler).toHaveBeenCalledWith('message1');
+
+    // Update handler (simulates React re-render with new handler)
+    const secondHandler = vi.fn();
+    handlersRef.current = { onMessage: secondHandler };
+
+    // Send second message
+    processMessage('message2');
+
+    // First handler should NOT be called again (would happen with stale closure)
+    expect(firstHandler).toHaveBeenCalledTimes(1);
+    // Second handler should be called (ref pattern works)
+    expect(secondHandler).toHaveBeenCalledWith('message2');
+  });
+
+  it('demonstrates stale closure problem without ref pattern', () => {
+    // This demonstrates the bug we fixed - without refs, handlers become stale
+    interface Handlers {
+      onMessage?: (msg: string) => void;
+    }
+
+    // First handler version
+    const firstHandler = vi.fn();
+    let handlers: Handlers = { onMessage: firstHandler };
+
+    // BAD PATTERN: capture handlers in closure (this is what we fixed)
+    const capturedHandlers = handlers;
+    const processMessageBad = (data: string) => {
+      // This captures `capturedHandlers` at creation time - STALE!
+      capturedHandlers.onMessage?.(data);
+    };
+
+    // Send first message
+    processMessageBad('message1');
+    expect(firstHandler).toHaveBeenCalledWith('message1');
+
+    // Update handler
+    const secondHandler = vi.fn();
+    handlers = { onMessage: secondHandler };
+
+    // Send second message - BUG: still calls first handler!
+    processMessageBad('message2');
+
+    // This demonstrates the stale closure bug
+    expect(firstHandler).toHaveBeenCalledTimes(2); // Called twice - bug!
+    expect(secondHandler).not.toHaveBeenCalled(); // Never called - bug!
+  });
+});
@@ -33,6 +33,19 @@ export function useWebSocket(options: UseWebSocketOptions) {
  const reconnectTimeoutRef = useRef<number | null>(null);
  const [connected, setConnected] = useState(false);

+  // Store options in ref to avoid stale closures in WebSocket handlers.
+  // The onmessage callback captures this ref, and we keep the ref updated
+  // with the latest handlers. This way, even though the WebSocket connection
+  // is only created once, it always calls the current handlers.
+  const optionsRef = useRef<UseWebSocketOptions>(options);
+
+  // Keep the ref updated with latest options
+  useEffect(() => {
+    optionsRef.current = options;
+  }, [options]);
+
+  // Connect function - uses ref for handlers to avoid stale closures
+  // No dependencies needed since we access handlers through ref
  const connect = useCallback(() => {
    // Determine WebSocket URL based on current location
    const protocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:';
@@ -68,25 +81,27 @@ export function useWebSocket(options: UseWebSocketOptions) {
    ws.onmessage = (event) => {
      try {
        const msg: WebSocketMessage = JSON.parse(event.data);
+        // Access handlers through ref to always use current versions
+        const handlers = optionsRef.current;

        switch (msg.type) {
          case 'health':
-            options.onHealth?.(msg.data as HealthStatus);
+            handlers.onHealth?.(msg.data as HealthStatus);
            break;
          case 'contacts':
-            options.onContacts?.(msg.data as Contact[]);
+            handlers.onContacts?.(msg.data as Contact[]);
            break;
          case 'channels':
-            options.onChannels?.(msg.data as Channel[]);
+            handlers.onChannels?.(msg.data as Channel[]);
            break;
          case 'message':
-            options.onMessage?.(msg.data as Message);
+            handlers.onMessage?.(msg.data as Message);
            break;
          case 'contact':
-            options.onContact?.(msg.data as Contact);
+            handlers.onContact?.(msg.data as Contact);
            break;
          case 'raw_packet':
-            options.onRawPacket?.(msg.data as RawPacket);
+            handlers.onRawPacket?.(msg.data as RawPacket);
            break;
          case 'message_acked': {
            const ackData = msg.data as {
@@ -94,14 +109,14 @@ export function useWebSocket(options: UseWebSocketOptions) {
              ack_count: number;
              paths?: MessagePath[];
            };
-            options.onMessageAcked?.(ackData.message_id, ackData.ack_count, ackData.paths);
+            handlers.onMessageAcked?.(ackData.message_id, ackData.ack_count, ackData.paths);
            break;
          }
          case 'error':
-            options.onError?.(msg.data as ErrorEvent);
+            handlers.onError?.(msg.data as ErrorEvent);
            break;
          case 'success':
-            options.onSuccess?.(msg.data as SuccessEvent);
+            handlers.onSuccess?.(msg.data as SuccessEvent);
            break;
          case 'pong':
            // Heartbeat response, ignore
@@ -115,7 +130,7 @@ export function useWebSocket(options: UseWebSocketOptions) {
    };

    wsRef.current = ws;
-  }, [options]);
+  }, []); // No dependencies - handlers accessed through ref

  useEffect(() => {
    connect();