mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 21:39:01 +00:00
xarmian
53252ee5d0
* fix(web): close BUG-2129 stale-fields gap in the collection-edit switch fence
The EditCollectionModal onupdated handler's switch fence used
`{@const keyedSlug = itemSlug}` to detect a superseded save, but Svelte 5
compiles that as a lazily-pulled derived signal — since keyedSlug was only
read inside the async callback, it always evaluated the CURRENT itemSlug,
not the value at modal-open time, making the fence a no-op. Replace it with
a genuine gen+id snapshot (pendingCollectionEditGen/-ItemId) captured
synchronously in the onmanage click handler, and use it to (a) skip
navigation/archive/close for a genuinely superseded save and (b) still
refresh the currently-shown item's fields when it's in the collection that
just migrated, closing the stale-fields clobber gap BUG-2129 describes.
Adds two Playwright regression tests: a same-collection pane-switch repro
(BUG-2129's literal scenario) and a cross-collection full-page navigation
test that mutation-testing confirms discriminates the fix (fails on the
pre-fix code with an observable wrong-page hijack).
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(web): thread editedCollectionId through EditCollectionModal to fix overlap + rename/archive gaps
Codex review of the initial fix found two real gaps: (1) the
pendingCollectionEditGen/-ItemId snapshot was a single shared mutable slot,
so opening a second collection-edit (for a different item) while an earlier
save was still in flight would overwrite it, letting the earlier save's
completion misapply the wrong item's context; (2) the superseded-but-same-
collection branch only handled the reload case, not a pending rename
(still uses the stale collSlug -> 404) or archive (silently did nothing).
Replace the parent-side snapshot with a value EditCollectionModal itself
captures synchronously (before its API call) and echoes back through
onupdated(updated, editedCollectionId). ItemDetail's fence collapses to one
check — does the currently-shown item still belong to editedCollectionId —
applied uniformly to the reload, rename-redirect, and archive-redirect
paths, so all three now behave correctly whether or not the pane switched
items mid-save.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(web): gate the collection-edit fence on route/load settlement + fix a PATCH-timing test race
Codex round 2 found a narrower race: between a route change (collSlug/
itemSlug updating) and loadData()'s async resolution, item/collection can
transiently still hold the PREVIOUS item's data while collSlug/itemSlug
already reflect the new one. A mixed read across that window could pass
the collection-id fence using the stale `item` but build a navigation URL
from the already-updated collSlug/itemSlug, hijacking to a mismatched URL.
Gate the whole onupdated body on the existing `itemMatchesRef` invariant
(already used elsewhere in this file for the same "has loadData() caught
up" check) — bailing there is always safe since the route's own in-flight
loadData() will fetch fresh state regardless.
Also fixes a regression-test-only flake Codex flagged: the field-update
PATCH readback used waitForRequest (resolves on dispatch) instead of
waitForResponse (resolves on commit), so the immediate belt-and-suspenders
GET could race an in-flight write.
Deferred (documented in the PR, not fixed here): a further compound race
where a still-open second collection-edit modal on a different item could
be closed by an earlier, unrelated same-collection save's completion —
narrow, pre-existing-adjacent, and squarely in PLAN-2154 Phase 1's
dedicated R14 fence-sweep scope (TASK-2167) rather than this Phase 0 fix.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(web): allow a safe corrective reload during route transitions; sync test on PATCH response
Codex round 3: gating the ENTIRE onupdated body on itemMatchesRef (added
last round) was too broad — it also suppressed the corrective void
loadData() call during a route-transition window, reopening BUG-2129's
core symptom (an in-flight load that raced the migration and lost gets no
second chance). loadData() is idempotent and gen-fenced against any
in-flight load, so calling it again is always safe, even mid-transition.
Split the behavior: the relevance + reload decision runs unconditionally
(using whatever `item` is currently known, stale or not — false positives
just cause a harmless redundant reload), while only the RISKY navigation
(rename-redirect / archive-redirect, which builds a URL from collSlug/
itemSlug) stays gated on itemMatchesRef, since those are what a mixed
stale-item/fresh-route read could misdirect (Codex round 2's finding).
collectionStore.loadCollections(wsSlug) now runs unconditionally too,
keeping the global sidebar in sync regardless of pane relevance.
Also tightens the cross-collection test: replaces a fixed 1.5s sleep
after releasing the migration with an explicit wait for the PATCH
response, so the assertion can't false-pass by running before the
(possibly buggy) navigation had a chance to fire.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(web): fence the collection-edit callback on collSlug instead of item state
Codex round 4: gating rename/archive redirects on itemMatchesRef (added
last round) fixed the cross-collection hijack but broke the SAME-collection
case — during a same-pane item switch, collSlug never changes, so a
pending rename/archive that resolves mid-transition would skip the needed
redirect (itemMatchesRef false) and then loadData() 404s fetching the
now-stale/gone collSlug, with nothing to correct it afterward.
Replace the item-state-based fence entirely with a comparison against
`collSlug` — a plain reactive prop derived straight from the route params,
with no async lag (item/collection require a loadData() round-trip to
catch up; collSlug updates synchronously with navigation). EditCollectionModal
now also echoes back `editedCollectionSlug` (captured the same way as
editedCollectionId). `collSlug === editedCollectionSlug` is correct in
every case with no separate itemMatchesRef gate needed: unchanged for a
same-pane switch (so rename/archive/reload all still fire correctly),
and updated immediately for a cross-collection navigation (so a stale
edit's redirect correctly no-ops). This resolves the tension between
rounds 2-4 of Codex's findings with one simpler, more robust check.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(web): guard the collection-edit callback against a destroyed instance + workspace reuse
PR-level Codex review found two more real gaps in the fence:
1. The onupdated closure can still fire after this ItemDetail instance is
torn down entirely (pane closed, or the whole page navigated away)
while a save was pending — JS doesn't cancel a lingering promise on
unmount. Unguarded, that stale closure's goto()/collectionStore writes
would visibly affect whatever the user has since navigated to. Added a
one-way `destroyed` flag (set in onDestroy, alongside the existing
loadGeneration bump) checked first in the callback.
2. Collection slugs are workspace-scoped, so if this component instance
is ever reused across a workspace switch (no remount, same as the
existing collSlug/itemSlug reuse this fence already relies on), a
collSlug match alone can't tell two different workspaces' same-named
collections apart. EditCollectionModal now also echoes back
`editedWsSlug`, compared against the live `wsSlug` prop.
Also documents (not fixed here) a narrow pre-existing-adjacent edge case
Codex flagged: an embedded pane driven by a hand-crafted `?item=` whose
item lives in a different collection than the host page's collSlug won't
match this fence when its real collection is edited. This doesn't regress
anything (the prior fence was dead code and refreshed unconditionally
regardless of relevance) and is deferred to PLAN-2154 Phase 1 (TASK-2167).
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra