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
This commit is contained in:
xarmian
2026-10-14 05:41:18 -04:00
committed by GitHub
parent fc9ceae92f
commit 69fbbe6453
3 changed files with 284 additions and 30 deletions
@@ -155,6 +155,12 @@
// that a `Set`-only filter let `updated_at DESC` order override the
// ranking.
let searchResultRank = $state<Map<string, number> | 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<typeof setTimeout>;
/**
@@ -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 @@
</div>
{/if}
{#if bodySearchCapped && searchResultRank !== null}
<p class="search-cap-notice" role="status">
Showing the first {BODY_SEARCH_MAX} matches; refine your search.
</p>
{/if}
<!-- Saved views live in the View menu now (PLAN-2290 Phase 3);
the tab bar is retired. TASK-1366's pin/default semantics
carry over unchanged. -->
@@ -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;
}
@@ -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<string, unknown>;
@@ -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<string, unknown>).items)) as Record<string, unknown> | 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<string, unknown>).items)) as Record<string, unknown> | 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();
}
});
});
@@ -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\($/,