diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index 966e196..9231f9b 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -44,6 +44,8 @@ frontend/src/ │ └── PushSubscriptionContext.tsx # Push subscription state context/provider ├── lib/ │ └── utils.ts # cn() — clsx + tailwind-merge helper +├── stores/ +│ └── rawPacketStore.ts # Overheard packet stream + session stats, outside React ├── hooks/ │ ├── index.ts # Central re-export of all hooks │ ├── useConversationActions.ts # Send/resend/trace/block conversation actions @@ -60,7 +62,6 @@ frontend/src/ │ ├── useBrowserNotifications.ts # Per-conversation browser notification preferences + dispatch │ ├── usePushSubscription.ts # Web Push subscription lifecycle, per-conversation filters │ ├── useFaviconBadge.ts # Browser tab unread badge state -│ ├── useRawPacketStatsSession.ts # Session-scoped packet-feed stats history │ └── useRememberedServerPassword.ts # Browser-local repeater/room password persistence ├── components/ │ ├── AppShell.tsx # App-shell layout: status, sidebar, search/settings panes, cracker, modals, security warning @@ -249,6 +250,10 @@ High-level state is delegated to hooks: `App.tsx` intentionally still does the final `AppShell` prop assembly. That composition layer is considered acceptable here because it keeps the shell contract visible in one place and avoids a prop-bundling hook with little original logic. +**The overheard packet stream is the one piece of app state that deliberately does not live in React.** It is held in `stores/rawPacketStore.ts` and read through `useSyncExternalStore`, because it updates several times a second with every packet the node hears — far more often than anything else — and only four surfaces consume it (`MapView`, `VisualizerView`, `RawPacketFeedView`, `CrackerPanel`). Held in `App` state it re-rendered the entire tree, including `MessageList`, which is neither memoized nor cheap on a long history. + +That gives the store a load-bearing invariant: **no ancestor of `MessageList` may call `useRawPackets()` / `useRawPacketStatsSession()`.** Nothing about the prop signatures enforces it — an innocuous-looking subscription added to `App`, `AppShell`, or `ConversationPane` silently restores the original slowdown. `src/test/appPacketIsolation.test.tsx` pins it by mounting the real ancestor chain and asserting `MessageList` does not re-render when packets arrive; it carries a negative control so the assertion cannot pass vacuously. Reach for packets in a new view by subscribing in that view, never by lifting them up. + `ConversationPane.tsx` owns the main active-conversation surface branching: - empty state - map view diff --git a/frontend/src/hooks/useRealtimeAppState.ts b/frontend/src/hooks/useRealtimeAppState.ts index e37034e..f66d428 100644 --- a/frontend/src/hooks/useRealtimeAppState.ts +++ b/frontend/src/hooks/useRealtimeAppState.ts @@ -11,7 +11,7 @@ import { toast } from '../components/ui/sonner'; import { getStateKey } from '../utils/conversationState'; import { mergeContactIntoList } from '../utils/contactMerge'; import { getContactDisplayName } from '../utils/pubkey'; -import { clearRawPackets, recordRawPacket } from '../stores/rawPacketStore'; +import { clearRawPackets, MAX_RAW_PACKETS, recordRawPacket } from '../stores/rawPacketStore'; import { emitStatusDotPulse } from '../utils/statusDotPulse'; import type { Channel, @@ -57,6 +57,7 @@ interface UseRealtimeAppStateArgs { packetId?: number | null ) => void; notifyIncomingMessage?: (msg: Message) => void; + /** Buffer cap override. Defaults to the store's own cap; tests use it to force eviction. */ maxRawPackets?: number; } @@ -105,7 +106,7 @@ export function useRealtimeAppState({ removeConversationMessages, receiveMessageAck, notifyIncomingMessage, - maxRawPackets = 500, + maxRawPackets = MAX_RAW_PACKETS, }: UseRealtimeAppStateArgs): UseWebSocketOptions { const mergeChannelIntoList = useCallback( (updated: Channel) => { diff --git a/frontend/src/stores/rawPacketStore.ts b/frontend/src/stores/rawPacketStore.ts index cee4c3f..0cdc18c 100644 --- a/frontend/src/stores/rawPacketStore.ts +++ b/frontend/src/stores/rawPacketStore.ts @@ -118,11 +118,19 @@ export function seedRawPacketStore(next: { packets?: RawPacket[]; statsSession?: RawPacketStatsSessionState; }): void { + // Copy rather than alias. The whole store rests on "a snapshot is immutable, and its + // identity changes only when its contents do". Holding the caller's array would let + // them mutate the live snapshot in place, and because useSyncExternalStore compares + // snapshots with Object.is, React would then bail out of every subsequent render — + // leaving the UI permanently disagreeing with getRawPackets() and no way to tell why. if (next.packets) { - packets = next.packets; + packets = [...next.packets]; } if (next.statsSession) { - statsSession = next.statsSession; + statsSession = { + ...next.statsSession, + observations: [...next.statsSession.observations], + }; } emit(); } diff --git a/frontend/src/test/appPacketIsolation.test.tsx b/frontend/src/test/appPacketIsolation.test.tsx new file mode 100644 index 0000000..320cdaa --- /dev/null +++ b/frontend/src/test/appPacketIsolation.test.tsx @@ -0,0 +1,239 @@ +/** + * The invariant this store exists to protect: nothing on the chat render path may + * subscribe to the raw packet stream. + * + * The original bug was not that the chat view read packets — it never did. It was that + * the stream lived in `App` state, and `App` is an *ancestor* of `MessageList`. Nothing + * on that path is memoized, so every overheard packet re-rendered the whole message list + * regardless of which props it actually received. + * + * That means the regression can only be caught by mounting the real ancestor chain + * (`App` → `AppShell` → `ConversationPane` → `MessageList`). A test that renders + * `ConversationPane` on its own cannot see it: the offending subscription lives above. + */ +import React from 'react'; +import { act, render, screen, waitFor } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + messageList: vi.fn(() =>
), + api: { + getRadioConfig: vi.fn(), + getSettings: vi.fn(), + getUndecryptedPacketCount: vi.fn(), + getChannels: vi.fn(), + getContacts: vi.fn(), + getHealth: vi.fn(), + }, + hookFns: { + observeMessage: vi.fn(() => ({ added: false, activeConversation: false })), + refreshUnreads: vi.fn(async () => {}), + }, +})); + +vi.mock('../api', () => ({ api: mocks.api })); + +vi.mock('../useWebSocket', () => ({ useWebSocket: vi.fn() })); + +vi.mock('../contexts/PushSubscriptionContext', () => ({ + usePush: () => ({ + isSupported: false, + isSubscribed: false, + currentSubscriptionId: null, + allSubscriptions: [], + pushConversations: [], + loading: false, + subscribe: vi.fn(async () => null), + unsubscribe: vi.fn(async () => {}), + toggleConversation: vi.fn(async () => {}), + isConversationPushEnabled: () => false, + deleteSubscription: vi.fn(async () => {}), + testPush: vi.fn(async () => {}), + refreshSubscriptions: vi.fn(async () => []), + refreshConversations: vi.fn(async () => {}), + }), +})); + +vi.mock('../hooks', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useConversationMessages: () => ({ + messages: [], + messagesLoading: false, + loadingOlder: false, + hasOlderMessages: false, + hasNewerMessages: false, + loadingNewer: false, + fetchOlderMessages: vi.fn(async () => {}), + fetchNewerMessages: vi.fn(async () => {}), + jumpToBottom: vi.fn(), + reloadCurrentConversation: vi.fn(), + observeMessage: mocks.hookFns.observeMessage, + receiveMessageAck: vi.fn(), + reconcileOnReconnect: vi.fn(), + renameConversationMessages: vi.fn(), + removeConversationMessages: vi.fn(), + clearConversationMessages: vi.fn(), + }), + useUnreadCounts: () => ({ + unreadCounts: {}, + mentions: {}, + lastMessageTimes: {}, + unreadLastReadAts: {}, + recordMessageEvent: vi.fn(), + renameConversationState: vi.fn(), + removeConversationState: vi.fn(), + markAllRead: vi.fn(), + refreshUnreads: mocks.hookFns.refreshUnreads, + }), + }; +}); + +// Mocked to keep the tree small and deterministic. None of them are mounted while a +// chat conversation is active, so removing them cannot mask a chat-path subscription. +vi.mock('../components/StatusBar', () => ({ StatusBar: () =>
})); +vi.mock('../components/Sidebar', () => ({ Sidebar: () =>
})); +vi.mock('../components/MessageList', () => ({ MessageList: mocks.messageList })); +vi.mock('../components/MessageInput', () => ({ + MessageInput: React.forwardRef((_props, ref) => { + React.useImperativeHandle(ref, () => ({ appendText: vi.fn(), focus: vi.fn() })); + return
; + }), +})); +vi.mock('../components/NewMessageModal', () => ({ NewMessageModal: () => null })); +vi.mock('../components/SettingsModal', () => ({ + SettingsModal: () => null, + SETTINGS_SECTION_ORDER: ['radio'], + SETTINGS_SECTION_LABELS: { radio: 'Radio' }, +})); +vi.mock('../components/MapView', () => ({ MapView: () => null })); +vi.mock('../components/VisualizerView', () => ({ VisualizerView: () => null })); +vi.mock('../components/CrackerPanel', () => ({ CrackerPanel: () => null })); +vi.mock('../components/ui/sonner', () => ({ + Toaster: () => null, + toast: { success: vi.fn(), error: vi.fn() }, +})); +vi.mock('../utils/urlHash', () => ({ + parseHashConversation: () => null, + parseHashSettingsSection: () => null, + updateUrlHash: vi.fn(), + pushUrlHash: vi.fn(), + updateSettingsHash: vi.fn(), + pushSettingsHash: vi.fn(), + getSettingsHash: (section: string) => `#settings/${section}`, + getMapFocusHash: () => '#map', +})); + +import { App } from '../App'; +import { + getRawPackets, + recordRawPacket, + resetRawPacketStore, + useRawPackets, +} from '../stores/rawPacketStore'; +import type { RawPacket } from '../types'; + +function createPacket(overrides: Partial = {}): RawPacket { + return { + id: 1, + observation_id: 1, + timestamp: 1700000000, + data: 'aabb', + payload_type: 'GROUP_TEXT', + snr: 7.5, + rssi: -80, + decrypted: false, + decrypted_info: null, + ...overrides, + }; +} + +const publicChannel = { + key: '8B3387E9C5CDEA6AC9E5EDBAA115CD72', + name: 'Public', + is_hashtag: false, + on_radio: false, + last_read_at: null, + favorite: false, + muted: false, +}; + +describe('overheard packets and the chat render path', () => { + beforeEach(() => { + vi.clearAllMocks(); + resetRawPacketStore(); + mocks.api.getRadioConfig.mockResolvedValue({ + public_key: 'aa'.repeat(32), + name: 'TestNode', + lat: 0, + lon: 0, + tx_power: 17, + max_tx_power: 22, + radio: { freq: 910.525, bw: 62.5, sf: 7, cr: 5 }, + path_hash_mode: 0, + path_hash_mode_supported: false, + }); + mocks.api.getSettings.mockResolvedValue({ + max_radio_contacts: 200, + auto_decrypt_dm_on_advert: false, + last_message_times: {}, + advert_interval: 0, + last_advert_time: 0, + flood_scope: '', + known_regions: [], + blocked_keys: [], + blocked_names: [], + }); + mocks.api.getUndecryptedPacketCount.mockResolvedValue({ count: 0 }); + mocks.api.getChannels.mockResolvedValue([publicChannel]); + mocks.api.getContacts.mockResolvedValue([]); + mocks.api.getHealth.mockResolvedValue(null); + }); + + it('does not re-render the message list when packets arrive', async () => { + render(); + await waitFor(() => { + expect(screen.getByTestId('message-list')).toBeInTheDocument(); + }); + + const rendersBefore = mocks.messageList.mock.calls.length; + act(() => { + for (let i = 1; i <= 25; i++) { + recordRawPacket(createPacket({ id: i, observation_id: i })); + } + }); + + // Guards the assertion below against passing for the wrong reason + expect(getRawPackets()).toHaveLength(25); + expect(mocks.messageList.mock.calls.length).toBe(rendersBefore); + }); + + it('re-renders the message list once per batch if an ancestor subscribes', async () => { + // Negative control. Proves the assertion above can actually fail — without this, a + // render counter that never increments would look identical to a passing test. + // + // The ancestor has to create the element inside its own render. Taking it as + // a `children` prop would defeat the point: that element is built once by the caller, + // so its identity never changes and React bails out of the subtree — the test would + // pass while proving nothing. + function SubscribingAncestor() { + useRawPackets(); + return ; + } + + render(); + await waitFor(() => { + expect(screen.getByTestId('message-list')).toBeInTheDocument(); + }); + + const rendersBefore = mocks.messageList.mock.calls.length; + act(() => { + for (let i = 1; i <= 25; i++) { + recordRawPacket(createPacket({ id: i, observation_id: i })); + } + }); + + expect(mocks.messageList.mock.calls.length).toBeGreaterThan(rendersBefore); + }); +}); diff --git a/frontend/src/test/rawPacketStore.test.tsx b/frontend/src/test/rawPacketStore.test.tsx index 64ef5f6..a7e4139 100644 --- a/frontend/src/test/rawPacketStore.test.tsx +++ b/frontend/src/test/rawPacketStore.test.tsx @@ -8,8 +8,10 @@ import { getRawPackets, recordRawPacket, resetRawPacketStore, + seedRawPacketStore, useRawPackets, } from '../stores/rawPacketStore'; +import { MAX_RAW_PACKET_STATS_OBSERVATIONS } from '../utils/rawPacketStats'; import type { Channel, Contact, @@ -48,6 +50,28 @@ function createPacket(overrides: Partial = {}): RawPacket { }; } +/** A stats session already holding `count` distinct observations, for trim-boundary tests. */ +function sessionAtObservationCap(count: number): RawPacketStatsSessionState { + return { + sessionStartedAt: 1700000000000, + totalObservedPackets: count, + trimmedObservationCount: 0, + observations: Array.from({ length: count }, (_, i) => ({ + observationKey: `seeded-${i}`, + timestamp: 1700000000 + i, + payloadType: 'GROUP_TEXT', + routeType: 'Flood', + decrypted: false, + rssi: null, + snr: null, + sourceKey: null, + sourceLabel: null, + pathTokenCount: 0, + pathSignature: null, + })), + }; +} + const channel: Channel = { key: '8B3387E9C5CDEA6AC9E5EDBAA115CD72', name: 'Public', @@ -204,6 +228,69 @@ describe('rawPacketStore', () => { expect(screen.getByTestId('count').textContent).toBe('1'); }); + /** + * Asserted through a mounted subscriber rather than getRawPackets(), because the + * failure mode is specifically a missing emit(): the module state would be correct + * while every view kept rendering packets that no longer exist. On a quiet mesh the + * next packet — and so the next repaint — can be minutes away. + */ + it('notifies subscribed views when the buffer is cleared on reconnect', () => { + function PacketCount() { + return {useRawPackets().length}; + } + act(() => recordRawPacket(createPacket({ id: 1, observation_id: 1 }))); + render(); + expect(screen.getByTestId('count').textContent).toBe('1'); + + act(() => clearRawPackets()); + + expect(screen.getByTestId('count').textContent).toBe('0'); + }); + + it('does not hand out a snapshot the seeding caller can still mutate', () => { + const fixture = [createPacket({ id: 1, observation_id: 1 })]; + seedRawPacketStore({ packets: fixture }); + + fixture.push(createPacket({ id: 2, observation_id: 2 })); + + // Aliasing the caller's array would mutate the live snapshot in place. Because + // useSyncExternalStore compares snapshots with Object.is, the identity would not + // change and React would bail out of every later render for good. + expect(getRawPackets()).toHaveLength(1); + }); + + /** + * MAX_RAW_PACKET_STATS_OBSERVATIONS is 20k, far past what a test can reach by + * recording, so the trim boundary is exercised by seeding a session that already + * sits on it. Without this, both the `<=` comparison and the trimmed-count + * arithmetic can be broken without any test noticing. + */ + it('retains exactly the observation cap before trimming starts', () => { + seedRawPacketStore({ + statsSession: sessionAtObservationCap(MAX_RAW_PACKET_STATS_OBSERVATIONS - 1), + }); + + recordRawPacket(createPacket({ id: 999999, observation_id: 999999 })); + + const session = getRawPacketStatsSession(); + expect(session.observations).toHaveLength(MAX_RAW_PACKET_STATS_OBSERVATIONS); + expect(session.trimmedObservationCount).toBe(0); + }); + + it('trims the oldest observation once the cap is exceeded', () => { + seedRawPacketStore({ + statsSession: sessionAtObservationCap(MAX_RAW_PACKET_STATS_OBSERVATIONS), + }); + + recordRawPacket(createPacket({ id: 999999, observation_id: 999999 })); + + const session = getRawPacketStatsSession(); + expect(session.observations).toHaveLength(MAX_RAW_PACKET_STATS_OBSERVATIONS); + expect(session.trimmedObservationCount).toBe(1); + // The evicted entry is the oldest, not the newest + expect(session.observations[0].observationKey).not.toBe('seeded-0'); + }); + /** * The reason this store exists: overheard traffic used to live in App state, so every * packet re-rendered the whole message list. That cost scales with history length and