Skip to content

Map panel drops a read in flight when the pin moves within the same repo #315

Closed

Description

@ToRvaLDz

Parent: 🗺 trailhead mod: Claude Code function hooks for on-screen state, context and in-process guards

Symptom

A map read that is in flight when the pin root moves within the same repo (e.g. the session moves into a per-ticket <repo>-t<n> checkout) is dropped and never repeated. The pane keeps the previous list until the next trigger (a turn end, the slow timer, a write).

Where

  • plugins/trailhead/hooks/mod/map-pane.tsx onPinMove: bumps generation, then returns early when the move stays in the same repo, so it never calls requestRefresh($, { bypassFloor: true }) (only a move to another repo does).
  • fetchMap / fetchDashboard isStale: on a generation mismatch they return without calling wantsAnother(), so the dropped read is not replaced.

Repro (found while diagnosing #314)

A test in plugins/trailhead/tests/map-pane-ui.test.ts: gate the gh reads, start a read, change the tracker, move the pin root to another checkout of the same repo (the band's refresh), release the gate, advance one floor: the stale row is still shown (red in both surfaces).

Fix direction

In onPinMove, also request a refresh (bypassing the floor) when the root changes inside the same repo, or have isStale ask for another read on a generation mismatch while the pane is open.

Narrow race: it needs the dropped read to be the last one before an idle period.

Activity

  1. self-assigned this
    on Oct 23, 2026
  2. ToRvaLDz commented on Oct 23, 2026

    @ToRvaLDz
    OwnerAuthor

    REPRO

    Steps (in plugins/trailhead/tests/map-pane-ui.test.ts, modelled on the #314 race test "a refresh in flight when only the band's refresh moves the pin ..."):

    1. Open the pane on map 🗺 trailhead mod: Claude Code function hooks for on-screen state, context and in-process guards #198 in ROOT and let it settle.
    2. Gate every gh read (the answer is computed when the call starts, released later).
    3. Start a read, then change the tracker (a ticket closes / a new one appears).
    4. Move the pin to another checkout of the same repo (same remote, different root), via the band's refresh (a turn start).
    5. Release the gate, advance one refresh floor, settle.

    Expected: the in-flight read is dropped and replaced by a fresh one, so the pane shows the changed tracker.
    Observed: the stale rows stay shown until an unrelated trigger (turn end, slow timer, write).

    Code path: onPinMove bumps generation on every pin change, but only a move to another repo calls requestRefresh($, { bypassFloor: true }); the in-flight fetchMap / fetchDashboard then sees token !== generation in isStale and returns without wantsAnother().

  3. ToRvaLDz commented on Oct 23, 2026

    @ToRvaLDz
    OwnerAuthor

    DIAGNOSIS

    Root cause (falsifiable): onPinMove (plugins/trailhead/hooks/mod/map-pane.tsx) bumps generation on every pin change, but asks for a fresh read only on a move to another repo. On a same-repo move the bump makes the in-flight read stale through the plain token !== generation test in fetchMap / fetchDashboard isStale, which returns without wantsAnother(). Nothing else asks for a read, so it is dropped and never repeated. If the bump were skipped on a same-repo move, isStale's own pin check (!isSamePin, or a changed target) would call wantsAnother() and the read would be replaced.

    Falsification test (run): two new tests in plugins/trailhead/tests/map-pane-ui.test.ts, per surface: a map read and a dashboard read gated in flight, the tracker changed, the pin moved to another checkout of the same repo, gate released, one floor advanced.

    • As is: 4 fail (exactly the new tests), the rest green.
    • With generation bumped only on a move to another repo (or to no pin): all green.

    Eliminated alternative: a bypass-floor refresh in the same-repo branch of onPinMove turns the new tests green but doubles the reads when the move happens inside the refresh's own pin sync (the manual /clear test reads 4 times instead of 2).

    Safety of the fix: a read in flight under the old checkout cannot commit under the new pin: isStale compares the pin captured at the read's start with the current one, drops the result and asks for another read.

  4. ToRvaLDz commented on Oct 23, 2026

    @ToRvaLDz
    OwnerAuthor

    VERIFY (round 1)

    status: clean
    critical: 0 · warning: 1 · info: 0

    Findings ledger

    • W1: open (being fixed in round 2)

    Ticket diff: 5b86544 (tests), 0ca3f72 (fix), read per commit.

    W1 (warning): a same-repo move that lands between isStale's pin read and its last token check is not caught. refreshPin calls onMove and then updates the repoPin atom, and the band's refresh runs outside the refresh queue. If the move lands after isStale read the old pin and before its final token !== generation check, the read commits under the old checkout and no new read is asked for. Narrow interleaving, same repo's data. Fix: a pin-move counter bumped on every pin change, captured at the read's start, a mismatch in isStale's final check treated like a moved pin.

    Goal-backward verification

    status: met

    The dropped read is replaced in both the map and dashboard reads; a read under the old pin never commits under the new one; a move to another repo keeps its behaviour; no read is added; the regression tests drive the real race (red before, green after) on both surfaces.

    Tests

    Stale-base preflight: current (0 behind). Mod suite 648 pass / 0 fail; node scripts/test.js 66 ok, 0 failed.

  5. ToRvaLDz commented on Oct 23, 2026

    @ToRvaLDz
    OwnerAuthor

    VERIFY (round 2)

    status: clean
    critical: 0 · warning: 1 · info: 1

    Findings ledger

    • W1 (warning): fixed in 794c5ee. A pin-move counter, bumped on every pin change and captured right after the pin sync, is checked at the final test of both stale checks; a mismatch drops the read and asks for another. Covered by "a same-repo pin move landing inside the stale check is read again".
    • I1 (info): accepted. The dashboard copy of the counter check has no in-check test of its own; same code shape as the map path, and the dashboard same-repo move test covers its main path.

    Ticket diff: 5b86544, 0ca3f72, 794c5ee, read per commit. A move between the counter capture and the read's start costs at most one redundant read, bounded by the refresh queue.

    Goal-backward verification

    status: met

    A read in flight on a same-repo pin move is replaced (map and dashboard, both surfaces); a read under the old pin never commits under the new one, including a move landing inside the stale check; a move to another repo keeps its behaviour; no read is added in the normal paths (the /clear read-count tests stay green).

    Tests

    Stale-base preflight: current. Mod suite 650 pass / 0 fail; node scripts/test.js 66 ok, 0 failed. Acceptance: covered by the automated regression tests driving the real race (gated gh reads, pin moved mid-read), by the user's choice; no manual UAT owed.

  6. ToRvaLDz commented on Oct 23, 2026

    @ToRvaLDz
    OwnerAuthor

    Resolution

    Cause: onPinMove bumped the read generation on every pin change but asked for a fresh read only on a move to another repo, so on a same-repo move (e.g. into a per-ticket checkout) the read in flight failed the plain generation check and returned without asking for another read.

    Fix:

    • 0ca3f72 fix(mod): read again when the pin moves within the same repo: the generation is bumped only on a move to another repo or to no pin; a same-repo move leaves the read in flight to its stale check, which drops it and reads again.
    • 794c5ee fix(mod): catch a same-repo pin move landing inside the stale check: a pin-move counter closes the narrow window where a move landed inside the stale check itself (review finding W1).
    • 5b86544 test(mod): cover a same-repo pin move during a read in flight: regression tests for the map and dashboard reads on both surfaces (plus the in-check test in 794c5ee).

    Verified: see the round 2 VERIFY comment above for the findings ledger (W1 fixed, I1 accepted, none open). Acceptance covered by the automated regression tests, by the user's choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions