diff --git a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte index be968421a..79fdaf34d 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte +++ b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte @@ -155,6 +155,12 @@ // that a `Set`-only filter let `updated_at DESC` order override the // ranking. let searchResultRank = $state | null>(null); + // The most matches one body search walks (BUG-3710), matching the + // server's list max (#2143). Never silent: a walk that stops here with + // more matches left sets `bodySearchCapped`, and the page says so, + // because a filter can then hide a match past the cap (Heron's ruling). + const BODY_SEARCH_MAX = 1000; + let bodySearchCapped = $state(false); let searchTimeout: ReturnType; /** @@ -973,6 +979,7 @@ // Typed input. searchQuery = ''; searchResultRank = null; + bodySearchCapped = false; quickCreateTitle = ''; saveViewName = ''; draftText = {}; @@ -2097,6 +2104,7 @@ const snapshotColl = collSlug; clearTimeout(searchTimeout); + bodySearchCapped = false; if (!trimmed) { searchResultRank = null; // null = no search active return; @@ -2126,6 +2134,11 @@ searchResultRank = null; const snapshotQuery = trimmed; const snapshotArchived = archivedInView; + // The server's /search page size. The walk stops at BODY_SEARCH_MAX, + // so a body search with more matches than that can still hide one + // the filters would keep: that is the bound, and the page shows a + // notice when it bites. The local path has none. + const BODY_SEARCH_PAGE = 200; // Fenced INSIDE the timer body, not at the effect: 200ms separates // the two, and a check at the effect says nothing about who is // signed in when this fires (BUG-3084). @@ -2149,36 +2162,52 @@ const epochAtSchedule = untrack(() => captureIdentity()); searchTimeout = setTimeout(async () => { if (!identityHeld(epochAtSchedule)) return; + // Stale-response guard: drop the result if the user + // navigated away or changed the query while a page was + // in flight, or a newer run of this effect armed its own + // timer (same query, so the snapshots alone cannot tell; + // a multi-page walk is long enough to be overtaken). + const myTimer = searchTimeout; + const moved = () => + searchTimeout !== myTimer || + searchQuery.trim() !== snapshotQuery || + wsSlug !== snapshotWs || + collSlug !== snapshotColl || + (showArchived || parsedSearch.archived) !== snapshotArchived; try { - const resp = await api.search(bodyQuery, { - workspace: snapshotWs, - collection: snapshotColl, - limit: 200, - includeArchived: snapshotArchived, - }); - // Stale-response guard: drop the result if the user - // navigated away or changed the query while the - // request was in flight. - if ( - searchQuery.trim() !== snapshotQuery || - wsSlug !== snapshotWs || - collSlug !== snapshotColl || - (showArchived || parsedSearch.archived) !== snapshotArchived - ) { - return; + // Every match, page by page, not the first 200 (BUG-3710): + // the active filters apply in `filteredItems`, AFTER this + // rank map, so a capped map hid a match that sat past the + // cap behind ones the filters then removed. The server + // pages at 200; BODY_SEARCH_MAX bounds the walk. + const ranked: string[] = []; + let capped = false; + for (;;) { + const resp = await api.search(bodyQuery, { + workspace: snapshotWs, + collection: snapshotColl, + limit: BODY_SEARCH_PAGE, + offset: ranked.length, + includeArchived: snapshotArchived, + }); + if (moved() || !identityHeld(epochAtSchedule)) return; + const page = resp.results ?? []; + for (const r of page) ranked.push(r.item.id); + // Stopped by the max with more matches left: a full last + // page, and no total saying this was all of them. + if (ranked.length >= BODY_SEARCH_MAX && page.length >= BODY_SEARCH_PAGE && ranked.length < (resp.total ?? Infinity)) { + capped = true; + break; + } + if (page.length < BODY_SEARCH_PAGE || ranked.length >= (resp.total ?? 0) || ranked.length >= BODY_SEARCH_MAX) break; } - if (!identityHeld(epochAtSchedule)) return; - searchResultRank = new Map( - resp.results.map((r, i) => [r.item.id, i]), - ); + searchResultRank = new Map(ranked.map((id, i) => [id, i])); + bodySearchCapped = capped; } catch { - if ( - identityHeld(epochAtSchedule) && - searchQuery.trim() === snapshotQuery && - wsSlug === snapshotWs && - collSlug === snapshotColl && - (showArchived || parsedSearch.archived) === snapshotArchived - ) { + // The same fence as the success arm, including the timer: + // an overtaken walk whose page fails must not clear the + // newer run's results (BUG-3710, codex r2). + if (identityHeld(epochAtSchedule) && !moved()) { searchResultRank = null; } } @@ -2200,10 +2229,14 @@ // the `coll:` / `is:archived` / `#N` / ref vocabulary works // uniformly across consumers (collection page + CommandPalette, // TASK-1367). + // No cap (BUG-3710): the active filters apply in `filteredItems`, + // AFTER this rank map, so a capped map hid every match past the cap, + // including ones the filters would keep. MiniSearch already returns + // every match; the limit only truncated them. const hits = localSearch.search(snapshotWs, trimmed, { collection: snapshotColl, includeArchived: archivedInView, - limit: 200, + limit: Infinity, }); searchResultRank = new Map(hits.map((h, i) => [h.id, i])); }); @@ -4390,6 +4423,12 @@ {/if} + {#if bodySearchCapped && searchResultRank !== null} +

