gsadmin/pad
1
0
Fork 0
mirror of https://github.com/PerpetualSoftware/pad.git synced 2026-09-11 13:28:57 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(web): a failed links refresh keeps the links it has, so a click cannot be lost to it (BUG-2871) (#1315)

Browse Source 
Three same-item links refreshes in ItemDetail swallowed a request failure into
an EMPTY array — `api.links.list(...).catch(() => [])` — throwing away rows
that were on screen and correct. A failed request says nothing about the links
it did not fetch. The initial load did the same on throw, unconditionally.

Two reasons that mattered beyond the lost data.

`{#if relationshipGroups.length > 0}` sits ABOVE both `{#each}` keys, so keying
the rows on `(group.label)` and `(entry.key)` protects nothing against an empty
list: the whole relationships section is destroyed and later rebuilt. A click
straddling that is swallowed entirely, because a click needs mousedown and
mouseup on ONE node — no navigation, no error. That is the defect BUG-2871
fixed for the Children pane, at the same altitude, through a different door,
and the Children fix had to cover its error branch as well as its loading one
for exactly this reason.

Second, silence on a failed same-item refresh is the RULED behaviour on that
trail — last-good rows are valid data. That ruling only holds if the rows
survive the failure, which is the half this closes.

It does NOT promise a retry, and the code no longer implies one. Turning the
error into a successful return leaves the full-refresh caller's
`syncService.markSynced()` advancing the cursor, so stale links can persist with
nothing scheduled to correct them — not introduced here, since `.catch(() =>
[])` returned successfully too, but this makes the staleness survivable rather
than visibly empty. Filed as BUG-2992 and cited from both comments rather than
left implied.

THE GATE IS WHOSE ROWS THEY ARE, not which function is running. That was wrong
in the first version of this change, which treated `loadData` as always a first
load or a switch and cleared there unconditionally; the edit-collection handler
calls it for a SAME-item reload after a schema change, so the defect survived
through that door (codex P1). The load now captures the item its links belong
to before it can replace `item`, and clears only when that differs — keeping
them across a same-item reload, dropping them on a real switch, where holding
them would render one item's relationships under another's title.

WHAT THIS DOES NOT CLAIM. It does not close the `:194` E2E flake. The failure
is located — `waitForEvent` timing out at the ctrl-click's popup wait with
`click()` itself resolving, so Playwright believed it clicked and nothing
navigated — and this makes that observation impossible via this route. But
there is no evidence from the failing run that a links request actually failed;
the job log carries the test's view, not the server's responses. What is
established is that the path is reachable (every API route sits behind a
600/min burst-60 limiter whose own comment cites cascading SSE refreshes) and
that two rival explanations are refuted: the `<a>`/`<span>` href flip (all four
`itemLinks` sites call the same `api.links.list`, so there is no leaner payload
to flip to) and interception on modifier clicks (`shouldOpenInPane` returns
false for `ctrlKey` before any `preventDefault`). If the flake recurs after
this, the cause is elsewhere and the trail should say so rather than reading a
green run as a diagnosis.

Tests: a source-level guard, following `itemDetailUsesPicker.test.ts` for the
same reason — the property is structural and mounting a 7,900-line component to
observe it costs more than it is worth. FIVE of its eight assertions fail
against pre-fix source, verified by running it against `git show origin/main:`
rather than counted by hand — the earlier claim of three in this message was
wrong, and review caught it.

Seven mutants, each killing exactly one leg: a helper spelled `catch { return
[]; }`; `catch { itemLinks = []; return itemLinks; }`, which satisfied a
return-value assertion while reintroducing the defect; the same via
`itemLinks.length = 0` and via `itemLinks.splice(0)`, which an
assignment-only check misses; restoring the unconditional clear; an
unconditional clear sitting BESIDE the gated one; and an unconditional
`splice(0)` beside it. That progression is why the assertions pin absence and a
COUNT rather than presence, why the mutation check enumerates in-place emptying
as well as assignment, and why the regions are matched by BALANCED BRACES
instead of fixed-length windows — the fixed windows were escapable past their
end, and the load one bled past its `catch` into unrelated code where an
ordinary edit could fail the count for the wrong reason.

