Merge pull request #1302 from david87131/fix/issues-1227-1224-1222-1208

fix: resolve stale closures, memory leak, and add split-view telemetry

Author: leojay-net

Dechat/dex_with_fiat_frontend/src/hooks/__tests__/useSplitView.test.ts

@@ -1,7 +1,8 @@
-import { describe, expect, it } from 'vitest';
+import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest';
 import { ChatSession } from '@/types';
 import { useSplitView } from '@/hooks/useSplitView';
 import { renderHook, act } from '@testing-library/react';
+import { setTelemetryConsent, type ChatEvent, type SplitViewTelemetryPayload } from '@/lib/chatTelemetry';
 
 function makeSession(id: string, title = 'Session'): ChatSession {
   const now = new Date();
@@ -120,3 +121,95 @@ describe('useSplitView – state management', () => {
     expect(result.current.state.selectedMessageId).toBeNull();
   });
 });
+
+describe('useSplitView – structured telemetry (#1208)', () => {
+  function captureEvents(): { events: ChatEvent<SplitViewTelemetryPayload>[] } {
+    const box = { events: [] as ChatEvent<SplitViewTelemetryPayload>[] };
+    window.addEventListener('chat:telemetry', ((e: Event) => {
+      const detail = (e as CustomEvent<ChatEvent<SplitViewTelemetryPayload>>).detail;
+      if (detail.name === 'split_view') {
+        box.events.push(detail);
+      }
+    }) as EventListener);
+    return box;
+  }
+
+  beforeEach(() => {
+    setTelemetryConsent(true);
+    // `chatTelemetry`'s emitter defers the actual `chat:telemetry`
+    // CustomEvent dispatch to `requestAnimationFrame` to avoid blocking
+    // renders. Run the callback synchronously here so tests don't depend on
+    // real frame timing (which is flaky/inconsistent across jsdom/happy-dom
+    // versions when multiple events are scheduled back to back).
+    vi.spyOn(window, 'requestAnimationFrame').mockImplementation((cb) => {
+      cb(0);
+      return 0;
+    });
+  });
+
+  afterEach(() => {
+    vi.restoreAllMocks();
+  });
+
+  it('emits an "open" event with the session ids when the panel opens', () => {
+    const box = captureEvents();
+    const { result } = renderHook(() => useSplitView(sessions));
+
+    act(() => {
+      result.current.open('s1', 's2');
+    });
+
+    expect(box.events).toHaveLength(1);
+    expect(box.events[0].payload).toMatchObject({
+      action: 'open',
+      leftSessionId: 's1',
+      rightSessionId: 's2',
+    });
+  });
+
+  it('emits a "close" event when the panel closes', () => {
+    const box = captureEvents();
+    const { result } = renderHook(() => useSplitView(sessions));
+
+    act(() => {
+      result.current.open('s1', 's2');
+    });
+    act(() => {
+      result.current.close();
+    });
+
+    const closeEvents = box.events.filter((e) => e.payload.action === 'close');
+    expect(closeEvents).toHaveLength(1);
+  });
+
+  it('emits a "swap_sessions" event with the post-swap session ids', () => {
+    const box = captureEvents();
+    const { result } = renderHook(() => useSplitView(sessions));
+
+    act(() => {
+      result.current.open('s1', 's2');
+    });
+    act(() => {
+      result.current.swapSessions();
+    });
+
+    const swapEvents = box.events.filter((e) => e.payload.action === 'swap_sessions');
+    expect(swapEvents).toHaveLength(1);
+    expect(swapEvents[0].payload).toMatchObject({
+      leftSessionId: 's2',
+      rightSessionId: 's1',
+    });
+  });
+
+  it('does not emit telemetry without user consent', () => {
+    setTelemetryConsent(false);
+    const box = captureEvents();
+    const { result } = renderHook(() => useSplitView(sessions));
+
+    act(() => {
+      result.current.open('s1', 's2');
+    });
+
+    expect(box.events).toHaveLength(0);
+  });
+});

Dechat/dex_with_fiat_frontend/src/hooks/useChatPagination.test.ts

@@ -77,6 +77,39 @@ describe('useChatPagination', () => {
     }).not.toThrow();
   });
 
