Put the diff first and the destinations beside it - #207
Merged
Conversation
juanmaguitar
force-pushed
the
juanmaguitar/notes-and-guidance-for-the-pull-request
branch
from
August 9, 2026 12:54
a4919f1 to
681b1fb
Compare
The patch used to sit under the destinations, which put the choice above the thing being chosen for: a contributor scrolled past three cards to read their own code, then scrolled back up to act on it. The code is what they opened the screen to look at and the largest thing on it, so it takes the room, and where it can go stands beside it — in view the whole time they are reading rather than something to scroll back to. This is the shape of an earlier take on the same screen (#6, unmerged since November), rebuilt on the destinations this app has now. Three things fell out of the restructure: The Save and Copy buttons move from floating over the diff into the column header. Floating was survivable across the full width and covers the first line of a hunk once the pane is a column. The columns and the breakpoint are CSS, not inline styles. Flex-wrap was tried first and cannot express this: wrapping keeps both columns on one flex line whose height the modal fixes, so the wrapped sidebar had nowhere to go. Stacking has to change what scrolls as well as what sits where — side by side the sidebar scrolls alone and the diff stays put; stacked, the pair scrolls as one and the diff keeps a definite height so it cannot collapse. And a pre-existing bug the new layout made visible: the diff pane is height:100% with 12px of padding and content-box sizing, so it was always 24px taller than its container. Harmless while it spanned the modal and the overflow fell off the bottom; beside a sidebar it sat on top of the destinations. Measured, not eyeballed: the pane now ends exactly on its column, and stacked there are the full 16px between it and what follows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copying is the one action on this screen with no visible result: the clipboard is somewhere else and the diff does not move, so a button that answers nothing reads as a button that did nothing. It gets pressed again, and the contributor is left unsure whether they have the patch at all. The label carries the outcome rather than a toast or a tooltip — it is the thing that was just pressed, so it is where the eye already is, and a screen reader announces the change on the focused control. Two seconds, then back, so it never lies about the press after this one. A failed copy says so instead of staying quiet. writeText rejects when the document is not focused, which is exactly the case where someone clicked away mid-action and is least likely to notice nothing happened. The same pattern the pull request link and the device code already use, which is why the timer lives in a ref here: a second press restarts the message rather than inheriting the timer of the one before it, and unmounting clears it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reported from a real run: the staleness guard refused a pull request because 'trunk has moved under CONTRIBUTING.md' — a file upstream has not touched since February 2021. The guard compared two lookups: the file's blob at the fork's trunk tip, and its blob at the commit the checkout started from. The second one was the mistake. It spent a request per file to learn something local git already knew, and it made the answer depend on the fork resolving a commit a fork need not have heard of. A 404 there is indistinguishable from 'the file was not there', so an unrecognised base commit reads as every touched file having changed — and the guard fires on all of them. The base side is now the blob sha computed from the content the checkout already holds, over git's own object format so it can be compared with what GitHub reports. Raw bytes, not the EOL-normalised ones that get uploaded: this has to equal the sha upstream recorded, and upstream recorded what it stored. Half the requests, and no answer a fork can get wrong. Tests cover the four cases the comparison has — unchanged, changed upstream, deleted upstream, added on both sides — plus the false positive itself, and pin blobSha against two values git hash-object prints. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
juanmaguitar
force-pushed
the
juanmaguitar/diff-first-patch-screen
branch
from
August 9, 2026 12:55
030d77e to
9e24225
Compare
juanmaguitar
changed the base branch from
juanmaguitar/notes-and-guidance-for-the-pull-request
to
trunk
August 9, 2026 12:55
This was referenced Aug 9, 2026
Closed
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.
Stacked on #204 (→ #200 → #179). Continues #186, and revives the layout from #6.
Prior art
The layout in the v0.1.1 release notes is not hypothetical — it is #6 by @adamziel, open since November and 72 commits behind trunk. It put the diff on the left and the destinations in a sidebar. This takes that shape and rebuilds it on the destinations this app has now.
Worth recording what the two takes did differently on the GitHub side, since #6 is still open and someone will ask:
repoelectron-store#6 also skipped
.github/workflows/files, which needs theworkflowscope. This flow does not, and a patch touching one will fail — filed separately.Why this layout
The patch sat under the destinations, which put the choice above the thing being chosen for: a contributor scrolled past three cards to read their own code, then scrolled back up to act on it. The code is what they opened the screen to look at and the largest thing on it, so it takes the room, and where it can go stands beside it — in view the whole time they are reading rather than something to scroll back to.
What fell out of it
Save and Copy move into the column header. Floating over the diff was survivable at full width; over a column it covers the first line of a hunk.
The columns are CSS with a media query, not inline styles. Flex-wrap was tried first and cannot express this — wrapping keeps both columns on one flex line whose height the modal fixes, so the wrapped sidebar had nowhere to go and the diff overlapped it. Stacking has to change what scrolls as well as what sits where: side by side the sidebar scrolls alone and the diff stays put; stacked, the pair scrolls as one and the diff keeps a definite height so it cannot collapse to a sliver.
A pre-existing bug the layout exposed. The diff pane is
height: 100%withpadding: 12pxand content-box sizing, so it has always been 24px taller than its container. Harmless while it spanned the modal and the overflow fell off the bottom; beside a sidebar it sat on top of the destinations.Testing
npm test— 496 pass, unchanged.npm run lintclean.Checked at 1400px, 900px and 560px, and measured rather than eyeballed — after the box-sizing fix the pane ends exactly on its column bottom, and stacked there are the full 16px between it and what follows. That measurement is what caught the bug: it looked like a rendering artefact until the numbers said 24px, twice.
🤖 Generated with Claude Code