+ Showing the first {BODY_SEARCH_MAX} matches; refine your search. +

+ {/if} + @@ -5213,6 +5252,12 @@ background: var(--accent-primary, var(--accent-blue)); } + .search-cap-notice { + margin: 0 0 var(--space-2); + font-size: 0.8125rem; + color: var(--text-muted); + } + .filters-panel { padding: var(--space-3) 0; } diff --git a/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.svelte.test.ts b/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.svelte.test.ts index eb17ddadd..2a03d6e32 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.svelte.test.ts +++ b/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.svelte.test.ts @@ -322,6 +322,7 @@ vi.mock('$lib/stores/collections.svelte', () => ({ import { page } from '$app/state'; import { goto } from '$app/navigation'; import { api } from '$lib/api/client'; +import { localSearch } from '$lib/stores/localSearch.svelte'; import CollectionPage from './+page.svelte'; type Props = Record; @@ -890,3 +891,211 @@ describe('the collection page stops a commit when the identity moves mid-flight' ).toBe(0); }); }); + +// BUG-3710: the search rank map is applied in `filteredItems` AFTER the active +// filters, so a map capped at 200 hid every match past the cap, including one +// the filters would keep (200 done items outranking the one open item a status +// filter asks for). Both search paths now hand the page every match. Observed +// at the page's bindings: the server walk at api.search, the local path at +// localSearch.search. +describe('the collection page searches every match, not the first 200 (BUG-3710)', () => { + beforeEach(() => { + auth.resetEpoch(); + }); + afterEach(() => { + cleanup(); + vi.useRealTimers(); + vi.mocked(api.search).mockReset(); + vi.mocked(api.search).mockImplementation(async () => ({ results: [] }) as never); + }); + + async function searchBox() { + await mountPage(); + searchOpeners[0]!(); + await tick(); + return waitFor(() => { + const fn = findProp<(q: string) => void>('onSearchChange'); + if (!fn) throw new Error('filter bar not rendered yet'); + return fn; + }); + } + + it('the body search walks every page of matches', async () => { + const onSearchChange = await searchBox(); + const page = (from: number, n: number) => ({ + results: Array.from({ length: n }, (_, i) => ({ item: { id: `m${from + i}` } })), + total: 201, + }); + vi.mocked(api.search).mockReset(); + vi.mocked(api.search) + .mockResolvedValueOnce(page(0, 200) as never) + .mockResolvedValueOnce(page(200, 1) as never); + vi.useFakeTimers(); + onSearchChange('body:foo'); + await vi.advanceTimersByTimeAsync(250); + const calls = vi.mocked(api.search).mock.calls as unknown as Array<[string, { offset?: number; limit?: number }]>; + expect(calls.map(([, f]) => [f.offset ?? 0, f.limit]), 'the walk stopped at the first page').toEqual([ + [0, 200], + [200, 200], + ]); + }); + + it('control: a body search whose first page holds every match asks once', async () => { + const onSearchChange = await searchBox(); + vi.mocked(api.search).mockReset(); + vi.mocked(api.search).mockResolvedValueOnce({ results: [{ item: { id: 'only' } }], total: 1 } as never); + vi.useFakeTimers(); + onSearchChange('body:foo'); + await vi.advanceTimersByTimeAsync(250); + expect(vi.mocked(api.search).mock.calls.length).toBe(1); + }); + + // Heron's ruling on BUG-3710: the 1000 cap stays, but never silently. + const capPage = (from: number, n: number, total: number | undefined) => ({ + results: Array.from({ length: n }, (_, i) => ({ item: { id: `m${from + i}` } })), + total, + }); + const capNotice = () => document.querySelector('.search-cap-notice'); + + it('a body search stopped by the 1000 cap says so', async () => { + const onSearchChange = await searchBox(); + vi.mocked(api.search).mockReset(); + vi.mocked(api.search).mockImplementation((async (_q: string, f: { offset?: number }) => + capPage(f.offset ?? 0, 200, 1500)) as never); + vi.useFakeTimers(); + onSearchChange('body:foo'); + await vi.advanceTimersByTimeAsync(250); + expect(vi.mocked(api.search).mock.calls.length, 'the walk did not stop at the cap').toBe(5); + expect(capNotice()?.textContent, 'a capped search showed no notice').toMatch(/Showing the first 1000 matches; refine your search/); + // A new search that is not capped takes the notice down. + vi.mocked(api.search).mockReset(); + vi.mocked(api.search).mockResolvedValueOnce(capPage(0, 1, 1) as never); + onSearchChange('body:bar'); + await vi.advanceTimersByTimeAsync(250); + expect(capNotice(), 'the notice outlived the capped search').toBeNull(); + }); + + it('a capped body search followed by a local search drops the notice', async () => { + const spy = vi.spyOn(localSearch, 'search'); + try { + const onSearchChange = await searchBox(); + vi.mocked(api.search).mockReset(); + vi.mocked(api.search).mockImplementation((async (_q: string, f: { offset?: number }) => + capPage(f.offset ?? 0, 200, 1500)) as never); + vi.useFakeTimers(); + onSearchChange('body:foo'); + await vi.advanceTimersByTimeAsync(250); + expect(capNotice(), 'premise: the body search was capped').not.toBeNull(); + spy.mockClear(); + onSearchChange('foo'); + await vi.advanceTimersByTimeAsync(10); + expect(spy, 'premise: the local path ran').toHaveBeenCalled(); + expect(capNotice(), 'the body search notice stayed on a local search').toBeNull(); + } finally { + spy.mockRestore(); + } + }); + + it('a capped body search followed by a failed one drops the notice', async () => { + const onSearchChange = await searchBox(); + vi.mocked(api.search).mockReset(); + vi.mocked(api.search).mockImplementation((async (_q: string, f: { offset?: number }) => + capPage(f.offset ?? 0, 200, 1500)) as never); + vi.useFakeTimers(); + onSearchChange('body:foo'); + await vi.advanceTimersByTimeAsync(250); + expect(capNotice(), 'premise: the body search was capped').not.toBeNull(); + vi.mocked(api.search).mockReset(); + vi.mocked(api.search).mockRejectedValueOnce(new Error('network') as never); + onSearchChange('body:bar'); + await vi.advanceTimersByTimeAsync(250); + expect(vi.mocked(api.search).mock.calls.length, 'premise: the failing search ran').toBe(1); + expect(capNotice(), 'a failed search kept the previous notice').toBeNull(); + }); + + it('control: exactly 1000 matches, all walked, shows no notice', async () => { + const onSearchChange = await searchBox(); + vi.mocked(api.search).mockReset(); + vi.mocked(api.search).mockImplementation((async (_q: string, f: { offset?: number }) => + capPage(f.offset ?? 0, 200, 1000)) as never); + vi.useFakeTimers(); + onSearchChange('body:foo'); + await vi.advanceTimersByTimeAsync(250); + expect(vi.mocked(api.search).mock.calls.length).toBe(5); + expect(capNotice(), 'every match was shown, yet the page said it was capped').toBeNull(); + }); + + it('control: a short walk shows no notice', async () => { + const onSearchChange = await searchBox(); + vi.mocked(api.search).mockReset(); + vi.mocked(api.search) + .mockResolvedValueOnce(capPage(0, 200, 201) as never) + .mockResolvedValueOnce(capPage(200, 1, 201) as never); + vi.useFakeTimers(); + onSearchChange('body:foo'); + await vi.advanceTimersByTimeAsync(250); + expect(vi.mocked(api.search).mock.calls.length).toBe(2); + expect(capNotice()).toBeNull(); + }); + + it('a walk overtaken by a newer run of the same search does not commit', async () => { + const onSearchChange = await searchBox(); + let releaseOld!: (v: unknown) => void; + vi.mocked(api.search).mockReset(); + vi.mocked(api.search) + .mockImplementationOnce(() => new Promise((r) => (releaseOld = r)) as never) + .mockResolvedValueOnce({ results: [{ item: { id: 'i1' } }], total: 1 } as never); + vi.useFakeTimers(); + onSearchChange('body:foo'); + await vi.advanceTimersByTimeAsync(250); // the old walk is in flight + onSearchChange('body:foo '); // same trimmed query: same snapshots + await vi.advanceTimersByTimeAsync(250); // the new walk commits i1 + const shown = () => + ((stubProps().find((p) => Array.isArray((p as Record).items)) as Record | undefined) + ?.items as Array<{ id: string }> | undefined)?.map((i) => i.id); + expect(shown(), 'premise: the newer walk committed').toEqual(['i1']); + releaseOld({ results: [], total: 0 }); + await vi.advanceTimersByTimeAsync(10); + expect(shown(), 'the overtaken walk overwrote the newer result').toEqual(['i1']); + }); + + it('a walk overtaken by a newer run does not clear its results when its page fails', async () => { + const onSearchChange = await searchBox(); + let failOld!: (e: unknown) => void; + vi.mocked(api.search).mockReset(); + vi.mocked(api.search) + .mockImplementationOnce(() => new Promise((_, r) => (failOld = r)) as never) + .mockResolvedValueOnce({ results: [{ item: { id: 'i1' } }], total: 1 } as never); + vi.useFakeTimers(); + onSearchChange('body:foo'); + await vi.advanceTimersByTimeAsync(250); + onSearchChange('body:foo '); + await vi.advanceTimersByTimeAsync(250); + const shown = () => + ((stubProps().find((p) => Array.isArray((p as Record).items)) as Record | undefined) + ?.items as Array<{ id: string }> | undefined)?.map((i) => i.id); + expect(shown(), 'premise: the newer walk committed').toEqual(['i1']); + failOld(new Error('network')); + await vi.advanceTimersByTimeAsync(10); + // A cleared rank map falls back to the substring scan, which "body:foo" + // does not match on the row's title, so a clear shows up as no items. + expect(shown(), 'the overtaken walk cleared the newer result').toEqual(['i1']); + }); + + it('the local search is not capped', async () => { + const spy = vi.spyOn(localSearch, 'search'); + try { + const onSearchChange = await searchBox(); + onSearchChange('foo'); + await tick(); + await waitFor(() => { + if (!spy.mock.calls.length) throw new Error('local search not run yet'); + }); + for (const [, , opts] of spy.mock.calls as unknown as Array<[string, string, { limit?: number }]>) { + expect(opts.limit, 'the local search was capped').toBe(Infinity); + } + } finally { + spy.mockRestore(); + } + }); +}); diff --git a/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityGate.test.ts b/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityGate.test.ts index 1326da64b..573e6b36c 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityGate.test.ts +++ b/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityGate.test.ts @@ -47,7 +47,7 @@ identityGateSuite({ nested: [ { body: /event\.type === 'collection_updated'/, why: "SSE item events: pageIdentityHeld() at entry; the refresh arm checks collGen, the slug, then identityHeld(epochAtEntry) before its write", reviewed: '7b0ae5504172' }, { body: /result\.workspace !== wsSlug/, why: 'sync results: pageIdentityHeld() at entry, identityHeld(epochAtEntry) after the progress refresh', reviewed: 'c75a0156ba81' }, - { body: /resp\.results\.map/, why: 'search timer, as an async function: identityHeld(epochAtSchedule) at fire time and after the search, with the query, route and archived-flag snapshots (TASK-2864)', reviewed: 'ed922fb58cfd' }, + { body: /BODY_SEARCH_PAGE/, why: 'search timer, as an async function: identityHeld(epochAtSchedule) at fire time and after EVERY page of the search, with the query, route and archived-flag snapshots (TASK-2864) and the timer it fired under (a newer run of the effect supersedes it), on the success AND catch arms; one commit after the walk (BUG-3710); the cap flag commits beside the rank map under the same fence', reviewed: '0025dd6fffa1' }, ], markup: [], continuations: [ @@ -56,7 +56,7 @@ identityGateSuite({ { call: /api\.views\.list\(ws, coll\)\.catch\($/, body: /./, why: 'loadCollection views fetch: a failure reads as none; commits nothing', reviewed: '2a319a915b32' }, { call: /handleStatusChange\(item, value, 'priority'\)\.catch\($/, body: /./, why: 'card priority write (TASK-2214): the rejection was already toasted inside handleStatusChange, which fences its own commits; the catch commits nothing', reviewed: '8405d65b80af' }, { call: /api\.members\.list\(ws\)\.catch\($/, body: /./, why: 'loadCollection members fetch: a failure reads as none; commits nothing', reviewed: 'cfb203c9497b' }, - { call: /^setTimeout\($/, body: /resp\.results\.map/, why: 'search timer, as the deferred callback: the same unit as the nested row above', reviewed: 'ed922fb58cfd' }, + { call: /^setTimeout\($/, body: /BODY_SEARCH_PAGE/, why: 'search timer, as the deferred callback: the same unit as the nested row above', reviewed: '0025dd6fffa1' }, { call: /^setTimeout\($/, body: /identityHeld\(epochAtSchedule\)\) return; bypassNavGuard/, why: 'leave-guard popstate reset: identityHeld(epochAtSchedule) before clearing bypassNavGuard', reviewed: 'b3974e1c39d9' }, { call: /^goto\(url\)\.finally\($/,