(panel): offer Changes only where git can answer, and let the shell fill the panel - #305
Merged
Merged
Conversation
devsuitup
approved these changes
Sep 18, 2026
devsuitup
left a comment
devsuitup
left a comment
Owner
There was a problem hiding this comment.
Reviewed 5eccb1c against 0018635 with the discriminator, the memo and the openSession fix traced and mutation-tested in the worktree; CI green on the head (build ×4, lint, test ×4).
Holds, confirmed: isWorkTree's branch table matches the doc table; reverting it to "any 128 withdraws" turns 6 tests red (remote fatal + unsupported repositoryformatversion, broken gitdir file, garbage gitdir file, deleted main-repo worktree, subdirectory of a refused repo) — the exact count claimed. Removing the in-flight dedupe → 1 red (10 !== 1); making a non-answer overwrite an answer → 1 red. Stale replies: state is written under the probed session id, DOM repaint gated on currentPanelSessionId === sessionId on every branch including the withdrawal. Error text reaches the DOM through textContent. isPanelShellSession is the single predicate at main.js:774, main.js:1697 and the new session-cache.js:544. type:'terminal' only originates from the two hard-coded creators and from isPlainTerminal set at spawn, so a Claude session — placeholder included — cannot acquire it; an exited terminal reopens under its own id. eslint 0 errors / 333 warnings as stated; the two locale tests skip with a stated reason here (git does not translate under fr_FR.UTF-8 on this box). 9 commits, no trailers.
One sentence to correct, in .ai/contexts/changes-view.md ("Withdrawal goes through the tab's own close control") and the body's "Interaction with #302": "exactly one Changes close path" is not true — changesCloseBtn → handleClose() (file-panel.js:186, ~229-249) sets currentTab = null and hides the panel without going through toggleChangesTab, and no test in the new suite clicks that button. No consequence for this PR (read-only tab) nor for #302 as it stands: #302 gates handleClose itself (confirmDiscardChangesEdits at its line 260) and each of its other exits independently, which is the property that actually protects the buffer. So: reword to "the withdrawal reuses toggleChangesTab; the tab's own close button is a second path and carries its own gate", and add the one test that clicks the X. Approving on that basis.
Observed once, not reproducible: in a combined run of the six touched files, git-changes-runner-real-git.test.js:208 (directory-symlink null case) failed with Cannot read properties of undefined (reading 'some') — status() returned an error shape under concurrent git spawns; standalone and two reruns are 231/233 with 2 skips. A defensive shape check would turn a transient into a clean assertion failure.
Judgement calls from the body stand for a human: hidden button vs disabled-with-tooltip (nothing says why Changes is absent); the local/remote asymmetry; the full-height shell on a short screen; the openSession report not reproduced end to end.
A session whose working directory is not inside a git repository showed the Changes button, and clicking it put git's own output in the panel verbatim: a localised `fatal:` from status, and the whole 150-line usage page from `git diff --numstat`. The condition is now detected by exit code — `git rev-parse --is-inside-work-tree` returns 128 outside a repository in every locale — and the button is simply not offered for that session. One implementation (`isWorkTree()`) serves both the local and the remote transport, so the two cannot disagree. It runs once per panel switch and is asked again on the next one, so a `git init` or a deleted repository is picked up without polling; a repository that disappears under an open tab is caught by `status()`, which reports `reason: 'not-a-repo'` and withdraws the tab rather than reporting into it. A session that is in a repository still runs the same three commands per refresh and pays nothing for the probe. Every other git failure is still reported, now bounded to 5 lines and 500 characters: a corrupt repository or a permission error is a different case from a missing one, and git's usage page is not a panel message.
With a shell open and no Changes list, file or diff, the tab area still took its space as an empty dark block and the shell sat in its stored height underneath it. `margin-top: auto` on the handle is what bottom-anchors the region while a tab is shown; with every flexible child above hidden, it anchored the region against nothing. `renderTabContent` now marks `#file-panel-content` as `.shell-only` whenever it renders no tab, and two rules key off it: the region takes the free space and the handle goes away. A drag handle with nothing above it to give space back to would only shrink the shell and re-create the empty block. The region's height is untouched by this: `flex-basis: 0` overrides it for layout only, so the desired height, `localStorage.panelTerminalHeight` and the inline height all still hold what the last drag asked for, and opening a tab restores exactly that. The class toggle refits the shell, since the region's geometry changes with it.
`openSession` chose its launch options with `customOptions ||
resolveDefaultSessionOptions(...)`, and `resolveDefaultSessionOptions` resolves
Claude launch options only — it never sets `type: 'terminal'`. A session whose
own `type` is `'terminal'` therefore reached `open-terminal` without it, so
`isPlainTerminal` was false and the handler took the Claude branch, running
`claude --resume <uuid>` against an id that `launchTerminalSession` had minted
for a shell and that has no transcript. The reattach branch hid this for as
long as `activeSessions` still held the PTY; it bites once the shell has
exited and the entry is gone, or after a renderer reload.
The options are now chosen in one chain: an explicit `customOptions` first
(only the resume-with-config dialog supplies them, and the sidebar never
renders its button on a terminal row), then `{type: 'terminal'}` for a
terminal session, then the Claude defaults.
An exited terminal also reopens under its own id instead of calling
`launchTerminalSession` for a fresh one: both branches of the `openSessions`
check now fall through to the same reopen, as they already did for a Claude
session. Minting a second id left the row the user clicked pointing at an id
nothing could open, while the new shell appeared on a row they had not asked
for. `main.js` needs nothing for this — its plain-terminal branch ignores
`isNew`, and the resume-cwd lookup is already guarded on the terminal type.
Found while chasing a report of a Terminal row opening empty. This is a real
defect on that path; whether it is the one that was hit has not been
reproduced end to end.
`buildProjectsFromCache` injects every live plain-terminal PTY from `activeSessions` into the `get-projects` payload so a terminal sorts among the cached session rows. A panel shell is a plain terminal, and the injection had no exclusion for one, so it arrived in the payload like any other: a second sidebar row under the owning project, reading `Terminal` from the hard-coded `summary` here rather than the `Shell` the renderer sets on its own object, which is never the one displayed. The same rows feed `cachedAllProjects`, so a shell also added one to the status bar's "N sessions". The injection now skips a panel shell with `isPanelShellSession`, the predicate `get-active-terminals` already uses. `panel-terminal-target.js` requires nothing, so importing it from `session-cache.js` introduces no cycle, and the test keys on a session carrying `panelFor` rather than on a `panel:` id, so a re-spelled id cannot quietly reopen the hole. Reviewed every other `isPlainTerminal` reader. The ones in `cli-session-state.js` and `session-transitions.js`, and the `!isPlainTerminal` branches in `main.js`, all skip plain terminals already and therefore exclude a shell for free; the rest are per-PTY spawn and wiring decisions in `open-terminal` that a panel shell should get exactly like any other shell. `get-active-terminals` and this injection were the only two readers that select plain terminals as user-facing sessions. The #300 review recorded "no sidebar row, no session-cache row". The renderer paths were checked; this injection was not, and it is what produced the row. The renderer-side count was measured and is correct: `renderDefaultStatus` already goes through `countSessionsWithoutPanelShells(activePtyIds)`, which returns 1 for the captured two-id set. `activePtyIds` still contains the panel id, unchanged — that membership is what keeps `lruEvictOne` from evicting a live shell.
Exit 128 is git's generic fatal code, not a "no repository" code. `isWorkTree` treated every 128 as `isRepo: false`, so a repository git declines to open — dubious ownership, a `.git` whose permissions it cannot read, an unsupported `core.repositoryformatversion`, a `.git` file whose gitdir is gone, a worktree whose main repository was deleted — withdrew the Changes button with no message anywhere. That is strictly less diagnosable than the stderr dump it replaced, for exactly the class of failure whose message names the one-line fix, and it contradicted this branch's own claim that a permission error or a corrupt repository is still reported. A control is now withdrawn only on positive evidence. The probe keeps its locale-free reading of the exit code and the `true`/`false` token, and a 128 is corroborated by `gitEntryAtOrAbove`: an `lstat` for a `.git` entry at the cwd and each ancestor up to the filesystem root. Only a walk that reaches the root seeing nothing answers "no repository"; a `.git` that exists but is broken, an `EACCES`, or any other unexpected error is undecidable and reports the bounded message instead. The walk needs no process and no locale, and it answers the one question the exit code cannot. There is no remote equivalent that does not re-read git's translated message or add ssh round-trips, so a remote 128 is never corroborated and always reports. A remote directory that is genuinely not a repository therefore keeps its button and shows git's message; a remote cwd that is merely unmounted no longer loses the control either, which is the same divergence read the safe way. A `-uall` run that only overruns the stdout cap no longer pays for the probe: that is a volume problem with its own fallback, and the repositories large enough to hit it are the ones an extra spawn per refresh costs most. The new real-git tests build each refused-repository fixture, assert it really does exit 128, and pin that none of them produces `reason: 'not-a-repo'` — exercising the runner rather than stubbing its result.
…the tab's own close The availability probe was unmemoised, undeduped and never ignored a session it had left. It fires from `switchPanel`, which is reached from every sidebar click and from `resyncPanelFor` — so every panel-shell open, close, exit and refusal fired one too. Measured against the real renderer in jsdom: ten rapid switches left ten probes in flight, and five switches into the same session cost five probes despite a cached answer. For a remote session each of those is an ssh with a 20 s kill timer. This is the same cost the branch's own rationale rejects for the refresh path. A session that has answered "in a repository" is now never probed again, a second probe for a session whose first is still out is dropped, and an answer that lands after the panel has moved on is discarded. Only a session that answered "no repository" is re-asked on a later switch — the rare case, and the one where re-asking is what makes a `git init` appear. `noteChangesUnavailable` also stops tearing the tab down itself and calls `toggleChangesTab`, so there is one Changes close path rather than two. The second path was the one nobody would think to guard: a future change that gates the close on unsaved work would leave this one ungated, discarding a dirty buffer with no prompt while simultaneously hiding the button needed to get back to it. If the close does not happen, the button is left visible, because hiding a control whose tab is still open strands whatever that tab is holding.
…bar row `layoutGridCards` walks the sidebar's DOM rows and wraps every id that is also in `openGridSessionIds()`, which does contain `panel:<owner>`. Grid mode has no panel-shell predicate of its own: the shell keeps its container, and the WebGL context the mount/unmount pair manages, only because the session payload gives it no row to be laid out from. Keeping a panel shell out of that payload is therefore load-bearing for a second subsystem, with nothing asserting the coupling. Two tests now hold it from both sides: the shell is in the grid-eligible set, and it gets a card the moment a row for it exists.
A local `execFile` against a cwd that no longer exists, is a file, or is a dangling symlink never spawns: it yields code `-1`, which fell through to the generic branch and reported `spawn git ENOENT` — a message that reads as "git is not installed" when the truth is that the directory is gone. The remote transport spells the same condition differently (`git -C <gone>` exits 128), and the corroborating walk answers "no `.git` anywhere" for a path that is not there, so a deleted worktree outside a repository would have withdrawn the Changes button had the local exec ever reported 128 the way the remote one does. A cwd that cannot be a working directory is now its own outcome, checked before the corroboration is trusted and on the spawn-failure path alike, and the message names the directory. Fixtures for all three shapes exercise real git. The locale tests asserted that two iterations agree without checking that git ever changed language, so on any host without `fr_FR.UTF-8` — every GitHub runner — both iterations were English and the assertion was vacuous. They now probe once whether git's output actually differs and skip with a reason when it does not, so an absent locale reads as "not exercised" rather than "passed". The probe table also named the bare repository as the only `exit 0, false` case; a cwd inside an ordinary repository's `.git/` answers the same way.
…ht session
`refreshChangesAvailability` wrote `changesAvailable` only for a boolean
`isRepo`, so an `{ok: false}` was never remembered. A remote cwd outside a
repository always answers that way by design, and so do a local repository git
refuses and an unreachable host — each one re-probed on every session
activation and every panel-shell open or close, forever, to learn something
that can never change the button. Measured with a counting fake: one activation
plus six revisits cost 7 probes where an `isRepo: true` session costs 1. On a
remote session each is an ssh with a 20 s kill timer; on an unreachable host it
is a 5 s connect stall per click, with no convergence.
That answer is now its own memoised state and the session is not asked again.
The button is visible either way, so nothing is lost by not re-asking. Only a
session that answered "no repository" is still re-asked, which is what makes a
`git init` bring the button back; the same six revisits still cost 7 there, and
an `isRepo: true` session still costs 1.
The stale-reply guard also stood in front of the memo, so a correct answer that
landed after the panel had moved on was discarded and that session paid another
probe on its next activation. The answer is about its own session, so it is
recorded first and the guard now covers only the repaint — which is what it was
for. A test distinguishes the guard from its own removal by counting writes to
the button's display; without it, removing the guard left 216 tests green.
A withdrawal the tab refuses no longer leaves it frozen on its loading render:
it falls through to the ordinary error path and says why instead. Unreachable
through the gate that exists today, reachable through any future one that
declines earlier.
The doc claimed a deleted repository takes the button away again. It does not,
and re-probing every activation to make it true is the cost this memo exists to
remove; the doc now says what happens.
`CHANGES_UNANSWERED` was written unconditionally, so a probe that could not answer displaced one that already had. "No repository" is the single answer this code deliberately re-asks — that is what makes a `git init` bring the button back — which also makes it the one a transient failure can land on. A scratch directory correctly hid its button, the re-ask hit an ssh blip or a sleeping host, and the memo became "unanswerable": button visible again, never asked again for the rest of the run, opening a tab that closes itself. Measured with a counting fake: display `"none"` then `""` with probes frozen at 2. The non-answer now fills a gap only — it is written for a session nothing has been established for yet. A session that keeps its `false` stays re-askable, so the blip costs one probe and nothing else, and the same sequence now ends hidden at 2 probes and shown at 3 once git can answer again. A session that has only ever been unanswerable is still asked exactly once, which is what the memo was added for. The repaint guard on that branch is pinned the way its twin already was: a reply for a session the panel has left writes nothing to the button, whichever way the probe failed. Both guards write the same value, so this states an invariant rather than an outcome — which is the reason to state it on both branches rather than on one. `missingCwdError` also stops accepting a partial `fsOps`. Its catch read a `TypeError` from a seam with no `stat` as "the cwd is fine", turning the check off with nothing to show for it; one test in the suite had that shape and passed only because it never needed the check to fire. Production never injects `fsOps`, so this was a trap for the next test author rather than a live defect.
…ond one The withdrawal reuses `toggleChangesTab` rather than tearing the tab down itself, but that is not the only exit: the panel's own X (`changesCloseBtn` → `handleClose`) clears `currentTab` and hides the panel directly, and no test clicked it. What keeps a tab from being torn down without asking is that each exit carries its own guard, not that they funnel into one — reusing an existing exit is how the withdrawal avoids becoming a new one. Two tests now cover the X: it closes the tab without passing through `toggleChangesTab` and stops the idle refresh, and it leaves the Changes button alone so the view it closed can be reopened. Closing the view is not the same as the session having no repository. The directory-symlink case in the real-git suite also checks `status()`'s shape before indexing into its file list. Under concurrent git spawns that call can come back as an error, and the assertion then failed as a TypeError naming nothing instead of an assertion naming what came back.
The withdrawal routes through `toggleChangesTab`, which now carries `confirmDiscardChangesEdits`, so a repository that stops being one under an open editor asks before discarding the buffer. Neither side could test that on its own: the editor and the gate arrived with the editable Changes panel, the withdrawal with the no-work-tree detection, and they only meet here. Three cases: a refused discard keeps the buffer, the tab and the Changes button — hiding the control while its tab still holds the edit is what would strand it; a confirmed discard closes the tab and withdraws the button; and a clean buffer is never asked about at all.
jbr-sekoia
force-pushed
the
fix/panel-empty-and-norepo
branch
from
September 18, 2026 14:26
417334f to
2270f39
Compare
Merged
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.
Why
Four defects in the right-hand panel, all of them things the panel does in front of you rather than things it computes wrongly.
What
1. No Changes tab where there is no git work tree. A session in a non-repo directory showed the Changes button; clicking it put git's raw output in the panel — a localised
fatal:fromstatus, andgit diff --numstat's entire 150-line usage page. The button is not offered there. Detection isgit rev-parse --is-inside-work-tree's exit code and itstrue/falsetoken, never message text.2. The panel shell fills the panel when no tab is open. It sat at its stored height under an empty dark block.
.shell-onlyon#file-panel-contentgives the regionflex: 1 1 0and hides the drag handle; the dragged height is untouched and comes back when a tab reopens.3. A plain terminal stays a plain terminal when reopened.
openSessiondroppedtype: 'terminal', so a shell whose entry was gone was resumed asclaude --resume <uuid>against an id with no transcript. An exited terminal also reopens under its own id instead of minting a second sidebar row.4. The panel shell is not a sidebar session.
buildProjectsFromCacheinjected it as a row labelledTerminal, which also inflated the status bar's count and would have let grid mode tear the shell's container out of its region."Not a repository" is not a thing git will tell you
Exit 128 is git's generic fatal code, so reading it as "no repository" withdrew the control on repositories that are perfectly real: dubious ownership,
.gitat chmod 000, an unsupportedcore.repositoryformatversion, a broken gitdir file, an orphaned linked worktree. Each one silently hid the button with no message.Measured before choosing a fix: 8 fixtures × 6 probes, every refused fixture returns 128 on every probe, identically to a plain directory.
--resolve-git-dirseparates 2 of the 6. There is no git-only discriminator, by design.So the discriminator comes from outside git: an
fswalk for a.gitentry at the cwd and each ancestor up to the filesystem root. It returns three ways, and only a walk that reaches the root having seen nothing withdraws the control. A.gitthat exists but is broken reports; anEACCESreports; a cwd that has been deleted reports, naming the directory. git's own message is what the panel shows, bounded to 5 lines and 500 characters.Verified against real git on 14 fixtures, including
.gitas a file (linked worktree, submodule), a bare repository and a cwd inside.git/— both of which answer exit 0 /falseand are correctly withdrawn. Loop termination is proven on posix,C:\, UNC and\\?\path shapes.Local and remote diverge, deliberately. A remote session has no filesystem to corroborate with, so a remote 128 always reports. The consequence is stated in the docs rather than hidden: a remote non-repo keeps its button and shows git's message. That is also why a remote cwd that is merely unmounted no longer loses the control.
What the availability probe costs
The probe runs from
switchPanel, which is reached on every session activation and on every panel-shell open and close. Unmemoised that is onesshwith a 20 s kill timer per click, forever, for an answer that cannot change.It is memoised per session, deduplicated while one is in flight, and answers in three shapes: in a repository (never asked again), not a repository (deliberately re-asked, so a
git initrestores the button), and a non-answer — an unreachable host, a refused repository — which is also never asked again, because the button stays visible either way and nothing is lost by not asking.Measured with a counting fake against the real renderer, one activation plus six revisits: a non-answer went 7 probes → 1, a repository stays at 1, a non-repo stays at 7 by design.
A non-answer fills a gap and never overwrites an answer. Without that rule, a scratch directory correctly hidden, then one transient failure on its re-ask, ends up showing a Changes button that opens a tab closing itself immediately — and never being asked again. Measured: the button now stays hidden through the blip and the session stays re-askable.
Interaction with #302
The withdrawal reuses
toggleChangesTabrather than tearing the tab down itself, so a gate on that path applies to it too. The tab's own close button is a second path and carries its own gate. On #302 that gate isconfirmDiscardChangesEdits: merged, a dirty editor buffer is asked about rather than discarded while the button needed to reach the stash disappears. The textual conflict set is 4 files.Verification
Three adversarial review rounds, 11 findings, all closed by execution. Every behavioural claim has a mutation that turns a named test red: the discriminator's three-way return (6 red on reverting to "any 128"), the memo and its dedupe, the non-answer rule, the stale-reply invariant on both branches, the withdrawal routing (5 red), the cwd outcomes (5 red), the cap-overrun skip, the sidebar exclusion, the grid coupling from both sides.
Locale independence is exercised under
fr_FR.UTF-8against real git, and the tests skip with a reason where git does not translate — verified by pointing them at a bogus locale, 19 pass / 2 skipped — so they cannot pass vacuously on a CI runner that lacks the locale.npx eslint .→ 0 errors, 333 warnings (baseline 331; the 2 are theno-redeclare/no-unused-varspair every renderer cross-file global produces, here forsetPanelTerminalShellOnly).npm test→ 1732 / 1729 / 1 fail, and 120 / 119 / 0 on the second batch. The single failure istest/ipc-path-validator.test.js"allows files under ~/.claude/", pre-existing and environmental: that machine's~/.claude/CLAUDE.mdis a symlink out of~/.claudeand the validator resolves on disk. +89 tests over the baseline, no new failure.A healthy repository runs the same 3 commands per refresh as before and 0 extra filesystem calls — the cwd check is on failure branches only.
What a human has to judge, rather than verify