Three review rounds each defeated the previous version of these assertions with
a mutant written against them, and the file now says where that stops: a source
guard checks spellings, not behaviour, and the remaining escapes need someone
writing deliberately around a test in the file they are editing.

Comments are stripped before asserting, which is load-bearing rather than tidy:
the new helper's doc comment quotes `.catch(() => [])` as the thing it replaced,
so a raw-source guard would pass on documentation. The strip control is anchored
on a long-standing comment, after a first version anchored on the new one
reported "the strip is broken" against pre-fix source, where the strip was fine
and only the fix was absent — a control that fails for the wrong reason is not a
control.

Two assertions pass on BOTH builds, deliberately: the load site still has a
clear to gate, and the rows are still keyed. The second is the premise the whole
fix rests on — if someone unkeys those rows, this fix stops being sufficient and
that test is what should say so.

A WEDGE I SHIPPED INTO CI AND HAD TO FIX. The first version captured the held
item id with a plain `item?.id` read at the top of `loadData`. `loadData` is
called from an `$effect` tracking wsSlug/collSlug/itemSlug, and it WRITES `item`
further down — so that read made the effect self-invalidating. Dev throws
`effect_update_depth_exceeded`; the PRODUCTION build silently wedges the global
effect scheduler, and the app stops re-rendering with no error anywhere. CI's
E2E job died on it: 77 failures across the attachment specs, nothing to do with
relationships, and the job hit its 15-minute timeout.

CONVE-1688 names exactly this, and I had loaded it before writing the change.
The read is now `untrack(() => item?.id ?? null)`, with the reason at the site
and a test pinning it — because every cheaper gate passed while the built app
rendered nothing: svelte-check, vitest and the unit suites are all blind to it.

The gap that let it through is that my local gates never BUILT the change. The
worktree's `web/build` was copied from the main checkout, so vitest and
svelte-check read my source while nothing exercised it compiled. Verified the
fix the way that gap demanded: `vite build` + `make build-go`, then the failing
spec against that binary — 11/11 fail before, 11/11 pass after, and 11/11 pass
on a main-built binary as the control.

