# PR 23519 review B, round 2 (read-only, immutable objects)

PR: [#23519](https://github.com/pingdotgg/t3code/pull/23519) / [GitCafe #23519](https://git.cafe/pingdotgg/t3code/pulls/23519), `fix/issue-16534` → `main`, merge-base `43879d9702`.

## Head drift (again)
- The brief named `44215dadd1`. GitCafe now reports head **`a9dcdf37fd`** ("fix(fixtures): translate shell resume cursors after replay").
- That commit changes only `scripts/fixtures/issue-16534/replay.js` (+21/−5). The five product files are byte-identical to `44215dad`.
- So "no further implementation changed" holds for product code, but the head did move. I reviewed the full diff at `44215dad`, plus the delta to `a9dcdf37`.

## Verdict: MERGE (code), pending CI and the separately tracked capture evidence
- **Product code.** I found no concrete regression in `AppSidebarLayout.tsx`, `Sidebar.tsx`, `LegacySidebar.tsx`, `sidebar/routeReveal.ts`, or the test. The change is worth having: the active thread is revealed after back/forward, the palette, links, Settings, and drawer remount. The shape is close to minimal: one mutable 2-field object owned above the remount boundary, plus one effect per sidebar.
- **Fixture tooling.** Committed seeding/replay is resolved by the user's explicit mandate. I am not re-raising it.
- **This is not a visual sign-off.** Runtime capture is still underway, and the proof-matrix items are evidence debt that the capture task tracks. I verified nothing visually.
- **CI on `a9dcdf37`.** 9 passed, 20 pending, 0 failing at review time. The GitHub conflict check is still pending for the new base. Treat MERGE as conditional on green CI.

## Product code: fresh pass
- **`routeReveal.ts`.** A null key is ignored. A new key sets both `observed` and `pending`, and the same key returns the existing pending value. `complete` clears pending. Mutation happens only inside effects (`revealRouteRow` is called from `useEffect`), so it is StrictMode-safe. The object is shared between the legacy and v2 sidebars, which is harmless because only one is mounted at a time.
- **v2 `Sidebar.tsx` L1070–1105.**
  - The route row is always rendered when it is visible under the current scope. `useSidebarThreads` keeps it rendered through collapsed shelves (`routeThreadOnly`) and past the settled tail (L429–446). Waiting for `aria-current="page"` therefore cannot stall on a hidden-by-shelf row.
  - `aria-current="page"` within `[data-sidebar="content"]` comes only from sidebar rows and draft blocks (`Sidebar.logic.ts:56`). Breadcrumbs that also use it render outside the sidebar.
  - Scope widening runs only on a route-change reveal, and only when the thread is hidden by scope but visible under `null` scope. Archived threads never widen scope.
  - Mount-only reveals never widen scope, so a manually chosen scope survives a drawer or Settings remount.
- **Legacy `LegacySidebar.tsx`.**
  - Two hooks compute the overflow and retained-active-row logic: `useProjectThreadPanel` (L1204–1229) and `useLegacySidebarThreads` (L3213–3226). I traced both again. They yield the same keys in the same order, so `routeRowRendered`, jump shortcuts, and the rows on screen agree.
  - "Show more" no longer appears when the only row past the cut is the active one.
  - The reveal waits on `routeRowRendered`, which covers late environment shells.
- **Minor, non-blocking (unverified at runtime).** In v2, a draft-only route whose key is not yet in `useQuietThreadShells()` hits `thread === undefined` and returns before the DOM lookup.
  - Those routes get no reveal until the draft persists.
  - This is not a regression, since main had no reveal at all.
  - Correction to my round-1 note "draft routes work too": that is true for the DOM selector, but the shell lookup gates it.
- **Behavioural notes (intended).** Widening persists "All" and clears the multi-selection, as the PR body states. A deferred reveal during search can widen scope after a scope that was picked manually mid-search. This is rare and consistent with "waits for search to clear".
- **Performance.** The v2 effect re-runs on every `threads` change. When nothing is owed it does one map-free early return. Owed work does one `threads.find` plus one `querySelector`. That is negligible, and there are no animations or extra wire traffic.
- **Compatibility and sensitive surfaces.** No contracts, RPC, server, persistence, or auth changes. Web and desktop share this renderer. Mobile has separate navigation and is out of scope; I did not check it.
- **Regex and parsers.** No product regex was added. The only fixture regex is anchored and alternation-only, so it runs in linear time and needed no benchmark. The `replay.js` parse loops are bounded by frame size and are fixture-only.

## Fixture delta `44215dad` → `a9dcdf37` (replay.js)
- **What changed.**
  - `send` now translates `subscribeShell.afterSequence` from client space back to server space by subtracting the socket's latest stream offset.
  - The new stream starts at the client cursor and carries that offset.
  - The frame is re-serialised before the native send, in NDJSON or JSON form.
- **Correctness.**
  - `parseFrame` always returns an array when `ndjson` is set, so `value.map` is safe.
  - The class field `this.latestShellStream` and the closure `latestShellStream` are distinct and used correctly.
  - The `sequence` and `offset` bookkeeping stays in client space, consistent with the incoming `node.sequence += stream.offset`.
  - A new socket starts at offset 0. Its comment relies on a new RPC session reloading the HTTP shell. That assumption is plausible, but I did not exercise it.
- **Ordering.** The fixture now sends after it mutates, where before it sent first. This keeps the native send on parse failure (the catch falls through to `super.send(data)`).
- **Scope.** None of this is loaded by product code.

## Test gaps (meaningful, non-blocking)
- **Duplicated legacy overflow and retained-row math (two hooks).** This is the most likely place for drift. A single pure helper with a table test would cover both copies.
- **v2 widen vs. no-widen decision.** This is component-effect logic and is best covered by the integrated capture, which is tracked separately.
- **The 2 `routeReveal` tests** cover the real Settings null-key regression. They are adequate.

## Verification
- **Read from git objects only:** the full diff `43879d97..44215dad` and the delta to `a9dcdf37`. The worktree has capturer edits (`apps/web/index.html`, untracked `public/sidebar-capture-inert.html`). I did not touch them.
- **Not run this round:** tests, lint, typecheck, browser. Round 1 ran `Sidebar.logic.test.ts`, 57 passed, on product-identical code.
- **CI:** `cafe pr status` on `a9dcdf37` showed 0 failing and 20 pending.

## FRICTION
- The head moved again (`44215dad` → `a9dcdf37`) after the brief said no further changes. The brief should pin "review at X; report if head ≠ X". Only fixture code changed, so the code verdict is unaffected.
- The capturer's uncommitted edits in the shared review worktree mean the working tree is not the head. Reviews must use `git show`, as the brief anticipated.