Desktop: the wave-2 redesign — navigation, GitHub surfaces, and seven sweeps of repair - #26
Draft
antonarnaudov wants to merge 180 commits into
Draft
Desktop: the wave-2 redesign — navigation, GitHub surfaces, and seven sweeps of repair#26antonarnaudov wants to merge 180 commits into
antonarnaudov wants to merge 180 commits into
Conversation
…surface The second wave of the section-page redesign. Nine phases, each landing with its own tests: the app now answers questions it used to send you to a browser for, and stops hiding metadata GitHub already gives us. Pipelines (A1, A2) Mapper consolidation into github/maps.ts, then real log depth: inline, virtualized, ANSI-aware panes with live tail, replacing the small modal. Runs show #runNumber (not the internal id), actor, attempt and durations; jobs show their runner, queue latency and per-step timings. Clone control (E1, E2) Where clones land is a setting, with a per-action destination sheet and a folder-name override on every surface. ghrepo:open returns structured codes, so a collision reopens the sheet prefilled instead of dead-ending. Settings gained a manager for every clone on this machine — origin, badges, reveal, forget, and a typed-confirm delete whose refusal rule is one pure function. Metadata (A3) PRs carry merged-by, requested reviewers, commits, review comments and fork origin; issues distinguish closed-as-completed from closed-as-not-planned; comments carry edited markers, association badges and reactions. subjectRef() parses a notification's subject url, which is what lets Inbox Release and Commit rows open in-app instead of bouncing to github.com. Filters (A4) One facet vocabulary across Actions, PRs, Issues, Inbox and My Work. A facet without a predicate is server-side: Actions' filters re-fetch, so narrowing reaches runs the first page never loaded. Explore (E3, E4, E5) Global GitHub search as a rail page — repositories, people, organizations and code — with full repository pages (routed breadcrumbs, ref switcher, go-to-file over the whole tree) and account pages. Search is metered by a token-bucket guard that refuses before spending and reports how long to wait. Cmd-K searches GitHub live, debounced and generation-checked. Pure logic lives in DOM-free modules so it is node-testable: facetModel, exploreRoutes, searchDebounce, logModel, searchQuery, searchGuard, cloneName. 269 tests pass; both tsconfigs clean; every surface verified in dark and light through the in-repo headless harness (also added here). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Phase 1 and 2 of the polish pass. Both are root causes, not screens — each
one closes a cluster of findings from the surface sweep.
Overlays
Every floating layer mounts on document.body, so a view swap could not take
it with it: an Inbox facet menu survived navigation and hovered over the next
view, filtering a list that was no longer on screen. Layers now register how
to dispose themselves (overlays.ts) and routeView dismisses whatever is open.
openMenu also stopped leaking: it removed the previous menu's element but
never its capture-phase listeners, leaving a handler that still answered
Escape and refocused a detached anchor.
The command palette highlighted the wrong row. It re-selected by item
identity while search groups are PREPENDED, so the selection slid downward
as results arrived above it and Enter fired the bottom row. Identity is now
preserved only for groups appended below.
Eleven ad-hoc modal widths (420/480/520/540/560/640/760) collapse to three
tokens; two dialogs had been inheriting a width by accident.
Lists
The count badge reports what is ON SCREEN — it used to report the fetched
page size and never move, so filtering to two rows still read "8", and an
empty result read "8" above an empty state. It now reads "2 of 8".
The Inbox counted unread once and never recomputed, contradicting its own
summary line; that line is gone, the badge is the single source.
One meta order everywhere: author, then assignees, then counts. Issues led
with assignees, PRs trailed with the author, My Work used bare text — the
same circle in the same slot meant something different on each list and said
so nowhere. Avatars now carry their role.
Facet menus speak the language of the rows. They listed raw API values
("subscribed", "PullRequest") beside rows reading "watching" and "PR" — and
"manual" maps to "subscribed" while "subscribed" maps to "watching", so the
raw value was actively misleading.
Empty states gained a secondary action, ending four hand-copied "Clear
filters" blocks (one of which offered to clear filters that were not set),
and no longer sit 400px below the filter that emptied the list.
Issues' "Reason" is "Closed as" and only appears where it can match. The
Inbox toggle is a segment showing state instead of a button naming the next
action. Releases adopted the shared segmented control.
Initials no longer render punctuation: "s-ohta" was a tile reading "S-".
The notifications popover had two left edges (the unread dot was absent
rather than hidden on read rows), clipped every title mid-word at 424px, and
wrapped its refresh button onto an otherwise empty row. It is wider, titles
wrap to two lines, and the facet bar stays on the full Inbox page.
278 tests green; both tsconfigs clean; verified in dark and light.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he run
Phase 3 and the first half of phase 5 of the polish pass.
Pull Requests
The list was permanently open-only — it fetched `state=open` and had no
control — while Issues had Open/Closed/All one rail item away. It now has
Open/Merged/Closed/All. "Merged" is not a GitHub state (a merged PR is
closed with merged_at set), so it fetches closed and narrows locally; Closed
means closed-and-not-merged, so the two segments are disjoint rather than one
quietly containing the other.
The facet named "State" contained no states: Ready / Draft / From-a-fork.
Draft-ness and where the head branch lives are different questions, so they
are now "Review" and "Origin".
Actions
The run detail was two hollow rows in 900px of empty page: every step, every
runner, every duration was hidden behind a chevron. Jobs now open with their
steps, which is the content of the page.
One run had two numbers on one screen — the crumb showed the internal id
("#9100") while the title showed the run number ("#411"). The run number is
the identity now; the id stays in the rail where it is copyable.
The rail printed Status, Branch and Trigger that the header already showed
40px away; those are gone. Status returns only while a run is live, where it
is news rather than an echo.
Cancel is hidden on a finished run and "Re-run failed" on a successful one,
instead of sitting there greyed out forever. "View all logs" became a real
toggle rather than a one-way button that kept offering to do what it had
already done.
Run rows dropped the status word — the coloured icon says it, and now
carries the label for hover and screen readers — and no longer print the
workflow name twice when a scheduled run's title IS the workflow name.
Toolbars
gh-head-tools had no flex-wrap and no min-width, so a row of up to twelve
controls could only squash, clipping the search field and facet labels. It
wraps now; verified at 1100px.
278 tests green; both tsconfigs clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Phases 4-7 of the polish pass, worst-first.
Log viewer
`##[endgroup]` was emitted as a line. It carries no payload, so every group
left a blank NUMBERED row the raw log does not contain; the group now closes
at its last real line instead. The pane also had no ground of its own — in
dark its background was byte-identical to the app's, in light it sat 1/255
from its own toolbar — so a terminal dissolved into the page. Its height was
a fixed 380px, so a three-line log reserved the full box.
Searching stranded you: a search or error jump turned follow-tail off but
left "Jump to latest" hidden, so there was no way back to the tail. The
toolbar's titles never tracked their state ("Show timestamps" while
timestamps were showing) and used a history icon to mean timestamps.
Organizations
The header ran name → @login → buttons → a full-width rule → description, so
the description was cut off from the thing it described by the actions and a
divider, and "Copy login" led the page. It is now one identity block —
avatar, name, @login, description — with the actions beside it.
Cards were 280px and clipped mid-number ("★ 2,1…"); they are wider and their
meta wraps. The filter placeholder no longer clips to "Filter repos, teams,
mer".
Safeguards and paths
The typed-delete confirmation used the required text as the input's own
placeholder — showing the answer inside the box you must type it into, which
teaches copying what is already on screen. Paths now shorten from the MIDDLE
(new textFit.ts): right-truncation ate the repo folder, so two clones under
different parents both read ".../Developer/GitStu…".
Rebase's loading state was a spinner in the top-left corner of a blank pane,
and became permanent if the response was an unexpected shape — the check
read `state.ok` outside the try. It is centred, inside the try, and offers a
retry.
Naming
The repo menu was the app's only Title Case surface. Cloning was called
three different things within two clicks; it is "Clone repository"
everywhere.
Also: the Inbox gained the search field every sibling list already had.
283 tests green; both tsconfigs clean.
Note: the sweep's "Commits has no toolbar" finding was a false positive — it
was observed against an empty graph fixture. With real rows the view has its
ref chip, count, search and filters.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e, Code
The last of the sweep's layout findings.
Changes
The M/A status letters could not be scanned as a column: staged rows carry
one action ("Unstage") and unstaged two ("Stage", "Discard"), so the two
groups reserved different widths for their hover cluster and the letters sat
~50px apart. The slot is now sized for the widest case — measured, all five
letters land on the same pixel.
"Changes" named the view, the pane, and the unstaged group all at once, so
one set of files had three names. The group is "Unstaged", which pairs with
"Staged" the way it always should have.
Commits
Two empty states used to sit side by side, the second contradicting the
first: "No commits yet — this branch has no history" beside "Select a commit
to inspect its message". With no history there is nothing to select, so the
details pane now stands down.
Branches
Group counts were pushed to the far right edge of the window — ~1300px from
the label they count — at 75% alpha, which in light theme was invisible.
They sit beside their label at full contrast.
Compare
Empty count badges rendered as grey dots, so the tab strip looked like it
carried two unread markers.
Code
The repository name was printed twice within 45px, both with a folder-ish
icon: once in the top-bar switcher and again as the only breadcrumb at the
root. The crumb trail appears when there is a trail to draw.
283 tests green; both tsconfigs clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A native checkbox renders as a bright white box — on the dark settings page it was the highest-contrast element on screen, louder than every heading around it. It now uses the same border, panel and accent tokens as everything else, with a real focus ring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tag peek told you to fetch from the remote and then offered no way to do it: an instruction with no affordance. It has a Fetch button. Expanding a job log grew the pane to 78vh while it sat ~320px down the page, pushing its tail — the error line, the toolbar — below the fold, so 'expand' made the thing you wanted less visible. It now scrolls into view. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Actions Opening a log from the row's Logs button left the card's chevron pointing right and its steps hidden, so the two ways in produced visibly different cards. The dispatch menu truncated every workflow to ".github/workflows/…", cutting the file name — the only part that tells them apart; it shows the file name. Explore The repository page had no title: the repo was named only in 13px of toolbar breadcrumb. The ref switcher said "default branch" — the KIND of thing selected — while the rail said the default is "main"; it now names the branch. Star and fork counts print with separators, matching the footer on the same screen. The header subtitle and the start state said the same thing twice in different words; the start state now says what to do. Issues and Gists A not-planned row stated the same fact twice, two glyphs apart — a slash-circle icon and a "Not planned" pill. The icon carries the words in its tooltip. Gists' badge counted the unfiltered set, so it read "2" above "No matching gists". Compare The swap control was a bare glyph with no border or fill, wedged between two bordered pickers — it read as a separator. It is a real button, and uses a two-way arrow rather than the view's own compare icon. Settings and dialogs The SSH card said "Public keys found in ~/.ssh" and then, on the next line, "No SSH keys found in ~/.ssh". The clone dialog's GitHub tab said the folder name is "Derived from the URL" — a field that tab does not have. The command palette's hint column restated the group header it sat under. 283 tests green; both tsconfigs clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A re-inspection of the fixed build confirmed 17 defects gone and found that four of my own fixes were incomplete or had introduced new problems. Changes composer The branch label showed "…" and never resolved, and the button stayed "Commit" instead of "Commit to main" — the placeholder that replaced the false "detached HEAD" was never filled in, which is worse than the wrong answer it replaced. The composer now reads the awaited `head:get` (what the top bar uses) and repaints when it lands. Facet menus Only the selected option carried a check glyph and nothing reserved the gutter for the rest, so the label column jumped 25px depending on what was selected. Every row now occupies the icon slot. Actions header at narrow widths Letting the tools row wrap fixed the clipping but not the collision: below ~1200px the search field ran into the first facet pill and lost its border, and the facet GROUP — one atomic flex item — still ran off the right edge. The search field has a floor, the header stacks below 1280px, and the facets wrap individually. Verified at 1150px: nothing overflows. Log pane expand Resizing changes the pane's scroll height, which the scroll listener read as "the user scrolled away from the bottom" — so expanding silently turned follow-tail off and dumped you into the middle of the log. Resizing is not scrolling. The expanded height is also capped to the viewport so the pane's toolbar and "Jump to latest" pill stay reachable. 283 tests green; both tsconfigs clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e than one question The harness could only show that a surface renders. It could not show that the count badge tracks the filter, that a disabled button is disabled, that a menu is dismissed on navigation, or that two columns share an x — and those are the things that kept regressing. harness/check.mjs drives a scene in headless Chrome, runs an assertion from harness/checks.js INSIDE the page after the driver finishes, and reports pass/fail with a non-zero exit code. 36 cases cover the behaviour this polish pass changed: overlay dismissal, palette selection, facet labels and their alignment, the commit button's enabled state, Compare's default refs, the Changes status column, log rendering and follow-tail, run identity, PR state, Explore search, org card clipping, toolbar overflow at 1150px, and the Settings list's per-row action rules. Writing them found three defects in my own checks (wrong selectors, and a driver that could not type into a textarea) before they could vouch for anything — which is the point. Fixtures: every run returned the same two jobs and every job the same failing log, so the success path, the failure path and a multi-job matrix were all unreviewable — the fixture answered every question the same way. Runs now carry job sets that match their conclusion, and a green job's log ends green. Step duration bars are normalised across the whole RUN rather than per job. Per-job scaling drew an 11s step and a 7m48s step at the same length in adjacent cards, which makes a bar that exists to be compared actively misleading. Asserted: bar width is monotonic in duration across every card. 36/36 functional checks pass; 283 unit tests green; both tsconfigs clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Row meta The cluster packs right-to-left, so any slot whose width depends on its content — one avatar vs three, "+12 −4" vs "+1240 −180" — dragged every slot left of it out of column, and a row missing a datum slid its avatars into the place where the next row shows its comments. Optional slots are now reserved (blanked, not omitted) and each kind reserves the same width on every row. Asserted: author avatars share one x and times share one right edge, across Issues and PRs. Command palette The highlighted row IS the button Enter will press, and it measured 1.14:1 against the panel — a wash, not a state. It has an accent bar and a real fill, and the check computes the contrast rather than trusting the eye. Dialogs In the state you first see, the primary action is usually disabled, and a washed-out purple beside a solid Cancel made Cancel the loudest thing in the dialog. Cancel recedes instead. Peek cards The header packed three buttons and a close X into one row, leaving the identity — the reason the card is open — about a third of the width. 283 unit tests; 40 functional checks; both tsconfigs clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Workflows tab Each row was a name at the far left and a path at the far right with ~1000px of nothing between, and carried no state at all — you could not tell from the list whether a workflow had ever run. Rows now show the last run's status, number and age, and the file name sits with the workflow name instead of stranded at the opposite edge. The first version of this claimed "never run" on every row, because the Workflows tab never loaded runs — asserting something false rather than admitting we had not looked. It now loads them (sharing the Runs tab's cache key, so switching tabs is free) and only says "never run" once that is actually known. The check asserts both halves. Settings Every block inside a card was separated by the same gap with a few ad-hoc margins on top, so hierarchy read flat. Headings get air; their explanatory line stays tight to them. Housekeeping Removed CSS orphaned by this pass (the inbox toggle became a segment; run rows dropped the duplicated status word), and dropped two rules I had just written for classes the renderer never emits. Fixtures: runs now carry the workflowId of the workflow they belong to, so the last-run column is exercisable instead of uniformly empty. 283 unit tests; 41 functional checks; both tsconfigs clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Explore people/org results A 40px row with a small avatar in a 1350px pane read as ~93% empty. My first attempt added a sub-line — which said "Person on GitHub" identically on every row, i.e. filler, the exact fault this pass has been removing. Reverted: the answer to a sparse row is a DENSER row. The search API gives a login, an avatar and a type, so the row is one tight line with a larger avatar. Projects The board's only action was a bare unlabelled external-link glyph sitting alone under the title. Labelled "GitHub", like the Organizations header. Gists The visibility pill was placed before the H1, pushing the title ~110px right of the left rule every other heading starts on. It follows the title now. 283 unit tests; 40 functional checks; both tsconfigs clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…for the
CSS failure I just caused
While de-emphasising one button I replaced a rule's body and left
`.dc-createpr {` unclosed, which swallowed every rule after it and collapsed
the section lists into inline fragments two per line. The functional harness
caught it on a view I was not looking at; a screenshot of the view I WAS
looking at would have shipped it.
So the stylesheet now has structural guards (test/stylesheet.test.ts), and
they carry negative tests, because a guard that cannot fail proves nothing:
• no comment closes early on a star-slash inside a selector glob — the bug
that silently killed --sp-1, .sec-list padding and .gh-head-tools margin
• braces balance — an unclosed rule swallows everything after it
• no selector sits directly inside a rule block (at-rules excepted)
Writing them turned up a fourth instance of the first bug: in the test file's
own doc comment.
Changes
Selecting a file no longer reflows the toolbar — "Stage lines" and the
whitespace toggle are disabled rather than hidden, so the button you were
aiming at stops sliding ~160px out from under the cursor.
Staged rows carry an invisible twisty cell, so the list has one left edge
instead of two ~29px apart. Asserted.
"Create pull request" is no longer the only accent-coloured control on a
screen whose job is staging and committing.
Stash is labelled: it moves your working tree, and its only affordance was
an unlabelled archive glyph between two text buttons.
Actions
Run rows adopt the same column contract as the other lists: actor, branch,
event and duration each keep their slot, so a run missing one does not slide
the rest out of line.
288 unit tests; 42 functional checks; both tsconfigs clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… actions and header anchoring Branches drew ahead/behind three ways and styled a live network action as a passive count: the behind badge WAS the pull button. Ahead and behind are now a matched pair beside the branch name, and Pull joins Checkout and Delete in the row's action cluster. The branch peek's chips now say the same thing the list does. Organization members rendered the three-person ORGANIZATION glyph when they had no avatar, so a person read as the org itself; they use the person avatar (initials + per-login hue) like every other list. The card grid held three densities in one track size — a lone team card sat in a 330px column beside 1200px of nothing, a member card was 90% empty at the same width. Repos and teams use auto-fit so a short list reads as full-width rows; members are fixed-width chips that wrap from the left. The bottom dock's Output tab carried a 32px toolbar Terminal did not, so the content origin jumped as you switched tabs — and on an empty log that bar offered 0
…r where you asked, one clone form, and two views the harness could never see Six lists printed their meta as "whatever this row happens to have", packed right-to-left. A gist without comments slid "1 file" 69px; an unpublished release shoved its tag and author 90px past every other row's; a read inbox thread landed 79px right of an unread one because the hover cluster is one button narrower; an Actions run with a long branch name pushed its actor avatar out of line. Every optional datum now holds its column and goes invisible rather than absent, and each column has a floor sized to its widest real value — the Inbox in particular went from three ragged edges to three clean ones. The functional check that guards this was rewritten to measure what actually matters: for every KIND of meta a list carries, one column. It works on any list instead of assuming issue-shaped rows, keys the Nth of a kind separately (a row can hold an author stack AND an assignee stack), and now runs against six lists. It found every misalignment above. "No results" was centred: ~290px below and ~600px right of the search box you were still looking at. emptyState grew an `anchor` — hero for "there is nothing here at all", inline for "your query matched nothing", which anchors top-left beside the control that emptied the list. Applied to Explore, the four GitHub lists (filtered only) and the org sub-tabs. The clone dialog's three consecutive fields had three structures: a bare placeholder-only input, a label-value-button row in a bordered card, and a caption above an input inside a SECOND bordered card. One `cloneField` now builds all of them — caption above control, one left edge, real <label>s. Explore's People results were 40px rows holding one login across a 1350px pane; they are chips that wrap from the left, like the org Members tab. A code hit rendered one bordered box PER MATCHED LINE — stitched by :has() rules that could not close the row body's own gap — so one hit read as three; it is one block with a diff-style separator between non-adjacent fragments. Rebase and Compare had no fixtures, so every screenshot of them was their ERROR state. With fixtures: the rebase anchor row now stands in the action column so every subject shares a left edge, its legend stopped wrapping two of six glosses, and Compare's file list adopted the Changes name-then-directory row (a 370px column was truncating away the filename) and says "2 commits only on <base>" instead of making you work out whose commits those were. Also: empty project-board columns yield their width (a lone "No status · 0" held a quarter of the board), and sentence case reaches the last Title Case labels — Create branch here…, New issue, Start rebase, Open repository…, and Clone repository… on the welcome card, which the palette had already called that. 288 tests, 70 functional checks, both tsconfigs clean.
…the two fixtures that were hiding it The commit graph's CHANGES cells left-aligned their file count, so 3, 11 and 17 stepped every proportion bar right by a character. A column of meters you cannot line up is not a column: the count now sits in a fixed right-aligned slot and every bar starts on one x. This is in the shared webview package, so the VS Code graph gets it too. The reason nobody had seen the column at all is that the harness answered `commit:rowStats` with nothing, so five rows of empty cells sat under a labelled header and read as an app bug. Two fixtures were also parked in the wrong table — the shim keeps plain values in `fixtures` and callables in `dynamic`, and a function left in `fixtures` is handed back AS a function, so the caller silently gets nothing. Also extends the meta-column check to My Work, and adds one for the graph's own columns — it reads through the Lit element's shadow root, which nothing in the suite had needed before. 288 tests, 72 functional checks, three tsconfigs clean.
…l rhythm The Settings column was pinned to the 820px reading measure, which left ~255px of empty gutter on each side at 1600px while squeezing the rows that are not prose at all — a clone row carries a name, an owner, a badge, a path and an action cluster. The column moves to its own token (1040px) and the prose inside keeps a reading measure of its own, so a paragraph never runs 130 characters just because the form got wider. Inside a card every child was 12px from the next, which meant "App icon" sat exactly as far from the sentence explaining it as that sentence sat from the control above it. There were no groups, only a list. A label now sits tight to what it introduces and pushes off from what precedes it. The check asserts the relationship rather than the numbers: for every field label, the gap above must exceed the gap below. 288 tests, 73 functional checks.
… filter that shows its states, a palette hint column worth reading, and a peek that is about its subject Collapsed to icons, the rail hid the group LABEL and the group RULE, so fifteen destinations became one undifferentiated column separated by nothing but a slightly bigger gap. The label goes; the rule stays and carries the group's name in its tooltip. The Actions Status filter listed its five states as plain text — in a product that gives those exact five a colour and a glyph on every run row, every job card and every step. The menu now uses the same lead icon the rows use, which also meant fixing the generic `.dropdown-item .glyph` muted rule that was repainting them all grey. The command palette's hint column restated its own group header: "view" six times under GO TO, "branch" three times under BRANCHES & TAGS, "tag" beside a tag glyph. Only hints that say something new survive — "current", "#104". And the list fades its bottom edge instead of slicing the last row exactly in half, with the fade off when the list fits. A peek's header gave three buttons and a close X ~540px of a 700px card and left the identity — the thing the card is about — around 230. The actions wrap to their own line, the name keeps a floor, and a member peek shows that person's avatar rather than a generic account glyph. Settings' About and SSH cards stacked two action languages (a bordered button above a bare purple text link); both are buttons now, side by side, and the version stopped rendering as code. A gist's detail page dropped the "updated 2d ago" line that sat directly above an About rail saying "Updated · 2d ago". 288 tests, 77 functional checks. Every finding in the 100-item sweep is now either fixed or recorded as a false positive.
…he shared checks shot.sh shows what a surface looks like and check.mjs asserts a named invariant, but neither lets you simply ASK a rendered scene a question — how wide is that column, does that button have an accessible name, what does the focus ring compute to in light theme. Answering one meant first adding a case to harness/checks.js, which serialises investigation behind a single shared file. probe.mjs runs a JS body inside the driven page and prints what it returns, with a small vocabulary ($, $$, box, css, text, settle) so every caller measures things the same way. It works on surfaces no check has ever touched — a PR detail page answers its tabs and crumb on the first try. Rebased onto main (prune-on-fetch + 1.12.0 / 1.6.0). The Settings conflict was two additive sibling cards; both are kept and both are wired. 288 tests, 77 checks green on the new base.
Three bugs in the stale-while-revalidate cache that every list in the app reads
through. None of them look like anything on screen — the list just quietly
stops updating, or shows an error it will never recover from — which is why
they survived a hundred-finding visual sweep.
`gget` short-circuits on an entry's in-flight marker BEFORE it looks at the TTL.
That is correct for deduping concurrent readers, but the marker was only ever
cleared inside an `if (!superseded())`, so any request that settled after the
cache had been invalidated left its marker in place forever:
- Staging a file fires bust("status") and bust("diff"). Both bump the global
epoch. If a GitHub list happened to be loading at that moment, its entry
kept a settled promise, and every later read returned that same answer for
the rest of the session. Refresh did nothing.
- If that request had FAILED instead, the entry served the rejection forever:
a list stuck on an error with no way back short of switching repos.
Clearing the marker and publishing the value are two different decisions, so
they are now two different things: always retire the marker, publish only when
nothing invalidated the cache meanwhile. A failed read leaves the last-known-good
readable via `peek` but keeps its ORIGINAL timestamp, so the next `gget` still
refetches.
The third bug fell out of the same rewrite: settling now checks that the entry
it is about to touch is still OURS. A value seeded by `prime()` from a push
event — newer than a read that started earlier — used to be clobbered by that
older read's answer, and a request that outlived a bust could overwrite the
newer request that replaced it.
Twelve tests drive the cache through a stubbed host with hand-settled promises,
because every one of these only happens in the window between a request
starting and finishing. Three of them failed before this change.
…po that is actually open `removeRecent` already knew that raw string equality is the wrong way to compare repo roots — it resolves both sides, with a comment explaining that a recent stored through a symlink would otherwise never match and "Forget" would silently do nothing. `promoteRecent` compared raw strings anyway, and the constructor trusted whatever the persisted JSON held. So the same repo could occupy two slots in a twelve-slot list: the file said "/x/repo/", discovery handed back "/x/repo", and opening it added a second entry instead of moving the first. That list is the app's memory of where your work is; two of the same thing pushes a real repo off the end. The comparison is now one exported function, used on the way in, on the way out and when persisting, and the list math is pure so the ordering rules are testable without a git repo on disk. Separately: when a second `open()` overtakes the first, the loser correctly touches no shared state — but it still returned the root IT discovered. Its caller was told "you opened A" while the active context was B, so the window would render A's identity against B's repo. It now reports whatever is actually open, falling back to its own root only when nothing is (so a race on first launch cannot raise a false "not a Git repository"). Also: the Open Recent submenu's empty label was the last Title Case string in the app.
…g unparseable tags through verbatim
sanitizeHtml is the one security boundary in the renderer. Every issue body, PR
description, review comment, release note, gist and README the app shows is
attacker-controlled text from GitHub.
The allowlist pass rewrites each tag it matches, but its pattern needs a `>`
after a balanced run of attributes. A tag with an UNTERMINATED attribute quote
— `<img src="x` — therefore does not match at all, and unmatched text was
passed through VERBATIM: the one tag that most needed filtering was the one tag
that skipped it.
That is not merely untidy. The browser runs the open quote on to the next `"`
anywhere in the document, and the sanitizer supplies one itself from a later
tag's `title="…"`. Everything after that quote is then parsed as attributes of
the unfiltered tag. A body containing
<img src="x
some ordinary text
<b title="onerror=alert(document.domain) x">bold</b>
produced an <img> carrying a live onerror attribute. Verified in headless
Chrome rather than by reading the regex: the element came back with
["src","onerror",…] and the handler fired.
SEVERITY, stated accurately. In the shipping app that handler does NOT run: the
renderer's CSP is `script-src 'self' blob:` with no 'unsafe-inline', which
blocks inline event handlers, and the window is contextIsolation:true,
nodeIntegration:false, sandbox:true. I confirmed this too — the same payload
under the app's real CSP reports pwned:false, with the onerror attribute
present but inert. So this was defence-in-depth doing its job, not a live RCE.
It is still worth fixing properly, because the sanitizer was failing OPEN. It
put attacker-chosen attributes onto elements it never inspected, and the only
thing standing between that and script execution was one meta tag.
The fix is to stop trusting anything the pattern could not parse. Allowlisted
tags are parked behind a sentinel, every remaining `<` is escaped, then the
parked tags are restored. A `<` that the sanitizer did not itself produce can no
longer reach the DOM — which also subsumes the old lone-`<` rule, so `a < b`
still reads as prose. Well-formed but disallowed tags are still dropped rather
than escaped, so an <iframe> does not become visible source.
The regression test asserts the PROPERTY rather than a list of payloads: every
`<` in the output must begin a tag the sanitizer produced. That is what makes
the class impossible instead of one instance of it. Sentinel forgery is covered
too — a body carrying U+E001 cannot name a parked slot.
328 tests.
This function's own contract says it removes absolute paths "INCLUDING the
file/project names in the tail", and it calls itself the last line of defense
before anything leaves a user's machine. It was not doing that in the two
places real users actually live.
**Windows, for the user's own machine.** `safeHome()` collapses the home
directory to `~` before the path pass runs, and on Windows what follows a home
directory is a BACKSLASH. The tail rule only accepted a forward slash, so it
matched the `~` and stopped. Every crash report from a Windows user shipped
their whole project path:
at load (~\Projects\acme-secret\src\billing.ts:42:11)
**Any path with a space in it.** Every path pattern used `[^\s"':]+`, so
`C:\Users\John Smith\projects\acme-secret\index.ts` redacted as far as the
space and left `<path> Smith\projects\acme-secret\index.ts`. Same for
`/Users/John Smith/…` and for UNC shares, where spaces are the norm
(`\\CORP-FS01\Team Share\…`). A trailing run is now redacted too — but only
when it still contains a separator, so a real sentence ("/Users/bob is not a
repository") keeps its words instead of collapsing to a marker.
**IPv6 was never handled** despite "IPs" in the contract. It is now, in the
full eight-group form and the compressed `::` form.
The IPv6 rule is deliberately narrow. The obvious pattern — two or more
colon-separated hex groups — redacts every `01:23:45` timestamp in a log and,
much worse, the `:42:5` line and column this function goes out of its way to
preserve so a crash stays locatable. I wrote that version first and it silently
turned `billing.ts:42:11` into `billing.ts:42<ip>`. The compressed rule now
requires a literal `::` ahead of it and refuses to start after a word
character.
Also: env-var-rooted Windows paths (`%USERPROFILE%\Documents\Acme`) redact
everything after the variable, which identifies nobody by itself.
Six regression tests, written as "this string must not appear in the output"
rather than as expected-output comparisons, so they keep meaning if the exact
markers change. 23 tests in the package.
… a template Both callers pass literal labels today, so this is not a live hole. But the line above it already assigns remote error text, and a template that interpolates a label straight into innerHTML becomes an injection the moment someone passes a branch name or an error string through it.
…operty, not a habit
Every mutation on the git bridge takes strings straight from the renderer and
hands them to git, which reads any argument beginning with "-" as an OPTION.
`safeArg` exists for exactly this and is applied in two dozen places — but
"applied in two dozen places" is a habit, and a habit is what the next handler
skips.
So the rule is now checked. A structural test reads gitBridge.ts, finds every
method returning a CommitActionResult, and requires each to either guard its
arguments or appear in a reviewed list WITH the reason it does not need to
("path goes after `--`", "message is the value of -m", "no arguments"). A third
test keeps that list honest in the other direction: an entry that has since
grown a guard, or that no longer exists, is a comment claiming something
untrue.
Auditing the 36 mutations to write the list turned up one real gap.
`WorktreeProvider.remove` builds `["worktree", "remove", path]` with no `--`,
so a worktree path beginning with "-" reaches git as a flag. The paths come
from git's own worktree listing today rather than from free text, so this is
hardening rather than a live hole — but it was the one mutation on the bridge
without the guard all its neighbours have, which is precisely the shape this
test exists to notice.
Parsing note: the first version matched signatures with one regex and a lazy
span between the method name and its return type. That span happily runs
THROUGH the following method to find a matching return type, so it reported
`stashList` and `opState` as mutations and lost `stage` entirely. It counts
parens and braces now.
332 tests.
Every git command the open repo runs is streamed to the renderer's Output tab
as `git ${args.join(" ")}`, verbatim. Two of those commands take a URL as a
positional argument — `git remote add <name> <url>` and `git remote set-url` —
so a remote with credentials in it puts the token on screen:
git remote add origin https://oauth2:ghp_…@github.com/Acme/repo.git
That log is a surface we actively invite people to read: it has its own tab, a
copy button, and its whole purpose is to be pasted into a bug report. git's
stderr, forwarded on the same channel, echoes remote URLs on auth failures too.
`scrub()` already exists but is the wrong tool — it is built for crash reports
and removes hosts, paths and repo names, which would leave the log saying
nothing. So this adds a narrow sibling, `redactCredentials`, that takes out
only the secret: the password half of a URL's userinfo, and any bare GitHub
token. The user half stays, because it is usually "oauth2" or
"x-access-token" and knowing which is half the reason to read the log.
Applied to args, to the rendered command string, and to stderr. Four tests,
including one asserting that ordinary commands come through byte-identical —
a redactor that quietly rewrites `git status` is worse than none.
… pane
Every tool that draws a progress bar — npm, pip, docker, gradle, cargo —
rewrites ONE logical line in place with carriage returns and terminates it with
a single newline. The log parser kept that text verbatim, and a `\r` paints as
nothing in HTML, so the pane rendered
Downloading 0%Downloading 25%Downloading 60%Downloading 100%
as a single run-on line. Any real `npm ci` or `docker pull` step looked like
garbage, which is most of what people open a job log to read.
Carriage returns are now applied the way a terminal applies them: return to
column 0, and what follows overwrites what was there. That is a genuine
overwrite rather than "keep the last segment", so a short redraw over a longer
line leaves the longer line's tail behind exactly as a terminal does — "abcdef"
then "\rxy" is "xycdef". It also drops the stray trailing `\r` that a CRLF log
was leaving on every single line, which nobody could see but which came along
whenever you copied the log.
Order matters and cost a first attempt: the overwrite has to run AFTER the
timestamp is stripped. GitHub stamps once per newline, so the stamp sits before
the first segment, and applying the overwrite to the whole raw line lets a
later redraw paint over the timestamp. There is a test for exactly that, and
one for a `##[error]` directive still classifying when it arrives after a
redraw.
338 tests.
…caped its href
Every panel in this extension builds HTML by concatenating strings, and each
carries its own three-line `esc()` because most of them live inside a webview
script that cannot import anything. Five copies of the same function is a shape
where one quietly falls out of step, and one had: the AI results panel escaped
`&`, `<` and `>` but not `"`, while its four siblings all escape it.
That matters because `esc()` runs over the WHOLE markdown source before any
HTML is built, and the link rule then writes the URL straight into an
attribute:
[t](https://a"onmouseover="alert(1))
-> <a href="https://a"onmouseover="alert(1" target="_blank" …>
which is a live event handler on the anchor. `safeUrl` did not catch it either:
it tests what a URL STARTS with, not what it contains.
Severity, stated accurately: the webview CSP is `script-src 'nonce-…'` with no
'unsafe-inline', so that handler would not have run. Same story as the desktop
sanitizer fixed earlier today — the escaper failed open and the CSP was what
stood behind it. The input is model output rather than a raw issue body, which
lowers it further, though model output does quote repository content.
Both layers are fixed: `esc()` now escapes `"` and `'` like its siblings, and
`safeUrl` rejects a URL containing a quote or an angle bracket rather than only
checking its scheme.
And the rule is now checked rather than remembered. A new test walks every
`function esc(` in the extension and requires it to cover `& < > "`, and to
replace `&` first (or it double-encodes its own output). Removing the quote
again fails it, by name and line — I confirmed that before keeping it.
116 tests in the extension.
Two defects in the commit composer, both from the same root: amend state was written in more than one place and read in fewer. **Ticking Amend left both commit buttons dead.** The prefill assigns `textarea.value` directly, and a programmatic write fires no `input` event — which is what `syncCommitEnabled` and the surviving draft both hang off. So the previous commit's message appeared in the box, the button relabelled to "Amend commit", and it stayed greyed out insisting you "write a commit message first" while it sat in front of you. Commit & Push was dead the same way, so there was no working alternative. There is now one way to put text in the composer, and it goes through the same path a keystroke does. **Repainting the list made the label lie.** Staging a file re-runs showChangesView(), and the label was rewritten unconditionally afterwards without consulting `amend`. The toggle stayed lit, the message was wiped, and the button went back to reading "Commit to main" — while a click still sent `amend: true`. You would type a fresh message for what you read as a new commit and rewrite the previous one instead. The label now comes from one function that both writers call, and the prefilled message survives the repaint because it reaches `composerDraft` like any typed text. Also in `streamInto`: the closing `.trim()` is a programmatic write too. If a model returned only whitespace it emptied the box without telling anyone, and the commit buttons stayed enabled over an empty message. Found by a 270-agent adversarial sweep of the surfaces no functional check covered — 84 findings confirmed of 128, after two independent verifiers had to agree on each. These two were the only ones that could destroy work. Three new checks hold the line: the prefill enables BOTH buttons, the label still says "Amend commit" after a repaint (a label that disagrees with the flag is the whole bug), and — from the same sweep — no element anywhere in the app carries an inline event handler, plus a route-churn check that a view rendered for the third time weighs exactly what it did the first. 81 functional checks, 338 tests.
The same work-loss bug fixed for rebase hours ago, introduced one
operation over by the very change that fixed it — `sideLabels` lumped
`am` in with `rebase`.
They are not alike. A rebase checks the upstream out first and replays
your commits onto it, so stage 2 ("ours") is the upstream. `git am` does
nothing of the kind: it applies a mailbox patch onto the branch you are
standing on, so stage 2 is YOUR branch and stage 3 is the patch.
Verified against real git, in the test.
So during a patch application the button reading "Take Upstream (what
you're rebasing onto)" handed you your own branch, and "Take Your commit
(being replayed)" handed you someone else's mailed patch — with the
tooltip and the success toast agreeing with the label rather than with
what happened. Whichever side you took, you were told you had taken the
other one.
`opState` never confused the two and does not need changing: a rebase on
the apply backend uses the same `rebase-apply/` directory but writes
`rebasing` rather than `applying`, and is reported as "rebase". Only the
labels were wrong.
Two tests, one of them driving a real conflicting `git am` to prove the
stage assignment rather than assuming it. Both fail on the old branch.
The existing case asserted that sideLabels(cherry-pick) equals sideLabels(merge) — which is a statement about the function agreeing with itself and proves nothing about git. That is precisely how the am labels came to be wrong: grouped by assumption, never checked. All four operations that can leave a conflict now have their stage assignment proven by a real conflicting operation: merge, rebase, am, cherry-pick and revert.
The checks written this morning against the new push-event registry
invented their payloads. `git:log` was sent as `{at, args, code, ms,
action}`; the real `GitLogEntry` is `{id, args, command, durationMs,
exitCode, failed, action, actionId, at}` — almost none of those field
names. `terminal:exit` omitted `exitCode` entirely.
Both checks passed anyway, which is the point. A fixture can be wrong
about reality and everything built on it still goes green: the Output
check counted four rows for four entries and called that "the commands
are logged", and that reading was an artifact of the wrong shape.
With the real one it is four rows for one, because four identical
commands under a single `actionId` COALESCE into one row with a ×4
badge — which is what stops the status poller flooding the pane, and is
better behaviour than the check was asserting. It now pins that, plus
the other half: distinct commands each keep their own row.
Verified the shim's `on()` against the real preload while here — same
contract, listener receives the payload and gets an unsubscribe back.
The agent emits tool_denied and then a tool_result carrying the sentence it feeds back to the MODEL — "Do not retry it; adapt or stop and explain" — which was rendered as a red error step, addressed to the person who had just made the decision. The check drives both halves at once: one tool denied, one tool that genuinely fails. The declined step must be marked declined and not an error, must say so in a word the reader owns, and must not put the model-facing instruction on screen; the failure must still read as one. Two assertions fail on the old code.
…keeps it `is-danger` was styled for `.dropdown-item` only. The merge bar's "Delete the file" — offered for the side of a modify/delete conflict that has no version of it — carried the class and got nothing from it, so the two buttons on that bar were pixel-identical: one keeps your file, one removes it and stages the deletion. Added the treatment for `.mini-btn.is-danger` too, and the state-table check now asserts the two buttons do not render the same colour. It fails on the missing rule. A destructive control that looks like its opposite is worse than an unlabelled one — this was found by asking what a class I had just added actually did, rather than assuming it did something.
`routeView` disposes exactly one thing — `activeMonacoView` — and a view that does not register there leaks whatever it built. The commit page built a DiffPanel and registered nothing, so every commit you read left an editor behind with its two models and their tokenizers, for the life of the window. That is the third leak of this shape (the PR diff panel, the job log's pane and its 200,000-line document, now this), and the third hand-rolled MutationObserver written to fix one. They share `disposeOnDetach` in views/common.ts now; the other two are migrated onto it. No check guards it, deliberately. The leak is in the editor OBJECTS, and `monaco` is not exposed to the page, so a probe can only count `.monaco-editor` DOM nodes — which go away when the host's children are replaced whether anything was disposed or not. Measured: it reports success on the broken build, which is worse than not checking at all. The reason is recorded at the call site.
From sweep 6's state table over the Assistant, which enumerates the gate against the turn state — the two axes these all sit on. - a ✨ action fired while the Assistant was already on screen routed to it with `force: true`, which drops the view from the cache and rebuilds it. So asking for a second explanation while the agent was answering the first destroyed the transcript and the Stop button and orphaned the run, with no confirm and no cancel. Exactly the defect the refreshAll exemption fixes, reached through a different door. A live Assistant now takes the goal directly, and says so if it is busy rather than discarding the work. - the four quick-action chips stayed enabled during a run, where `runGoal`'s `if (running) return` swallowed the click. - both "New chat" entry points — the header + and the history menu — stayed enabled while gated, where their own `if (gated) return` swallowed it. Each guard was correct and none of them was visible. A control that cannot act now says so, through one rule that owns the composer, the chips and the chat buttons together, rather than three places setting `disabled` and drifting apart. Both new states are pinned; each fails on the old code.
`is-streaming` draws a blinking caret after the last line of a partial answer. The throw path in runAgentTurn removed the thinking indicator and appended the error, but never settled the stream — so a turn that died mid-sentence left its half-written reply apparently still arriving, for as long as the chat stayed open, with a failure message underneath it. The other failure path, `!done.ok`, already goes through finalizeStream. Getting the check to fail on the old code took three attempts, each instructive: an `assistant` event settles the block by itself, so driving it with one proves nothing; deltas do create the streaming block synchronously, but only while the turn is still live, so ai:chatSend has to be held open and rejected AFTER them.
- the Assistant's gate could NEVER lift. The listener that re-runs it was guarded on `wrap.isConnected` — added 40 minutes ago to stop it leaking — and connecting a model means going to Settings, which PARKS this view: detached, alive, about to be shown again. So the guard fired on the one path that matters and unsubscribed, undoing the fix it was added to protect. The leak is answered by identity instead: only the newest build acts, older listeners drop out with their closures. - opening any file rewrote the whitespace toggle's tooltip to the "turn it on" text, whatever the toggle was set to. `syncWs` owns that title; the line-control gating added an hour ago overwrote it. That is the "titles never change with their state" defect the log toolbar was fixed for, reintroduced one toolbar over. - a deleted EMPTY tracked file was reported as "staged as a new file but is no longer on disk". Two things arrive with both sides empty and the path gone: git's `AD`, and a file that was empty in HEAD and has now been deleted. Only the first is new; saying it about the second tells someone their committed file was never committed. `onlySide` tells them apart. - a turn whose whole answer arrives at the END — no streaming, which is every non-streaming provider — landed below the fold, because the end-of-turn settle measured "is the reader at the bottom" AFTER appending the block that had just pushed them away from it. The same before/after mistake the streaming path was fixed for. - returning to a parked view restored a pixel offset. For the Assistant's transcript, which keeps taking a live turn's output behind your back, that pinned you to a fixed point while the answer wrote past you. The snapshot records whether the offset WAS the tail, which is a different intention from "this many pixels down". 531 tests; every Assistant and diff check green.
- the forward-truncate ran BEFORE the "is this the same place" check, so every route that reached it discarded the forward entries — including the one `refreshAll` performs, which the file watcher fires on every save, a window focus fires whenever the fingerprint moved, and every git action fires too. Going Back and then saving a file killed Forward, constantly, for no visible reason. The truncate belongs with the push it accompanies: re-routing to where you already stand is not navigating. - the Assistant's transcript had no tabindex, so it could not take focus and therefore could not be scrolled by PageUp, Home or the arrows at all. It is the longest-lived scroller in the app — a chat you have been working in all day — and only a pointer could move it. - closing the dock while the keyboard was in the terminal left focus on <body>, so the next Tab started from the top of the window and no shortcut bound to a view could fire. The restore only covered the case where something outside had been remembered, and the dock is usually opened from inside itself. The view host takes `tabIndex = -1` and the keyboard lands there — Tab then continues from the view rather than from nowhere. Three checks, each confirmed to fail on the old behaviour.
that keeps nothing Two from sweep 6, on the surface the owner has reported before. - the drop indicator was a fixed line UNDER the hovered row and the insert was always `move(from, i)`. Those two agree only when you drag DOWNWARD: dragging up, `splice(i, 0, …)` puts the commit ABOVE the row while the line underneath it promised below. So every upward drag landed one row away from where the app said it would — on the view whose entire job is to say where commits will land. It also made position 0 unreachable with a pointer, there being no row to draw a line above. The line now follows the pointer's half of the row and the insert follows the line. Pinned by a four-cell table: up/down × top/bottom half, plus the first position specifically. - a plan that drops every commit passed validation, which only looked for orphaned folds. Pressing Start then erased the whole range and offered to force-push it — `git reset --hard` wearing a rebase's clothes, with the preview reading "5 → 0 commits" beside a lit button, and a force-push confirm downstream that talks about rewriting history rather than deleting all of it. Start is closed on that plan, and says which tool actually means it.
- "Create pull request" was hidden by two independent conditions writing
to the same flag. The swr answer ("GitHub could take a PR") went
straight to `prBtn.hidden`, and the base===head path then hid the
button on its own — but nothing ever un-hid it: the swr callback had
already delivered its cached answer and never fires again, and the
success path never touched the button. So picking your own current
branch as the base ONCE removed the view's whole purpose for the rest
of the session. The two conditions are kept apart and re-asserted on
every exit.
- Swap exchanged `undefined` into the HEAD slot when there was no base,
which renders a picker with an icon, a chevron and an empty label, and
then compares against nothing. It needs two sides to exchange, and
says so when it hasn't got them.
- the summary and both tab badges kept the PREVIOUS comparison's numbers
while a new one loaded, so "Comparing A … B" sat directly under
"12 commits · 9 files" describing an entirely different pair of refs.
`last` was already nulled for exactly this reason; only the body
honoured it.
- and the base default only ever searched local heads, so a fresh clone
with one branch got "This repository has only one branch" while the
picker eight pixels above it listed every remote-tracking branch and
tag in the repo. It falls through to the upstream now.
- ⌘Enter on a branch row ran the row's verb AND opened the row. The shortcut sheet documents "⌘Enter — run the focused row's main action" and `wireListNav` implements it, but `promoteToDivRow`'s own keydown had no modifier guard, so both fired for one keypress: the branch was checked out and the list you were working in disappeared underneath the action you had just taken. - every tick in the one-list staging model was named for its STATE — "Not included", "Included in the commit" — so six ticks shared three names. That is useless to a screen reader, which hears "not included" with no idea what isn't, and it broke the focus rescue outright: `sameThing` matches on `title`, so ticking the fourth file moved the keyboard to the second. Each tick names its file. - a restored assistant message rendered at the pane's full 820px while the identical message, live, was 760: only `.assistant-turn` carries the measure, and `restoreChat` appended the block without one. The same text at two widths depending on whether you had left the chat and come back.
- the integrated terminal read `background` and `foreground` from the live tokens and then hardcoded all sixteen ANSI entries to VS Code's DARK palette. In the light theme that paints a dark palette on a white ground: brightWhite #ffffff is 1.00:1 — literally invisible — and yellow, brightYellow, brightGreen and white are all under 2:1. Any tool that colours its output printed whole lines nobody could read. The light row is GitHub's, matching what the job log one pane over already ships, so both surfaces speak one ANSI vocabulary. - an ANSI run that sets a BACKGROUND and no foreground — `ESC[41m` on its own — got `log-bg-1` and no fg class, so the block was painted from the theme-independent true palette while the text fell through to the page's ink: near-black on dark red in the light theme. The pairing rules added earlier only covered spans that set both. It takes the palette's own default foreground now, and the two pale blocks take its black instead of white on white. - the exited-terminal row I added an hour ago receded with a blanket `opacity: 0.62`, which multiplies with whatever each child already uses: the "exited" badge, already muted, measured 2.41:1 in light and the shell's NAME — the only thing saying which one died — sat at 3.72:1. It recedes by colour instead. The contrast check took four attempts to make honest, each worth recording: `css()` is a probe helper and throws in a check; `getComputedStyle().color` does not include an ancestor's opacity, so it passed on the broken build; `document.body` paints no background here, which drove every ratio to 1.00:1; and walking opacities to the root folds in `.dock-body`, which sits at 0 behind a transition this harness never completes. It measures between the text and the surface it sits on, and reproduces the reported figures to two decimal places.
…eakpoint The graph|details resizer clamps the details column so the graph is never squeezed past the width where it drops its Date and SHA columns — "otherwise columns silently vanish and their resize handles go with them". It restated that width as a literal 800, beside a comment quoting a third value (760), and the graph package has since raised its breakpoint to 860. So the resizer permitted a drag 60px past the point it exists to prevent. The threshold moves to webview-ui/limits.ts, which is exactly the module for shared responsiveness thresholds, and both sides read it. A literal in one package describing another package's behaviour cannot help but go stale; this one had, twice over.
width scrolled the whole window sideways Sweep 6's top finding, and it corrects my own work. `seedAssistantGoal` was given a busy-check so a ✨ action fired during a run would refuse rather than rebuild the view. It gated on `live.el.isConnected` — and every ✨ action fires from ANOTHER view, where a keep-alive Assistant is parked DETACHED. So the branch was unreachable in every real case and its toast had never once been shown, while the force-route went on destroying the transcript, orphaning the run in the main process, and starting a second turn against the same chat. The same file carries a comment about exactly this, twenty lines below, on `onAiChanged`: "NOT gated on `wrap.isConnected` … PARKS this view in the keep-alive cache — detached, but very much alive." I wrote that an hour before making the same mistake one function away. The busy test asks about identity now. The IDLE branch keeps `isConnected` deliberately — a parked view can be evicted, and running a goal into an evicted node would lose it — and the route that follows is never forced, so the same node comes back with its transcript and its live Stop button. Measured: one turn instead of two, the answer intact. Also: the PR detail header's action cluster could not shrink below its labels and had no wrap, so at the window's own 880px minimum it overflowed the body to 902px and the topbar slid off to reveal it. It wraps. A new check asserts that no surface scrolls the app sideways at 880, across four views.
None of a branch row's meta slots can give up a pixel — the upstream name is a fixed 160px, the ahead/behind track 78, the remote 70 — so at the window's own 880px minimum the row's content sums to about 717px in a 648px row. `.sec-row` is `overflow: visible` inside a clipping ancestor, which means the surplus does not scroll: it renders OUTSIDE the window. Measured: three of five rows overflowing, their actions ending at x=941, unreachable by pointer and by keyboard. The upstream NAME drops below 1040px — the biggest slot, the most droppable, and already in the row's tooltip; the track beside it is the part you act on. 1040 is the breakpoint the topbar already uses. The sideways check missed this entirely, which is the more useful half of the finding: `document.body.scrollWidth` reports a page that fits while rows render past its edge, because the overflow is clipped by an ancestor rather than scrolled. It now measures the rows themselves.
- pressing Stop ended the turn in the main process and left the agent's "Approve destructive action" dialog on screen. `onConfirm` opened a modal and awaited it forever, and nothing in the cancel path closed it — so its Approve button then posted an approval for a run that no longer existed. Measured: one `ai:agentConfirm` sent for a dead turn. `confirmDialog` takes a signal now, and the turn owns one: Stop, an outside cancel and the `finally` all fire it, so no approval dialog can be left behind by any of the three. - folding a group in the job log rebuilds the whole window of rows, so the row the keypress came from was destroyed by its own handler. Focus fell to <body> and the next Enter went nowhere: folding one section of a build log by keyboard ended the keyboard's involvement with it. The row carries its doc index — `data-doc-idx`, deliberately not `data-num`, which focusReturn keys its own identity off and which log line numbers would poison — and focus follows the rebuild. - ⌘Enter did not post a comment on the Issue or PR detail pages, though the shortcut sheet says it does. Neither composer wired it, so the one keystroke people reach for after typing a comment did nothing at all, on both pages.
- the commit page reopened file #1 on every build, and `refreshAll` re-routes the current view with its history target on ANY save anywhere in the repository. So a build touching one file swapped the diff you were reading for the first in the list, silently, while you were reading it. `SectionTarget.file` exists for exactly this and the Code browser already used it; the page records which file is open and restores it, falling back to the first as before. - the Assistant's model, thinking and access chips are read when a turn STARTS and travel with it. Changing one mid-run relabelled the chip and left the running turn on the old value, so the chip stated in the present tense something the agent working below it was not doing — most consequentially for Access, which is the write-permission control. They stay usable, because setting up the next message during a run is the normal thing to want, and they say when the change applies.
- the Rebase footer preview counted ROWS while the confirm dialog beside it counted `state.replayCount`. The list is capped, so on a range longer than the cap the preview read "50 → 50 commits" for a rebase the dialog one line down correctly called 214 — and the smaller, safer number was the more prominent of the two. It counts what will be replayed, and says how many are not shown. - a transient rebase banner hid itself after four seconds, and it shares the banner with the host's PERSISTENT note — the one saying the base fell back or the list was capped. So one refused squash permanently destroyed the only statement that the plan was not the whole story. Flashes restore the note now, sequenced so an interrupted one cannot restore over a newer message. - the chat restore skipped itself while a turn was running, which was the right guard for an ordering problem that no longer exists (runGoal awaits the gate). Skipping it was its own bug: a seeded ✨ turn ran into a chat whose history was never drawn, so the answer arrived with no sign of the conversation it was continuing. - two contrast values. A label chip whose GitHub hex is dark — the `bug` red, `documentation` blue — rendered at 3.20:1 in the DARK theme: the 70/30 pull toward the foreground was tuned for light and never applied to the other side. And Monaco's inactive line numbers sat at 1.93:1 on white, under the 2:1 the log pane's own gutter was raised off, in the surface where a line number is how you refer to a line at all. That closes all 16 of sweep 6's confirmed findings. 895 unit tests and five typecheckers green.
Sweep 7's regression lens, plus what it found next door. - the commit page's `setPageTarget` had no `isConnected` guard, so a page abandoned while still loading stamped its auto-opened file onto the history entry of whatever view you had moved to — and the next refresh re-routed that view with it. The job log's `openJob` carries this exact guard, written an hour before this line and then not written here. - the rebase empty-plan guard counted only the rows ON SCREEN, and the list is capped. Above the cap, dropping every visible commit disabled Start with "this plan keeps no commits" for a rebase that would in fact have replayed everything the view never drew. `total` four lines up had already been taught to use `replayCount`; two fixes from one batch, in one function, disagreeing. `hiddenTail()` was already there for it. - the ANSI default-ink rule I added an hour ago picked WHITE. That is a dark-theme palette: measured against all sixteen blocks, black wins on fourteen and white on two — the greys. So thirteen of sixteen blocks were under 3:1 and block 15 was white on white, 1.00:1. Inverted, the worst pairing in the set is now 3.26:1, and a check walks all sixteen. - `openCodeFile` set `activeMonacoView` before awaiting the file read and never checked the route afterwards. Leaving during that read built a Monaco editor into a node its owner had already let go of — nothing held a reference that could dispose it. `showCodeView` guards its own read this way. - and the dock's reserve reached the detail pages and three scrollers but not the Code file surface, the section detail panes, or a project board's columns, so the end of each sat behind an open terminal.
Sweep 7 carried a lens whose whole job was to re-drive the things Anton reported and ask whether the complaint would still be made. Two would. - "it teleports u to the commit graph which tells u nothing about the changed files" was reported about a commit chip, and every sha in the app was moved to the commit page for it — pull requests, releases, notifications, ref detail. The Actions RUN page's chip was missed, so the complaint was still one click away from a surface people live in. It opens the commit now. (`refDetail`'s remaining graph route is a deliberately-named "Show in the graph" action and stays.) - "scrolling super fast or instead of me is pure ragebait" was fixed by disarming the tail when the reader scrolls away — but the test for "still at the bottom" is a two-line dead band, and the wheel is damped to 0.45. So ONE notch on a trackpad moves less than the band, left `follow` armed, and four seconds later the tail poll pulled the reader back down with nothing to say why. The keyboard already disarms on intent rather than distance; the wheel now says the same thing. Also: the abandoned-commit-page guard from the last commit moves earlier, where the sweep's refuter showed it belongs. Guarding `openFile` stops the stamp; guarding the build stops the whole detached render, and settles a second race it found — two refreshes in quick succession, the first finishing detached and re-stamping file #0 over the file the second had just restored.
The state table sweep 7 ran over conflict kinds — the highest-stakes surface in the app, and the one a mistake in cannot be undone. - "Mark resolved" was gated on `conflictsPending`, and the result pane is seeded with the BASE. So a block nobody had accepted still held the pre-merge original whether it conflicted or not, and the button unlocked while auto-mergeable hunks sat at base — saving wrote the original over both sides' work in every one of them. The gate added earlier tonight to stop exactly this was counting the wrong thing. It counts every pending block now, and the view starts from git's own auto-merge (`applyAllNonConflicting`) so the stricter gate is invisible in the ordinary case. Without that second half the fix would trade data loss for drudgery: a file with one true conflict and forty clean hunks would need forty gestures to redo work git had already done. - "Stage all" held back modify/deletes because they cannot contain markers, and the guard is "no markers means resolved". A conflicted BINARY cannot contain markers either — so the one kind of conflict the app itself refuses to open a text merge for was the one kind Stage all waved through, declaring it settled with whichever side happened to be in the worktree. - git's `DD` (deleted on both sides) was folded into `missingSide` and drawn as a modify/delete, offering a "Take <side>" button for a side with nothing to take — which `conflictTakeSide` then refuses, correctly, contradicting the panel that offered it. It is its own state, with the two buttons removed and Discard named instead. - and `UA`/`AU` — added on one side, no common ancestor — were told the same modify/delete story: "deleted in X", about a file with no history to have been deleted from. They branch on `hasBase` now. 534 tests, including real-git coverage for all four.
- the Assistant's gate listener returned early when the Assistant was ungated, so it could open the gate and never close it. Removing the last model left the composer live and the header still advertising a connection that no longer existed — measured: "· Claude (BYOK)" beside an empty account. It is symmetric now, and pinned in both directions. - adding a KEYLESS preset — a local model, a CLI agent — never announced the change. Every other path does (`setKey`, `removeConnection`, `setDefault`), but a keyless provider is usable the moment it is added and no key dialog follows, so nothing fired: the users of the one provider class that needs no key could never lift the gate at all. - the Appearance card unsubscribed its own live-sync hook the first time Settings was left. Settings is keep-alive, so leaving PARKS the card and returning re-attaches it verbatim — with the hook gone, a theme changed from anywhere else meanwhile left the card showing the old one for the rest of the session, highlight and `aria-pressed` both stale. Nothing accumulates without the guard: it is a single slot, overwritten by the next build. That is the third `isConnected` guard on a keep-alive view found this session, each firing on precisely the path it needed to survive.
- Enter on a control nested inside a clickable row ran the ROW's action. On a project card the kebab opened the issue instead of the item menu, which made "Move to" unreachable by keyboard entirely; on a release asset the Delete button DOWNLOADED the asset — a destructive control you could not reach, and a different action silently taken in its place. `orgs.ts`, `explore.ts` and `common.ts` already carried the `e.target !== row` guard; these two did not. Pinned as one invariant, including the half a careless guard would break: the row itself must still answer Enter. - the clone dialog and its destination sheet declared no `hasUnsavedWork`, and every file saved in the open repository fires the watcher, whose overlay sweep takes every layer down. So a build touching one file destroyed a half-typed clone URL, a repository picked from the list, or a clone already in flight. Both compare against what the form OPENED with rather than truthiness, since both prefill.
It signs you out and then looks for the Sign-in button on the rebuilt card. But `showSettingsView` returns as soon as the card's SHELL is in the DOM, and everything the card actually shows arrives in an async body it does not await — so the handler searched a card still holding a loading spinner, found nothing, started nothing, and left you signed out, under a label promising the opposite. The comment above it already described this failure and the `await` it added was of the wrong thing. The card's body is a promise now, and the handler awaits it. The instructive half is the check. One already existed for this button and was green the whole time: it asserted a `.gh-flow` was on screen without stubbing `github:status`, which the fixture answers SYNCHRONOUSLY — so the card had always painted before the handler looked and the race never opened. The new one holds that read for 400ms, which is what a network call does, and fails on the old code.
- one failed `ai:settings` read left the AI Models card permanently dead. The catch returned before the "Connect a model" button was built, so a transient error — a locked settings file, a slow disk — took away the card's entire purpose with no way to retry short of restarting the app. The button is hoisted so both exits have it, and the error path offers Try again. - a clone COLLISION retry reopened the destination sheet with a new name and no destination, so the sheet fell back to the configured default folder and confirming quietly relocated the clone — on the one path whose whole premise is that the user had chosen somewhere else. The sheet takes a destination now and shows it rather than "Loading…", and neither the settings read nor its failure path overwrites one. - the job log page opened with the keyboard on the Back button. `detailPage` focuses Back on every new page, which is right for a page you read top-down and wrong for this one: j/k, n/N between failures, Home, End and / all live on the scroller, so the page opened with none of them working and nothing saying why. It focuses the log — with `preventScroll`, because moving the port on a live job is the one thing this pane must never do. Not taken: the Split/Inline whitespace disagreement. The refuter's analysis is that the tempting fix — making `normalizeAllWhitespace` collapse inner runs — would make the extension's "Trim whitespaces" and "Ignore whitespaces" identical and weaken the merge model's own whitespace handling, and that NO test pins the current inner-collapse behaviour, so it would pass the suite green while changing what the product means. That is a decision about diff semantics across two packages, not a bug fix, and it is Anton's to make.
`replaceResultLines` routes a block that reaches end-of-file to a branch that replaces to the end WITHOUT appending a newline. Monaco reports one line for an empty document, so `endExclusive > lineCount` is false for a single-block accept into an empty result — and the other branch appends one, writing a trailing blank line the accepted side never had. The empty result is the ordinary case for a conflict with no common ancestor (git's AA / UA / AU), where the seed is the empty base: accept one side, mark resolved, and the file gains a line nobody wrote. An empty document is always at end-of-file whatever the span says, so that is what the condition says now. That closes 22 of sweep 7's 23 confirmed findings. The one left is the Split/Inline whitespace disagreement, deliberately: the tempting fix changes what "ignore whitespace" MEANS across two packages and the extension, no test pins the current behaviour, and it is a product decision rather than a defect.
… views Split computes through the engine in-process; Inline computes in Monaco's own diff worker, whose only whitespace option is `ignoreTrimWhitespace`. Each derived that flag from the app's toggle separately, and the app sent the engine's "all" mode — which additionally collapses whitespace runs INSIDE a line, something no editor option can do. So a file whose only change was a doubled internal space read as unchanged side-by-side and as a change unified: the same file, the same setting, two answers. Both now read the rule from one exported `ignoreTrimWhitespaceFor`, and the toggle sends "trailing" — the mode Monaco can actually match. Its label says what it now does rather than overselling it. The harness gains a whitespace-only fixture and the discriminating one (a doubled space mid-line); the second check fails on the old mapping.
antonarnaudov
changed the title
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
2 participants
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.
This branch takes the Electron desktop app from what its owner called "a rough construction of a POC" to something that can ship. On
mainit was a set of disconnected tabs over a read-only tree of HEAD; here it is a navigable Git client — three new rail destinations, a ⌘K palette, a real back/forward history, and roughly a dozen routed full-page surfaces replacing the modals and leftover-height panes people were expected to work in. Underneath that, about a dozen commits repair ways GitStudio could silently delete, corrupt or orphan work while reporting success, several of which ship to the VS Code extension too because they live inpackages/git-service. The 180 commits are 10feat:and 137fix:, the fixes produced by seven adversarial review sweeps — and the pattern that kept repeating is that each sweep's most productive lens was the one pointed at the previous sweep's own fixes.What this adds (user-facing)
apps/desktop/src/renderer/views/notifications.ts) is a triage page with facets and per-rowe-to-mark-read instead of a top-bar bell popover. My Work (views/mywork.ts, backed bymain/github/myWork.ts) runs four@mesearches in parallel and dedupes each item into the most actionable of Review requested / Assigned / Your PRs / Mentions. Explore (views/explore.ts) puts GitHub repo, people, org and code search in the app. The app no longer opens on Code; it lands on Changes, with the rail reordered by daily use (⌘1 Changes … ⌘6 Code).renderer/commandPalette.ts(new, 360 lines) fuzzy-searches sections, every branch and tag, recent repositories, open PRs and issues, and the headline verbs (new branch/PR/issue, fetch/pull/push, clone, open repo, terminal, theme, check for updates). Local groups paint instantly, GitHub groups stream in; typing past your local repo offers "Search GitHub for …" with live results, debounced and generation-checked inrenderer/searchDebounce.tsso a slow answer to an old query never renders over a newer one.renderer/navStack.ts, and every detail page's Back button pops it rather than pushing (onmain, Back left Forward disabled — proof it was appending). Back now names its true destination: leaving PR #106 for a pipeline and pressing back says "Pull Request #106", not "Actions".renderer/focusReturn.tsreturns the cursor to the row you opened instead of<body>.views/commit.tsshows message, both identities, parents, refs and every changed file with a/filter, plus cherry-pick, revert, branch-from and reset. Previously every "show me this commit" routed to the graph and calledreveal(sha), which shows no files and returns silently when the sha is outside the loaded page — so from a long-lived PR the click did nothing at all, with nothing on screen saying so.views/jobLog.tsoverrenderer/logView.ts+logModel.ts. It virtualizes 200k lines to ~120 DOM nodes, colours ANSI, folds##[group]steps, keeps a sticky step strip, draws an error map whose ticks land on the failure, and addsnfor next failure, in-log search, a timestamps toggle, live tail with a Follow mode that goes dead when there is nothing left to follow, and save-to-Downloads.renderer/peek.ts+peeks.tsadd a stacked drill-in overlay across branches, remotes, tags, stashes, commits and org repos/teams/members — click a branch to read its history, click a commit inside for its files,←to come back without losing your place. Branches itself became one kind per screen behind a segmented switch, with Fetch promoted out of a hover-revealed kebab, standing facets (merged, upstream gone, diverged, unpublished), an Active/Stale cut and a "Delete N finished…" sweep.renderer/repoBrowser.ts(peek stack) andviews/exploreRepo.ts(full page with breadcrumbs, ref switcher, go-to-file, rendered README, Monaco-highlighted contents);renderer/ghOpen.ts(ghrepo:open) opensowner/repoas a normal local repo — instantly if a clone exists, otherwise cloning behind a progress card — withdestinationSheet.tsfor where it lands. Onmainan org repo dead-ended in a metadata card.detailPage()inviews/common.tsbacks commit, ref, job log, the issue/PR composer, the release composer, and the PR/issue/release/gist/run/Explore pages. The release composer replaces a 560px card that gave the notes ~180px, and adds a tag combobox that says out loud when it will create a tag, a target ref picker, "Generate release notes", latest-release, and publish-vs-draft as two named buttons.renderer/mdEditor.tsgives all five composers one Write/Preview editor whose preview renders through the samerenderMarkdownthe published body uses, plus ⌘Enter to submit;renderer/draftStore.tskeeps unsent text through an accidental Escape, keyed by repo (the app had already shipped handing repo A's draft to repo B's issue #31).renderer/proseNav.tsroutes links and bare#123refs in READMEs, bodies, comments and release notes to the in-app page instead of the browser;renderer/highlight.tscolours fenced code through Monaco's tokenizers.compare:fileDiff); Actions runs show run number, actor, attempt, per-step timings and artifacts, with Cancel / "Re-run failed" hidden when inapplicable rather than greyed out forever; the repository manager moved out of Settings onto the top-bar repo chip and ⌘K;?opens a cheat sheet audited against the keys the app answers to;renderer/prefs.tslets view modules honour Settings → "Prune on fetch", which only two of four fetch buttons obeyed.Correctness and data safety
sideLabels(),main/gitBridge.ts:3202). The fix then lumpedgit amin with rebase and re-created the same lie one operation over.test/conflictSideLabels.test.tsdrives a real conflicting rebase and a real conflictingamand asserts the stage assignment, not the strings.conflictsPending, leaving auto-mergeable blocks at base, so it now counts every pending block and seeds from git's auto-merge (renderer/diffPanel.ts:679-705). It wrote withwriteFile(abs, text, "utf8"), which follows a conflicted symlink out of the repo and puts binaries through a UTF-8 round trip — a 4,508-byte PNG came back 4,565 with89504e47rewritten toefbfbd504e47, staged (nowtextWriteSafe()atgitBridge.ts:2867plus a realpath of the parent dir). AndreadWorkingcaps at 512KB, so a larger conflicted file was truncated to its first 512KB.git addon an unmerged path is declaring the conflict resolved; four routes ignored it. "Stage all" then Commit produced a commit with<<<<<<<in five files, and staging cleared the unmerged entries so the conflict count fell to zero and re-enabled Continue.add -A -- . ':!path'stages the excluded path anyway against real git, so the guard is an explicit allow-list (gitBridge.ts:897-907); UD/DU are held back on kind; a conflicted binary was waved through because "no markers means resolved" is vacuously true for it. Per-filestage()and both partial-staging routes had the same hole, closed inlineStageable, later narrowed to paths git reports unmerged so a file documenting conflict markers stays stageable.ls-files -u -- <path>with unfilteredls-files -uplus hand-rolled matching; without-z,core.quotePathprints"caf\303\251.txt"while the renderer sendscafé.txt, so nothing matched and "no stages found" fell into the branch that runsgit rm. On one six-file conflict,café.txt,emoji🎉.md,日本語.mdandsub dir/ünï.tswere deleted with the deletions staged andok:true. Fixed with-zand a[\s\S]path capture, and an unreadable listing is now a refusal rather than a delete;missingSidewas brought to the same shape.git show :2:/:3:unconditionally, so taking the deleting side surfaced a rawfatal:for a button the app offered — an absent stage now reads as that side's answer and stages a deletion.DDwas drawn as a modify/delete;UA/AUwere told "deleted in X" about a file with no base; Discard rangit checkout -- <path>, which refuses unmerged paths, so a destructive confirm was followed by raw stderr — it usescheckout --mergenow (gitBridge.ts:814-839).%(refname:short)returns the shortest unambiguous name, so a branch colliding with a tag comes back asheads/stacked-aand went intoupdate-ref refs/heads/heads/stacked-a— junk branch, success reported, the user's real branch orphaned outside the rewritten history, which is exactly what--update-refsexists to prevent, pre-ticked by default (main/rebaseBridge.ts:290-330). Same root cause inSyncOps.push, which sent a bare branch name and failed withsrc refspec v2 matches more than one; every path now sendsrefs/heads/x:refs/heads/y(packages/git-service/src/SyncOps.ts:154-205), and branches checked out in another worktree are excluded via%(worktreepath).git log <base>..HEADlists merges and already-upstream commits; the sequencer uses--no-merges --topo-order --cherry-mark --right-only. Either shape left the repo detached at the base, mid-rebase, clean, with the in-progress card asking the user to resolve conflicts that do not exist and Continue re-running the same todo forever. Inpackages/git-service/src/rebasePlan.ts,update-refis now emitted after the last folding row (foldEnd,droptransparent) — the old placement pointed a branch at a commit a followingfixupthen rewrote. A plan dropping every commit also passed validation: Start erased the range and offered to force-push it.--continueran withGIT_EDITOR=true. Sha-keying then introduced a wildcard ({sha: ""}from an optional-field shim,startsWith("")matching everything) that wrote the first message onto every reworded commit in the shipping extension; the identity stamp meant to fence an abandoned queue was defeated becausegit rebase --abortrestoresonto/orig-headbyte-identically. The queue now lives inside git's ownrebase-merge/, whose lifetime git enforces. Same area:rebaseInProgressgreppedgit statusfor a translated string, so on a French or Chinese git the "exit 0 is not finished" guard was always false.git rebase --skiphard-resets the tree, and the banner offered it as the primary button at a deliberateeditstop (diff --cached --quiet HEADis true there too).opStatenow decideskind/canContinue/canSkiponce where the git knowledge is;test/opMatrix.test.ts(462 lines) crosses every mid-operation state with every tree shape and asserts capability, not button labels. Skip is confirm-gated, banner buttons disable for the round trip, andserialize()queues a second call instead of dropping it — two clicks used to throw away two unrecoverable patches.fileDiffbuilds working-tree diffs as HEAD-vs-working whatever the stage state, but the unstage path matched those line numbers against index coordinates: on aMMfile, clicking one staged line left it staged and silently rolled back a different change, returningok: trueunder "Unstaged selected lines." The pre-existing test could not catch it (it writes nothing after staging, so the two spaces coincide). SeparatelyindexModereturned100644for any path with no index entry, so a new executable script staged by line or hunk committed non-runnable (packages/git-service/src/StagingProvider.ts).Architecture and shared packages
RebasePlan.rewordMessages: string[]becamerewords: Array<{sha, message}>inrebasePlan.ts/RebaseRunner.ts, non-optional on purpose (therewords?shim is what minted thestartsWith("")wildcard). Both extension callers migrated:apps/extension/src/graph/graphPanel.ts:605andsrc/rebase/rebaseWorkspacePanel.ts:185.runRebasePlanalso refuses to start while a rebase is in flight, before writing the queue.apps/extension/src/rebase/rebaseRunner.tsis a 40-line binding, so VS Code picks up sha-keyed rewords, the persisted queue,core.commentChar=auto+pickCommentChar(a#123body line no longer stripped), and exit-0-with-a-live-rebase reporting aneditstop. It does not pick upskipRebase, the message-carryingabortRebase, or theonRunobserver.apps/desktop/src/main, extracted so they unit-test:githubPaging.ts(RFC-5988Linkrel=next + a per-surfacePAGE_CAPS),github/maps.ts(one home formapRun/mapPull/mapIssue, which existed in two divergent copies),github/searchGuard.ts(clock-injectable token bucket for GitHub's separate 30/min and 10/min search budgets, returninglimited{retryInMs}instead of a 403),github/searchQuery.ts,githubStatus.ts,githubRemote.ts,localRepos.ts,appSettings.ts,ghRepoOpen.ts, plusshared/cloneName.ts.githubClient.tsgrew a transport layer: a privatefetchResbehindrequest, newrequestPaged/requestPagedKeywith an explicit policy (first-page failure throws, a follow-up page's failure returns what was gathered),AbortSignal.timeouton every fetch (20s API, 30s body-less, 300s asset upload), a separate fetch foruploads.github.com, andlistOpenPulls→listPulls(state).apps/desktop/src/shared/ipc.ts1458 → 2193 lines, addingsearch:*,repos:*,ghrepo:*,settings:*,update:*, theam:*/revert:*/cherryPick:*operation controls,tag:delete/tag:push,actions:jobLogChunk, andGitOpState(which namesamApplyingseparately fromrebasingbecause both live inrebase-apply/).main.ts'shandle()wrapper is now the single error seam: everyok:falseand every throw reachesErrorReporterwith a humanactionLabel(channel), gated byisExpectedError.GitHubBridge.adoptLegacyTokenis gone andensureLoadedunlinks any leftoversafeStorageblob unread. Consequence: anyone upgrading from <1.4 is signed out once, in exchange for the macOS keychain prompt never firing again on a re-signed binary.diffView.ts/mergeView.tsdroppededitor.getModel()?.dispose()(the standalone editor owns the model; the double-dispose threw on every teardown and aborted worker-sync unregistration) — that also fixes the extension's diff/merge webview.--gs-ambernow prefers--vscode-charts-yellowovergitDecoration-modifiedResourceForegroundin bothtokens.cssandhostTokens.ts, reversing the previous comment's AA-contrast rationale without answering it.COLUMN_DROP_TAIL_ATmoved intopackages/webview-ui/src/limits.tsand changed 760 → 860, so the extension's graph drops Date/SHA at a wider host width.How this was verified
934 unit tests across five packages, 364 headless UI checks, five typecheckers clean.
apps/desktop/harness/is 11,222 new lines across five committed files (checks.js8,818,shim.js1,603,check.mjs548,probe.mjs139,gen.sh/shot.sh75).gen.shbuilds the renderer and copiesdist/renderer/{renderer.js,renderer.css,theme-boot.js}next to the shim; Chrome runs--headless --dump-domagainstharness.html?scene=…&check=…, and the verdict comes back indocument.titleasCHECK {json}. No Electron, no git, no network."<view>~step~step"withopen<N>,click:,text:,type:,key:,scroll:,palette,bell,esc— e.g.prs~open106~text:Files~click:.pr-files-list%20.file-row:nth-child(5). Two details are load-bearing: atext:needle prefers a match inside#view-hostover the nav rail (shim.js:1508), andkey:dispatches ondocument.activeElementrather thandocument, or every handler guarded bye.target.closest()no-ops and the check passes on a broken build (shim.js:1532).shim.jsfixtures ~89 of the 242 IPC channels, with adynamicblock for state that must change mid-scene, a realon()/__gsEmit()registry for push paths,?fail=pr:commitsto make a channel reject, and switches likestaging=checkboxes,norepo=1,clean=1,onfeature=1,op=merge. Unfixtured channels are collected and printed once per run — an unfixtured read answersundefined, so a check can pass over a throw; that is how the PR label picker went unchecked.changes57,actions47,prs43,issues32,branches31,code28…), re-run across widths 880–1920, both themes andargvalues;no-view-hides-its-own-content-or-locks-out-the-keyboardruns over all 15 views. Cases run serially, one Chrome each (parallel Chromes fight the GPU lock and produce flaky geometry), capped at 90s..test.tsfiles (+64, +440test()calls, none removed) with no jsdom by policy. Three kinds: pure functions (textFit,truncate,logModel), in-process modules with a hand-settled host stub (cache.test.ts's stale-while-revalidate races), and real-git integration tables —opMatrix.test.ts(462 lines),opState.test.ts(519, pinninggit amvs rebase apart by the marker file in.git/rebase-apply),stagingSafety.test.ts(a PNG staged line-by-line is no longer round-tripped through a JS string).gitBridgeArgGuards.test.tsrequires everyCommitActionResultmethod ingitBridge.tsto callsafeArgor appear in aREVIEWEDmap with a written reason — writing that list found the one real gap,WorktreeProvider.removebuilding["worktree", "remove", path]with no--.destructiveGuards.test.tscensuses destructive renderer controls for a confirm or an in-flight disable.readFailuresSurface.test.tsbans.catch(() => [])ingithubBridge.ts.cssTokens.test.tsandstylesheet.test.tscatch the twoapp.cssfailures that have actually shipped here (an undeclaredvar(--accent), and a comment terminated early by a*/inside a selector glob).Reviewing this
Read in this order:
docs/desktop-redesign.md(+362, new) — the spec every GitHub view was converted to: the list-page/detail-page/right-rail contract and thenav("issues", {number})⇄nav("issues", {list:true})routing rule.apps/desktop/src/shared/ipc.ts(+743) — the typed catalogue of everything the app can now do.apps/desktop/src/renderer/views/common.ts(+1,368) — ~30 exports (detailPage,secRow,sectionList,facetBar,wireListNav,disposeOnDetach) that are the vocabulary every other view file is written in. Most remaining view files are instances of these three.Then the riskiest diffs, in descending order:
37e62b5("visibility, flexibility and reach across every GitHub surface") is effectively unreviewable as a commit: 89 files, +21,662, including a wholesaleapp.cssrewrite (8,987 lines in that commit) across nine independently-scoped phases. If there is budget for one deep read, spend it here — the 100+ followingfix:commits are largely its fallout, and reviewing them without it is reviewing patches to code you have not seen.apps/desktop/src/renderer/diffPanel.ts(+594) — split/inline diff, the conflict merge editor, the "Mark resolved" gate. No harness check can reach it (see below) and theconflict*.test.tsfiles driveGitBridgein the main process, not the renderer. This is where the fix that could write the merge base over both sides' work landed with no test, and where the commit five later says that fix's gate "was counting the wrong thing." Read it line by line.packages/git-service/src/RebaseRunner.ts(+483/-25) — highest blast radius, because the shipping extension routes through it. Read the reword-queue chain as one unit: SHA keying, then the identity stamp, then the move into.git/rebase-merge/. The first two each declared the bug class closed and were wrong.apps/desktop/src/main/gitBridge.tsconflict paths —sideLabels()(:3202),textWriteSafe()(:2867), the staging allow-list (:897-907), Discard (:814-839).apps/desktop/src/renderer/renderer.ts— 8,721 lines, ~8,480 of them oneclass Appwith 142 methods and 226 private fields, grown +4,827/-560 across 40+ commits. There is no seam;routeView(line 1103) is the only entry point worth tracing.Changes an existing desktop user will notice on first launch, none of them in a changelog: the default view is Changes and the rail is ⌘1–6; ⌘W no longer closes the repository (⌘⇧W) and ⌘R is Refresh rather than Electron's hard reload; the clone/repository manager moved out of Settings onto the top-bar repo chip; the whitespace toggle now sends
"trailing"instead of"all"so Split and Inline agree, which changes what the toggle does; and "Prune on fetch" is now honoured by the Branches and Releases fetch buttons that ignored it.Known gaps
git diff main...HEAD -- .github/is empty;.github/workflows/ci.ymlstill runs onlynpm run check-types+npm test, andapps/desktop/package.json's test script is stilltsx --test "test/**/*.test.ts". There is nonpm run harness. The 364 checks are a manual, macOS-only pre-commit gate — all three entry points hardcode/Applications/Google Chrome.app/…. Several commit messages cite check counts as evidence; nothing on the branch makes that reproducible on a PR.gen.shcopies onlyrenderer.js/renderer.css/theme-boot.js, soeditor.worker.jsnever runs, and Monaco paints its diff decorations on an animation frame that headless Chrome's virtual clock starves — an idle page never advances to arequestAnimationFrame, so mid-transition geometry reads stale (hence thetransition:none!importantsheet atchecks.js:36). The unified diff view and the merge accept gutter are therefore pinned by unit tests and by reasoning, not by any end-to-end check — the unified-diff half is handed topackages/engine/test/whitespaceRule.test.ts, where both surfaces readignoreTrimWhitespaceFor()from one function. That is also the exact surface where every work-destroying bug on this branch lived.apps/desktop/src/main/is exercised by it; there is no pixel diffing (screenshots are for human review); and the only production-side hook iswindow.__GS_ROUTES(renderer.ts:1104,:7639), pushed only when the array already exists.refClick.commit-graph.tsnow callspreventDefault()/stopPropagation()and returns on any.chip[data-ref]before the row-selection path, and only the desktop'sgraphMount.ts:48handles the resulting action —packages/webview-ui/src/graph/main.tshas norefClickcase and no default, so in VS Code clicking a branch or tag chip does nothing where it previously selected the commit and opened the details dock. The same commit gave the chiprole="button",cursor: pointerand a hover underline, so it now looks more interactive while doing less.apps/extension/changed (+254) against 45 underpackages/.StagingProvider.indexModenow asks the working tree for a file's mode, which also means hunk/line staging in the extension stages achmod +xand pays two extragitprocesses (ls-files,config --bool core.fileMode) per call.commit-graph.ts(+288),gutter.ts,refLayout.tsandRefProvider.ts(4 newfor-each-reffields) all ship to the extension graph, verified only through the desktop harness.be5f1b4fixes a per-visit Monaco diff-editor leak on the commit page and declines to add a check because a DOM probe "reports success on the broken build"; it notes this is the third leak of that shape. The class is still open: five Monaco owners under two disposal conventions (activeMonacoViewinrenderer.ts, 3 sites;disposeOnDetachinviews/common.ts, used bycommit.tsandprs.ts), with nothing stopping a sixth from registering with neither. 32 of the 162 source-touching commits carry no test or harness change at all.main.tsreplaces the blanketsetPermissionRequestHandler(→false)withcallback(permission === "clipboard-sanitized-write"), and the renderer→main channel count went 194 → 242 whilesrc/preload/preload.tsremains a 36-line genericipcRenderer.invoke(channel, payload)passthrough with no runtime allowlist — the types are compile-time only, so each handler's ownsafeArg/containedPathguards are the whole boundary. Against that,3e17dd8closes a fail-open hole inmarkdown.ts's sanitizer and pins it as a property, and7a109e7fixes three leaks in the crash-report scrubber (Windows backslashes, paths with spaces, IPv6).gitBridge.ts:876-887.Three of the branch's worst data destroyers were introduced by earlier commits on this branch: the
amlabels by the commit that fixed the rebase labels; the non-ASCIIgit rmby a commit whose message called the change "not a bug being fixed — it is a dependency being removed"; and the symlink/binary write by the sibling of the function that had just been hardened against exactly that. Each was caught by the next sweep rather than by a test, and each commit message says so in its first paragraph. That is the strongest available evidence the sweeps did real work, and the weakest possible argument for trusting any single one of these fixes without the negative test that accompanies it.🤖 Generated with Claude Code