From 6f21a6c364cc50d3fa055d33d01084ac77ec563b Mon Sep 17 00:00:00 2001 From: jkingsman Date: Thu, 9 Jul 2026 17:08:32 -0700 Subject: [PATCH] Maybe we actually fixed the flaky test this time --- frontend/src/hooks/useConversationRouter.ts | 30 ++++--- frontend/src/test/appStartupHash.test.tsx | 94 +++++++++++++-------- 2 files changed, 78 insertions(+), 46 deletions(-) diff --git a/frontend/src/hooks/useConversationRouter.ts b/frontend/src/hooks/useConversationRouter.ts index af2eaa1..552807f 100644 --- a/frontend/src/hooks/useConversationRouter.ts +++ b/frontend/src/hooks/useConversationRouter.ts @@ -17,6 +17,13 @@ import { getContactDisplayName } from '../utils/pubkey'; import { toast } from '../components/ui/sonner'; import type { Channel, Contact, Conversation } from '../types'; +function resolvePublicFromChannels(channels: Channel[]): Conversation | null { + const publicChannel = findPublicChannel(channels); + return publicChannel + ? { type: 'channel', id: publicChannel.key, name: publicChannel.name } + : null; +} + function resolveConversationFromHash( channels: Channel[], contacts: Contact[] @@ -98,15 +105,10 @@ export function useConversationRouter({ setActiveConversationState(conv); }, []); - const getPublicChannelConversation = useCallback((): Conversation | null => { - const publicChannel = findPublicChannel(channels); - if (!publicChannel) return null; - return { - type: 'channel', - id: publicChannel.key, - name: publicChannel.name, - }; - }, [channels]); + const getPublicChannelConversation = useCallback( + (): Conversation | null => resolvePublicFromChannels(channels), + [channels] + ); // Phase 1: Set initial conversation from URL hash or default to Public channel // Only needs channels (fast path) - doesn't wait for contacts @@ -304,7 +306,15 @@ export function useConversationRouter({ // Settings hash transitions are handled by useAppShell if (parseHashSettingsSection() !== null) return; - const conv = resolveConversationFromHash(channelsRef.current, contactsRef.current); + // Resolve the target from the hash. When the hash is empty or doesn't + // resolve (e.g. back/forward to the bare URL), fall back to the Public + // channel rather than clearing to null: the initial-load phases are gated + // by hasSetDefaultConversation and won't re-resolve, so a null here would + // strand the app on an empty view with no recovery. + const conv = + resolveConversationFromHash(channelsRef.current, contactsRef.current) ?? + resolvePublicFromChannels(channelsRef.current); + if (!conv) return; hashSyncEnabledRef.current = true; isHandlingPopstateRef.current = true; setActiveConversationState(conv); diff --git a/frontend/src/test/appStartupHash.test.tsx b/frontend/src/test/appStartupHash.test.tsx index c5b15d9..8a9a1bd 100644 --- a/frontend/src/test/appStartupHash.test.tsx +++ b/frontend/src/test/appStartupHash.test.tsx @@ -150,31 +150,28 @@ const publicChannel = { favorite: false, }; -// App startup fans out several async fetches (config, settings, channels, -// contacts, unreads) that must all settle before conversation selection -// renders. This suite passes in isolation, but under the full parallel suite -// (~70 files) CPU contention can stretch that startup well past RTL's 1000ms -// default — and occasionally past 10s on GitHub's shared runners — for any -// waitFor in this file. It's starvation, not a hang, so give this file generous -// headroom on both timeouts: a healthy render still settles in ~100ms (nothing -// is slowed), only a starved run waits longer. -// -// These MUST run at module scope, not in beforeAll: vitest bakes each test's -// timeout in when it() registers the test (during collection, before any -// beforeAll runs), so a beforeAll setConfig is too late and the test keeps the -// 5000ms default. configure() worked from beforeAll only because waitFor reads -// asyncUtilTimeout dynamically — which left an inverted pair (waitFor allowed -// 10s but the test killed at 5s) that flaked under load. vi.setConfig is scoped -// to this file (each file runs in its own isolated worker), so other files are -// unaffected. -vi.setConfig({ testTimeout: 45000 }); -configure({ asyncUtilTimeout: 30000 }); +// Modest timeout headroom for genuine CPU contention under the full parallel +// suite (a healthy render settles in ~100ms). Must run at module scope, not in +// beforeAll: vitest bakes each test's timeout in when it() registers during +// collection, before any beforeAll runs. vi.setConfig is scoped to this file. +vi.setConfig({ testTimeout: 15000 }); +configure({ asyncUtilTimeout: 5000 }); + +// Set the URL hash the way the app reads it, WITHOUT the jsdom side effect that +// makes this suite flaky. Assigning `window.location.hash = ...` queues an +// *asynchronous* popstate in jsdom (real browsers only fire hashchange); those +// stray popstates drain during a later test and reset the App's resolved +// conversation to null via the popstate handler, permanently stranding it. +// history.replaceState updates the hash without ever queuing a popstate. +function setHash(hash: string) { + window.history.replaceState(null, '', hash || window.location.pathname); +} describe('App startup hash resolution', () => { beforeEach(() => { vi.clearAllMocks(); localStorage.clear(); - window.location.hash = `#contact/${'a'.repeat(64)}/Alice`; + setHash(`#contact/${'a'.repeat(64)}/Alice`); mocks.api.getRadioConfig.mockResolvedValue({ public_key: 'aa'.repeat(32), @@ -201,10 +198,9 @@ describe('App startup hash resolution', () => { }); afterEach(() => { - // window.location.hash is intentionally NOT reset here: setting it fires a hashchange - // event while the component is still mounted (RTL cleanup runs after this describe-level - // hook), which queues a stale state update that races the next test's setup. beforeEach - // sets a known hash before each render, so this reset is redundant. + // Reset via replaceState (never queues a popstate) so no hash state leaks + // into the next test. beforeEach also sets a known hash before each render. + setHash(''); localStorage.clear(); }); @@ -219,7 +215,7 @@ describe('App startup hash resolution', () => { }); it('restores the trace tool from the URL hash', async () => { - window.location.hash = '#trace'; + setHash('#trace'); render(); @@ -231,7 +227,7 @@ describe('App startup hash resolution', () => { }); it('restores the trace tool from the URL hash even when channels are unavailable', async () => { - window.location.hash = '#trace'; + setHash('#trace'); mocks.api.getChannels.mockResolvedValue([]); render(); @@ -244,7 +240,7 @@ describe('App startup hash resolution', () => { }); it('reopens the last viewed trace tool even when channels are unavailable', async () => { - window.location.hash = ''; + setHash(''); localStorage.setItem(REOPEN_LAST_CONVERSATION_KEY, '1'); localStorage.setItem( LAST_VIEWED_CONVERSATION_KEY, @@ -275,7 +271,7 @@ describe('App startup hash resolution', () => { favorite: false, }; - window.location.hash = ''; + setHash(''); localStorage.setItem(REOPEN_LAST_CONVERSATION_KEY, '1'); localStorage.setItem( LAST_VIEWED_CONVERSATION_KEY, @@ -307,7 +303,7 @@ describe('App startup hash resolution', () => { favorite: false, }; - window.location.hash = ''; + setHash(''); localStorage.setItem( LAST_VIEWED_CONVERSATION_KEY, JSON.stringify({ @@ -338,7 +334,7 @@ describe('App startup hash resolution', () => { favorite: false, }; - window.location.hash = ''; + setHash(''); mocks.api.getChannels.mockResolvedValue([publicChannel, chatChannel]); localStorage.setItem( LAST_VIEWED_CONVERSATION_KEY, @@ -380,7 +376,7 @@ describe('App startup hash resolution', () => { first_seen: null, }; - window.location.hash = ''; + setHash(''); localStorage.setItem(REOPEN_LAST_CONVERSATION_KEY, '1'); localStorage.setItem( LAST_VIEWED_CONVERSATION_KEY, @@ -413,7 +409,7 @@ describe('App startup hash resolution', () => { favorite: false, }; - window.location.hash = `#channel/${opsChannel.key}/Ops`; + setHash(`#channel/${opsChannel.key}/Ops`); mocks.api.getChannels.mockResolvedValue([publicChannel, opsChannel]); render(); @@ -425,7 +421,7 @@ describe('App startup hash resolution', () => { }); act(() => { - window.location.hash = `#channel/${publicChannel.key}/Public`; + setHash(`#channel/${publicChannel.key}/Public`); window.dispatchEvent(new PopStateEvent('popstate', { state: null })); }); @@ -455,7 +451,7 @@ describe('App startup hash resolution', () => { first_seen: null, }; - window.location.hash = ''; + setHash(''); mocks.api.getContacts.mockResolvedValue([aliceContact]); render(); @@ -473,7 +469,7 @@ describe('App startup hash resolution', () => { await act(async () => {}); act(() => { - window.location.hash = `#contact/${aliceContact.public_key}/Alice`; + setHash(`#contact/${aliceContact.public_key}/Alice`); window.dispatchEvent(new PopStateEvent('popstate', { state: null })); }); @@ -483,10 +479,36 @@ describe('App startup hash resolution', () => { } }); }); + + it('recovers to Public when a popstate lands on an empty/unresolvable hash', async () => { + setHash(''); + render(); + + await waitFor(() => { + for (const node of screen.getAllByTestId('active-conversation')) { + expect(node).toHaveTextContent(`channel:${publicChannel.key}:Public`); + } + }); + + // A back/forward navigation to the bare URL (empty hash) must not strand + // the app on a blank conversation: the initial-load phases are gated by + // hasSetDefaultConversation and won't re-resolve, so the popstate handler + // itself has to fall back to Public rather than clearing to null. + act(() => { + setHash(''); + window.dispatchEvent(new PopStateEvent('popstate', { state: null })); + }); + + await waitFor(() => { + for (const node of screen.getAllByTestId('active-conversation')) { + expect(node).toHaveTextContent(`channel:${publicChannel.key}:Public`); + } + }); + }); }); it('stays on radio settings section even when radio is disconnected', async () => { - window.location.hash = '#settings/radio'; + setHash('#settings/radio'); mocks.api.getRadioConfig.mockRejectedValue(new Error('radio offline')); render();