+  it('regression: uses the latest messages/pageSize when the pending timer fires, not the stale closure from when loadMore was called', () => {
+    const initialMessages = createMessages(30);
+    const { result, rerender } = renderHook(
+      ({ messages, pageSize }) => useChatPagination(messages, pageSize),
+      { initialProps: { messages: initialMessages, pageSize: 20 } },
+    );
+
+    expect(result.current.visibleMessages).toHaveLength(20);
+
+    act(() => {
+      result.current.loadMore();
+    });
+
+    // Before the 400ms timer fires, several new messages arrive and the
+    // list grows from 30 to 45. `getNextMessageCount` caps the next visible
+    // count at the *total* message count it's given. A stale closure over
+    // the original 30-message array would wrongly cap the next count at 30
+    // (min(20 + 20, 30) = 30) even though 45 messages are now available -
+    // under-showing 10 messages the user should be able to see immediately.
+    const grownMessages = createMessages(45);
+    rerender({ messages: grownMessages, pageSize: 20 });
+
+    act(() => {
+      vi.advanceTimersByTime(500);
+    });
+
+    // Correct: min(20 + 20, 45) = 40, computed against the *current*
+    // message list at the time the timer fires - not the stale 30-message
+    // list captured when loadMore() was called.
+    expect(result.current.visibleMessages).toHaveLength(40);
+    expect(result.current.visibleMessages[39].id).toBe('45');
+  });
+
   it('isLoadingMore resets to false after loadMore completes', () => {
     const messages = createMessages(50);
     const { result } = renderHook(() => useChatPagination(messages, 20));

Dechat/dex_with_fiat_frontend/src/hooks/useChatPagination.ts

@@ -20,6 +20,31 @@ export const useChatPagination = (
   const [isLoadingMore, setIsLoadingMore] = useState(false);
   const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
 
+  // Root cause (#1224): `loadMore` (below) closes over `allMessages` and
+  // `pageSize` from the render in which it was created, then reads them
+  // again 400ms later inside a `setTimeout` callback. If new messages
+  // arrive, or the user switches sessions, while that timer is pending, the
+  // callback still computes the next visible count against the *stale*
+  // message list captured at call time - not the list that's actually on
+  // screen when the timer fires. That can under- or over-count how many
+  // messages should become visible after a session switch.
+  //
+  // Fixed by mirroring `allMessages`/`pageSize` into refs that are always
+  // kept current, and reading from the refs inside the timeout callback
+  // instead of the closed-over values - the same pattern already used for
+  // `addToastRef` in `usePaystackWebhookStatus` and `messagesRef` in
+  // `useChat`.
+  const allMessagesRef = useRef(allMessages);
+  const pageSizeRef = useRef(pageSize);
+
+  useEffect(() => {
+    allMessagesRef.current = allMessages;
+  }, [allMessages]);
+
+  useEffect(() => {
+    pageSizeRef.current = pageSize;
+  }, [pageSize]);
+
   // Reset visible count when session changes (if we had a way to detect it)
   // Actually, useChat will manage messages per session, so we just react to allMessages length decreasing
   // (which happens on new chat or session switch)
@@ -54,10 +79,12 @@ export const useChatPagination = (
 
     timerRef.current = setTimeout(() => {
       timerRef.current = null;
-      setVisibleCount((prev: number) => getNextMessageCount(allMessages, prev, pageSize));
+      setVisibleCount((prev: number) =>
+        getNextMessageCount(allMessagesRef.current, prev, pageSizeRef.current),
+      );
       setIsLoadingMore(false);
     }, 400);
-  }, [hasMore, isLoadingMore, allMessages, pageSize]);
+  }, [hasMore, isLoadingMore]);
 
   return {
     visibleMessages,

Dechat/dex_with_fiat_frontend/src/hooks/useMasking.test.ts

@@ -0,0 +1,79 @@
+import { describe, expect, it, vi } from 'vitest';
+import { renderHook } from '@testing-library/react';
+import { useMasking, SENSITIVE_TERMS_UPDATED_KEY } from './useMasking';
+
+describe('useMasking', () => {
+  it('returns unmasked text when disabled', () => {
+    const { result } = renderHook(() =>
+      useMasking('this is damn annoying', { enabled: false }),
+    );
+    expect(result.current).toBe('this is damn annoying');
+  });
+
+  it('masks sensitive terms when enabled', () => {
+    const { result } = renderHook(() =>
+      useMasking('this is damn annoying', { enabled: true }),
+    );
+    expect(result.current).not.toContain('damn');
+  });
+
+  it('regression: cleans up its storage listener on unmount (no leaked subscription)', () => {
+    const addSpy = vi.spyOn(window, 'addEventListener');
+    const removeSpy = vi.spyOn(window, 'removeEventListener');
+
+    const { unmount } = renderHook(() => useMasking('hello world', { enabled: true }));
+
+    const storageAddCalls = addSpy.mock.calls.filter(([type]) => type === 'storage').length;
+    expect(storageAddCalls).toBe(1);
+
+    unmount();
+
+    const storageRemoveCalls = removeSpy.mock.calls.filter(([type]) => type === 'storage').length;
+
+    // Before the fix, there was no cleanup function returned from the
+    // effect, so `removeEventListener('storage', ...)` was never called and
+    // this assertion would fail (0 !== 1) after unmount.
+    expect(storageRemoveCalls).toBe(1);
+
+    addSpy.mockRestore();
+    removeSpy.mockRestore();
+  });
+
+  it('regression: repeated mount/unmount cycles do not accumulate listeners', () => {
+    const addSpy = vi.spyOn(window, 'addEventListener');
+    const removeSpy = vi.spyOn(window, 'removeEventListener');
+
+    for (let i = 0; i < 5; i += 1) {
+      const { unmount } = renderHook(() => useMasking('hello world', { enabled: true }));
+      unmount();
+    }
+
+    const totalAdds = addSpy.mock.calls.filter(([type]) => type === 'storage').length;
+    const totalRemoves = removeSpy.mock.calls.filter(([type]) => type === 'storage').length;
+
+    expect(totalAdds).toBe(5);
+    expect(totalRemoves).toBe(5);
+
+    addSpy.mockRestore();
+    removeSpy.mockRestore();
+  });
+
+  it('rebuilds the masking manager when a sensitive-terms storage event fires', () => {
+    const { result, rerender } = renderHook(
+      ({ text }) => useMasking(text, { enabled: true }),
+      { initialProps: { text: 'this is damn annoying' } },
+    );
+
+    expect(result.current).not.toContain('damn');
+
+    // Simulate another tab/settings screen signalling that the sensitive
+    // terms configuration changed; this should not throw and the hook
+    // should keep functioning (manager rebuilt via refreshToken).
+    window.dispatchEvent(
+      new StorageEvent('storage', { key: SENSITIVE_TERMS_UPDATED_KEY }),
+    );
+
+    rerender({ text: 'this is damn annoying' });
+    expect(result.current).not.toContain('damn');
+  });
+});

Dechat/dex_with_fiat_frontend/src/hooks/useMasking.ts

@@ -4,7 +4,7 @@
 
 import { SensitiveTermsManager } from '@/lib/sensitiveTerms';
 import { MaskingStyle, maskText } from '@/lib/textMasking';
-import { useMemo } from 'react';
+import { useEffect, useMemo, useState } from 'react';
 
 export interface UseMaskingOptions {
   enabled: boolean;
@@ -13,26 +13,87 @@ export interface UseMaskingOptions {
 }
 
 /**
- * Hook to mask sensitive terms in text based on user preferences
+ * Storage key another part of the app (e.g. a compliance/settings screen
+ * that lets a user edit their sensitive-terms list) writes to when the
+ * masking configuration changes, so every open tab/component can refresh.
+ */
+export const SENSITIVE_TERMS_UPDATED_KEY = 'stellar_sensitive_terms_updated';
+
+/**
+ * Hook to mask sensitive terms in text based on user preferences.
+ *
+ * Root cause (#1222): every mounted instance of this hook needs to know
+ * when the sensitive-terms configuration changes elsewhere (another tab, a
+ * settings panel) so its memoized `manager`/`maskedText` can refresh. That
+ * requires listening for a `storage` event on `window`. Wiring that up
+ * without an unsubscribe path leaks one `storage` listener per mount - in a
+ * chat view that can render dozens of `<Message>` components, and where
+ * sessions/messages re-mount frequently, the listener count grows without
+ * bound and each stale listener keeps a reference to that render's closure
+ * alive, preventing it (and everything it captured) from being
+ * garbage-collected.
+ *
+ * Fixed by registering the listener inside a `useEffect` with a cleanup
+ * function that calls `removeEventListener` on unmount/re-subscribe, the
+ * same pattern used elsewhere in this codebase (see `addToastRef` cleanup
+ * in `usePaystackWebhookStatus`).
  */
 export const useMasking = (
   text: string,
   { enabled, style = 'asterisk', customTerms }: UseMaskingOptions,
 ) => {
+  // Bumped whenever we're told (via a `storage` event) that the
+  // sensitive-terms configuration changed elsewhere, forcing the memoized
+  // manager below to rebuild.
+  const [refreshToken, setRefreshToken] = useState(0);
+
+  useEffect(() => {
+    if (typeof window === 'undefined') {
+      return undefined;
+    }
+
+    const handleStorageChange = (event: StorageEvent) => {
+      if (event.key === SENSITIVE_TERMS_UPDATED_KEY) {
+        setRefreshToken((prev) => prev + 1);
+      }
+    };
+
+    window.addEventListener('storage', handleStorageChange);
+
+    // Cleanup: without this, every mount of a component using `useMasking`
+    // (e.g. one per chat message) leaves a dangling `storage` listener
+    // behind on unmount, leaking memory for the lifetime of the page.
+    return () => {
+      window.removeEventListener('storage', handleStorageChange);
+    };
+  }, []);
+
   // Create or use provided manager
   const manager = useMemo(() => {
     if (customTerms instanceof SensitiveTermsManager) {
       return customTerms;
     }
     return new SensitiveTermsManager();
-  }, [customTerms]);
+    // `refreshToken` intentionally triggers a rebuild when the sensitive
+    // terms configuration changes elsewhere; it does not affect the value
+    // itself.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [customTerms, refreshToken]);
 
   // Apply masking only if enabled
   const maskedText = useMemo(() => {
     if (!enabled) {
       return text;
     }
-    return maskText(text, manager, style);
+    try {
+      return maskText(text, manager, style);
+    } catch (error) {
+      // Fail safe rather than silently: log so the failure is visible to
+      // whoever is watching the console/error monitoring, and fall back to
+      // the original text instead of throwing and breaking the chat view.
+      console.error('useMasking: failed to mask sensitive text', error);
+      return text;
+    }
   }, [text, enabled, style, manager]);
 
   return maskedText;