Skip to content

Commit 9f8ffed

Browse files
committed
feat(review): diff staleness detection — 'out of date · Refresh' notice
Files changing mid-review (an agent editing/committing while the user reviews) silently left a stale snapshot on screen. Now the client polls a cheap server-side fingerprint check every 5s and shows a non-blocking amber notice in the toolbar; the user refreshes when ready (never automatic — annotations are line-anchored). Architecture (write-once, both servers inherit): - packages/shared: getGitDiffFingerprint (per git mode: commit-anchored → rev-parse only; working-tree → hash of the same diff the patch is built from + untracked contents, capped), getJjDiffFingerprint (jj snapshots the working copy per command, so commit ids are content-complete), WorkspaceReviewSession.getFingerprint (combines all children), VcsProvider.getDiffFingerprint + VcsApi dispatch. P4: no fingerprint → always fresh (no banner) in v1. All git probes use --no-optional-locks so background polling never races concurrent git add/commit. - Bun + Pi servers: fingerprint captured beside every patch snapshot (startup + all switch endpoints); GET /api/diff/fresh recomputes and compares. PR layer scope is platform-side → never stale locally; PR full-stack fingerprints the local checkout's HEAD. - UI: useDiffFreshness hook (5s poll, paused while hidden, off in demo), amber toolbar pill with Refresh (existing switch path with preserveFile — reviewer stays on their file; PR full-stack re-runs scope switch) and Dismiss (stays dismissed until a DIFFERENT change lands — endpoint returns the probe fingerprint to tell those apart). Tested against a real throwaway git repo (8 cases incl. the already- modified-file-modified-again case that git status alone cannot detect).
1 parent 791ca06 commit 9f8ffed

14 files changed

Lines changed: 647 additions & 1 deletion

File tree

