Design notes: identity-based escape restore + plan to solve the three entangled problems

Capture a design discussion (no code yet; implementation is a future session):

- The escape-from-staging restore should anchor on the explorer view's current
  patch identity at escape time, not a saved numeric scroll/index, since staging
  or dropping hunks changes the content. This is the inverse direction of the
  diff-line-metadata primitive (identity -> rendered row) and the same operation
  the -U scroll-preservation consumer needs; record it as consumer #6 and split
  the consumer list into forward (1-4) and inverse (5-6) directions.
- Record the escape-routing cases the current prototype gets wrong (staging the
  last hunk should land in the staged half; <tab> between staged/unstaged; the
  empty-view and custom-patch-builder cases).
- Decide to solve the new restore mechanism, the §11 timing races, and the
  BufferLineForViewLine staleness trap together in the prototype rather than
  defer them to productionization (you can't plan around unsolved entangled
  mechanisms), with a dependency-first attack order (§8 fix, characterize the
  races, then the predicate-scroll restore).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

diff-line-metadata-notes.md

@@ -32,6 +32,18 @@ several consumers, not a click-to-stage helper:
    (`ScrollToOriginYForNextTask`, commits `054d139fe`/`625e7dbad`). Anchor on
    the **nearest change line**, which survives any `-U` change (context lines
    don't).
+6. **Restore selection/scroll when escaping back from staging / patch building**
+   — land on the line the explorer view was *currently* selecting at escape
+   (after its auto-advance), not the line you entered on, since you may have
+   staged/dropped hunks meanwhile. Replaces the brittle numeric-index restore;
+   see focused-main-view-notes.md §12 (incl. the escape-routing special cases).
+
+Consumers **1–4** use the primitive in the **forward** direction (rendered row →
+identity). Consumers **5–6** use the **inverse** (identity → rendered row): they
+scan the rendered rows' metadata for the one matching a target patch identity,
+which the host does *as the buffer loads* via a predicate generalization of
+`ScrollToOriginYForNextTask` (focused-main-view-notes.md §12.3). The inverse
+direction is what motivates solving the §8 staleness trap up front.
 
 Because it's one primitive, it's worth building as a clean standalone
 capability rather than welding it to staging.

focused-main-view-notes.md

@@ -612,3 +612,103 @@ characterised — next session should:
   selection restore; the `afterLayout`-deferred pty task creation racing a layout
   pass; and `CopyContent` vs. the task's first write.
 - Memory: `focused-main-view-flicker-timing-races`.
+
+---
+
+## 12. Restore by patch identity, escape routing, and the plan to solve the three entangled problems
+
+A design discussion (it did **not** result in code; the implementation is for a
+**new session**). Three things came out of it: a better model for the escape
+restore, a set of escape-routing cases the current prototype gets wrong, and a
+decision about *when* to solve the hard async problems.
+
+### 12.1 The restore should anchor on a patch identity, not a numeric scroll/index
+
+The current escape (§6/§11) saves the main view's `OriginY` + selection **index**
+and replays them. That's only right when the content is unchanged. But the whole
+reason you'd escape *after doing something* in the staging/patch view is that you
+changed the content — you staged a hunk, or `d`-dropped one — so a numeric index
+now points at a different line. (§4 already conceded "the selection may be
+slightly off; no fix planned.")
+
+The right model: on escape, read the explorer view's **current selection at that
+moment** as a patch identity `(file, type, source-line, side)` — *after* the
+host's auto-advance has already moved it to a still-valid line — then find that
+identity in the freshly re-rendered main view and scroll it into view. This is
+the **inverse** of the diff-line-metadata primitive (identity → rendered row,
+rather than row → identity), and it is the **same operation** as the `-U`
+context-change scroll-preservation consumer (see diff-line-metadata-notes.md §1
+item 5). It also lets the `FocusedMainViewSnapshot` store *less* (the `OriginY` +
+main-view `SelectedLineIdx` become derivable), which is a simplification.
+
+Note the difference from the `-U` case's "anchor on the nearest surviving change
+line" fallback: that's for when context lines genuinely vanish. For staging-escape
+the host's auto-advance normally hands us a valid line directly; the
+nearest-surviving fallback is only for the degenerate cases below.
+
+### 12.2 Escape routing — which half to return to, or whether to at all
+
+The escape target is really `(context, identity)`: which focused-main half
+corresponds to where the explorer's selection ended up. The current prototype
+gets several cases wrong:
+
+- **Stage a non-last hunk** — selection auto-advances within unstaged → restore
+  there. The common, already-conceptually-fine case.
+- **Stage the *last* unstaged hunk** — unstaged becomes empty and the selection
+  crosses to the **staged** panel → escape should land in the **staged half**
+  (the secondary view), not unstaged. *Currently lands in the files panel — bug.*
+- **`<tab>` between unstaged/staged inside the staging view** — escape should
+  return to the half matching the side you're on. *Currently broken — bug.*
+- **Drop the last unstaged hunk with nothing staged** — the staging view
+  auto-closes; focus goes to the files panel. *Correct as-is*: the main view
+  would only show "No changed files", so there's no point focusing it. (Probably
+  not deliberate in the prototype, but it's the right outcome.)
+- **Drop a hunk in the custom patch builder (dropping it from the commit), or
+  "Remove patch from original commit" from the custom-patch menu** — the patch
+  builder *always* closes (it can't mutate the commit from within it), not just on
+  the last hunk. Re-focusing the main view is right, but here there is **no host
+  auto-advance**, so we'd have to advance the selection ourselves. Since these are
+  infrequent, acceptably focusing the **side panel** instead is a fine shortcut.
+
+So the return target is: unstaged-selection → main; staged-selection → secondary;
+no valid selection / empty → files panel; custom-patch-builder → side panel
+(shortcut) or self-advance.
+
+### 12.3 Decision: solve the three entangled problems in the prototype (next session)
+
+The new restore mechanism, the **§11 timing races**, and the
+**`BufferLineForViewLine` staleness trap** (diff-line-metadata-notes.md §8) are
+entangled: the identity-based restore reads/parses the buffer *while it is still
+loading*, which is exactly where both the races and the staleness bite. We
+decided **not** to defer these to productionization. A production plan written
+around three unsolved, entangled mechanisms isn't a plan; the prototype exists to
+retire that unknown cheaply (build-order §7). Resolve them here, and
+productionization becomes transcription.
+
+Attack order (dependency-first, to keep it from ballooning):
+
+1. **§8 staleness fix first.** A bounded *correctness* fix with a known shape
+   (snapshot `viewLines`+`lines` under one lock, or tie the read to the task that
+   produced the buffer). Needed regardless, and it's what makes
+   reading/parsing-during-load safe — which everything else depends on.
+2. **Characterize the §11 races early, timeboxed, before designing around them.**
+   The one real sink risk is that they're not yet characterised: we don't know if
+   each is a specific bad interleaving (bounded fix) or a fundamental tension in
+   the async-lazy-render model (e.g. no flicker-free first paint without buffering
+   a screenful first). That distinction changes the restore design *and* is itself
+   a key production-plan input, so pin it cheaply up front. This is "understand the
+   race to choose the right fix", not "decide whether to fix it" (we fix it).
+3. **The new restore mechanism**, which splits:
+   - **sync half** — the §12.2 routing (which context / side panel); largely
+     independent of timing, can progress in parallel.
+   - **async half** — a *predicate* scroll: generalize
+     `ScrollToOriginYForNextTask(y)` (§11) to "scroll to the row matching this
+     predicate, applied the first refresh at which it's satisfiable" (the read
+     loop scans the lines read so far via the metadata primitive; the fixed-`y`
+     case is then just a trivial predicate). This is the "keep parsing as it
+     loads" piece and is the part that sits on §8+§11.
+
+### 12.4 Memory
+
+`focused-main-view-flicker-timing-races` already covers the races. The escape
+restore reworking and the three-problem plan above are recorded here.