Repository navigation
Give 3D Diff the 2D panel's four scopes #34
Description
Part of #1
Question
3D Diff shows one thing: the branch's commits, stepped. The 2D Diff panel offers four scopes. Give 3D the same four. The base-ref picker is deliberately not here — it is the follow-on ticket, because the first question below decides how big it is.
Design settled by #32; read its resolution comment before starting, since three of that ticket's stated premises were wrong.
The four scopes
| Entry | Where the data comes from |
|---|---|
| Working tree | the overlay's existing working-tree step, shown as a single frame, no scrubber |
| Branch changes | new: a server-computed net aggregate over the range — see below |
| Latest turn | activeThread.checkpoints — client-side, no fetch |
| Turn › | the same, one entry per checkpoint |
The commit scrubber stays, as a drill-down of Branch changes rather than a fifth scope. The other three scopes are single frames with no scrubber.
Branch changes needs server work
This is the one place #32 said "presentation only" and was wrong. #29 removed step accumulation — every step is one isolated commit (DiffSurface.tsx:92-98) — so there is no aggregate frame to reuse. And a client sum of steps would be churn, while the 2D panel's branch entry shows net; #28 measured that they disagree for any file touched more than once.
So the aggregate is computed server-side from one git diff --numstat <base>...HEAD and carried as a new field on DiffOverlay. Three dots, matching the 2D panel's branch diff — note readSteps uses two dots for git log on purpose (DiffOverlay.ts:215-218), and that asymmetry is correct.
Height for the scrubber's per-commit frames stays churn on the absolute log curve #33 landed (4.83 * ln(1 + churn / 7), columns.ts) — not linear, which an earlier draft of this line said; linear against the busiest file was the bug #33 fixed, and it put two thirds of every city on the floor. The aggregate frame is net rather than churn, and that is a deliberate difference in what the two frames mean, not an inconsistency to reconcile. It reaches the same diffColumns and therefore the same curve.
Turn scopes are free, and their paths already line up
Checked rather than assumed, per the map's vocabulary rule. OrchestrationCheckpointFile is { path, kind, additions, deletions } (orchestration.ts:285), already the shape diffColumns takes, and already on the thread the client holds via useTurnDiffSummaries. Paths come from parsePatchFiles (checkpointing/Diffs.ts:20) with a//b/ stripped, repo-root-relative (Diffs.test.ts:32-33) — the citymap's vocabulary. No ghost columns.
Two things still to watch at runtime rather than in the fixtures:
- A checkpoint reports a rename's new path only, while the overlay runs
--no-renames. Minor, but the two scopes will read a rename differently. - Checkpoint paths are relative to the git root the checkpoint was captured at.
diff3dCwdisgitStatusCwd(ChatView.tsx:2456) and the overlay resolves to that repo's root (DiffOverlay.ts:150-156). Guard the mismatch rather than assuming it away — this is the same shape as the subdirectory bug Serve the commit-stepped diff overlay #28 was corrected for.
The surface takes a thread ref
Sub-question 2 from #32 answers itself: activeThreadRef is in scope two lines above the mount (ChatView.tsx:5711-5716), so keying by cwd was a prop choice. Pass both. Turn scopes disable — with a tooltip, per the map's standing UX note — when there is no thread.
Settled: 3D uses diffPanelStore, it does not keep its own
This was going to be a question. The dev settled it as a principle instead, and it is now on the map: a 3D surface is a different renderer of an existing T3 query, not a new feature — same store, same settings, same server call, only the drawing differs.
So the scope selection lives in diffPanelStore (diffPanelStore.ts:9-23), the same record the 2D panel writes. Not a copy, not a subscription with local override, and not "seed on open then diverge", which was floated and rejected: a snapshot that drifts is the thing being avoided.
Two objections were raised against sharing and both fall down:
- "The stores are keyed differently —
byThreadKeyvs 3D'scwd." The 2D panel is also showing a repository-scoped diff while storing its selection per thread. That is T3's model, deliberately: what a view shows and where its preference lives are different scopes. - "Switching threads would silently change the 3D view." The 2D panel already behaves exactly that way. Matching it is parity, not a surprise. If the behaviour is wrong it is wrong in T3 and should be fixed once, for both.
Practical consequence: DiffSurface is keyed key={diff3dCwd} (ChatView.tsx:5714), so it does not remount on a thread switch. It has to read the store reactively rather than at mount, and it needs the ScopedThreadRef alongside the cwd.
The same rule decides #35: the base ref writes to branchBaseRefByThreadKey, the field the 2D combobox already uses.
Also in scope
- One turn's diff covers the workspace, not one commit. The dev's original objection to turn checkpoints (scratch work, console logs as terrain) applied to summing them, not to viewing one — confirm that reading holds once you can look at it, and say so either way.
packages/contracts/src/diffOverlay.ts:11-12still documents "the client sums steps 1..i", stale since c205d0d. Fix it here.- Already done, do not redo: the three settings drifts this ticket would otherwise have inherited are fixed on
fix/3d-surfaces-use-t3-settings— the endpoint now takesignoreWhitespace, the scrubber usesformatShortTimestamp, andInspector/Timelineuse a sharedsceneClockthat honours the 12/24-hour setting. When adding the turn and branch scopes, keep passing both settings through.
Done when
Verified in the browser on all four scopes in both themes, per AGENTS.md "Hit every surface": the Branch changes frame's file set and +N −M are identical to the 2D panel's for the same range, idle draw calls are zero on each scope, and switching scopes does not leak the previous scope's selection or scrubber position. Commit to a branch and push it; no PR — review is the separate cold pass.
Activity
Dillpickleschmidt commentedon Aug 2, 2026
Built and pushed on
feat/3d-diff-scopes(41175c35f). Not closing: the "Done when" browser pass could not run in this session — see the end.What landed
The four scopes, out of
diffPanelStore.byThreadKey— the same record the 2D panel writes, read reactively becauseDiffSurfaceis keyeddiff3dCwdand does not remount on a thread switch.DiffSurfacetakesthreadRefalongsidecwd.Resolving a selection into a scope moved into
apps/web/src/diffScope.ts, which both panels now call. Two selection kinds cover four menu entries, and "is this the latest turn" is a property of the thread rather than of the selection — so it is resolved on read, not stored, and a turn stops being called "Latest turn" the moment the next one checkpoints.DiffPanellost its private copy of the ordering and the label derivation;useTurnDiffSummariesnow returnsorderedTurnDiffSummariesso there is one sort, not two.Three of this ticket's premises were wrong
Same pattern as #32, so recording them rather than quietly working around them.
1. "
packages/contracts/src/diffOverlay.ts:11-12still documents the client sums steps 1..i." It does not —6b3ce56a2swept it up along with the settings fixes. Verified before acting rather than after; nothing to do.2. "Turn scopes are free —
activeThread.checkpoints, client-side, no fetch." They are not free, and taking them at face value would have re-shipped the bug this surface was just fixed for. Checkpoint summaries are captured once, withignoreWhitespace: falsehardcoded (CheckpointReactor.ts:265).diffIgnoreWhitespacedefaults to on, and the 2D panel's turn view honours it viauseCheckpointDiff. So reading the summaries would have rendered a reformatted turn as towers in 3D and as nothing in the 2D panel's view of the same turn — the map's exact "if the user changes this setting, does the 3D surface move too?" test, failed.The turn scopes therefore read the 2D panel's own
useCheckpointDiffquery, on the identical target (fromTurnCount/toTurnCount/cacheScope), so the two panels share one fetch rather than making two and cannot disagree. The checkpoint list still comes live off the thread. Cost: the scopes are one cached fetch rather than zero. That is the map's rule applied literally — same store, same settings, same server call.3. "Branch changes … three dots, matching the 2D panel's branch diff." Right about the dots, silent about renames — and renames change both the file set and the totals.
GitVcsDriverCore.getReviewDiffPreviewleaves rename detection on. The aggregate therefore leaves it on too, deliberately asymmetric withsteps, which run--no-renamesso a rename reads as one building emptying and another filling. That is the right answer on a city and the wrong one for a number that has to match the panel next door. The existing-zparser already returns the rename's new path, so this needed no parser change — checked, not assumed.The aggregate
New
aggregate: DiffOverlayFile[] | nullonDiffOverlay, from onegit diff --numstat <base>...HEAD. Committed work only; uncommitted is the working-tree scope's, the same split the 2D panel makes.Netted over the window the steps cover, not over
baseRefblindly. When the branch has not diverged,baseRef..HEADis empty and the steps fall back to recent commits — netting the base there would hand the scrubber an aggregate covering none of the commits under it. Where the 2D panel shows anything at all, the range and the numbers are identical.Null in two reachable cases, both degrading to parity with the 2D panel rather than inventing a number: a
working-treerange has no commits, and arecent-commitswindow whose oldest step is the repository's root commit has no^to net from (a repository with fewer than twenty commits).Verified
The aggregate, through the real HTTP endpoint on a seeded server, against an independently spelled
git diff --numstat: 33/33 files identical, anddiffIgnoreWhitespacedemonstrably reaches it —truegives +2777/−198 andfalsegives +2798/−219, each matching git run the same way. The churn/net gap this scope exists to close is live on this repo right now: summing the steps would have shown 3355 where the range did +2777/−198.New service tests (14 pass), on a repository built long enough that the fallback window starts past the root commit:
git diff --numstat <oldest>^ HEADspelled line-oriented rather than through the-zgrammar the service parses;The differently-spelled reference caught a real difference while writing it: without
-z, git prints a rename as oneold => newpath where-zsplits it into fields. The service was right; the test's reference parser was not.diffScopeunit tests (7 pass) pin the vocabulary both panels share. Typecheck and lint clean on web, server and contracts.Also in scope, answered
"One turn's diff covers the workspace, not one commit — confirm the original objection applied to summing, not viewing." I cannot confirm this by looking at it without the browser pass, so it stays open. What I can say from the data: the objection was about summing checkpoints, and a single turn's diff is bounded by one turn's work, so nothing structurally reproduces the scratch-work problem. Worth a sentence either way once someone has looked.
Two things this surfaced
Turn scopes cannot ghost a file the turn deleted. Ghosts come from
GET /api/mindwalk/diff, which is keyed by working directory and knows nothing about the thread, so a file a turn deleted has no building to stand on. Rather than draw a frame that quietly omits it, the header now reads "N not on the map" whenever a frame names a path the citymap lacks. That doubles as the map's standing vocabulary check made visible: it should read zero on the repository scopes. Fixing it properly means the endpoint learning about the thread, which contradicts this ticket's "no server work for turns" — fog, not this branch.Behaviour changes worth a reviewer's attention. The working-tree step is no longer the scrubber's last position — it is its own scope, which is strictly more reachable. Branch changes opens on the netted range rather than the last commit. The scrubber's reset button now means "back to the whole range" rather than "back to the first commit".
Not done: the browser pass
The "Done when" — all four scopes in both themes, idle draw calls zero on each, no selection or scrubber leak across scopes, Branch changes identical to the 2D panel on screen — has not been run, and this ticket should not close until it has.
The developer's dev server on 5733 could not serve it: its API half (pid 1301525, up since 08:33) predates these changes, its
node --watchsupervisor does not restart the child on a source edit (verified by touchingDiffOverlay.tsand watching the pid for 20s), and restarting a process this session did not spawn is out of bounds. Sinceaggregateis a required contract field, that stale server would fail the surface's decode outright rather than degrade — so nothing was verifiable there.An isolated stack was stood up instead on web 5734 / server 13774, seeded with a
VACUUM INTOcopy of real data, and it is what the endpoint checks above ran against. But this environment has no preview automation host, no Playwright and no Chromium, so no browser could be driven. The stack is left running for whoever picks this up.Dillpickleschmidt commentedon Aug 2, 2026
Follow-up: a real disagreement, found by verifying against the panel itself
Running on 5733 turned up a bug in the aggregate, of exactly the kind this ticket exists to catch. Fixed in
0e56dd7f6.The 2D panel's branch source runs
git diff --patch --no-color --no-ext-diff --no-textconv --minimal [--ignore-all-space] <base>...HEAD. The aggregate rangit diff --numstat -z <base>...HEADand nothing else.--minimalis not a formatting flag — it spends extra work searching for a smaller diff, and on a heavily rewritten file it finds a different one:So on this very branch,
apps/web/src/mindwalk/DiffSurface.tsxread 385/61 in 3D against the panel's 384/60 — one file, one line apart, two panels under the same scope name.--no-ext-diffand--no-textconvare now passed for the same reason: a configured external diff driver would otherwise apply to one panel and not the other.The flag set is named as
AGGREGATE_ARGSbesideLOG_ARGS, and pinned by a test that spells the panel's flags rather than asserting a number, so it fails if either side drifts.Worth noting how it was found, since it bears on the review that is left. The first comparison I ran said the two sides differed, and my first instinct — that my throwaway shell counter was at fault — was right once and wrong once. It was genuinely miscounting (double-excluding a header line, which is where the
grep -crecount came from), and underneath that miscount there was also a real flag mismatch. Two bugs stacked, one mine and one the code's, and stopping at the first explanation would have shipped the second.--numstatand the patch text agree exactly within any given flag set, which is what made the flags the remaining suspect.Re-verified after the fix
Against a server actually running this branch, comparing the served aggregate to the panel's own command over
origin/main...HEAD:Service tests now 15, including the flag-parity one. Typecheck and lint clean.
Decided: no version-skew tolerance
I had floated giving
aggregateawithDecodingDefault(null)so an older server degrades instead of failing the surface's decode. The dev's call is no — keep the contract strict and restart servers as needed rather than carry compatibility machinery. Recording it so the cold pass does not re-raise it as an oversight.Still not done
The browser pass. The stack is now up on 5733/13773 against the worktree
.t3and serving this branch correctly, but this session still has no way to drive a browser, so all four scopes in both themes, idle draw calls, and cross-scope leak remain unverified by eye.Dillpickleschmidt commentedon Aug 2, 2026
Second real bug, and a correction to my own diagnosis
Opening the surface on an ordinary
vp run devserver failed outright withEnvironmentResourceNotFoundError. Fixed in88e9cdd93.The server bounds every diff request to
config.cwd, which defaults toprocess.cwd(). The dev runner launches the server viavp run --filter=t3 dev→node --watch src/bin.ts, executed insideapps/server— so the workspace root is<repo>/apps/server, and a request for the project root is refused:The 2D panel has always handled this.
DiffPanel.tsx:324-343watches for the workspace-root rejection and retries atserverConfig.cwd. 3D Diff never learned to, so it died where the panel beside it succeeded — on the same repository, on the same server, in the same session. That is precisely the "different renderer of an existing T3 query" rule broken, and it was pre-existing, from #29.The retry costs nothing in correctness: the endpoint resolves whatever cwd it is given to the repository root (
handle.repository.rootPath), so asking atapps/serverreturns the identical overlay the project root would have — verified,cwd: /home/dylank/Projects/t3code, 11 aggregate files, a 15,843-file citymap. Matched on the error's_tagandreasonrather than a message substring, since the endpoint deliberately answers an unreadable path the same way it answers a missing one.The correction
I first read this as an environment problem and proposed changing how the dev server is launched — splitting
dev:webfrom a hand-run server with an explicit cwd. That was wrong, and the dev caught it: the regular diff view was showing numbers for these very files the whole time, which was the evidence that the boundary is survivable and that something in my surface was failing to survive it. I had even read the fallback while porting the scope selector out ofDiffPaneland did not connect it.Worth recording as a pattern, since it is the second time on this ticket: both bugs were found by comparing against the 2D panel's actual behaviour, and both times my first explanation was about the environment or my tooling rather than the code. With no automated review left, "the panel next door does this correctly" is the single most productive question to ask of this surface.
Also worth a look, separately
Bounding a repository view by the server's own process directory is odd on its face, and the 2D panel's retry is a client-side workaround for it rather than a fix. That is pre-existing and well outside this ticket, but somebody should decide whether the boundary is meant to be that narrow. Not filed — flagging for the cold pass.
Branch now at
88e9cdd93. The in-browser pass on the four scopes is still the one thing outstanding.Dillpickleschmidt commentedon Aug 2, 2026
Unified: one source of per-file counts, both panels
8ec1272b3. The dev pushed back on the pattern behind the last three findings, correctly — 3D Diff kept re-asking questions T3 already answers, and every defect on this branch came from exactly that.diffPreview→working-treegit diff+ls-files+ per-file--no-indexdiffPreview→branch-rangegit diff --numstatdiffPreview6b3ce56a2DiffPanel's retryEnvironmentResourceNotFoundErroruseCheckpointDiffgit log --numstatFour duplications, four defects; two genuinely-new things, none. That is not a run of bad luck, it is the mechanism.
What changed
Per-file counts now come from
getReviewDiffPreview, which already resolves the refs and already runs these diffs. It emits--numstatbeside--patchwith the flags that decide what git counts, and hangs the result on eachReviewDiffPreviewSource. Both panels read that one field, so the question "do they agree?" no longer has anywhere to be answered wrongly — there is no second number.The overlay endpoint keeps only what nothing else provides: the per-commit breakdown and the citymap.
aggregateandreadWorkingTreeStepare deleted (−139 lines from that module). It still asks which paths were deleted uncommitted, by name only — laying out the city is its job alone, and a path list has no numbers to disagree about.Also shared rather than copied: the
--numstat -zgrammar has one parser invcs/numstat.tsinstead of one per consumer, and the workspace-boundary retry has one home inuseReviewDiffPreview, so the next caller cannot forget it the way this one did.A bug it fixed on the 2D side
The panel counted hunks out of the patch it renders, and that patch is capped at 120,000 bytes. Past the cap its own
+N −Mwere short — measured on this repo, a 200-commit range is a 62MB patch against 1MB of numstat. Serving the counts fixes the panel too, which is the tell that this was the right seam: a shared thing was being computed privately by everyone who needed it.Verified
GitVcsDriverCoretest asserting a source'sfilescovers exactly what the commit touched and that an untracked file reaches the working-tree stat through its separate--no-indexcall.ghostPathsOftest for uncommitted deletions, and a service test proving the overlay's steps are commits only.keys = cwd, generatedAt, range, steps, citymap, step kindscommitonly.On the map
The rule now says same data, not just same store and settings, with these four bugs recorded as what the narrower reading cost — and the sharper test: not "do the two agree?" but "is there a second thing that could disagree?" Plus the corollary this turned up: a workaround living in one caller is a bug in every other one; move the compensation, do not copy it.
This lands before #35 deliberately. The base-ref picker writes
branchBaseRefByThreadKey, and against a split-brain overlay it would have inherited a base ref meaning one thing to the panel and another to the terrain.Still outstanding, unchanged: the in-browser pass over the four scopes.
Dillpickleschmidt commentedon Aug 2, 2026
Correction: the 2D "bug" was not one, and the change was not free
78a56d982. The dev challenged the claim that serving the counts fixed a 2D defect. Checked, and they were right on both counts.It is intended behaviour. The panel renders this above a truncated diff:
So counting the rendered patch, beside a banner saying the render is partial, is coherent — and it is what something named
getReviewDiffPreview, capped byREVIEW_DIFF_PATCH_MAX_OUTPUT_BYTES, is for. Serving whole-range totals would have put a header on screen contradicting the notice underneath it. That is a behaviour change dressed as a fix.And it cost real time on a hot path. Measured on this repo:
ls-filesThe last one roughly doubles the untracked git work already being done, on a path that runs whenever the diff panel opens.
Resolution — the dev's own suggestion
includeFileStatsdefaults off. The 2D panel never sets it, so its behaviour and its cost are exactly upstream's;diffLineStatis back to counting hunks. 3D Diff sets it, because the terrain has to place every changed file — past the cap a file is a building that never appears, where in the panel it is a row below a notice explaining the list is short. Different question, different consumer, and it pays for its own answer. A test pins that the default path computes no stats at all.The two panels can now differ on a truncated range. That is correct rather than drift: one is explicitly labelled a partial preview.
Upstream exposure, which I should have weighed first
GitVcsDriverCore.tsDiffPanel.tsxreview.tsnumstat.ts,useReviewDiffPreview.ts,DiffOverlay.tsPurely-additive edits conflict far less than modifications of existing lines, which is the shape to aim for in the two busy files.
On the map
Two rules added beside the same-data one: prefer additive in upstream files, and never "fix" upstream behaviour without asking — with this as the worked example, including the measurement, since "it looked wrong to me" is exactly how a deliberate bound gets removed.
307 tests green. Still outstanding: the in-browser pass over the four scopes.
Dillpickleschmidt commentedon Aug 2, 2026
Footprint pass —
10f42d6d1Final cleanup with one aim: smallest conflict surface against
pingdotgg/t3codewithout reopening the duplication this branch closes.Upstream files, this branch's contribution:
GitVcsDriverCore.tsGitVcsDriverCore.test.tsreview.tsserver.test.tsChatView.tsxuseTurnDiffSummaries.tsDiffPanel.tsxTests removed, and why each was safe to remove:
"commit", so a working-tree step is a compile error; the test pinned the type-checker.Also swept: the now-vacuous
kind === "commit"filters in the surface and tests (every step is a commit), an unusedDiffOverlayStepimport, an unnecessaryparseCountexport, a three-line comment in ChatView.306 tests green, typecheck and lint clean. Branch tip
10f42d6d1; still outstanding: the in-browser pass.Dillpickleschmidt commentedon Aug 2, 2026
Resolved and merged to
mainat992594957.The build (see the handoff comments above for full detail)
All four 2D scopes on the 3D surface, read reactively from
diffPanelStore— the 2D panel's own record — with scope resolution shared throughdiffScope.tsso the two panels cannot name a scope differently. The commit scrubber survives as a drill-down inside Branch changes, withnullas a real whole-range position. Per-file counts unified:getReviewDiffPreviewemits--numstatbeside--patch(opt-in viaincludeFileStats, the 2D panel pays nothing), one parser invcs/numstat.ts, the overlay endpoint shrunk to what nothing else provides.The cold review pass found and fixed two defects
ReviewDiffPreviewSource.fileswas required — failed typecheck inapps/mobile(a consumer the build never checked), and a required field added to a response is a decode failure against every server that predates it; clients and servers upgrade independently here. Now wire-optional with a decoded default (73e6d76c4). The handoff's "typecheck clean" claim was false at handoff time.992594957) — the overlay's retry target was read off the review preview's answer, a query turn scopes deliberately disable, so on exactly those scopes the citymap fetch failed withEnvironmentResourceNotFoundError. Found by the dev's browser pass. The target now comes from the server's own config, the same sourceuseReviewDiffPreviewuses.The browser pass, and what it taught
All four scopes verified by the dev in the running app. It surfaced one behaviour worth recording rather than fixing: the workspace-boundary retry silently answers from a different repository when a project sits outside the server's workspace — the citymap then belongs to another repo and every turn file reads "not on the map". Verified byte-identical in upstream (
shouldRetryBranchDiffAtEnvironmentCwd, upstream PR pingdotgg#3169): the 2D panel does the same swap invisibly, 3D merely exposes it because a map has to agree with a diff. The dev ruled: intended behaviour, no divergence from upstream, no ticket. It cannot occur on a normal launch (projects under the serve directory); the dev stack now passes~/Projectsas the server's cwd argument — upstream's own knob — so turn scopes are testable.On the open sub-question: one turn's diff reads fine as terrain — the scratch-work objection applied to summing turns, not viewing one.