From f7bb735771fe242212d16884fa6bade92972eb26 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 3 Sep 2026 22:14:47 +0000 Subject: [PATCH] fix(web): state the cold rule positively; catch the epoch reset (TASK-2877) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review round 5, two P1s, both about `localIndex.reset()` — the sign-out / 403-purge / deleted-workspace path. THE FLAG WAS THE WRONG WAY ROUND. `coldFailed` asked "did the last search fail", and that was false in three states that are not answers at all: before the first request, after a failure, and after a reset drops every row while the query sits in the box. Each one read as "fine" and put a create row on screen backed by nothing. Inverted to `coldAnswered` — set in exactly one place, by the event that earns it, and cleared wherever the answer stops describing what is in the box. A flag that must be cleared everywhere is one that will be missed somewhere; this is the same defect arriving twice (round 3 caught the failure case, round 5 the reset case) because the polarity made silence indistinguishable from success. THE EPOCH FENCE HAD TO BE TWO-SIDED. `upsert`'s own guard refuses a captured epoch BELOW the current one, which catches a resync. But `reset()` DELETES the workspace state and the next bootstrap starts a fresh one at `scopeEpoch` 0 — so a captured 7 is not below 0, sails through, and links a row minted under an identity that no longer holds. `createRelationTarget` now requires equality. The residual is in the code comment rather than papered over: a reset plus resyncs landing back on exactly the captured number would compare equal, which an exposed reset generation would catch and this does not. Also dropped the `loading` term from `showCreate`. It and the per-query `coldAnswered` reset were a redundant PAIR — each survived removal while the other stood, which is one guard and one line that looks like a guard, not defence in depth (this repo has a note about exactly that shape). `coldAnswered` is the one kept: it states the rule (something authoritative has answered FOR THIS QUERY) where `loading` is a UI state that correlates with it. Matrix: 24 mutants, all killed; baseline and restore both 85/85. Killing the per-query reset needed `aria-expanded`, not the row's absence — with `loading` still gating the MARKUP, `.picker-create` is missing either way and asserting on it measures the branch instead of the rule. Third time this suite has been fooled by that same separation. Re-verified end to end in a real browser on this exact build: create row offered for a non-matching query and keyboard-reachable; Enter created COLO-6 "Chartreuse" in COLORS (colors 2 -> 3, cars unchanged) with `status: approved` — the schema's declared default, which the "+ New" `options[0]` heuristic would have gotten wrong; the car's field holds that id; a second pass at the same text offers the existing row and no create; Escape leaves the value untouched; no bare UUID anywhere on the page. --- .../FieldEditor.relation.svelte.test.ts | 27 ++++++++ .../lib/components/fields/FieldEditor.svelte | 15 ++++ .../lib/components/items/ItemPicker.svelte | 69 +++++++++++-------- .../items/ItemPicker.svelte.test.ts | 60 ++++++++++++++++ 4 files changed, 143 insertions(+), 28 deletions(-) diff --git a/web/src/lib/components/fields/FieldEditor.relation.svelte.test.ts b/web/src/lib/components/fields/FieldEditor.relation.svelte.test.ts index a581cba9..8eb464ef 100644 --- a/web/src/lib/components/fields/FieldEditor.relation.svelte.test.ts +++ b/web/src/lib/components/fields/FieldEditor.relation.svelte.test.ts @@ -676,6 +676,33 @@ describe('FieldEditor — relation, inline create (PLAN-2857 U8)', () => { expect(toastMock.show).not.toHaveBeenCalled(); }); + it('a create that lands after the workspace index was reset writes nothing', async () => { + // codex round 5 P1. `localIndex.reset()` DELETES the workspace state and + // the next bootstrap starts a fresh one at `scopeEpoch` 0, so a captured + // epoch of 7 is NOT below the current 0 and sails through `upsert`'s + // one-sided guard — linking a row minted under an identity that no + // longer holds. Equality is what catches the downward jump. + let release!: (v: unknown) => void; + createApi.mockReturnValue(new Promise((r) => { release = r; })); + localIndexMock.getByCollection.mockReturnValue([]); + localSearchMock.search.mockReturnValue([]); + const onchange = vi.fn(); + render(FieldEditor, { props: { ...editableProps, onchange } }); + await tick(); + + await typeQuery('Purple'); + createRow()!.click(); + await tick(); + + // The purge: state deleted, re-bootstrapped, epoch back to 0. + localIndexMock.scopeEpochFor.mockReturnValue(0); + release({ id: 'uuid-new', title: 'Purple', collection_slug: 'colors' }); + await tick(); + await tick(); + + expect(onchange).not.toHaveBeenCalled(); + }); + it('offers no create row while the collection list is not fresh for this workspace', async () => { // codex round 1 P2. `collectionStore.collections` is a single global // list, so during a workspace switch it still holds the PREVIOUS diff --git a/web/src/lib/components/fields/FieldEditor.svelte b/web/src/lib/components/fields/FieldEditor.svelte index 793713d4..e6951135 100644 --- a/web/src/lib/components/fields/FieldEditor.svelte +++ b/web/src/lib/components/fields/FieldEditor.svelte @@ -303,6 +303,21 @@ handlers — onchange is never called. localIndex.upsert(ws, item, epoch); if (destroyed || mySeq !== relationWrite) return; if (ws !== wsSlug || collSlug !== field.collection) return; + // The workspace's index must still be the one this create was + // authorized against (codex round 5). `upsert`'s own guard is + // ONE-SIDED — it refuses a captured epoch BELOW the current one, so + // it catches a resync — but `localIndex.reset()` on a sign-out or a + // 403 purge DELETES the state, and the next bootstrap starts a fresh + // one at epoch 0. A captured 7 is not below 0, so the write sails + // through and links a row minted under an identity that no longer + // holds. Equality catches both directions. + // + // Residual, stated rather than papered over: a reset followed by + // resyncs that land the epoch back on exactly the captured number + // would compare equal. An exposed reset generation would be exact; + // this needs no new store surface and the coincidence requires the + // purge and N resyncs to complete inside one create round trip. + if (localIndex.scopeEpochFor(ws) !== epoch) return; editingRelation = false; onchange(item.id); } catch (e: any) { diff --git a/web/src/lib/components/items/ItemPicker.svelte b/web/src/lib/components/items/ItemPicker.svelte index 356be9e6..8fce333f 100644 --- a/web/src/lib/components/items/ItemPicker.svelte +++ b/web/src/lib/components/items/ItemPicker.svelte @@ -157,16 +157,21 @@ let rawResults = $state([]); let loading = $state(false); /** - * The last cold search FAILED, as opposed to returning nothing. + * A cold search ANSWERED, and `rawResults` is that answer. * - * Both leave `rawResults` empty and `loading` false, and for the result - * list that is the same picture — "No results" either way. It is not the - * same picture for the create row: an empty answer is evidence that no such - * item exists, and a failed one is no evidence at all (codex round 3). - * Offering to create on no evidence is how a duplicate gets minted while - * the index is cold and the network is unhappy. + * Stated positively on purpose (codex rounds 3 and 5). The negative form + * — "the last search failed" — was false in three different states that + * are not answers at all: before the first request, after a failure, and + * after `localIndex.reset()` drops everything on a sign-out or 403 purge. + * Each left the flag reading "fine" and put a create row on screen backed + * by no evidence. A flag that must be cleared everywhere is a flag that + * will be missed somewhere; this one is set in exactly one place, by the + * event that earns it. + * + * Only the COLD path needs it. A settled warm index is authoritative by + * itself, and `indexCanProveAbsence` is what asks that question. */ - let coldFailed = $state(false); + let coldAnswered = $state(false); /** * The highlighted row's ID, not its index. Identity survives the list * changing under it — a delta landing, a late exclusion arriving — where an @@ -232,24 +237,33 @@ return (row.title ?? '').trim().toLowerCase() === wanted; } let showCreate = $derived.by((): boolean => { - if (!oncreate || !collection || !createTitle || loading) return false; - if (coldFailed) return false; + // No `loading` term. It and the per-query `coldAnswered` reset below are + // a redundant PAIR — either alone suppresses the row for the whole + // in-flight window, and a mutant removing either one survived while the + // other stood. That is not defence in depth, it is one guard and one + // line that looks like a guard. `coldAnswered` is the one kept, because + // it states the actual rule (something authoritative has answered FOR + // THIS QUERY) where `loading` is a UI state that merely correlates with + // it, and only the warm path can be settled while nothing is loading. + if (!oncreate || !collection || !createTitle) return false; const wanted = createTitle.toLowerCase(); if (rawResults.some((r) => titleIs(r, wanted))) return false; // Offer only where SOMETHING authoritative has answered "no such item", // which is the same rule the permission gate and `coldFailed` follow: no // evidence must not read as permission. // - // * COLD — `rawResults` came from `/search`, which is the server and - // therefore authoritative. Its empty answer is real evidence, so - // offer. (Refusing here would strand every user whose index has not - // hydrated.) + // * COLD — offer only once `/search` has actually ANSWERED. The server + // is authoritative and its empty answer is real evidence; not having + // asked yet, a failed request, and a workspace whose state was just + // dropped are all silence, and silence is not evidence. (Refusing + // outright would strand every user whose index has not hydrated, + // which is why this waits for the answer rather than the index.) // * READY, settled — the in-RAM collection is authoritative; scan it. // * READY, resyncing — the rows are a cache snapshot that delta-sync // has not reconciled, and `rawResults` came from THAT, so nothing in // reach can support the inference. Withhold until it settles; the // window is seconds and a duplicate outlives it. - if (!isWarm()) return true; + if (!isWarm()) return coldAnswered; if (!indexCanProveAbsence()) return false; return !localIndex.getByCollection(wsSlug, collection).some((r) => titleIs(r, wanted)); }); @@ -336,12 +350,12 @@ const res = await api.search(q, { workspace: wsSlug, collection }); if (mySeq !== seq) return; rawResults = (res.results ?? []).map((r) => r.item); - coldFailed = false; + coldAnswered = true; activeId = null; } catch { if (mySeq !== seq) return; rawResults = []; - coldFailed = true; + coldAnswered = false; activeId = null; } finally { if (mySeq === seq) loading = false; @@ -370,22 +384,17 @@ if (source === 'index' && isWarm()) { loading = false; rawResults = warmSearch(q); - coldFailed = false; activeId = null; return; } rawResults = []; - // NO `coldFailed` reset here, measured rather than assumed: mutants - // removing one here, on the empty-query branch, and in the - // workspace-reset effect all SURVIVED, so all three went. `loading` is - // true for the whole window this would cover and already suppresses the - // create row; when the request settles, both `coldSearch` branches - // assign `coldFailed` outright. The empty-query branch is covered - // twice over, since an empty query offers no create row at all. The - // ONE reachable reset is the warm branch above — the only path that - // produces a fresh verdict without going through `coldSearch`, so - // without it a single blip suppresses the affordance past hydration. + // The previous answer described the PREVIOUS query. This is the line + // that makes `coldAnswered` mean "answered for what is in the box now", + // and with the `loading` term gone it is load-bearing on its own: drop + // it and query B offers a create row on the strength of query A's + // answer, while B is still in flight. + coldAnswered = false; activeId = null; loading = true; debounceTimer = setTimeout(() => coldSearch(q, mySeq), COLD_SEARCH_DEBOUNCE_MS); @@ -568,6 +577,10 @@ seq++; clearTimeout(debounceTimer); rawResults = []; + // The rows this answer described were just dropped, so it + // describes nothing (codex round 5). Without this the picker + // offers to create over a purged workspace. + coldAnswered = false; activeId = null; loading = false; return; diff --git a/web/src/lib/components/items/ItemPicker.svelte.test.ts b/web/src/lib/components/items/ItemPicker.svelte.test.ts index b430e42d..1a2ad4a0 100644 --- a/web/src/lib/components/items/ItemPicker.svelte.test.ts +++ b/web/src/lib/components/items/ItemPicker.svelte.test.ts @@ -1061,6 +1061,66 @@ describe('ItemPicker — inline create (PLAN-2857 U8)', () => { expect(createRow()).not.toBeNull(); }); + it('does not carry one query\u2019s answer onto the next', async () => { + // `coldAnswered` has to mean "answered for what is in the box NOW". + // Without the per-query reset, typing a second query offers a create row + // immediately, on the strength of the first query's answer and against + // an empty result list that describes nothing. + vi.useFakeTimers(); + setBootstrapState('cold'); + loadIndex([]); + searchApi.mockResolvedValue({ results: [] }); + render(ItemPicker, { props: { ...createProps, oncreate: vi.fn() } }); + await tick(); + + await type('Purple'); + await vi.advanceTimersByTimeAsync(300); + await vi.waitFor(() => expect(createRow()).not.toBeNull()); + + // Second query, still in flight — nothing has answered for it yet. + let release!: (v: unknown) => void; + searchApi.mockReturnValue(new Promise((r) => { release = r; })); + await type('Teal'); + await vi.advanceTimersByTimeAsync(300); + // `aria-expanded`, not the row's absence — the markup renders the + // loading branch INSTEAD of the listbox, so `.picker-create` is missing + // either way and asserting on it measures the branch rather than the + // rule. Same trap this suite hit on the `loading` guard; it is the + // options list, and therefore the combobox's expanded state, that + // carries the stale answer. + expect(input().getAttribute('aria-expanded')).toBe('false'); + expect(createRow()).toBeNull(); + + release({ results: [] }); + await vi.waitFor(() => expect(createRow()).not.toBeNull()); + vi.useRealTimers(); + }); + + it('offers nothing to create after the workspace state is dropped', async () => { + // codex round 5 P1. `localIndex.reset()` — sign-out, a 403 membership + // purge, a deleted workspace — drops every row, and the picker's own + // effect clears what it was showing. The query is still in the box, so + // with a negative "did it fail" flag the picker read that silence as a + // clean empty answer and offered to create over a workspace it can no + // longer see anything in. + setBootstrapState('cold'); + loadIndex([]); + searchApi.mockResolvedValue({ results: [] }); + render(ItemPicker, { props: { ...createProps, oncreate: vi.fn() } }); + await tick(); + + await type('Purple'); + await vi.waitFor(() => expect(createRow()).not.toBeNull()); + + // The drop. + setBootstrapState('reset'); + bumpEpoch(); + await tick(); + await tick(); + + expect(createRow()).toBeNull(); + }); + it('CONTROL: a cold index falls back to the returned rows and still offers create', async () => { // The collection scan needs a hydrated index. While cold there is no // authoritative answer to fall back ON, so the behaviour is the