apps/pi-extension/plannotator-browser.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
reviewRuntime,
1111
detectManagedVcs,
1212
getVcsContext,
13+
getVcsDiffFingerprint,
1314
getVcsFileContentsForDiff,
1415
canStageFiles,
1516
runVcsDiff,
@@ -108,6 +109,7 @@ async function buildLocalWorkspaceReview(
108109
getVcsContext,
109110
runVcsDiff,
110111
getVcsFileContentsForDiff,
112+
getVcsDiffFingerprint,
111113
canStageFiles,
112114
stageFile,
113115
unstageFile,

apps/pi-extension/server.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ export {
3232
detectVcs,
3333
getGitContext,
3434
getVcsContext,
35+
getVcsDiffFingerprint,
3536
getVcsFileContentsForDiff,
3637
prepareLocalReviewDiff,
3738
resolveInitialDiffType,

apps/pi-extension/server/serverReview.ts

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ import {
109109
canStageFiles,
110110
detectRemoteDefaultCompareTarget,
111111
getVcsContext,
112+
getVcsDiffFingerprint,
112113
getVcsFileContentsForDiff,
113114
resolveVcsCwd,
114115
reviewRuntime,
@@ -288,6 +289,44 @@ export async function startReviewServer(options: {
288289
let currentBase = options.initialBase || detectedCompareTarget();
289290
let baseEverSwitched = false;
290291

292+
// --- Diff staleness fingerprint (mirrors packages/server/review.ts) -------
293+
// Captured beside every patch snapshot; GET /api/diff/fresh recomputes and
294+
// compares so the client can show a "diff out of date — refresh" notice when
295+
// files change mid-review. Best-effort: null = "cannot fingerprint" and is
296+
// reported fresh, never stale.
297+
let currentFingerprint: string | null = null;
298+
const computeDiffFingerprint = async (): Promise<string | null> => {
299+
try {
300+
if (workspace) return await workspace.getFingerprint();
301+
if (isPRMode) {
302+
if (currentPRDiffScope === "layer") {
303+
// Platform-computed diff — immutable locally; recaptured on pr-switch.
304+
return `pr-layer:${prMeta?.url ?? ""}`;
305+
}
306+
const fullStackCwd =
307+
(options.worktreePool && prMeta ? options.worktreePool.resolve(prMeta.url) : undefined) ??
308+
options.agentCwd;
309+
return await getVcsDiffFingerprint("last-commit", currentBase, fullStackCwd);
310+
}
311+
if (!hasLocalAccess) return null;
312+
return await getVcsDiffFingerprint(
313+
currentDiffType as DiffType,
314+
currentBase,
315+
options.gitContext?.cwd,
316+
{ hideWhitespace: currentHideWhitespace },
317+
);
318+
} catch {
319+
return null;
320+
}
321+
};
322+
// Fire-and-forget capture: never delays the snapshot response it describes.
323+
const captureDiffFingerprint = (): void => {
324+
void computeDiffFingerprint().then((fingerprint) => {
325+
currentFingerprint = fingerprint;
326+
});
327+
};
328+
captureDiffFingerprint();
329+
291330
// Fire-and-forget: query the remote for its actual default branch.
292331
if (options.gitContext && !options.initialBase && !isPRMode) {
293332
detectRemoteDefaultCompareTarget(options.gitContext.cwd, sessionVcsType).then((remote) => {
@@ -619,6 +658,27 @@ export async function startReviewServer(options: {
619658
semanticDiff: await getSemanticDiffAdvert(),
620659
serverConfig: getServerConfig(gitUser),
621660
});
661+
} else if (url.pathname === "/api/diff/fresh" && req.method === "GET") {
662+
// Cheap staleness probe — has the underlying VCS state changed since
663+
// the current diff snapshot was computed? Best-effort: anything that
664+
// cannot be fingerprinted reports fresh (no banner).
665+
const baseline = currentFingerprint;
666+
if (baseline == null) {
667+
json(res, { fresh: true });
668+
return;
669+
}
670+
const probe = await computeDiffFingerprint();
671+
// A diff switch landing mid-probe replaces the snapshot (and its
672+
// fingerprint); report fresh and let the next poll compare against
673+
// the new baseline.
674+
if (currentFingerprint !== baseline) {
675+
json(res, { fresh: true });
676+
return;
677+
}
678+
const fresh = probe == null || probe === baseline;
679+
// The probe fingerprint lets the client distinguish "still the same
680+
// staleness I dismissed" from "ANOTHER change landed since".
681+
json(res, { fresh, ...(fresh ? {} : { fingerprint: probe }) });
622682
} else if (url.pathname === "/api/semantic-diff" && req.method === "GET") {
623683
json(res, await getSemanticDiff(url));
624684
} else if (url.pathname === "/api/diff/switch" && req.method === "POST") {
@@ -646,6 +706,7 @@ export async function startReviewServer(options: {
646706
currentDiffType = workspace.diffType;
647707
currentError = snapshot.error;
648708
draftKey = contentHash(currentPatch);
709+
captureDiffFingerprint();
649710

650711
json(res, {
651712
rawPatch: currentPatch,
@@ -673,6 +734,7 @@ export async function startReviewServer(options: {
673734
currentBase = base;
674735
baseEverSwitched = true;
675736
currentError = result.error;
737+
captureDiffFingerprint();
676738

677739
// Recompute gitContext for the effective cwd so the client's
678740
// sidebar reflects the worktree we're now reviewing.
@@ -722,6 +784,7 @@ export async function startReviewServer(options: {
722784
currentGitRef = originalPRGitRef;
723785
currentError = originalPRError;
724786
currentPRDiffScope = "layer";
787+
captureDiffFingerprint();
725788
json(res, {
726789
rawPatch: currentPatch,
727790
gitRef: currentGitRef,
@@ -750,6 +813,7 @@ export async function startReviewServer(options: {
750813
currentGitRef = result.label;
751814
currentError = undefined;
752815
currentPRDiffScope = "full-stack";
816+
captureDiffFingerprint();
753817
json(res, {
754818
rawPatch: currentPatch,
755819
gitRef: currentGitRef,
@@ -785,6 +849,7 @@ export async function startReviewServer(options: {
785849
currentPRDiffScope = "layer";
786850
draftKey = contentHash(pr.rawPatch);
787851
prListCache = null;
852+
captureDiffFingerprint();
788853

789854
prStackInfo = getPRStackInfo(pr.metadata);
790855
if (prStackTreeCache.has(body.url)) {

apps/pi-extension/server/vcs.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ export const {
9898
prepareLocalReviewDiff,
9999
runVcsDiff,
100100
getVcsFileContentsForDiff,
101+
getVcsDiffFingerprint,
101102
canStageFiles,
102103
stageFile,
103104
unstageFile,

packages/review-editor/App.tsx

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ import { StackedPRLabel } from './components/StackedPRLabel';
6060
import { PRSelector } from './components/PRSelector';
6161
import { PRSwitchOverlay } from './components/PRSwitchOverlay';
6262
import { usePRStack } from './hooks/usePRStack';
63+
import { useDiffFreshness } from './hooks/useDiffFreshness';
6364
import { usePRSession, type PRSessionUpdate } from './hooks/usePRSession';
6465
import { useAnnotationFactory } from './hooks/useAnnotationFactory';
6566
import { DEMO_DIFF } from './demoData';
@@ -1459,6 +1460,29 @@ const ReviewApp: React.FC = () => {
14591460
fetchDiffSwitch(diffType, selectedBase, { preserveFile: true });
14601461
}, [diffHideWhitespace, origin, reviewMode]); // eslint-disable-line react-hooks/exhaustive-deps
14611462

1463+
// --- Diff staleness ---------------------------------------------------------
1464+
// Files changing mid-review (an agent editing/committing while the user
1465+
// reviews) make the snapshot on screen stale. The hook polls the server's
1466+
// cheap fingerprint check; the toolbar shows a non-blocking notice and the
1467+
// user refreshes when THEY are ready — never automatically (annotations are
1468+
// line-anchored; rug-pulling the diff under them is worse than staleness).
1469+
const diffFreshness = useDiffFreshness({
1470+
enabled: !!origin,
1471+
resetKey: diffData?.rawPatch ?? '',
1472+
});
1473+
1474+
const handleRefreshStaleDiff = useCallback(() => {
1475+
if (prMetadata) {
1476+
// Only the full-stack scope can go stale locally — the layer diff is
1477+
// computed platform-side and its fingerprint never flips.
1478+
if (prDiffScope === 'full-stack') handlePRDiffScopeSelect('full-stack');
1479+
return;
1480+
}
1481+
// Same params, fresh snapshot. preserveFile keeps the reviewer on the
1482+
// file they were reading.
1483+
void fetchDiffSwitch(diffType, selectedBase, { preserveFile: true });
1484+
}, [prMetadata, prDiffScope, handlePRDiffScopeSelect, fetchDiffSwitch, diffType, selectedBase]);
1485+
14621486
// Select annotation - switches file if needed and scrolls to it.
14631487
// isAllFilesActive is read through the ref (declared with the state): this
14641488
// handler is baked into Pierre slot portals, which only republish on item
@@ -2150,6 +2174,31 @@ const ReviewApp: React.FC = () => {
21502174
</div>
21512175
)}
21522176

2177+
{/* Diff staleness notice — files changed since this snapshot
2178+
was computed (agent editing mid-review). Non-blocking; the
2179+
user refreshes when ready. */}
2180+
{diffFreshness.isStale && !isLoadingDiff && (
2181+
<div className="flex items-center gap-2 text-xs text-amber-700 dark:text-amber-300 px-2 py-1 bg-amber-500/10 rounded border border-amber-500/25">
2182+
<span className="hidden md:inline">Diff out of date</span>
2183+
<span className="md:hidden">Stale</span>
2184+
<button
2185+
onClick={handleRefreshStaleDiff}
2186+
className="font-medium underline underline-offset-2 hover:text-amber-900 dark:hover:text-amber-100 transition-colors"
2187+
title="Re-run the diff with the current settings"
2188+
>
2189+
Refresh
2190+
</button>
2191+
<button
2192+
onClick={diffFreshness.dismiss}
2193+
className="text-amber-700/60 dark:text-amber-300/60 hover:text-amber-900 dark:hover:text-amber-100 transition-colors leading-none"
2194+
title="Dismiss"
2195+
aria-label="Dismiss stale diff notice"
2196+
>
2197+
×
2198+
</button>
2199+
</div>
2200+
)}
2201+
21532202
{/* Agent mode: Close/SendFeedback flip + Approve */}
21542203
{!platformMode ? (
21552204
<AgentReviewActions
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
import { useCallback, useEffect, useState } from 'react';
2+
3+
const POLL_INTERVAL_MS = 5000;
4+
5+
export interface DiffFreshness {
6+
/** True when the underlying files changed since the current diff snapshot
7+
* was computed AND the user hasn't dismissed this particular staleness. */
8+
isStale: boolean;
9+
/** Hide the notice for the CURRENT staleness. A further change (different
10+
* server fingerprint) re-shows it; a diff refresh resets everything. */
11+
dismiss: () => void;
12+
}
13+
14+
/**
15+
* Polls `GET /api/diff/fresh` while the review is open. The server compares a
16+
* cheap VCS fingerprint captured when the diff snapshot was computed against
17+
* the repo's state NOW — files changing mid-review (the normal agent-editing-
18+
* while-you-review workflow) flips `fresh` to false.
19+
*
20+
* Polling is timer-based on purpose: in this product the files change while
21+
* the user is actively IN the review (an agent works underneath them), so a
22+
* focus/visibility trigger would miss the case that matters. Ticks are
23+
* skipped while the document is hidden, and the whole hook no-ops in demo
24+
* mode (`enabled: false`).
25+
*/
26+
export function useDiffFreshness({
27+
enabled,
28+
resetKey,
29+
}: {
30+
enabled: boolean;
31+
/** Identity of the current diff snapshot (e.g. the rawPatch string). A new
32+
* snapshot (refresh / switch) clears staleness + dismissal and resumes. */
33+
resetKey: string;
34+
}): DiffFreshness {
35+
const [staleFingerprint, setStaleFingerprint] = useState<string | null>(null);
36+
const [dismissedFingerprint, setDismissedFingerprint] = useState<string | null>(null);
37+
38+
// New snapshot → clean slate.
39+
useEffect(() => {
40+
setStaleFingerprint(null);
41+
setDismissedFingerprint(null);
42+
}, [resetKey]);
43+
44+
useEffect(() => {
45+
if (!enabled) return;
46+
let cancelled = false;
47+
let timer: ReturnType<typeof setTimeout> | null = null;
48+
49+
const schedule = () => {
50+
if (!cancelled) timer = setTimeout(tick, POLL_INTERVAL_MS);
51+
};
52+
53+
const tick = async () => {
54+
// Nobody is looking — don't burn VCS commands on a hidden window.
55+
if (document.hidden) {
56+
schedule();
57+
return;
58+
}
59+
try {
60+
const res = await fetch('/api/diff/fresh');
61+
if (!cancelled && res.ok) {
62+
const data = (await res.json()) as { fresh: boolean; fingerprint?: string };
63+
// Keep polling even while stale: a reverted edit flips back to
64+
// fresh, and a FURTHER change updates the fingerprint so a
65+
// dismissed notice can reappear.
66+
setStaleFingerprint(data.fresh ? null : data.fingerprint ?? 'stale');
67+
}
68+
} catch {
69+
// Transient/network/server-gone: ignore — staleness is best-effort.
70+
}
71+
schedule();
72+
};
73+
74+
schedule();
75+
return () => {
76+
cancelled = true;
77+
if (timer) clearTimeout(timer);
78+
};
79+
}, [enabled, resetKey]);
80+
81+
const dismiss = useCallback(() => {
82+
setDismissedFingerprint(staleFingerprint);
83+
}, [staleFingerprint]);
84+
85+
return {
86+
isStale: staleFingerprint != null && staleFingerprint !== dismissedFingerprint,
87+
dismiss,
88+
};
89+
}

packages/server/review-workspace.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {
22
canStageFiles,
33
getVcsContext,
4+
getVcsDiffFingerprint,
45
getVcsFileContentsForDiff,
56
runVcsDiff,
67
stageFile,
@@ -35,6 +36,7 @@ const workspaceRuntime = {
3536
getVcsContext,
3637
runVcsDiff,
3738
getVcsFileContentsForDiff,
39+
getVcsDiffFingerprint,
3840
canStageFiles,
3941
stageFile,
4042
unstageFile,

0 commit comments

Comments
 (0)