Timeline shows each commit's own content, not HEAD's - #144
Merged
Conversation
Timeline renders the union manifest — every path that ever existed — but every content fetch hit HEAD. Files gone since 404'd, filling the console (#122), and the ones that survived were quietly worse: a logo that still exists rendered its CURRENT bytes no matter how far back you scrubbed. The 404s were the visible half of that. The bundle already knows which blob each path had at each commit, so the replay now carries the sha and GET /api/blob serves it. The file preview, README pane, ad panels, binary fingerprints and the font/waveform facades all address their own blob while scrubbing and fall through to the path in Live. POST /api/files takes an optional path->sha map so Timeline keeps the batching that exists to spare the connection pool. Two stale notes are now false and go: the preview and README follow the scrub. The street pane's stays — "all-time folder stats" is a different problem needing per-commit directory rollups, filed separately. Security: /api/blob resolves its path non-strict, since a historical path legitimately may not exist. That is not a traversal hole — resolve() still normalizes `..` and a path that doesn't exist has no symlink of its own to redirect through — and there's a test pinning the escape attempt at 403. git-lfs pointers resolve like the live scan, so LFS media isn't served as a pointer file. Closes #122
Two fixes on top of the blob work. Splitting /api/blob out was wrong. "Blob" is git's word for stored content, not a different resource: it is the same file at a different point in time. /api/file takes an optional sha now — absent reads the working tree, present reads that blob — which also matches how POST /api/files already worked. The cache-buster is renamed v -> mtime, since that is what it holds and the server never declares it at all. The 404s also survived the first pass, which the sha alone could not fix. A file absent at the scrub position has no blob to name, so the sha was null and the loader fell through to a by-path read of HEAD, where the file does not exist. Media, data facades and the README now skip the fetch entirely for a path with no content at this commit, rather than asking HEAD for it.
Period-correct content that only resolves once isn't much use: the file preview and README kept whatever they fetched when you selected them, so scrubbing changed the city but not the panes. Both now read the history manifest, so a file absent at the scrubbed commit reads as deleted there rather than quietly rendering HEAD's version, and the preview refetches when its blob changes. Fetches key on SETTLED_COMMIT, not the live position: dragging across a long history would otherwise refetch once per commit crossed. The tree and search stay live, since filtering the union is cheap and watching it change under the drag is the point.
The footer and tooltip said 46 lines next to a body that was 36. They were resolving at different points: content is one specific blob at the settled commit, while lines/bytes came from linesAt at the CONTINUOUS scrub position, which interpolates between commits. At pos 12.7 that is commit 12's bytes beside a number lerped most of the way to commit 13. The interpolation is right where it came from: building heights tween across a drag. It is wrong for a number someone reads, so the display path now resolves at the same discrete commit the fetch does. Heights still interpolate; scrubController reads linesAt directly.
The previous fix moved the display to the settled commit but left it reading linesAt, which lerps between CHANGE entries, not between commits. So any commit where the file didn't change still drifted toward the next edit: with edits at commit 0 (36 lines) and commit 3 (46), commit 2 reads 36 + 10*(2/3) = 42.67, which is the "42 lines" over a 36-line body. entryAt() returns the change entry actually in effect — a step lookup — and both the sha and the displayed lines/bytes now come from it, so the number and the bytes cannot describe different blobs. blobShaAt is now just entryAt().sha. Heights still lerp; scrubController calls linesAt directly, which is what that curve is for. The earlier test passed because its fixture changed the file at every commit, where step and lerp agree. It now spans commits that leave the file untouched, which is the only place the two differ.
One more of the same kind: scrubFile's lerped line count fed BOTH the height curve and isEmptyFile(). Height should tween, but "is this file empty" is a fact about the blob — between a 0-line commit and a later big one the lerp reads non-empty, so a building lost its flat empty slab while still serving the empty blob. The kind test reads the step entry now; height keeps the curve. bytesAt had no production caller left once the stats moved to entryAt, so it and its test go, and linesAt absorbs the interpolation it was sharing. Its docstring now says outright that it returns values no commit ever had, so the next reader doesn't reach for it to display. Everything else checked out: presenceAt drives opacity, lastModifiedIndexAt is already a step lookup, statsAtDeletion reads entries directly, and scrubFile never escapes to the picker or the tooltip.
The last of the three stale notes. Its numbers are subtree rollups (counts, size, ext breakdown, date span), so no blob endpoint reaches them — #143 assumed a per-commit-per-directory replay, with the payload worry that comes with it. That turned out to be unnecessary: /api/manifest?ref= already reconstructs a whole repo at any commit, read-only, using the same code a live scan runs. So the rollups match a real scan by construction, and there is nothing new to bake into the bundle. Fetched lazily and cached per (src, sha, excludes): only when a street pane is open to read it, only once the scrub settles, and a revisited commit is free since a reconstruction can't change. A superseded scan is aborted when you scrub past it, and a failed one keeps the last good manifest instead of blanking the pane. The note now shows only while that scan is in flight, when the union rollups genuinely are all-time. Closes #143
eslint require-yield: a generator with no yield is a lint error, and the call site catches a synchronous throw just the same.
Both knew 'sha means that blob, otherwise the working tree'. That part is _read_versioned now; what stays split is the error handling (precise codes vs silent omission) and the mime check, which the batch runs BEFORE reading so a non-image is never read at all.
POST /api/files read as "GET /api/file, plural". It isn't: it inlines base64, serves images only, caps at 8MB, and silently omits anything it can't serve, where the singular streams raw bytes with mime headers and distinct 400/403/404/413 codes. Those differences are the point, not an oversight — base64 costs ~33%, media skips gzip via Content-Encoding, and the image filter plus the cap are what make one request safe to inline. A "true" plural would be slower, larger and less safe than the singular, so the name had to move, not the behaviour. It is /api/images now, matching its actual sibling /api/fingerprints, which is named for what it returns and nobody mistakes for a plural. FileBatchEntry -> ImageBatchEntry, and the request shape both batches share is PathBatchRequest rather than borrowing the file endpoint's name.
_MAX_BATCH_FILE_BYTES only ever bounds an inlined image, so it is _MAX_BATCH_IMAGE_BYTES. _MAX_BATCH_PATHS stays: both /api/images and /api/fingerprints iterate against it, so 'paths' is what it means.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #122
Closes #143
Timeline renders the union manifest — every path that ever existed — but every content read went to HEAD. The reported symptom was a console full of
/api/file … 404for media that no longer exists.The 404s were the visible half. The quiet half: media that did survive rendered its current bytes no matter how far back you scrubbed. Gating the fetch on presence, as the issue suggested, would have silenced the console and left that in place. So this makes Timeline actually period-correct instead.
Content follows the commit
The bundle already knew each path's blob sha at each commit, so the replay carries it and
/api/filetakes an optionalsha: absent reads the working tree, present reads that blob. The file preview, README pane, ad panels, binary fingerprints and the font/waveform facades all pin their version while scrubbing and fall through to the path in Live.POST /api/filestakes a path→sha map so Timeline keeps the batching that exists to spare the connection pool.One endpoint, not two. This first shipped as a separate
GET /api/blob, which was wrong: "blob" is git's word for stored content, not a different resource. It's the same file at a different point in time, so it's one endpoint with a version selector — matching how the batch sibling already worked. The old cache-buster is renamedv→mtime, since that is what it holds and the server never declares it.Absent is not the same as versioned. A file gone at the scrubbed commit has no blob to name, so the sha resolves null and the loader used to fall through to a by-path read of HEAD — the original 404, by a new route. Media, data facades and the README now skip the fetch entirely when a path has no content at that commit.
Panes follow the scrub
Period-correct content that resolves once isn't much use: the preview and README kept whatever they fetched when selected, so scrubbing moved the city and not the panes. Both read the history manifest now, a file absent at the scrubbed commit reads as deleted rather than quietly showing HEAD's version, and the preview refetches when its blob changes.
Fetches key on a new
SETTLED_COMMIT, not the live position — dragging a long history would otherwise refetch once per commit crossed. The tree and search stay live, since filtering the union is cheap and watching it change under the drag is the point.Numbers describe the blob being served
The footer read "42 lines" over a 36-line body.
linesAtinterpolates between change entries so building heights can tween, and the display path was reading that curve: with edits at commit 0 (36) and commit 3 (46), commit 2 reads36 + 10×2/3 = 42.67.entryAt()returns the entry actually in effect — a step lookup — and both the sha and the displayed lines/bytes come from it, so a number and the bytes beside it cannot describe different blobs.An audit of the rest found one more of the same kind:
scrubFile's lerped count fed both the height curve andisEmptyFile(), so a file that was empty at this commit but big later lost its flat empty slab. Emptiness is a fact about the blob; it reads the step entry now. Everything else checked out —presenceAtdrives opacity,lastModifiedIndexAtis already a step,statsAtDeletionreads entries directly, andscrubFilenever escapes to the picker or tooltip.bytesAthad no production caller left and is gone.Folder stats (#143)
The street pane's numbers are subtree rollups, so no blob endpoint reaches them. #143 assumed a per-commit-per-directory replay with the payload cost that implies — unnecessary, as it turns out:
/api/manifest?ref=already reconstructs a whole repo at any commit using the same code a live scan runs, so the rollups match by construction.Fetched lazily and cached per
(src, sha, excludes): only when a street pane is open to read it, only once the scrub settles, and a revisited commit is free since a reconstruction can't change. A superseded scan aborts when you scrub past it; a failed one keeps the last good manifest rather than blanking the pane.All three
TimelineStaleNotes are gone in the steady state — the street pane's now shows only while its scan is in flight, when the union rollups genuinely are all-time.Security
/api/fileresolves its path non-strict when a sha is given, because a historical path legitimately may not exist. That isn't a traversal hole:resolve()still normalizes.., and a path that doesn't exist has no symlink of its own to redirect through. A test pins the escape attempt at 403, andassert_insidekeepsmust_exist=Trueby default so every existing caller is unchanged. git-lfs pointers resolve like the live scan, so LFS media isn't served as a pointer file.Worth watching
Reconstructing a commit is a real scan. It's lazy and cached, but the first visit to each commit with a folder selected does the work. If that drags on a large repo, the fallback (union stats plus the note) is intact.
Full gate green via the pre-push hook: 369 pytest, ruff, 2838 vitest, eslint, prettier, typecheck. Verified against mounted local repos.
🤖 Generated with Claude Code