fix(review): keep unsent PR review comments across new commits (#1590)

[backnotprop]

Part of #1590. PR reviews only. Local, working-tree, jj, GitButler, P4 and workspace reviews behave byte-identically to today.

Problem

Code-review drafts are keyed by contentHash(rawPatch). When a teammate pushes to a PR, the key changes and the unsent comments become unreachable, even though the file is still on disk under the old hash.

Design

Key format. In PR mode the draft is also stored under prDraftTargetKey(meta, scope):

pr-<sha256("v1|github|<host>|<owner>/<repo>|<number>|<scope>")[0:16]>
pr-<sha256("v1|gitlab|<host>|<projectPath>|<iid>|<scope>")[0:16]>

• Host, owner, repo and project path are lower-cased.
• scope is layer or full-stack. On a stacked PR these are two different patches with different line coordinates, so each is its own target.
• A pr- key can never collide with a 16-hex content hash.

Storage runs on the server. All of it is in one shared module, packages/shared/review-draft.ts, vendored to Pi. Each review server holds one createReviewDraftSession(). /api/draft, /api/feedback and /api/exit go through it. Without a target key, every call is the plain draft.ts call, including the historical always-ok response to a save.

• Save writes the body unchanged under the patch key. It also writes a copy under the target key, stamped with patchKey (the patch it was saved on) and patchKeys (every patch it was ever saved on). Any client-supplied values for these stamps (or for patchChanged) are stripped.
• Load tries the patch key first. It returns that copy unless the target copy has a strictly higher draftGeneration, which covers a force-push back to an earlier patch whose old file is stale. Otherwise it returns the target copy. If that copy was saved on a different patch, the response includes patchChanged: true. The patch-key lists never leave the server.
• Delete removes the patch key, the target key, and every remembered patch key, whether or not a generation is given.
• Decisions (/api/feedback, /api/exit) also clear every PR target this server session saved to or restored from. A client DELETE (clear-all, dismiss) touches only the target on screen.
• Tombstones. The target key's tombstone guards the whole logical draft:
  • A save at or below it is rejected under both keys. In PR mode the server answers 409 { ok: false, found, draftGeneration } instead of swallowing it.
  • A patch-key copy at or below it is not served. So a stale tab on a pre-push patch cannot bring back a submitted draft.
• In-place switches. /api/pr-switch and /api/pr-diff-scope responses carry draftState: { found, draftGeneration } for the new keys. The client's adoptDraftTarget raises its generation counter to that value; without this, every save after switching onto a previously submitted target would be refused. When the target holds a draft, the hook loads it and auto-merges its items into the session: no banner, just a small non-blocking toast ("Restored N unsent comments for this PR"). Ids the session already holds, or deleted earlier in this session, are skipped. The merged comments are re-checked against the diff on screen, and autosave then saves the merge normally. The only wait is while that read is in flight: autosave skips writing under the new target, because it would overwrite a draft it has not read yet. Each read makes up to two attempts, each with a 5s timeout, so a hung GET /api/draft counts as a failure instead of pausing autosave. If both attempts fail, autosave still never writes over the unread draft. Instead it retries the read on each later save attempt, and saves normally once a read succeeds. A newer switch or an unmount supersedes the read, and a per-switch counter turns a late load from an earlier target into a no-op, so nothing can get stuck. The page-load restore banner is unchanged.

Anchor checks run on the client, which already restores drafts and holds the parsed diff. In PR mode, withPRContext (the single path every code annotation is created through) records three fields on each line comment:

• anchorText: the anchored lines' text, taken from the patch hunks.
• anchorContext: two lines before and two after on the same side, plus the hunk header's function context (for example function load() {). A common line like } or return null; is therefore not "still valid" by coincidence, even when the same block appears in another function.
• anchorSnapshot: the review snapshot id whose line coordinates the comment uses.

reanchorCodeAnnotations runs on restore, and again whenever the snapshot on screen changes (a push picked up, a layer/full-stack switch, an in-place PR switch):

• If a line comment's text and context still match at the same side and line numbers, it is re-stamped with the current snapshot.
• Otherwise it gets outdated: true and keeps its old line numbers. Comments are never dropped and never moved to a guessed line.
• Unstamped comments (older drafts) are checked only when the server reports patchChanged. With no anchor fields, they are marked outdated. That is the safe side: nothing can confirm their lines still hold the same code, and an unchanged patch still restores exactly as today.
• File comments, scope:'general' comments, and comments bound to another PR or scope carry over unchanged.

What gets posted where.

• The app remembers the latest layer snapshot for each PR it has shown.
• buildReviewSubmission posts a line comment inline only if canPostInline holds: the comment is not outdated, and it is stamped with that PR's known snapshot. An unstamped comment is trusted only for the PR on screen.
• Everything else goes in the review body, labelled and including the code it was written on (anchorText).

Outdated comments in the UI:

• Not drawn on the diff; the review-state context filters them.
• Listed in the sidebar with a small "Outdated" chip (the same style as the plan editor's "Unanchored" chip), with Edit and Delete.
• Clicking the card opens the comment's file without issuing a scroll request that could never resolve.
• The agent export labels them [Outdated — the code changed after this comment].

Migration. Drafts saved only under a content hash still restore when the patch is unchanged. The first autosave in a PR session also writes the target key. Hash-only drafts are not scanned or migrated, because nothing on disk says which PR they belong to.

Out of scope:

• Local and other non-PR reviews. They still miss the draft when the diff changes.
• Re-anchoring moved lines to a new position.
• Persist per-file viewed state across review iterations using a per-file content hash #1136 viewed-file state.

Review fixes (second commit)

Addresses the independent review:

1. A submit after an in-place PR or scope switch left the earlier target key alive, so its comments came back after a push. The session now clears every target it saved to or restored from.
2. Switching onto a target with an old tombstone made every save fail silently. Switching onto a target with an unsent draft could overwrite it. Fixed with draftState, adoptDraftTarget plus the merge banner, and 409 on a rejected save.
3. Matching on the line text alone gave false "still valid" results on common lines. Two lines of context on each side must now match too.
4. Comments belonging to another PR or scope skipped the check and could be posted inline at stale lines. Snapshot stamps plus canPostInline fix this, and outdated comments posted in the body now include the code they were written on.
5. A delete without a generation left patch copies from two or more pushes ago. The target copy now remembers every patch key and deletes them all.
6. Clicking an outdated card now opens its file without a dead scroll target.

Review fixes (third and fourth commits)

The third commit held autosave while a merge offer was pending after a switch. Review found two ways that hold could get stuck: an unanswered banner stopped all autosave for the rest of the session, and a second switch while an offer was pending carried the stale offer onto the new PR. The fourth commit replaces the hold with a simpler design:

1. Auto-merge instead of offering. After switching onto a target with an unsent draft, its items are merged into the session (with a toast) and saved normally. Nothing waits on the reviewer.
2. One bounded wait. Autosave skips writing under the new target only while that target's single draft load is in flight. It resumes when the load settles, on success, failure, or supersede, so it cannot get stuck.
3. Stale loads. A per-switch counter ignores a late load from an earlier target, and every switch resets the previous switch's state.
4. Deleted comments (kept from the third commit): ids deleted this session are never merged back.
5. Hunk header context (kept from the third commit): The anchor context also compares the hunk header's function context. A missing header on either side only matches another missing header, so this check can only turn a comment outdated, never mark it valid.

Final items (fifth commit)

The branch first merges the latest origin/main in a merge commit (no force-push).

1. Timeout. Each attempt to read the switched-onto PR's draft is bounded by TARGET_LOAD_TIMEOUT_MS (5s). A hung read takes the same failure path as an error.
2. No overwrite on a failed read. A failed read is retried once. If both attempts fail, the target is marked unreadable: autosave never writes the session's state over that draft. The rest of the session keeps working, the next save attempt re-reads the draft, and a successful read merges it and saves. Moving to another PR also clears the unreadable state. This removes the "accepted loss" from the previous round.
3. Stale debounced save. A save that was scheduled before the read finished is cancelled and redone after the merged comments are in state, so it cannot save the pre-merge state.

To test in-place switches without a platform CLI or network, both review servers accept a prFetcher override, the same kind of seam as the existing prReviewSubmitter.

Tests

• packages/shared/review-draft.test.ts (19):
  • key identity rules
  • non-PR mode writes only the hash file
  • fast path, and a changed patch returns patchChanged
  • a newer target copy beats a stale patch copy
  • the server stamps cannot be forged or leaked
  • delete clears everything, with and without a generation (three pushes)
  • a late save after delete is rejected under both keys
  • a 404 carries the tombstone generation
  • an orphan patch copy at or below the tombstone is not served
  • legacy hash-only drafts still restore
  • a decision clears every target saved or restored (PR switch plus scope switch)
  • a client DELETE clears only the current target
  • a non-PR session remembers nothing
  • reviewDraftState reports the floor and a live draft
• packages/server/review-draft-pr.test.ts and its Pi mirror apps/pi-extension/serverReview-draft-pr.test.ts (9 each, real servers, temp PLANNOTATOR_DATA_DIR, PLANNOTATOR_BROWSER=/usr/bin/true):
  • reopening after a push restores with patchChanged
  • an unchanged patch restores exactly
  • feedback and exit after a push leave both patches at 404
  • a stale tab's late save gets 409 and nothing comes back
  • a local review is unchanged
  • submitting after an in-place A→B switch leaves A and B at 404 after a push
  • switching onto a previously submitted PR reports the floor, a stale save gets 409, and a floor-adopting save works
  • switching onto a PR with an unsent draft reports it
• packages/review-editor/utils/codeAnnotationAnchor.test.ts (12):
  • side and line reading, and context-expansion lines count as unknown
  • keep (re-stamped), changed, file-gone and legacy cases
  • a } whose surroundings changed is marked outdated
  • an identical block under a different function header is marked outdated
  • file and general comments pass through, and so do other-PR comments
  • in-session re-checks only touch comments stamped with a different snapshot, and return the same array when nothing changed
  • canPostInline rules
  • sidebar navigation for outdated comments
• packages/review-editor/utils/outdatedAnnotations.test.ts (4):
  • the export labels only outdated comments
  • outdated comments are never posted inline
  • an unverifiable other-PR comment goes in the body with its code, while a verified one stays inline
• packages/ui/codeAnnotationDraftPersistence.test.tsx (+12, DOM):
  • patchChanged passes through the hook
  • adopting the floor lets the next autosave land past an old tombstone
  • no-edit switch: the target's items are merged into the session and saved on disk, no banner appears, and later edits keep saving
  • empty session: the target's draft is merged in, not deleted
  • a comment deleted this session is not merged back
  • switching back onto the session's own items merges nothing
  • A→B (draft, slow load)→C (no draft): nothing stale is merged and C keeps saving
  • A→B (slow)→C (draft): only C's items merge, and B's late load is ignored
  • a failed read is retried once, and a successful retry merges and resumes saving
  • a hung read times out into the same retry path
  • when both attempts fail, the unread draft is never overwritten; the next change retries the read and saves once it succeeds
  • an unreadable target does not block the session: switching on resumes saving under the next target
• packages/review-editor/components/ReviewSidebar.outdated.test.tsx (3, DOM, in the DOM_TESTS step): chip, edit, delete.

Results on the branch head:

• bun run typecheck: clean. It covers core, shared, ai, server, ui, guide-viewer, guides-show, strict-consumer and pi-extension, after vendor.sh.
• Full bun test: 5047 pass, 1204 skip, 0 fail (6251 tests, 542 files).
• DOM step (DOM_TESTS=1 bun test --isolate over the workflow's list): 1283 pass, 0 fail (173 files).
• apps/review build: succeeds.
• guides.show manifest: regenerated. The first commit changed only the CSS hash. The later commits leave it unchanged.
• tsc -p packages/review-editor is not part of CI and already reports many errors on main (bun:test types, import.meta.env, pierre type drift). It reports nothing new in the changed code beyond the missing bun:test types in the new test files.

Risks and open questions

• A hash-only draft from before this change, on a PR that has since been pushed, is still not found. Finding it would take a directory scan by prUrl, which is left out.
• Lines added through context expansion have no anchor text, so after a push they are marked outdated even if the code did not change. This is conservative.
• Outdated or unverifiable comments posted to a PR go in the review body as text. They do not become GitHub "outdated" review threads.
• After an in-place switch onto a PR with an unsent draft, that draft's comments are merged into the session automatically and saved, with nothing waiting on the reviewer. If the draft cannot be read, the session's state is never written over it. While it stays unreadable, the session's edits are not saved under that PR's keys; they are still in the open tab and are saved as soon as a read succeeds. The page-load banner is unchanged: an edit made before answering it still overwrites the offered draft, which is existing behavior this PR does not change.
• A client clear-all only deletes the draft on screen. A comment cleared while viewing PR B that belonged to PR A's saved copy can reappear when A is reopened, until a decision clears it. Clearing every target on a clear-all would also delete another PR's unsent work, so this was left narrow.