fix(web): state the cold rule positively; catch the epoch reset (TASK-2877)

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.
This commit is contained in:
xarmian
2026-09-03 22:14:47 +00:00
parent 6a335dd120
commit f7bb735771
4 changed files with 143 additions and 28 deletions
@@ -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
@@ -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) {
+41 -28
View File
@@ -157,16 +157,21 @@
let rawResults = $state<ItemIndexRow[]>([]);
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;
@@ -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