From 69fbbe6453ec7c174a3729e198ca0b116eda61db Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 14 Oct 2026 05:41:18 -0400 Subject: [PATCH] fix(web): the collection page's search ranks every match, so a filter cannot hide one past the cap (BUG-3710) (#2149) * fix(web): the collection page's search ranks every match, so a filter cannot hide one past the cap (BUG-3710) The collection page turns a search into a rank map, and filteredItems applies the active filters (status, tags, parent, fields) to the collection and THEN keeps only the items in that map. Both search paths capped the map at 200: the local path asked localSearch for 200, and the body: path asked the server for one page of 200. So with 200 matching done items outranking one matching open item, filtering to open showed nothing. The local path now takes every match (MiniSearch already returns them all; the limit only truncated). The body: path walks the server's pages of 200 until it has every match, a short page, or 1000 (BODY_SEARCH_MAX), checking the identity and the query/route/archived snapshots after every page and committing once at the end. The TASK-3097 identity gate hashed the timer callback by its old body (resp.results.map); both rows now key on BODY_SEARCH_PAGE, re-reviewed on a542228bff48: identityHeld(epochAtSchedule) at fire time and after every page, one commit after the walk, the catch arm unchanged. Tests (in the page's behavioural suite): the body search asks for offset 200 after a full first page whose total is 201, a first page holding every match asks once (control), and the local search is uncapped. Counterfactuals: main fails both; restoring the local cap fails only the local test; cutting the walk to one page fails only the walk test; the control passes throughout. Claude-Session: https://claude.ai/code/session_01Rj4CMyK2EqxerMihbGs3Kr * fix(web): an overtaken body-search walk does not commit, and the walk's cap is stated (BUG-3710, codex r1) Codex r1: (1) a walk checks the query, route and archived snapshots after each page, but a newer run of the same effect (same trimmed query) has the same snapshots, and a multi-page walk is long enough to be overtaken, so the older walk could commit over the newer result. Each walk now captures the timer it fired under and stops once searchTimeout names another. Test: hold the first walk's page open, re-run the search with 'body:foo ' (same trimmed query), let the newer walk commit, then release the old one; the view must still show the newer result. Without the fence it shows the old (empty) result: the mutant fails exactly that test, and its premise passes. (2) BODY_SEARCH_MAX is a real cap: a body search with more than 1000 matches can still hide one. The comment now says so. The identity gate rows are re-reviewed on b64a3d75f5a0 and name the new fence. Claude-Session: https://claude.ai/code/session_01Rj4CMyK2EqxerMihbGs3Kr * fix(web): an overtaken body-search walk whose page fails does not clear the newer results (BUG-3710, codex r2) The catch arm checked identity and the query/route/archived snapshots but not the timer, so an overtaken walk (same trimmed query, same snapshots) whose page later rejected cleared the newer run's committed rank map. It now uses the success arm's fence (identityHeld plus !moved()). Test: hold the old walk's page open, let a re-run with the same trimmed query commit, then fail the old page; the view keeps the newer result. Restoring the old catch condition fails exactly that test, and its premise passes. Identity gate rows re-reviewed on the new hash, naming both arms. Claude-Session: https://claude.ai/code/session_01Rj4CMyK2EqxerMihbGs3Kr * feat(web): a body search stopped by the 1000-match cap says so on the collection page (BUG-3710) Heron's ruling: keep the cap, which matches the server's list max (#2143), but never silently, because a filter can hide a match past it. When the walk stops at BODY_SEARCH_MAX with matches left (a full last page and no total saying that was all), the page shows "Showing the first 1000 matches; refine your search". The flag commits beside the rank map under the same fence, resets on every run of the search effect, and clears on an identity change. The identity-gate rows are re-read and re-stamped. Mutants: never setting the flag fails the capped test; ignoring the total fails the exactly-1000 control; skipping the reset fails the body-then-local test. Claude-Session: https://claude.ai/code/session_01Rj4CMyK2EqxerMihbGs3Kr --- .../[workspace]/[collection]/+page.svelte | 101 ++++++--- .../collectionIdentityFence.svelte.test.ts | 209 ++++++++++++++++++ .../collectionIdentityGate.test.ts | 4 +- 3 files changed, 284 insertions(+), 30 deletions(-) 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\($/,