Put a blame column and a control row beside the editor - #57
Merged
Conversation
The editor grows a toolbar row under the breadcrumb, and the first two things in it: Locate, which selects the open file in the tree, and Blame, which attributes every line to the commit that last touched it (#52, #56). The row's controls become the one control shape the file tree header also uses (#54), and the selection bug that made a right-click on a project tab highlight its name is fixed underneath them (#55). Four issues on one branch at the maintainer's direction rather than the template's one-issue-per-PR. They interleave in style.css, Icon.tsx, ViewToolbar.tsx and FileTree.tsx, so splitting them would produce commits that do not individually build. Blame reads `git blame --porcelain` and keeps the format's own shape: each commit stated once, and a line-by-line index into them. A 5,000-line file from one commit crosses the bridge as one commit record and 5,000 integers rather than 5,000 copies of an author and a timestamp. What blame measures is the file on disk, and its answer is a line number. One unsaved insertion moves every line below it, so the column clears while the buffer is dirty and comes back on save, with the lines just written now attributed to nobody — which is what they are. The toggle stays pressed across that and the column keeps its width, so turning blame on and typing does not shift the code sideways. Two things in the parser are defensive rather than reachable. An entry is placed at the line number git gave it rather than appended, because if the output ever skips a line one blank entry is a better failure than every attribution below it being off by one. And that line number is bounded by the output's own line count — the file cannot have more lines than the blame has records — so a bad digit cannot size an allocation. internal/git takes a ratchet raise, 900 -> 1100 LOC and 21 -> 25 exported, and the note it lands in argues against itself: the #9 paragraph named this exact case as one that should become its own package. It did not, because a sibling cannot reach runGit and depguard forbids the import — so the seam is the runner, not the parser. That is #53, which blocks #35, rather than something done here. The tree header and the editor toolbar had three looks between them: an icon beside a word with no hover state, a bare icon that turned accent- coloured, and a word with no icon. Control (components/Control.tsx) is the one shape for both, and the naming rule matters more than the look: `show dotfiles` renamed itself to `hide dotfiles`, which leaves a user unable to tell whether the words describe the state they are in or the one the click leads to, and a screen reader announcing a different control each time. It is now `Show hidden` with aria-pressed carrying the state, and the two act-once controls carry no aria-pressed at all rather than a permanent false. The tree header's toggles are icon-only because four labelled controls measure 274px and the sidebar's default is 260, its minimum 180. The row measures 180 exactly; a control added to it has to be compact or it stops fitting. user-select was in the stylesheet four times and unprefixed every time. Unprefixed user-select only reached WebKit in Safari 17, and this window is a WKWebView on macOS and a WebKitGTK one on Linux, so every one of those rules was inert in the app while testing clean in a Chromium browser — which is why right-clicking a tab selected its label. It is now one rule on .shell with the prefix beside it, the panes holding a document opt back in, and style.test.ts fails the build on an unpaired declaration. Locate is not reveal with a file path. reveal expands what it is given, so a file lands in the expanded set as a directory listing nobody will fetch, and the hook then asks the backend to list a file. It also runs its hidden check over the file's own chain rather than its parent's, which is the case a parent-only check gets wrong: .gitignore at the root has no hidden ancestor, so the filter would stay on and the locate would do nothing at all. The tree cannot tell a locate from an ordinary selection by `selected` alone, because locating the file already selected changes nothing about it. TreeState carries a locateRequest counter for it: a locate centres the row, a selection only brings it into view. Centring every selection would drag the tree out from under someone arrowing through it. Three layout defects here were found by measuring in a browser rather than by reading the CSS, after two rounds of shipping arithmetic that looked right: the blame column truncated its date because CodeMirror gutters inherit the UI font, so 13ch was thirteen proportional zeros rather than thirteen monospace characters; the width then still truncated because everything in a CodeMirror view is border-box, so it sized the padded box and left the text a character short; and the toolbar's pressed background bled 4px past the pane's left edge. Closes #52 Closes #54 Closes #55 Closes #56
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
+ Coverage 93.64% 93.95% +0.30%
==========================================
Files 32 33 +1
Lines 1543 1621 +78
==========================================
+ Hits 1445 1523 +78
Misses 62 62
Partials 36 36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 #52
Closes #54
Closes #55
Closes #56
What this does
The editor grows a toolbar row under the breadcrumb and the first two things in it:
Locate, which selects the open file in the tree, andBlame, which attributes every line to the commit that last touched it. Those controls become the one control shape the file tree header also uses, and the selection bug that made a right-click on a project tab highlight its name is fixed underneath them.Blame (#52).
git blame --porcelainininternal/git, kept in the format's own shape: each commit stated once, and a line-by-line index into them. A 5,000-line file from one commit crosses the bridge as one commit record and 5,000 integers rather than 5,000 copies of an author and a timestamp. The column shows initials and the date, with the full name, timestamp, short SHA and subject on hover; a line git attributes to its all-zero SHA is marked uncommitted rather than attributed to whoever wrote the line it replaced.What blame measures is the file on disk, and its answer is a line number. One unsaved insertion moves every line below it, so the column clears while the buffer is dirty and comes back on save, with the lines just written now attributed to nobody — which is what they are. The toggle stays pressed across that and the column keeps its width, so turning blame on and typing does not shift the code sideways.
Two things in the parser are defensive rather than reachable, and both are cheaper than the failure they prevent. An entry is placed at the line number git gave it rather than appended, because if the output ever skips a line then one blank entry is a better outcome than every attribution below it being off by one. And that line number is bounded by the output's own line count — the file cannot have more lines than the blame has records — so a bad digit cannot size an allocation.
Locate (#56). It is deliberately not
revealwith a file path.revealexpands what it is given, so a file lands in the expanded set as a directory listing nobody will ever fetch, anduseFileTree.revealthen asks the backend to list a file. Its hidden check also runs over the file's own chain rather than its parent's, which is the case a parent-only check gets wrong:.gitignoreat the root has no hidden ancestor, so the filter would stay on and the locate would do nothing at all.The tree cannot tell a locate from an ordinary selection by
selectedalone, because locating the file already selected changes nothing about it.TreeStatecarries alocateRequestcounter for exactly that: a locate centres the row, an ordinary selection only brings it into view. Centring every selection would drag the tree out from under someone arrowing through it.One control shape (#54). The tree header and the editor toolbar had three looks between them — an icon beside a word with no hover state, a bare icon that turned accent-coloured, and a word with no icon.
components/Control.tsxis the one shape for both, and the naming rule matters more than the look:show dotfilesrenamed itself tohide dotfiles, which leaves a user unable to tell whether the words describe the state they are in or the one the click leads to, and a screen reader announcing a different control each time it is pressed. It is nowShow hiddenwitharia-pressedcarrying the state, and the two act-once controls carry noaria-pressedat all rather than a permanentfalse.The selection bug (#55).
user-selectwas already in the stylesheet four times, and unprefixed every time. Unprefixeduser-selectonly reached WebKit in Safari 17, and this window is a WKWebView on macOS and a WebKitGTK one on Linux — so every one of those rules was inert in the app while testing clean in a Chromium browser, and WebKit's own select-the-word-under-the-pointer behaviour took over on right-click. It is now one rule on.shellwith the prefix beside it, the panes that hold a document opt back in, andstyle.test.tsfails the build on an unpaired declaration.How it was verified
make verifygreen on the committed tree. Patch coverage 104/104 changed Go lines (100%); 856 frontend tests.Go:
internal/git/blame_test.godrives the real binary — two authors attributed per line, an uncommitted edit marked rather than misattributed, one commit record for a 50-line file from one commit, an empty file answering with slices rather than nulls, a path git refuses reported with git's own words, a file named-f.yaml, and a path outside the project refused before git runs. Parser tests cover a SHA-256 zero name, a truncated record, a skipped line, an unreadable timestamp, and a line number the output could not be describing.Frontend: the tab model, the hook (including that a blame is not re-read between keystrokes and that the buffer going dirty clears it), the CodeMirror gutter, the control, the toolbar, and an App-level test that opens a nested file, collapses its directory, presses Locate and checks the row comes back selected. That last one exists because the equivalent plumbing bug — a seam not threaded through
App— is one I shipped earlier in this branch and only an App-level test caught.Three layout defects were found by measuring in a browser rather than by reading the CSS, after two rounds of shipping arithmetic that looked right. The blame column truncated its date because CodeMirror gutters inherit the UI font, so
13chwas thirteen proportional zeros rather than thirteen monospace characters. Fixing the font was not enough, because everything inside a CodeMirror view isborder-box: the width sized the padded box and left the text a character short. And the toolbar's pressed background bled 4px past the pane's left edge. The tree header's own numbers came from the same measurements — four labelled controls want 274px against a 260px default sidebar, so its toggles are icon-only and the row measures exactly 180, the sidebar minimum.Not verified: nothing in this branch was seen in the packaged app. The measurements above were taken in Chromium against the real components and the real stylesheet, and #55 is specifically a bug that Chromium cannot reproduce — so the
-webkit-user-selectfix is reasoned from the compatibility fact and the gate, not observed. Worth a look in the built app before merge.Checklist
style.css,Icon.tsx,ViewToolbar.tsxandFileTree.tsx, so splitting them by issue would produce commits that do not individually build. Acceptance criteria are met for each, with one documented departure: One control style: tree header matches the editor toolbar, 'show hidden' becomes a real toggle #54 asked the changed-only filter to gain a visible label, which does not fit the sidebar — noted on the issue with the measurements.@codemirror/mergeis Diff view in the editor: working tree vs HEAD #35's, not this branch's.LoadBlamegoes throughrunGit, read-only, with the path after a--separator and an escape refused before git runs.The thing to actually review
internal/gittakes a ratchet raise: 900 → 1100 LOC and 21 → 25 exported. The note it lands in argues against itself, and that is the point — the #9 paragraph named this exact case as one that should become its own package. It did not, because a sibling package cannot reachrunGitanddepguardforbids the import, so the seam is the runner rather than the parser. That is #53, which now blocks #35. If you would rather have the extraction first and blame in its own package, this is the commit to send back.