A real three-way merge view — ours │ result │ theirs, the middle one editable - #53
Merged
Conversation
…le one editable v0.4.29 shipped a merge RESOLVER: one card per conflict, four buttons, no context and no way to say "take theirs, then fix the indentation". This is the view the reference tools have. Three Monaco panes, scroll-synced. Both sides come out of git's INDEX (`:1:`/`:2:`/`:3:`) rather than being scraped back out of the working copy, so they are whole marker-free files and `<<<<<<<` never reaches the screen — it is a file format, not a user interface. Each side is labelled with the branch it came from (`name-rev`, refused for anything but HEAD/MERGE_HEAD; a rebase writes no MERGE_HEAD, so both come back empty rather than naming the wrong branch). The middle pane is a full editor and typing in it IS the answer: an edit landing inside a region settles that region, which is the case the resolver could not express. Gutter chevrons move a side across, F7/Shift+F7 walk the conflicts, the scrollbar carries a mark per conflict, and inside a conflicted line only the characters that actually differ are tinted. Panes are draggable and the split is remembered. Regions still come from the markers, not recomputed from three texts: git has already decided where the conflicts are, and a second opinion would offer choices git will not accept. With no markers anywhere in the result there is nothing left to scan for, so resolution is explicit state. The coverage floors go UP (94/87/95/95 → 95/88/95/96) rather than being met: useMergePanes.js was 4.85% covered, so the logic deciding what gets written to a merged file moved to mergePaneOps.js and is now driven by fake editors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… hiding it The sweep has been dying on its FIRST theme since deps-diff was added ahead of status-band and cleared the comparison out from under it, so no run had reached a second theme in a while. It also only walked 14 of the 20 themes. Both fixed, plus the one the merge surface exposed: the launcher hides itself whenever something else takes focus — which a merge does, arriving in the main window from a second process — and screenshotting a hidden window HANGS rather than failing, which is the worst way for a gate to break. merge-view measures the chrome only. The bands and word tints inside the panes are Monaco decorations built from --dg-add/--dg-del/--dg-chg alone, and check-theme-depth already holds those three to a contrast floor and a pairwise deltaE floor; measuring an overlay div that carries no text would report a number nothing depends on. 1260 measurements across 20 themes, green. SWEEP_ONLY names a subset while a new surface is being built. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…while README, roadmap prose + board SVG, security.md (why an editable pane costs nothing: merge:write already took arbitrary text, so editing widens what the READER can express, not what the surface accepts), and the spec's record of where the build deviated from the plan. Also corrects "14 themes" wherever it was still written — README, CLAUDE.md and standards.md — since themes.css has shipped 20 for some time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Taking their side into a conflict where OUR side deleted the lines wrote over the first stable line after it, and that line was gone from the file git was then handed. Two causes, both live: `wholeLines()` could not tell the two apart. An empty region and a one-line one are both `(n,1)-(n,1)`, so it expanded the empty one to a whole line — the stable line that happened to sit there. Emptiness cannot be read back out of a Monaco range, so it is now tracked beside the decorations and passed in. Then the same region a second time. Monaco grows a decoration around inserted text by its own rule, and `(n,1)-(n+1,1)` does not mean here what it means there: this code reads it as TWO lines. Changing your mind after taking a side therefore deleted the line below. `writeChoice` now re-anchors the decoration over exactly the lines it wrote rather than trusting how the editor moved it. Found by reading, and it is the fake editor's blind spot: the unit tests passed the first fix because the fake never moves a range on edit. Both cases are e2e — real Monaco, real git, bytes compared on disk — which is the only level that could have caught the second one. Three more guards for paths nothing was holding: two conflicts resolved out of order, a CRLF file written back as CRLF, and a file that ended without a newline still ending without one. (The out-of-order fixture needs nine unchanged lines between the two changes — with three, git folds them into a single conflict and the test proves nothing.) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rom review An empty region was tracked in an array beside the decorations, which meant it could disagree with the text — and undo is exactly what made it disagree. Take theirs, Ctrl+Z, take theirs again: the cached flag still said "insertion point", so the answer went in ABOVE the restored lines and the line below was eaten. Typing into such a region had the mirror problem — its text read as empty forever, so Save never unlocked on a conflict the reader had visibly answered, and there is no chevron for that region to click instead. Both go away by not remembering it. A region holding lines is now anchored to the END of its last line, an emptied one to a POINT, and the difference is read back off the anchor. Monaco moves anchors through every edit including undo, so there is nothing left to go stale. From the review, in descending order of what a user would hit: - F7 and Shift+F7 never reached the merge view once the caret was in the result pane: Monaco binds both to wordHighlight.next/prev and stops propagation. They join F1 and Ctrl+Shift+P in the global unbind, which exists for this reason. - No chevrons and no word tints at all in a CRLF repository. findLines split on \n alone, leaving a \r on every line of the haystack, and region lines came through a parser that strips it — so nothing ever matched. Silent: the panes filled and the two features simply were not there. - The index stage reads had no size cap and no binary sniff, though the same file refuses a binary $MERGED by name. Three 40 MB strings decoded, sent over IPC and set as Monaco models, with git already waiting. - mergeInputsFor had no .catch(), so a rejection told the renderer nothing, wrote no sentinel, and left the terminal blocked forever. Degrades to markers. - A second mergetool launch while the view was open set new text under decorations anchored to the old file. Keyed by file name, so it remounts. - F7 stepped the merge from under an open dialog, pulling focus with it. `local` and `remote` were dead in the payload and contradicted the invariant stated three lines above them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… the dead slots row Two from the functional sweep. A file that mixes CRLF and LF comes back out of the result pane with one ending throughout — Monaco keeps a single EOL per model, and the merge view is the first thing to put the merged text through one. Lines the reader never touched change bytes on save. It cannot be prevented at this layer, so the band says so before anything is written; the note states what happens to the file, not the reason behind it. The file-slots row and the toolbar above it stayed on screen during a merge with every control disabled — a dead Left File… / Right File… pair, and a greyed Save sitting a few pixels above the live "Save the merge". A merge produces a file rather than comparing two, so the row goes. Also removes two throwaway audit specs that a `git add -A` swept into the previous commit. Playwright globs `e2e/*.spec.mjs`, so they would have joined `make e2e` and CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both were mine, and the first should not have reached CI: my local gate check grepped for "problems" and ESLint prints "1 problem", so a red run read as green. App.vue's template went five lines past its 120 cap. The file-slots row is now FileSlotsRow.vue with its own stylesheet, which is what the cap is asking for rather than a shorter way to write the same markup — App.vue loses three imports with it. The thirty-file walk exceeded Playwright's 30s per-test default on the Linux runner. It launches thirty Electron processes one after another because that is what git does; the default is a budget written for a test that drives one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
v0.4.29 shipped a merge resolver: one card per conflict, four buttons, no line context, no navigation, and no way to say "take theirs, then fix the indentation". This is the view the tools people already use have.
What it is
Three Monaco panes, scroll-synced, taking the whole window.
:2:/:3:), not scraped back out of the working copy. They are whole, marker-free files —<<<<<<<never reaches the screen, because it is a file format, not a user interface.name-rev, allowlisted toHEAD/MERGE_HEAD). A rebase writes noMERGE_HEAD, so both come back empty rather than naming the wrong branch.1 of 30counts the walk.Regions still come from the markers rather than being recomputed from three texts: git has already decided where the conflicts are, and a second opinion would offer choices git will not accept.
The audit found nine defects. Four were data loss.
The
/auditgate ran two agents; a read of my own found the first two. Every one is fixed and guarded except the last.mergeInputsForhad no.catch(), leavinggit mergetoolblocked foreverEmptiness is no longer remembered beside the decorations at all — a region holding lines is anchored to the end of its last line, an emptied one to a point, and the difference is read back off the anchor. Monaco moves anchors through every edit including undo, so there is nothing left to go stale. (9) cannot be prevented at this layer — Monaco keeps one EOL per model — so the band says so before anything is written.
Known, and not fixed here
A file whose ordinary text contains marker-shaped lines — a document about merge conflicts — has them parsed as real conflicts, and answering them drops one side. This is inherent to every marker-based merge tool and is true on
maintoday; it is filed rather than fixed, so this PR does not make it worse.Verification
npm run checkgreen — and the coverage floors go up, 94/87/95/95 → 95/88/95/96.useMergePanes.jswas 4.85% covered, so the logic deciding what bytes overwrite your file moved tomergePaneOps.jsand is driven by fake editors.make theme-sweepgreen — 1260 measurements across 20 themes, with a newmerge-viewsurface. Getting there fixed two pre-existing breaks in the sweep itself:status-bandhad been timing out sincedeps-diffwas added ahead of it, and a launcher hidden by something else taking focus hangs the screenshot rather than failing it. It also only walked 14 of the 20 themes.e2e/quick-look-window-recovery.spec.mjsis darwin-gated and was not exercised this round.🤖 Generated with Claude Code