Gates: svelte-check 0 errors (1087 files), vitest 154 files / 2351 tests passed,
go test exit 0 with no FAIL, golangci-lint 0 issues, gofmt clean. Full local E2E
on the built tree: 214 passed, 198 skipped, 2 failed — `pane-content-link-anchors:194`
and one no-store HEAD counting test, both of which also fail on main (see the
BUG-2871 trail: the ctrl-click popup timeout reproduces locally at roughly 1 run
in 3, on the CHILDREN leg too, which the merged Children fix did not eliminate).

Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm
This commit is contained in:
xarmian
2026-09-10 03:46:25 -04:00
committed by GitHub
parent 6d77856447
commit 8f6c62ec27
2 changed files with 314 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
web/src/lib/components/items/ItemDetail.svelte
+87 -6
View File
@@ -1407,7 +1407,7 @@
// still lose chars without this branch.
item = adoptServerItem(updated);
void refreshCollectionIfMoved(updated);
const links = await api.links.list(reqWsSlug, updated.slug).catch(() => []);
const links = await refreshLinksPreservingOnFailure(reqWsSlug, updated.slug);
if (!item || item.id !== reqItemId || myItemGen !== itemGen) return;
itemLinks = links;
} catch {
@@ -1512,7 +1512,7 @@
const myItemGen = ++itemGen;
item = adoptServerItem(updated);
void refreshCollectionIfMoved(updated);
const links = await api.links.list(reqWsSlug, updated.slug).catch(() => []);
const links = await refreshLinksPreservingOnFailure(reqWsSlug, updated.slug);
if (!item || item.id !== reqItemId || myItemGen !== itemGen) return;
itemLinks = links;
}
@@ -1529,10 +1529,14 @@
// adoptions above — a long tab absence can span a move too
// (codex round 2 P1).
void refreshCollectionIfMoved(updated);
const links = await api.links.list(reqWsSlug, updated.slug).catch(() => []);
const links = await refreshLinksPreservingOnFailure(reqWsSlug, updated.slug);
if (!item || item.id !== reqItemId || myItemGen !== itemGen) return;
itemLinks = links;
syncService.markSynced(); // Advance cursor now that reload succeeded
// Advance the cursor now that the ITEM reload succeeded. The links
// half may have failed and been preserved rather than fetched
// (`refreshLinksPreservingOnFailure`), which the cursor cannot
// express — BUG-2992.
syncService.markSynced();
} catch {
// Ignore — will catch up on next event
}
@@ -1592,6 +1596,25 @@
async function loadData() {
const myGen = ++loadGeneration;
// The item whose links `itemLinks` currently describes, captured BEFORE
// this load can replace `item`. `loadData` is not only a first load or a
// switch: the edit-collection handler calls it for a SAME-item reload
// after a schema change (see the `itemMatchesRef` gate below, which
// exists for that case). On such a reload a failed links request must
// keep the links it has, for the same reason the three refresh callers
// do — and on a real switch it must NOT, or one item's relationships
// render under another's title (BUG-2871, codex P1).
//
// READ THROUGH `untrack`, and that is not a style choice (CONVE-1688).
// `loadData` is called from an `$effect` whose tracked deps are
// `wsSlug`/`collSlug`/`itemSlug`, and it WRITES `item` further down. A
// plain read here adds `item` to that effect's dependencies, making the
// effect self-invalidating: dev throws `effect_update_depth_exceeded`,
// and the PRODUCTION build silently wedges the global effect scheduler,
// so the app stops re-rendering with no error at all. That is what this
// line did on its first version — the whole e2e suite failed, the
// attachment viewer included, because nothing downstream ever rendered.
const linksHeldForItemId = untrack(() => item?.id ?? null);
// Join the unified collection-snapshot fence so this load's `collection`
// write is dropped if a newer SSE refresh / callback landed a fresher
// snapshot while this load was in flight (Codex — separate lifecycle
@@ -1913,12 +1936,30 @@
// (BUG-1461 — previously this was a fire-and-forget call here,
// which raced the Y.Doc seed and could bake `[[X]]` text in).
// Load links for this item
// Load links for this item.
//
// This failure path clears CONDITIONALLY, and the condition is the
// point. `loadData` runs for a first load, for a switch to a
// DIFFERENT item, and — via the edit-collection handler after a
// schema change — for a SAME-item reload. Keeping the previous list
// across a switch would render one item's relationships under
// another item's title; clearing it on a same-item reload destroys
// good rows and takes any click straddling it with them, which is
// the whole defect. So the test is `linksHeldForItemId`, not which
// function we are in (BUG-2871; the unconditional clear here was
// codex P1 on the first version of this fix).
try {
const links = await api.links.list(wsSlug, itemData.slug);
if (myGen !== loadGeneration) return;
itemLinks = links;
} catch { if (myGen !== loadGeneration) return; itemLinks = []; }
} catch {
if (myGen !== loadGeneration) return;
// Clear only when the rows we hold belong to a DIFFERENT item (or
// to no item yet). A same-item reload keeps them, matching the
// three refresh callers; the asymmetry is about whose rows they
// are, not about which function is running.
if (linksHeldForItemId !== itemData.id) itemLinks = [];
}
// Load workspace members and agent roles for the assignment picker.
// Both are workspace-invariant, so reuse the cached copy on a
@@ -3857,6 +3898,46 @@
return fallback || 'Unknown item';
}
/**
* Refresh this item's links, KEEPING the current list if the request fails.
*
* The three same-item refresh callers used `.catch(() => [])`, which threw
* away rows that were on screen and correct: a failed request says nothing
* about the links it did not fetch. Two reasons that mattered beyond the
* lost data.
*
* An empty list collapses `{#if relationshipGroups.length > 0}`, which sits
* ABOVE both `{#each}` keys — so keying the rows protects nothing here, and
* the whole section is destroyed and later rebuilt. A click straddling that
* loses its target the same way BUG-2871's Children rows did: no navigation,
* no error, because a click needs mousedown and mouseup on ONE node.
*
* And silence on a failed same-item refresh is the ruled behaviour (BUG-2871
* trail): last-good rows are valid data. That ruling only holds if the rows
* actually survive the failure — which is the half this closes, exactly as
* the Children fix had to cover its error branch as well as its loading one.
*
* WHAT IT DOES NOT PROMISE is a retry. Turning the error into a successful
* return means the full-refresh caller's `syncService.markSynced()` still
* advances the cursor, so stale links can persist until something else asks
* — and a non-structural link change emits no event to ask on. That is not
* introduced here (`.catch(() => [])` returned successfully too) but this
* makes it survivable rather than visibly empty, so it is filed rather than
* left implied: BUG-2992.
*
* Callers still re-check item identity after awaiting, since this returns
* links for whatever `slug` they asked about.
*/
async function refreshLinksPreservingOnFailure(ws: string, slug: string): Promise<ItemLink[]> {
try {
return await api.links.list(ws, slug);
} catch {
// The current list, so the assignment at the call site is a no-op
// rather than a destructive one.
return itemLinks;
}
}
function relationHref(collectionSlug?: string, refOrSlug?: string): string | null {
if (!collectionSlug || !refOrSlug) return null;
return `/${username}/${wsSlug}/${collectionSlug}/${refOrSlug}`;
web/src/lib/components/items/itemDetailLinksSurviveFailedRefresh.test.ts
+227
View File
@@ -0,0 +1,227 @@
// Node-project test (no DOM): a SOURCE-level guard that a failed SAME-ITEM
// links refresh in ItemDetail keeps the links already on screen, instead of
// replacing them with an empty array (BUG-2871, the `:194` half).
//
// Why this matters beyond the lost data: an empty list collapses
// `{#if relationshipGroups.length > 0}`, which sits ABOVE both `{#each}` keys
// (`(group.label)` and `(entry.key)`) — so keying the rows protects nothing,
// and the whole relationships section is destroyed and later rebuilt. A click
// straddling that is swallowed entirely, because a click needs mousedown and
// mouseup on ONE node. That is the same defect BUG-2871 fixed for the Children
// pane, at the same altitude, through a different door.
//
// Why source text and not a render: ItemDetail is ~7,900 lines with collab,
// SSE and pane wiring, and the property is structural — the regression this
// guards is a future edit reintroducing `.catch(() => [])` at a refresh site,
// which shows up in the source and would not show up in any component suite.
// Same reasoning, and same file, as `itemDetailUsesPicker.test.ts`.
//
// WHAT A SOURCE GUARD CANNOT DO, stated so it is not mistaken for proof: it
// checks spellings, not behaviour. Review defeated three successive versions of
// these assertions with mutants written specifically against them — a helper
// returning [], one clearing before returning the field, an unconditional clear
// beside the gated one, and `itemLinks.length = 0` / `.splice(0)` in place of an
// assignment. Each is now refused, and the windows are brace-matched rather than
// fixed-length so a clear cannot sit just past the end of one. That is where the
// tightening stops: the remaining escapes require someone deliberately writing
// around a test in the file they are editing, and the honest instrument for
// behaviour is the E2E leg plus the reasoning on the BUG-2871 trail.
//
// COMMENTS ARE STRIPPED BEFORE ASSERTING, and that is load-bearing rather than
// tidiness: the helper's own doc comment quotes `.catch(() => [])` as the thing
// it replaced, and it names `api.links.list`. Asserting against raw source
// would match this file's own prose and the guard would pass or fail on
// documentation rather than on code — the same trap `itemDetailUsesPicker`
// records for HTML comments and `<ItemPicker`.
import { describe, it, expect } from 'vitest';
import { readFileSync } from 'node:fs';
import { fileURLToPath } from 'node:url';
const raw = readFileSync(
fileURLToPath(new URL('./ItemDetail.svelte', import.meta.url)),
'utf8'
);
/**
* Source with HTML comments, JS block comments and JS line comments removed.
*
* The line-comment rule skips `//` preceded by `:` so `https://` inside a
* string survives as code rather than eating the rest of its line — a URL is
* the one place `//` appears without starting a comment in this file.
*/
const code = raw
.replace(/<!--[\s\S]*?-->/g, '')
.replace(/\/\*[\s\S]*?\*\//g, '')
.replace(/(^|[^:])\/\/[^\n]*/g, '$1');
const count = (haystack: string, needle: string) => haystack.split(needle).length - 1;
/**
* The `{...}` block that starts at or after `from`, matched by BALANCING braces.
*
* The first version of this file sliced fixed-length windows (500 and 700
* characters). Both were escapable — a clear placed past the window passes —
* and the load-site window bled past its `catch` into unrelated member/role
* loading, so an ordinary edit there could fail the count for a reason that has
* nothing to do with this property (codex round 3). A balanced block is bounded
* by the code's own structure instead of by a guess.
*
* Brace-counting is not a JS lexer: a `{` inside a string or a regex literal in
* the region would skew it. Accepted here because the regions this is pointed at
* are small and contain neither, and the test asserting a KNOWN count would fail
* loudly rather than silently if that stopped being true.
*/
function balancedBlock(source: string, from: number): string {
const open = source.indexOf('{', from);
if (open < 0) return '';
let depth = 0;
for (let i = open; i < source.length; i++) {
if (source[i] === '{') depth++;
else if (source[i] === '}') {
depth--;
if (depth === 0) return source.slice(open, i + 1);
}
}
return source.slice(open);
}
/**
* Every way this file has of emptying `itemLinks` in place or by assignment.
*
* Assignment alone is not enough: `itemLinks.length = 0` and
* `itemLinks.splice(0)` clear the visible links while passing an
* assignment-only check (codex round 3 wrote both). A source guard cannot
* enumerate every possible spelling — what it can do is refuse the ones a
* reader would actually reach for.
*/
const MUTATES_ITEM_LINKS = /itemLinks\s*(?:=[^=]|\.length\s*=|\.splice\(|\.pop\(|\.shift\(|\.fill\()/;
describe('ItemDetail: a failed same-item links refresh keeps the links it has', () => {
it('strips comments effectively enough for the assertions below to be about code', () => {
// A control on the instrument itself (CONVE-30): if the strip silently
// did nothing, every assertion here would still "pass" while measuring
// prose rather than code.
//
// Anchored on a LONG-STANDING comment rather than on the helper's own
// doc comment. Anchoring it on the new comment made the control fail
// against pre-fix source — where the comment does not exist yet — so it
// reported "the strip is broken" when the strip was fine and only the
// fix was absent. A control that fails for the wrong reason is not a
// control.
expect(raw).toContain('isOwner now comes from workspaceStore');
expect(code).not.toContain('isOwner now comes from workspaceStore');
});
it('has no refresh site that swallows a failure into an empty array', () => {
// The exact shape that was there: `api.links.list(...).catch(() => [])`.
// Matched loosely on purpose — any `.catch` that yields `[]` on this call
// is the defect, however the arguments are spelled.
expect(code).not.toMatch(/api\.links\.list\([^)]*\)\s*\.catch\(\s*\(\s*\)\s*=>\s*\[\s*\]\s*\)/);
});
it('routes every same-item refresh through the preserving helper', () => {
// Three refresh callers: the SSE adopt path, the incremental adopt path,
// and the long-absence reload. All three fetch by `updated.slug`, i.e.
// the item already loaded, which is what makes preserving correct.
const calls = count(code, 'await refreshLinksPreservingOnFailure(reqWsSlug, updated.slug)');
expect(calls).toBe(3);
});
it('leaves exactly two direct api.links.list calls: the helper and the initial load', () => {
// If a fourth appears, a new caller has been added that does not go
// through the helper — which is how the three original sites drifted
// into having the same bug three times.
expect(count(code, 'api.links.list(')).toBe(2);
});
it('has a helper whose failure branch returns the CURRENT links, not an empty array', () => {
// The gap codex found in the first version of this test: every other
// assertion here passes against a helper spelled
//
// catch { return []; }
//
// which reintroduces the exact defect while routing through the helper.
// Rejecting the old spelling is not the same as asserting the new
// behaviour, so this pins the fallback VALUE.
expect(code).toMatch(
/async function refreshLinksPreservingOnFailure\([^)]*\)[^{]*\{[\s\S]{0,400}?catch\s*\{[^}]*return itemLinks;[\s\S]{0,40}?\}/
);
// And the helper MUTATES NOTHING. Pinning only the return value admits
// `catch { itemLinks = []; return itemLinks; }`, which satisfies every
// other assertion here while reintroducing the defect (codex round 2
// wrote that mutant). A helper that reads state and writes none is the
// property; anything emptying `itemLinks` inside it is the defect
// whatever it then returns.
const helperStart = code.indexOf('async function refreshLinksPreservingOnFailure');
expect(helperStart).toBeGreaterThan(-1);
const helperBody = balancedBlock(code, helperStart);
expect(helperBody).not.toMatch(/return\s*\[\s*\]/);
expect(helperBody).not.toMatch(MUTATES_ITEM_LINKS);
});
it('clears links on a failed load only when they belong to a DIFFERENT item', () => {
// `loadData` is not only a first load or a switch — the edit-collection
// handler calls it for a SAME-item reload after a schema change, and an
// unconditional clear there destroyed good rows (codex P1). The gate is
// item identity captured before this load can replace `item`, not which
// function is running.
expect(code).toContain('const linksHeldForItemId = untrack(() => item?.id ?? null);');
expect(code).toMatch(/if \(linksHeldForItemId !== itemData\.id\) itemLinks = \[\];/);
// And that catch block empties `itemLinks` EXACTLY ONCE, gated. Pinning
// only the presence of the gated line admits an unconditional clear
// sitting beside it, or an `else` branch that clears anyway — both of
// which restore the defect while passing the assertion above (codex
// round 2). Counting is what makes the gate the only writer, and the
// region is the catch block matched by BALANCED BRACES rather than a
// fixed-length window, so a clear cannot sit just past the end of it and
// an unrelated edit after the block cannot fail the count (round 3).
const loadStart = code.indexOf('const links = await api.links.list(wsSlug, itemData.slug);');
expect(loadStart).toBeGreaterThan(-1);
const catchAt = code.indexOf('catch', loadStart);
expect(catchAt).toBeGreaterThan(loadStart);
const loadCatch = balancedBlock(code, catchAt);
expect(loadCatch).toContain('linksHeldForItemId !== itemData.id');
// One gated clear, and nothing else in the block touches the list.
expect(count(loadCatch, 'itemLinks')).toBe(1);
});
it('still CLEARS links when the load is a real item SWITCH', () => {
// The clear must still EXIST at this site, gated. It is the half most
// likely to be "tidied" away by a reader who has just understood that
// refreshes preserve: on a switch to a different item, keeping the
// previous list shows one item's relationships under another's title.
// The sibling test above pins the GATE; this one pins that there is
// still something to gate.
expect(code).toMatch(
/const links = await api\.links\.list\(wsSlug, itemData\.slug\);[\s\S]{0,600}?catch\s*\{[\s\S]{0,400}?itemLinks = \[\];/
);
});
it('reads the held item id through untrack, because loadData runs inside an $effect that writes item', () => {
// CONVE-1688, and this one is not hypothetical: the first version of
// this line was a plain `item?.id` read. `loadData` is called from an
// `$effect` tracking wsSlug/collSlug/itemSlug, and it WRITES `item`
// further down — so the plain read made that effect self-invalidating.
// Dev throws `effect_update_depth_exceeded`; the PRODUCTION build
// silently wedges the global effect scheduler, and the whole e2e suite
// failed with nothing rendering, the attachment viewer included.
//
// Pinned here rather than left to review because the failure mode is
// invisible in every cheaper gate: svelte-check, vitest and the unit
// suites all passed while the built app rendered nothing.
expect(code).toContain('const linksHeldForItemId = untrack(() => item?.id ?? null);');
expect(code).not.toMatch(/const linksHeldForItemId = item\?\.id/);
});
it('keys the relationship rows, so the {#if} above them is the only node-identity gap', () => {
// Recorded as an assertion rather than a comment because the whole
// argument for this fix rests on it: the rows themselves ARE keyed, so
// a re-derive over the same links preserves them, and the section-level
// `{#if}` is what destroys nodes. If someone unkeys these, the fix stops
// being sufficient and this test should be the thing that says so.
expect(code).toContain('{#each relationshipGroups as group (group.label)}');
expect(code).toContain('{#each group.entries as entry (entry.key)}');
expect(code).toContain('{#if relationshipGroups.length > 0}');
});
});