From 871ca31bc64a7e5ac402d9e5819d201ce759cef2 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Renard Date: Fri, 18 Sep 2026 17:59:05 +0200 Subject: [PATCH 1/2] feat(panel): always offer Changes, and say what the directory is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A control that vanishes explains nothing: its absence is indistinguishable from a bug, and it gives the user nowhere to ask why. The Changes button is now unconditional, and the panel it opens reports whichever of four answers is true — the file list, "No changes", "This directory is not a git repository.", or git's own bounded message when there is a repository it refuses to open. Telling those last two apart is kept exactly as it was: exit 128 is git's generic fatal code, so "no repository" is still established by a filesystem walk for a .git entry (gitEntryAtOrAbove), never by a message match or an exit status, and status() still returns reason: 'not-a-repo' only on that positive evidence. Only what the renderer does with the answer changed. With the button unconditional there is nothing left to decide before a switch, so the eager availability probe goes with it: the git-changes-available IPC, its preload binding, the three-way memo, the in-flight dedupe, the stale-reply guard and the withdrawal path through toggleChangesTab. status() already asks the same question at the moment the tab needs the answer, and isWorkTree() stays on the runner for it. --- .ai/contexts/changes-view.md | 144 +++----- .ai/contexts/ipc-bridge.md | 3 +- docs/changes-view.md | 43 +-- main.js | 11 - preload.js | 1 - public/file-panel.js | 66 +--- public/style.css | 6 + test/dom-file-panel-changes.test.js | 542 ++++------------------------ test/panel-terminal.test.js | 1 - 9 files changed, 149 insertions(+), 668 deletions(-) diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md index 2f70cd22..ec9802c2 100644 --- a/.ai/contexts/changes-view.md +++ b/.ai/contexts/changes-view.md @@ -306,20 +306,20 @@ counts. ## Not a repository -A session's working directory need not be inside a git work tree, and when it -is not, the Changes affordance is not offered: `#changes-toggle-btn` is -`display: none` for that session. The panel never renders a refusal for this -case, because there is nothing to refuse — the button that would produce it is -not there. - -**Withdrawing a control is only correct on positive evidence.** A missing -button says nothing and offers no way to ask why, so it is the wrong answer to -every failure except the one it describes. `isWorkTree()` in -`git-changes-runner.js` therefore reports `isRepo: false` only when something -actually established that there is no work tree, and returns `{ok: false, -error}` — *not an answer* — for everything else. The renderer's -`typeof result.isRepo !== 'boolean'` guard leaves the button alone on a -non-answer, and `status()` reports its bounded message into the tab. +A session's working directory need not be inside a git work tree. The Changes +control is offered for every session regardless, and the panel reports whichever +of four answers is true: the file list when there is a work tree with changes, +`No changes` when that tree is clean, `This directory is not a git repository.` +when there is no work tree, and git's own bounded message when there **is** a +repository and git refuses to open it. + +**A control that vanishes explains nothing.** Its absence is indistinguishable +from a bug and offers no way to ask why, so the affordance is unconditional and +every answer is text in the panel it opens. That puts the whole weight on +telling the last two answers apart: "there is no repository here" and "git will +not open this repository" are indistinguishable from git's exit status, and they +must not read alike in the panel — the second is a repository the user has, +usually with a one-line fix in git's own message. **No message is ever matched.** git translates every diagnostic (this project's own host runs it in French), so the detection reads only `git rev-parse @@ -343,15 +343,15 @@ another user, on a mounted or NFS filesystem), a `.git` whose permissions it cannot read, an unsupported `core.repositoryformatversion`, a `.git` file whose gitdir is gone, and a worktree whose main repository was deleted. Every one of those is a repository the user has, usually with a one-line fix in git's own -message — exactly the case where silently removing the panel is worse than -printing the message. `test/git-changes-runner-real-git.test.js` builds those +message — exactly the case the plain "this is not a repository" line must never +be shown for. `test/git-changes-runner-real-git.test.js` builds those fixtures against real git, asserts each really does exit 128, and pins that none of them produces `reason: 'not-a-repo'`. **A cwd that is gone is ruled out before the corroboration is trusted.** The walk below answers "no `.git` anywhere" for a path that does not exist, so a deleted -worktree outside a repository would otherwise withdraw the panel. `execFile` -happens to fail to spawn for such a cwd — code `-1`, not 128 — but the local and +worktree outside a repository would otherwise be reported as "not a git +repository". `execFile` happens to fail to spawn for such a cwd — code `-1`, not 128 — but the local and remote transports differ here (`git -C ` exits 128), so the check is an outcome of its own rather than something left to a code that happens not to match. It also decides the wording: `spawn git ENOENT` reads as "git is not @@ -367,8 +367,8 @@ The corroboration is `gitEntryAtOrAbove(cwd)`: an `fs.lstat` for a `.git` entry at the cwd and at each ancestor up to the filesystem root. It needs no process and no locale, and it answers the one question the exit code cannot — *is there a repository here at all*. It returns three ways, and only `false` (a walk that -reached the root seeing nothing) withdraws the panel; an `EACCES` or any other -unexpected `lstat` error is `null`, undecidable, and reports. A `.git` that +reached the root seeing nothing) is "there is no repository here"; an `EACCES` +or any other unexpected `lstat` error is `null`, undecidable, and reports. A `.git` that exists but is broken counts as `true`: the repository is there, it is just unreadable. @@ -378,85 +378,29 @@ corroboration is a local filesystem walk and there is no remote equivalent that does not either re-read git's translated message or add ssh round-trips. So a remote session that exits 128 is never corroborated and always reports. The practical consequence: a remote working directory that is genuinely not a -repository keeps its Changes button and shows git's own bounded message when -clicked, instead of hiding the button. That is the pre-existing behaviour, and -it is the safe side of the trade — it also means a remote cwd that is merely -unmounted no longer loses the control. - -**Who asks, and when.** Two paths reach the same conclusion, and `status()` is -the cheaper of them: - -- `git-changes-available` runs the probe from `switchPanel()`. The answer is - cached on that session's `filePanelState` entry (`changesAvailable`) and - applied to the button before the round trip, so a known answer never flashes a - button that does not work. -- `status()` returns `{ok: false, reason: 'not-a-repo'}` when a command failed - **and** the probe then establishes there is no work tree. The renderer treats - that exactly like an `isRepo: false` availability answer. That covers the - window between a switch and the repository disappearing under a running - session, and it means a click landing before the availability answer arrives - is handled too. - -**The probe is bounded on both axes.** It is a *diagnosis, not a precondition*: -a session in a repository pays three commands per refresh, the same three as -before, pinned by `calls.length === 3`. A `-uall` run that overruns the stdout -cap does not ask either — that is a volume problem with its own fallback, and -the large repositories that hit it are the ones an extra spawn costs most. And -on the switch path: - -- a session already answered `true` is **never probed again**; -- a session git **could not answer for** is never probed again either. That - answer is memoised as its own state (`CHANGES_UNANSWERED`), because a - `{ok: false}` leaves the button visible and can never change it — asking again - buys nothing and costs an ssh with a 20 s kill timer. It is not a rare shape: - a remote cwd outside a repository, a local repository git refuses, and an - unreachable host all produce it, on every activation, forever; -- **a non-answer never displaces an answer.** `CHANGES_UNANSWERED` is written - only for a session nothing has been established for yet, so the two memo - writes cannot collide. "No repository" is the one answer deliberately - re-asked, which makes it the one a transient failure — an ssh blip, a sleeping - host — can land on; overwriting it would un-hide a button for a directory that - is definitely not a repository, and then never ask again. A session that keeps - its `false` stays re-askable, so the blip costs nothing beyond that one probe; -- a second probe for a session whose first is still in flight is **dropped** - (`changesAvailabilityInFlight`), so a burst of switches cannot put a burst of - ssh children on a remote host; -- an answer is **recorded against the session it is about**, then applied to the - button only while that session is still the one on screen — on every branch, - because "a stale reply never touches the DOM" is an invariant, not a - per-branch outcome. Discarding a - correct answer because the panel had moved on would cost that session another - probe on its next activation; painting from it would paint the wrong - session's state. - -Only a session that answered "no repository" is re-asked on a later switch. -That is the one answer worth re-checking — a `git init` turns it into a -repository — and it is what makes the button come back without polling -anything. The reverse transition is not tracked: a session that answered "in a -repository" keeps its button even if the repository is deleted under it, and -clicking Changes then closes the tab straight away through the `status()` path. -Re-probing every activation to catch that is exactly the cost this memo exists -to remove. - -**Withdrawal reuses the tab's own close control.** `noteChangesUnavailable` -calls `toggleChangesTab(sessionId)` rather than tearing the tab down itself, so -it takes the same path a user's click on the Changes toggle takes and inherits -`confirmDiscardChangesEdits` along with it. A repository that stops being one -under an open editor therefore asks before discarding the buffer, exactly as -the toggle does. - -A Changes tab has a second way out: the panel's own X -(`changesCloseBtn` → `handleClose`), which clears `currentTab` and hides the -panel directly without passing through `toggleChangesTab`, and carries its own -call to the same guard. What keeps a tab from being torn down without asking is -that **each exit is guarded**, not that they funnel into one — a new exit has to -be guarded on its own terms, and reusing an existing one is how the withdrawal -avoids being such an exit. - -If the close does not happen — the user refused the discard, so the tab is still -there afterwards — the button is left visible, because hiding the control while -its tab is still open would strand the edit it is holding with no way back to -it. +repository shows git's own bounded message rather than the plain-language line, +which is the safe side of the trade — the reported message is always true, and +only its wording is coarser than a local session's. + +**Who asks.** `status()` is the only production caller: it returns +`{ok: false, reason: 'not-a-repo'}` when a command failed **and** +`isWorkTree()` then establishes there is no work tree, and every other failure +carries its bounded message and no reason. The renderer turns that one reason +into the plain-language line (`tab.notARepo`, a note rather than an error +colour) and everything else into git's own words, so the distinction the probe +establishes is the distinction the user reads. `isWorkTree()` stays on the +runner's interface for the tests that pin the table above. + +**The probe is a diagnosis, not a precondition.** A session in a repository pays +three commands per refresh, pinned by `calls.length === 3`; the fourth runs only on a status that already failed. A +`-uall` run that overruns the stdout cap does not ask either — that is a volume +problem with its own fallback, and the large repositories that hit it are the +ones an extra spawn costs most. + +A repository that disappears under an open tab is reported into that tab on the +next refresh, and one that appears under a session that had none is listed on +the next refresh too. Nothing polls and nothing is memoised, because the answer +arrives with the status the tab was going to fetch anyway. ## Bounded error messages @@ -472,7 +416,7 @@ A genuine failure — a permission error, a corrupt repository, a transport problem — is still reported, in git's own words and in whatever language git chose. Only the volume is capped. That is what "Not a repository" above leans on: every failure the probe cannot positively explain falls back to this -message rather than to a missing button. +message rather than to the "not a git repository" line. ## cwd resolution (`git-changes-target.js`) diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index aa6e164d..afd80957 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -89,8 +89,7 @@ design (parser, runner, quoting, cwd resolution, refresh triggers, editing): | IPC | Args | Returns | Notes | |---|---|---|---| -| `git-changes-available` | `(sessionId)` | `{ok:true, isRepo} \| {ok:false, error}` | `git rev-parse --is-inside-work-tree`, one invocation. Answers by exit code and the `true`/`false` token, never by message — see `.ai/contexts/changes-view.md` ("Not a repository"). `isRepo:false` is what withdraws the Changes button and is returned only on positive evidence; exit 128 is git's generic fatal code and needs a corroborating filesystem walk (local only). Everything else — a refused repository, a cwd that is gone, ssh's own 255, a thrown exec — is `{ok:false, error}`, which leaves the button alone and is memoised so the session is not asked again. | -| `git-changes-status` | `(sessionId)` | `{ok, kind, branch, files, totals, untrackedCollapsed} \| {ok:false, reason?, error}` | `git status --porcelain=v2 --branch -uall` + `git diff --numstat` + `git diff --cached --numstat`, merged by `git-changes.js`'s `mergeChanges()`. A `-uall` run too large for the transport falls back to git's default untracked mode and reports `untrackedCollapsed: true`. `kind` is `'local'` or `'remote'` — the renderer decides from it whether the panel is editable. A cwd with no work tree comes back `reason: 'not-a-repo'`; every other failure carries a bounded message and no reason. | +| `git-changes-status` | `(sessionId)` | `{ok, kind, branch, files, totals, untrackedCollapsed} \| {ok:false, reason?, error}` | `git status --porcelain=v2 --branch -uall` + `git diff --numstat` + `git diff --cached --numstat`, merged by `git-changes.js`'s `mergeChanges()`. A `-uall` run too large for the transport falls back to git's default untracked mode and reports `untrackedCollapsed: true`. `kind` is `'local'` or `'remote'` — the renderer decides from it whether the panel is editable. A cwd with no work tree comes back `reason: 'not-a-repo'` — the panel's plain-language "not a git repository" line; every other failure carries a bounded message and no reason, including a repository git refuses to open. Telling the two apart is `git rev-parse --is-inside-work-tree` plus a filesystem walk, never a message match — see `.ai/contexts/changes-view.md` ("Not a repository"). | | `git-changes-diff` | `(sessionId, filePath, staged, untracked)` | `{ok, content, truncated, added, deleted} \| {ok:false, error}` | `git diff [--cached] -- `, or `git diff --no-index -- /dev/null ` when `untracked`; capped at 512 KB. `added`/`deleted` are filled for an untracked file only — see `.ai/contexts/changes-view.md` ("Untracked files"). | | `git-changes-file` | `(sessionId, filePath, {staged})` | `{ok, original, current, version, binary, truncated} \| {ok:false, error, reason}` | The content pair behind the editable diff: `git cat-file blob :` (or `HEAD:` when `staged`) and the working-tree file, both LF-normalised and strictly UTF-8. `version` is an opaque token the renderer hands back on save. Local sessions only. `reason` is one of `invalid-path`, `repo`, `missing`, `outside`, `symlink`, `hardlink`, `git-dir`, `sensitive`, `not-a-file`, `binary`, `too-large`, `encoding`, `mixed-eol`, `git`, `remote`. | | `git-changes-save` | `(sessionId, filePath, content, version)` | `{ok:true, version} \| {ok:false, error, reason}` | Writes the working-tree file the guard resolved, re-applying its line endings, and returns the token for the next save. Refused with `reason:'stale'` when the file changed since `version` was issued, and with `invalid-version` when no token is passed. Local sessions only; never creates a file. | diff --git a/docs/changes-view.md b/docs/changes-view.md index 4912ba41..1744a32b 100644 --- a/docs/changes-view.md +++ b/docs/changes-view.md @@ -6,22 +6,24 @@ Click the **Changes** button in the terminal header, next to the stop button. Click it again to close. -The button is only there for a session whose working directory is inside a git -repository. A session started somewhere that is not one — a scratch directory, a -notes folder — has no Changes button at all. Run `git init` there and the button -appears the next time the panel follows that session. - -The reverse is looser: a session that was in a repository keeps its button for -the rest of the run even if you delete the repository under it. Clicking Changes -then closes the view again straight away. Checking for that on every click of -every session would cost a git command each time, which is not worth it for a -case that ends the moment you restart. - -A repository git *refuses to open* is a different case and keeps its button. If -git will not read the repository — it is owned by another user, its permissions -are wrong, or its format is one this git does not support — Changes shows you -git's own message, which usually names the fix. The button disappears only when -there is genuinely no repository there. +Every session has the button. What the panel shows when you open it is whichever +of these is true: + +- the list of changed files, when there are any; +- **No changes**, when the working tree is clean; +- **This directory is not a git repository.**, when the session was started + somewhere that is not one — a scratch directory, a notes folder. Run + `git init` there and the files are listed from the next refresh; +- git's own message, when there *is* a repository and git will not open it: it + is owned by another user, its permissions are wrong, or its format is one this + git does not support. That message usually names the fix. + +The last two are deliberately not worded alike: the first is a directory you +never meant to be a repository, the second is a repository you have and cannot +read yet. + +Delete the repository under a running session and the panel says so on its next +refresh. A file link in the terminal opens here too, when it points at one of this session's changed files: the panel opens on that row, ready to edit against its diff. A link to a file the session has not touched, or to one outside its repository, opens in the plain viewer as before. @@ -113,10 +115,9 @@ The same parser and the same panel render both. Only the command runner differs: - **Local**: `git status`/`git diff` run directly against the session's real working directory (its worktree, if it has one — the same directory a `claude --resume` targets). - **Remote**: the same commands run over the existing ssh connection to the host, against the directory recorded in that session's descriptor. No attach, no tmux — this works even for a session you've never opened a terminal tab for. -A remote working directory that is not a git repository keeps its button and -shows git's message when you click, rather than hiding the button the way a -local one does. Telling "there is no repository here" apart from "git will not -open this repository" needs to look at the directory itself, which Switchboard -can only do on this machine. +A remote working directory that is not a git repository shows git's own message +rather than the plain "not a git repository" line. Telling those two cases apart +needs a look at the directory itself, which Switchboard can only do on this +machine. Diffs are capped at 512 KB; a diff larger than that is truncated with a note at the bottom. diff --git a/main.js b/main.js index 772da514..f48d6a52 100644 --- a/main.js +++ b/main.js @@ -1757,17 +1757,6 @@ function gitChangesRunnerFor(target) { : createGitChangesRunner({ kind: 'local', cwd: target.cwd }); } -// Decides whether the Changes affordance is offered at all — see .ai/contexts/changes-view.md ("Not a repository") -ipcMain.handle('git-changes-available', async (_event, sessionId) => { - const target = resolveGitChangesTarget(sessionId); - if (!target.ok) return target; - try { - return await gitChangesRunnerFor(target).isWorkTree(); - } catch (err) { - return { ok: false, error: err.message }; - } -}); - ipcMain.handle('git-changes-status', async (_event, sessionId) => { const target = resolveGitChangesTarget(sessionId); if (!target.ok) return target; diff --git a/preload.js b/preload.js index 1ff0d97a..0c16d842 100644 --- a/preload.js +++ b/preload.js @@ -34,7 +34,6 @@ contextBridge.exposeInMainWorld('api', { startSubagentWatch: (parentSessionId, agentId) => ipcRenderer.invoke('start-subagent-watch', parentSessionId, agentId), stopSubagentWatch: (watchId) => ipcRenderer.invoke('stop-subagent-watch', watchId), // see .ai/contexts/changes-view.md - gitChangesAvailable: (sessionId) => ipcRenderer.invoke('git-changes-available', sessionId), gitChangesStatus: (sessionId) => ipcRenderer.invoke('git-changes-status', sessionId), gitChangesDiff: (sessionId, filePath, staged, untracked) => ipcRenderer.invoke('git-changes-diff', sessionId, filePath, staged, untracked), gitChangesFile: (sessionId, filePath, opts) => ipcRenderer.invoke('git-changes-file', sessionId, filePath, opts), diff --git a/public/file-panel.js b/public/file-panel.js index b14b4707..350f521a 100644 --- a/public/file-panel.js +++ b/public/file-panel.js @@ -57,11 +57,9 @@ const MIN_CHANGES_LIST_HEIGHT = 96; const MIN_CHANGES_EDITOR_HEIGHT = 120; let changesListDesiredHeight = readStoredChangesListHeight(); -// No work tree, no Changes affordance — see .ai/contexts/changes-view.md ("Not a repository") +// see .ai/contexts/changes-view.md ("Not a repository") const NOT_A_REPO_REASON = 'not-a-repo'; -// see .ai/contexts/changes-view.md ("Who asks, and when") -const CHANGES_UNANSWERED = 'unanswered'; -const changesAvailabilityInFlight = new Set(); +const NOT_A_REPO_TEXT = 'This directory is not a git repository.'; const PANEL_WIDTH_KEY = 'filePanelWidth'; const DEFAULT_PANEL_WIDTH = parseInt(localStorage.getItem(PANEL_WIDTH_KEY), 10) || 450; @@ -356,7 +354,6 @@ function getSessionState(sessionId) { panelVisible: false, panelWidth: DEFAULT_PANEL_WIDTH, mcpActive: false, - changesAvailable: null, }); } return filePanelState.get(sessionId); @@ -557,7 +554,6 @@ function hidePanel() { function switchPanel(sessionId) { currentPanelSessionId = sessionId; updateMcpIndicator(); - refreshChangesAvailability(sessionId); if (typeof syncPanelTerminal === 'function') syncPanelTerminal(sessionId); if (!sessionId) { @@ -714,54 +710,6 @@ function handleDiffAction(sessionId, tab, action) { // ── Changes Mode — see .ai/contexts/changes-view.md ────────────────── -// see .ai/contexts/changes-view.md ("Not a repository") -function updateChangesToggle() { - if (!changesToggleBtn) return; - const state = currentPanelSessionId ? filePanelState.get(currentPanelSessionId) : null; - changesToggleBtn.style.display = state && state.changesAvailable === false ? 'none' : ''; -} - -// see .ai/contexts/changes-view.md ("Who asks, and when") -async function refreshChangesAvailability(sessionId) { - updateChangesToggle(); - if (!sessionId || typeof window.api?.gitChangesAvailable !== 'function') return; - const memo = getSessionState(sessionId).changesAvailable; - if (memo === true || memo === CHANGES_UNANSWERED) return; - if (changesAvailabilityInFlight.has(sessionId)) return; - - changesAvailabilityInFlight.add(sessionId); - let result; - try { - result = await window.api.gitChangesAvailable(sessionId); - } finally { - changesAvailabilityInFlight.delete(sessionId); - } - - if (!result || typeof result.isRepo !== 'boolean') { - const state = getSessionState(sessionId); - if (state.changesAvailable === null) state.changesAvailable = CHANGES_UNANSWERED; - if (currentPanelSessionId === sessionId) updateChangesToggle(); - return; - } - if (result.isRepo) { - getSessionState(sessionId).changesAvailable = true; - if (currentPanelSessionId === sessionId) updateChangesToggle(); - return; - } - noteChangesUnavailable(sessionId); -} - -// see .ai/contexts/changes-view.md ("Not a repository") -function noteChangesUnavailable(sessionId) { - const state = getSessionState(sessionId); - if (state.currentTab && state.currentTab.type === 'changes') { - toggleChangesTab(sessionId); - if (state.currentTab) return; - } - state.changesAvailable = false; - if (currentPanelSessionId === sessionId) updateChangesToggle(); -} - function toggleChangesTab(sessionId) { const state = getSessionState(sessionId); if (state.currentTab && state.currentTab.type === 'changes') { @@ -806,6 +754,7 @@ function openChangesTab(sessionId) { saveError: null, saving: false, externalChange: false, + notARepo: false, }; state.panelVisible = true; restoreChangesEdits(sessionId, state, state.currentTab); @@ -833,14 +782,15 @@ async function refreshChanges(sessionId) { tab.loading = false; if (result && result.reason === NOT_A_REPO_REASON) { - noteChangesUnavailable(sessionId); - if (!stillState.currentTab) return; - tab.error = result.error || 'not a git repository'; + tab.notARepo = true; + tab.error = NOT_A_REPO_TEXT; tab.data = null; } else if (!result || result.ok === false) { + tab.notARepo = false; tab.error = (result && result.error) || 'failed to load changes'; tab.data = null; } else { + tab.notARepo = false; tab.error = null; tab.data = result; tab.remote = result.kind === 'remote'; @@ -1085,7 +1035,7 @@ function renderChangesList(sessionId, tab) { changesSummaryEl.textContent = ''; changesListEl.innerHTML = ''; const err = document.createElement('div'); - err.className = 'changes-error'; + err.className = tab.notARepo ? 'changes-note' : 'changes-error'; err.textContent = tab.error; changesListEl.appendChild(err); if (branchInfoEl) branchInfoEl.textContent = ''; diff --git a/public/style.css b/public/style.css index 128c1599..2c8504ab 100644 --- a/public/style.css +++ b/public/style.css @@ -4635,6 +4635,12 @@ body { display: flex; flex-direction: column; } font-size: 12px; } +.changes-note { + padding: 12px; + color: var(--text-muted); + font-size: 12px; +} + .changes-degraded-note { margin-top: 4px; font-size: 11px; diff --git a/test/dom-file-panel-changes.test.js b/test/dom-file-panel-changes.test.js index 63ed9634..c22ff061 100644 --- a/test/dom-file-panel-changes.test.js +++ b/test/dom-file-panel-changes.test.js @@ -81,11 +81,11 @@ function makeEditorStub(window, mode, doc, created, onChange) { return view; } -function setupFilePanelDom({ statusImpl, diffImpl, fileImpl, saveImpl, confirmImpl, locateImpl, availableImpl } = {}) { +function setupFilePanelDom({ statusImpl, diffImpl, fileImpl, saveImpl, confirmImpl, locateImpl } = {}) { const dom = new JSDOM(INDEX_HTML, { url: 'http://localhost/', runScripts: 'outside-only', pretendToBeVisual: true }); const { window } = dom; - const calls = { status: [], diff: [], file: [], save: [], watch: [], unwatch: [], confirm: [], locate: [], readFile: [], available: [] }; + const calls = { status: [], diff: [], file: [], save: [], watch: [], unwatch: [], confirm: [], locate: [], readFile: [] }; const editors = []; const fileChangedListeners = []; @@ -95,10 +95,6 @@ function setupFilePanelDom({ statusImpl, diffImpl, fileImpl, saveImpl, confirmIm onMcpCloseAllDiffs: () => {}, onMcpCloseTab: () => {}, mcpDiffResponse: () => {}, - gitChangesAvailable: (sessionId) => { - calls.available.push(sessionId); - return Promise.resolve((availableImpl || (() => ({ ok: true, isRepo: true })))(sessionId)); - }, gitChangesStatus: (sessionId) => { calls.status.push(sessionId); return Promise.resolve((statusImpl || (() => makeStatusResult()))(sessionId)); @@ -1914,549 +1910,149 @@ test('Save follows the buffer in every mode, plain included (mutation target: th } finally { ctx.destroy(); } }); -// --- No git work tree — see .ai/contexts/changes-view.md ("Not a repository") --- - -test('a cwd with no work tree withdraws the Changes button instead of offering a tab that cannot fill', async () => { - const ctx = setupFilePanelDom({ availableImpl: () => ({ ok: true, isRepo: false }) }); - try { - ctx.window.switchPanel('s1'); - await flush(); +// --- What the panel says, for each answer git can give --------------------- +// The Changes control is unconditional; the panel reports. See +// .ai/contexts/changes-view.md ("Not a repository"). - const btn = ctx.document.getElementById('changes-toggle-btn'); - assert.equal(btn.style.display, 'none', 'no work tree, no Changes affordance'); - assert.equal(ctx.calls.status.length, 0, 'the availability answer costs no status call'); - } finally { ctx.destroy(); } -}); +function noRepoStatus() { + return { ok: false, reason: 'not-a-repo', error: 'fatal: not a git repository (or any of the parent directories): .git' }; +} -test('the Changes button stays visible for a session that is in a work tree', async () => { +test('a work tree with changes lists its files under a count', async () => { const ctx = setupFilePanelDom(); - try { - ctx.window.switchPanel('s1'); - await flush(); - assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none'); - } finally { ctx.destroy(); } -}); - -test('the button follows the session the panel shows, not the last answer that arrived', async () => { - const ctx = setupFilePanelDom({ availableImpl: (id) => ({ ok: true, isRepo: id !== 'norepo' }) }); - try { - ctx.window.switchPanel('norepo'); - await flush(); - assert.equal(ctx.document.getElementById('changes-toggle-btn').style.display, 'none'); - - ctx.window.switchPanel('s1'); - await flush(); - assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none', - 'a session in a repo must get its button back'); - - ctx.window.switchPanel('norepo'); - assert.equal(ctx.document.getElementById('changes-toggle-btn').style.display, 'none', - 'a known answer applies before the round trip, with no flash of a button that does not work'); - } finally { ctx.destroy(); } -}); - -test('a Changes tab already open when the cwd turns out to have no work tree is withdrawn, not left half-rendered', async () => { - let isRepo = true; - const ctx = setupFilePanelDom({ - availableImpl: () => ({ ok: true, isRepo }), - statusImpl: () => (isRepo ? makeStatusResult() : { ok: false, reason: 'not-a-repo', error: 'not a git repository' }), - }); try { ctx.window.switchPanel('s1'); ctx.window.openChangesTab('s1'); await flush(); - assert.equal(ctx.document.querySelectorAll('.changes-file-row').length, 2); - - isRepo = false; - ctx.setActivity('s1', true); - ctx.setActivity('s1', false); - await flush(); - assert.equal(ctx.document.getElementById('file-panel').classList.contains('open'), false, - 'the tab closes rather than reporting into itself'); - assert.equal(ctx.document.getElementById('changes-toggle-btn').style.display, 'none'); - - const statusCalls = ctx.calls.status.length; - ctx.setActivity('s1', true); - ctx.setActivity('s1', false); - await flush(); - assert.equal(ctx.calls.status.length, statusCalls, 'the tab is gone, so nothing refreshes it any more'); + assert.equal(ctx.document.querySelectorAll('.changes-file-row').length, 2); + assert.match(ctx.document.getElementById('changes-summary').textContent, /2 files changed/); } finally { ctx.destroy(); } }); -test('git stderr never reaches the panel as the message when the cwd has no work tree', async () => { +test('a clean work tree keeps the Changes control and says there is nothing to show', async () => { const ctx = setupFilePanelDom({ - statusImpl: () => ({ ok: false, reason: 'not-a-repo', error: 'not a git repository' }), + statusImpl: () => makeStatusResult({ files: [], totals: { files: 0, added: 0, deleted: 0 } }), }); try { ctx.window.switchPanel('s1'); - ctx.window.openChangesTab('s1'); - await flush(); - - assert.equal(ctx.document.querySelectorAll('.changes-error').length, 0, - 'a missing work tree is not an error to report, it is an affordance to withdraw'); - assert.doesNotMatch(ctx.document.body.textContent, /dépôt git|not a git repository/); - } finally { ctx.destroy(); } -}); + assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none'); -test('a genuine git failure is still reported in the tab, and the button stays', async () => { - const ctx = setupFilePanelDom({ - statusImpl: () => ({ ok: false, error: 'could not read directory: Permission denied' }), - }); - try { - ctx.window.switchPanel('s1'); ctx.window.openChangesTab('s1'); await flush(); - const err = ctx.document.querySelector('.changes-error'); - assert.ok(err, 'an unexpected failure must still be visible'); - assert.match(err.textContent, /Permission denied/); - assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none', - 'only a missing work tree withdraws the button — a transient failure must not'); - } finally { ctx.destroy(); } -}); - -// --- The availability probe is memoised, deduped and ignored when stale ------ -// For a remote session each probe is an ssh with a 20 s kill timer, and -// switchPanel is reached from every panel-shell open, close and exit as well as -// from every sidebar click — see .ai/contexts/changes-view.md ("Not a repository"). - -function deferredAvailable() { - const gates = []; - return { - gates, - impl: (sessionId) => new Promise((resolve) => gates.push({ sessionId, resolve })), - settle: (sessionId, value) => { - for (const g of gates.filter((x) => x.sessionId === sessionId)) g.resolve(value); - }, - settleAll: (value) => { for (const g of gates.splice(0)) g.resolve(value); }, - }; -} - -test('a session already known to be in a repository is never probed again', async () => { - const ctx = setupFilePanelDom(); - try { - ctx.window.switchPanel('s1'); - await flush(); - assert.equal(ctx.calls.available.length, 1); - - for (let i = 0; i < 5; i++) { - ctx.window.switchPanel('s1'); - await flush(); - } - assert.equal(ctx.calls.available.length, 1, - 'five re-entries into the same session must cost one probe, not five'); - } finally { ctx.destroy(); } -}); - -test('re-entering a session while its probe is still out does not start a second one', async () => { - const d = deferredAvailable(); - const ctx = setupFilePanelDom({ availableImpl: d.impl }); - try { - for (let i = 0; i < 10; i++) ctx.window.switchPanel('s1'); - await flush(); - assert.equal(ctx.calls.available.length, 1, - 'ten rapid switches must not put ten concurrent ssh children on a remote host'); - - d.settleAll({ ok: true, isRepo: true }); - await flush(); + assert.equal(ctx.document.querySelectorAll('.changes-file-row').length, 0); + assert.match(ctx.document.getElementById('changes-summary').textContent, /No changes/); } finally { ctx.destroy(); } }); -test('a session with no repository is re-asked, so a git init is picked up', async () => { - let isRepo = false; - const ctx = setupFilePanelDom({ availableImpl: () => ({ ok: true, isRepo }) }); +test('a directory that is not a repository keeps the Changes control and says so in plain words', async () => { + const ctx = setupFilePanelDom({ statusImpl: noRepoStatus }); try { ctx.window.switchPanel('s1'); - await flush(); - assert.equal(ctx.document.getElementById('changes-toggle-btn').style.display, 'none'); - - isRepo = true; - ctx.window.switchPanel('s2'); - await flush(); - ctx.window.switchPanel('s1'); - await flush(); - - assert.equal(ctx.calls.available.filter((id) => id === 's1').length, 2, - 'only the sessions that answered "no repository" pay a second probe'); - assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none'); - } finally { ctx.destroy(); } -}); - -test('a probe that answers after the panel has moved on changes nothing', async () => { - const d = deferredAvailable(); - const ctx = setupFilePanelDom({ availableImpl: d.impl }); - try { - ctx.window.switchPanel('s1'); - await flush(); - ctx.window.switchPanel('s2'); - await flush(); - - d.settle('s1', { ok: true, isRepo: false }); - d.settle('s2', { ok: true, isRepo: true }); - await flush(); - assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none', - 's1’s answer must not withdraw s2’s button'); - } finally { ctx.destroy(); } -}); - -// --- Withdrawal goes through the tab's own close control -------------------- + 'a control that vanishes explains nothing; the panel explains instead'); -test('withdrawing the tab goes through toggleChangesTab, not a teardown of its own', async () => { - let isRepo = true; - const ctx = setupFilePanelDom({ - availableImpl: () => ({ ok: true, isRepo }), - statusImpl: () => (isRepo ? makeStatusResult() : { ok: false, reason: 'not-a-repo', error: 'not a git repository' }), - }); - try { - ctx.window.switchPanel('s1'); ctx.window.openChangesTab('s1'); await flush(); - const toggles = []; - const realToggle = ctx.window.toggleChangesTab; - ctx.window.toggleChangesTab = (id) => { toggles.push(id); return realToggle(id); }; - - isRepo = false; - ctx.setActivity('s1', true); - ctx.setActivity('s1', false); - await flush(); - - assert.deepEqual(toggles, ['s1'], - 'the one close path a future gate will guard must be the one the withdrawal uses'); - assert.equal(ctx.document.getElementById('file-panel').classList.contains('open'), false); - assert.equal(ctx.document.getElementById('changes-toggle-btn').style.display, 'none'); + const note = ctx.document.querySelector('.changes-note'); + assert.ok(note, 'the answer reaches the panel as a note, not as a git failure'); + assert.match(note.textContent, /not a git repository/); + assert.equal(ctx.document.querySelectorAll('.changes-error').length, 0); + assert.doesNotMatch(note.textContent, /fatal:/, "git's own wording is not what this state says"); } finally { ctx.destroy(); } }); -test('a close the tab refuses leaves the button, so the tab can still be reopened', async () => { +test("a repository git refuses shows git's own message, not the no-repository text (mutation target: the reason check)", async () => { const ctx = setupFilePanelDom({ - statusImpl: () => ({ ok: false, reason: 'not-a-repo', error: 'not a git repository' }), + statusImpl: () => ({ ok: false, error: 'fatal: detected dubious ownership in repository at /srv/repo' }), }); try { ctx.window.switchPanel('s1'); ctx.window.openChangesTab('s1'); - // A gate that declines — what #302's confirmDiscardChangesEdits does on "cancel". - ctx.window.toggleChangesTab = () => {}; - await flush(); - - assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none', - 'hiding the control while its tab is still open strands whatever the tab is holding'); - } finally { ctx.destroy(); } -}); - -// --- A non-answer is memoised too — see .ai/contexts/changes-view.md ("Who asks, and when") --- -// A remote 128 is always {ok:false} by design, and a {ok:false} can never change -// the button, so re-asking costs an ssh with a 20 s kill timer to learn nothing. - -function countRevisits(ctx, sessionId, times) { - const before = ctx.calls.available.filter((id) => id === sessionId).length; - const run = async () => { - for (let i = 0; i < times; i++) { - ctx.window.switchPanel('other'); - await flush(); - ctx.window.switchPanel(sessionId); - await flush(); - } - return ctx.calls.available.filter((id) => id === sessionId).length - before; - }; - return run(); -} - -test('a session git could not answer for is asked once, not on every activation', async () => { - const ctx = setupFilePanelDom({ - availableImpl: (id) => (id === 'remote' ? { ok: false, error: 'fatal: …' } : { ok: true, isRepo: true }), - }); - try { - ctx.window.switchPanel('remote'); - await flush(); - assert.equal(ctx.calls.available.filter((id) => id === 'remote').length, 1); - - assert.equal(await countRevisits(ctx, 'remote', 6), 0, - 'six revisits must add no probes — the answer cannot change the button, so asking again buys nothing'); - assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none', - 'and the button stays, because nothing established that there is no repository'); - } finally { ctx.destroy(); } -}); - -test('a session in a repository is still asked once and never again', async () => { - const ctx = setupFilePanelDom(); - try { - ctx.window.switchPanel('s1'); - await flush(); - assert.equal(await countRevisits(ctx, 's1', 6), 0); - } finally { ctx.destroy(); } -}); - -test('a session with no repository is still re-asked, so the two memos do not collapse into one', async () => { - const ctx = setupFilePanelDom({ - availableImpl: (id) => (id === 'norepo' ? { ok: true, isRepo: false } : { ok: true, isRepo: true }), - }); - try { - ctx.window.switchPanel('norepo'); - await flush(); - assert.equal(await countRevisits(ctx, 'norepo', 3), 3, - 'only the answer that hides the button is worth re-checking'); - } finally { ctx.destroy(); } -}); - -// --- The stale-reply guard covers the DOM, not the memo --------------------- - -test('an answer for a session the panel has left is still recorded against that session', async () => { - const d = deferredAvailable(); - const ctx = setupFilePanelDom({ availableImpl: d.impl }); - try { - ctx.window.switchPanel('s1'); - await flush(); - ctx.window.switchPanel('s2'); - await flush(); - - d.settle('s1', { ok: true, isRepo: true }); - d.settle('s2', { ok: true, isRepo: true }); - await flush(); - - const before = ctx.calls.available.filter((id) => id === 's1').length; - ctx.window.switchPanel('s1'); - await flush(); - assert.equal(ctx.calls.available.filter((id) => id === 's1').length, before, - 'a correct answer must not be thrown away just because the panel had moved on'); - } finally { ctx.destroy(); } -}); - -test('an answer for a session the panel has left never touches the current button', async () => { - const d = deferredAvailable(); - const ctx = setupFilePanelDom({ availableImpl: d.impl }); - try { - ctx.window.switchPanel('s1'); - await flush(); - ctx.window.switchPanel('s2'); - await flush(); - - d.settle('s1', { ok: false, error: 'fatal: …' }); - d.settle('s2', { ok: true, isRepo: true }); await flush(); + const err = ctx.document.querySelector('.changes-error'); + assert.ok(err, 'a repository git will not open is a failure to report, with its own message'); + assert.match(err.textContent, /dubious ownership/); + assert.equal(ctx.document.querySelectorAll('.changes-note').length, 0); + assert.doesNotMatch(ctx.document.body.textContent, /This directory is not a git repository/, + 'telling the two apart is the whole point of the filesystem corroboration behind the reason'); assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none'); - - // ...and the memo it wrote is s1's, proven by s1 not being probed again. - const before = ctx.calls.available.filter((id) => id === 's1').length; - ctx.window.switchPanel('s1'); - await flush(); - assert.equal(ctx.calls.available.filter((id) => id === 's1').length, before); } finally { ctx.destroy(); } }); -// --- A refused withdrawal must not freeze the tab --------------------------- - -test('a close the tab refuses leaves a readable tab, not a permanent Loading', async () => { - const ctx = setupFilePanelDom({ - statusImpl: () => ({ ok: false, reason: 'not-a-repo', error: 'not a git repository' }), - }); +test('a repository that disappears under an open tab reports into it instead of closing it', async () => { + let repo = true; + const ctx = setupFilePanelDom({ statusImpl: () => (repo ? makeStatusResult() : noRepoStatus()) }); try { ctx.window.switchPanel('s1'); ctx.window.openChangesTab('s1'); - ctx.window.toggleChangesTab = () => {}; // a gate that declines await flush(); + assert.equal(ctx.document.querySelectorAll('.changes-file-row').length, 2); - const summary = ctx.document.getElementById('changes-summary'); - assert.doesNotMatch(summary.textContent, /Loading/, - 'the tab is staying open, so it has to say something other than the render it was stuck on'); - const err = ctx.document.querySelector('.changes-error'); - assert.ok(err, 'a tab that could not be withdrawn must explain itself'); - } finally { ctx.destroy(); } -}); - -// Counts writes to the button's display, which is the only trace a redundant -// repaint leaves: the value written is always the current session's. -function countDisplayWrites(ctx) { - const btn = ctx.document.getElementById('changes-toggle-btn'); - const style = btn.style; - let writes = 0; - const proto = Object.getPrototypeOf(style); - const descriptor = Object.getOwnPropertyDescriptor(proto, 'display'); - Object.defineProperty(style, 'display', { - configurable: true, - get() { return descriptor.get.call(style); }, - set(v) { writes++; descriptor.set.call(style, v); }, - }); - return { count: () => writes, reset: () => { writes = 0; } }; -} - -test('a reply for a session the panel has left repaints nothing (mutation target: dropping the guard around updateChangesToggle)', async () => { - const d = deferredAvailable(); - const ctx = setupFilePanelDom({ availableImpl: d.impl }); - try { - ctx.window.switchPanel('s1'); - await flush(); - ctx.window.switchPanel('s2'); - await flush(); - - const writes = countDisplayWrites(ctx); - writes.reset(); - - d.settle('s1', { ok: true, isRepo: true }); - await flush(); - assert.equal(writes.count(), 0, - 's1 is not the session on screen, so its answer has no button to paint'); - - d.settle('s2', { ok: true, isRepo: true }); - await flush(); - assert.equal(writes.count(), 1, 's2 is, so its answer does'); - } finally { ctx.destroy(); } -}); - -// --- A non-answer never displaces an answer --------------------------------- -// "no repository" is the one answer deliberately re-asked, so it is also the -// one that a transient failure can land on — an ssh blip, a sleeping host. See -// .ai/contexts/changes-view.md ("Who asks, and when"). - -test('a transient failure on the re-ask does not un-hide a button that was correctly hidden', async () => { - let answer = { ok: true, isRepo: false }; - const ctx = setupFilePanelDom({ availableImpl: (id) => (id === 'scratch' ? answer : { ok: true, isRepo: true }) }); - try { - const btn = ctx.document.getElementById('changes-toggle-btn'); - ctx.window.switchPanel('scratch'); - await flush(); - assert.equal(btn.style.display, 'none', 'a scratch directory is not a repository'); - - answer = { ok: false, error: 'ssh: connect to host h port 22: Connection refused' }; - ctx.window.switchPanel('other'); - await flush(); - ctx.window.switchPanel('scratch'); + repo = false; + ctx.setActivity('s1', true); + ctx.setActivity('s1', false); await flush(); - assert.equal(btn.style.display, 'none', - 'a probe that could not answer must not overwrite the answer that was already established'); - assert.equal(ctx.calls.available.filter((id) => id === 'scratch').length, 2); + assert.equal(ctx.document.getElementById('file-panel').classList.contains('open'), true, + 'the tab stays open and says what happened'); + assert.match(ctx.document.querySelector('.changes-note').textContent, /not a git repository/); - // And the session is still re-askable, so the blip costs nothing permanent. - answer = { ok: true, isRepo: true }; - ctx.window.switchPanel('other'); - await flush(); - ctx.window.switchPanel('scratch'); - await flush(); - assert.notEqual(btn.style.display, 'none', 'once git can answer again, the answer applies'); - } finally { ctx.destroy(); } -}); - -test('a session that has only ever been unanswerable is still asked exactly once', async () => { - const ctx = setupFilePanelDom({ - availableImpl: (id) => (id === 'remote' ? { ok: false, error: 'fatal: …' } : { ok: true, isRepo: true }), - }); - try { - ctx.window.switchPanel('remote'); - await flush(); - assert.equal(await countRevisits(ctx, 'remote', 4), 0, - 'the memo must still stop the forever-probe it was added for'); - } finally { ctx.destroy(); } -}); - -test('a transient failure before any answer is memoised, and a later one after an answer is not', async () => { - let answer = { ok: false, error: 'fatal: …' }; - const ctx = setupFilePanelDom({ availableImpl: (id) => (id === 't' ? answer : { ok: true, isRepo: true }) }); - try { - ctx.window.switchPanel('t'); + repo = true; + ctx.setActivity('s1', true); + ctx.setActivity('s1', false); await flush(); - assert.equal(ctx.calls.available.filter((id) => id === 't').length, 1); - - // Nothing was established, so the non-answer sticks and stops the asking. - answer = { ok: true, isRepo: false }; - assert.equal(await countRevisits(ctx, 't', 3), 0); - assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none'); + assert.equal(ctx.document.querySelectorAll('.changes-file-row').length, 2, + 'and a git init is picked up by the next refresh, with no probe to re-ask'); } finally { ctx.destroy(); } }); -test('a reply for a departed session repaints nothing on the unanswerable branch either', async () => { - const d = deferredAvailable(); - const ctx = setupFilePanelDom({ availableImpl: d.impl }); +test('a git failure after a no-repository answer is shown as a failure (mutation target: the reset)', async () => { + let answer = noRepoStatus; + const ctx = setupFilePanelDom({ statusImpl: () => answer() }); try { ctx.window.switchPanel('s1'); + ctx.window.openChangesTab('s1'); await flush(); - ctx.window.switchPanel('s2'); - await flush(); - - const writes = countDisplayWrites(ctx); - writes.reset(); + assert.ok(ctx.document.querySelector('.changes-note')); - d.settle('s1', { ok: false, error: 'fatal: …' }); + answer = () => ({ ok: false, error: 'fatal: detected dubious ownership in repository at /srv/repo' }); + ctx.setActivity('s1', true); + ctx.setActivity('s1', false); await flush(); - assert.equal(writes.count(), 0, - 'the twin of the isRepo branch: a stale reply never touches the DOM, whichever way it failed'); - d.settle('s2', { ok: false, error: 'fatal: …' }); - await flush(); - assert.equal(writes.count(), 1, 'the current session’s reply does'); + assert.equal(ctx.document.querySelectorAll('.changes-note').length, 0, + 'the previous answer must not colour the next one'); + assert.match(ctx.document.querySelector('.changes-error').textContent, /dubious ownership/); } finally { ctx.destroy(); } }); -// --- Withdrawal over a dirty editor buffer ---------------------------------- -// The one combination neither branch could have had a test for: the editor is -// #302's, the withdrawal is this branch's, and they meet at toggleChangesTab. -// See .ai/contexts/changes-view.md ("Withdrawal reuses the tab's own close -// control"). - -async function openDirtyFileThen(ctx, statusAfter) { - await openFile(ctx, 's1', 'src/a.js'); - ctx.editors[0].box.text = 'my unsaved edit\n'; - statusAfter(); - ctx.setActivity('s1', true); - ctx.setActivity('s1', false); - await flush(); -} - -test('a withdrawal over unsaved edits asks first, and a refusal keeps both the editor and the button', async () => { - let repo = true; - const ctx = setupFilePanelDom({ - confirmImpl: () => false, - statusImpl: () => (repo ? makeStatusResult() : { ok: false, reason: 'not-a-repo', error: 'not a git repository' }), - }); - try { - await openDirtyFileThen(ctx, () => { repo = false; }); - - assert.equal(ctx.calls.confirm.length, 1, 'the withdrawal asks the same question every other exit asks'); - assert.equal(ctx.editors[0].box.destroyed, false, 'a refused discard must not destroy the buffer'); - assert.equal(ctx.editors[0].box.text, 'my unsaved edit\n'); - assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none', - 'hiding the control while its tab still holds the edit is what strands it'); - assert.equal(ctx.document.getElementById('file-panel').classList.contains('open'), true); - } finally { ctx.destroy(); } -}); - -test('a withdrawal over unsaved edits proceeds once the user confirms, and then withdraws the button', async () => { +test('a repository that disappears under unsaved edits asks nothing and keeps the buffer', async () => { let repo = true; const ctx = setupFilePanelDom({ confirmImpl: () => true, - statusImpl: () => (repo ? makeStatusResult() : { ok: false, reason: 'not-a-repo', error: 'not a git repository' }), - }); - try { - await openDirtyFileThen(ctx, () => { repo = false; }); - - assert.equal(ctx.calls.confirm.length, 1); - assert.equal(ctx.document.getElementById('file-panel').classList.contains('open'), false); - assert.equal(ctx.document.getElementById('changes-toggle-btn').style.display, 'none'); - } finally { ctx.destroy(); } -}); - -test('a withdrawal with nothing unsaved never asks, it just withdraws', async () => { - let repo = true; - const ctx = setupFilePanelDom({ - confirmImpl: () => false, - statusImpl: () => (repo ? makeStatusResult() : { ok: false, reason: 'not-a-repo', error: 'not a git repository' }), + statusImpl: () => (repo ? makeStatusResult() : noRepoStatus()), }); try { await openFile(ctx, 's1', 'src/a.js'); + ctx.editors[0].box.text = 'my unsaved edit\n'; + repo = false; ctx.setActivity('s1', true); ctx.setActivity('s1', false); await flush(); - assert.equal(ctx.calls.confirm.length, 0, 'a clean buffer has nothing to discard'); - assert.equal(ctx.document.getElementById('changes-toggle-btn').style.display, 'none', - 'so the refusal path is never reached and the button goes'); + assert.equal(ctx.calls.confirm.length, 0, 'nothing is being discarded, so nothing is asked'); + assert.equal(ctx.editors[0].box.destroyed, false); + assert.equal(ctx.editors[0].box.text, 'my unsaved edit\n'); + assert.equal(ctx.document.getElementById('file-panel').classList.contains('open'), true); } finally { ctx.destroy(); } }); + // --- The panel's own X is a second close path ------------------------------- // `handleClose` clears currentTab and hides the panel directly, without passing // through toggleChangesTab — see .ai/contexts/changes-view.md ("Withdrawal @@ -2491,7 +2087,7 @@ test('the panel X closes a Changes tab on its own path, not through toggleChange } finally { ctx.destroy(); } }); -test('the panel X leaves the Changes button, so the tab it closed can be reopened', async () => { +test('the tab the panel X closed is reopened by the Changes button', async () => { const ctx = setupFilePanelDom(); try { ctx.window.switchPanel('s1'); @@ -2499,8 +2095,6 @@ test('the panel X leaves the Changes button, so the tab it closed can be reopene await flush(); ctx.document.querySelector('#file-panel-changes .fp-close-btn').click(); - assert.notEqual(ctx.document.getElementById('changes-toggle-btn').style.display, 'none', - 'closing the view is not the same as the session having no repository'); ctx.document.getElementById('changes-toggle-btn').click(); await flush(); diff --git a/test/panel-terminal.test.js b/test/panel-terminal.test.js index d44be4fd..bb814bf4 100644 --- a/test/panel-terminal.test.js +++ b/test/panel-terminal.test.js @@ -817,7 +817,6 @@ const STATUS_OK = { function setupPanelWithTab(extra = {}) { return setupPanel({ api: { - gitChangesAvailable: () => Promise.resolve({ ok: true, isRepo: true }), gitChangesStatus: () => Promise.resolve(STATUS_OK), }, ...extra, From 3940b675736772fbb412396d62c2e3db4bf5955a Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Renard Date: Fri, 18 Sep 2026 18:30:07 +0200 Subject: [PATCH 2/2] fix(panel): state the no-repository fact in the notice bar, not as a failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With a file open, the notice above the editor borrowed both the wording and the colour of a refresh that failed — the same sentence the list below deliberately renders as a neutral note. A directory that has no repository did not fail at anything, so the notice now states the fact itself and takes the neutral colour, while every genuine failure keeps "The file list could not be refreshed" and the error colour. The dirty-buffer case runs over the three ways the tree can stop answering under an open editor — the repository removed, the directory gone, and a repository git refuses — rather than only the first. --- .ai/contexts/changes-view.md | 9 ++-- public/file-panel.js | 6 ++- test/dom-file-panel-changes.test.js | 66 +++++++++++++++++++++++++---- 3 files changed, 68 insertions(+), 13 deletions(-) diff --git a/.ai/contexts/changes-view.md b/.ai/contexts/changes-view.md index ec9802c2..259386b9 100644 --- a/.ai/contexts/changes-view.md +++ b/.ai/contexts/changes-view.md @@ -386,9 +386,12 @@ only its wording is coarser than a local session's. `{ok: false, reason: 'not-a-repo'}` when a command failed **and** `isWorkTree()` then establishes there is no work tree, and every other failure carries its bounded message and no reason. The renderer turns that one reason -into the plain-language line (`tab.notARepo`, a note rather than an error -colour) and everything else into git's own words, so the distinction the probe -establishes is the distinction the user reads. `isWorkTree()` stays on the +into the plain-language line and everything else into git's own words, so the +distinction the probe establishes is the distinction the user reads. `tab.notARepo` +carries it to both surfaces that can show it: the list renders a neutral note +rather than an error, and the notice line above an open editor states the same +fact in the same neutral colour instead of reporting a refresh that failed — +a directory with no repository did not fail at anything. `isWorkTree()` stays on the runner's interface for the tests that pin the table above. **The probe is a diagnosis, not a precondition.** A session in a repository pays diff --git a/public/file-panel.js b/public/file-panel.js index 350f521a..05c3ef30 100644 --- a/public/file-panel.js +++ b/public/file-panel.js @@ -1263,14 +1263,16 @@ function updateChangesSaveButton(sessionId, tab) { function renderChangesNotice(tab) { const notes = []; - const alarming = !!(tab.saveError || tab.fileError || tab.error || tab.externalChange || tab.restoredEdits); + const listFailed = !!tab.error && !tab.notARepo; + const alarming = !!(tab.saveError || tab.fileError || listFailed || tab.externalChange || tab.restoredEdits); if (tab.remote) notes.push('Remote session — read-only.'); if (tab.fallbackReason) notes.push(`${tab.fallbackReason} — showing the diff read-only.`); if (tab.diffTruncated) notes.push('Diff truncated at 512 KB.'); if (tab.restoredEdits) notes.push('Unsaved edits kept from when the session opened something else in this panel have been restored.'); if (tab.externalChange) notes.push('This file changed on disk since you opened it — reload before saving, or your edits will not be accepted.'); if (tab.fileError) notes.push(`This file can no longer be read: ${tab.fileError}`); - if (tab.error) notes.push(`The file list could not be refreshed: ${tab.error}`); + if (tab.notARepo) notes.push(NOT_A_REPO_TEXT); + else if (tab.error) notes.push(`The file list could not be refreshed: ${tab.error}`); if (tab.saveError) notes.push(`Save failed: ${tab.saveError}`); changesDiffNoticeEl.textContent = notes.join(' '); diff --git a/test/dom-file-panel-changes.test.js b/test/dom-file-panel-changes.test.js index c22ff061..33516abf 100644 --- a/test/dom-file-panel-changes.test.js +++ b/test/dom-file-panel-changes.test.js @@ -2030,25 +2030,75 @@ test('a git failure after a no-repository answer is shown as a failure (mutation } finally { ctx.destroy(); } }); -test('a repository that disappears under unsaved edits asks nothing and keeps the buffer', async () => { +// The three ways the tree under an open editor can stop answering: the +// repository removed, the directory itself gone, and a repository git refuses. +const TREE_LOST_PAYLOADS = [ + ['the repository removed', noRepoStatus], + ['the working directory gone', () => ({ ok: false, error: 'the working directory no longer exists: /gone' })], + ['a repository git refuses', () => ({ ok: false, error: 'fatal: detected dubious ownership in repository at /srv/repo' })], +]; + +for (const [label, lostStatus] of TREE_LOST_PAYLOADS) { + test(`unsaved edits survive ${label}, and nothing is asked`, async () => { + let repo = true; + const ctx = setupFilePanelDom({ + confirmImpl: () => true, + statusImpl: () => (repo ? makeStatusResult() : lostStatus()), + }); + try { + await openFile(ctx, 's1', 'src/a.js'); + ctx.editors[0].box.text = 'my unsaved edit\n'; + + repo = false; + ctx.setActivity('s1', true); + ctx.setActivity('s1', false); + await flush(); + + assert.equal(ctx.calls.confirm.length, 0, 'nothing is being discarded, so nothing is asked'); + assert.equal(ctx.editors[0].box.destroyed, false); + assert.equal(ctx.editors[0].box.text, 'my unsaved edit\n'); + assert.equal(ctx.document.getElementById('file-panel').classList.contains('open'), true); + } finally { ctx.destroy(); } + }); +} + +test('the notice bar states the no-repository fact without borrowing the failure colour or its wording', async () => { + let repo = true; + const ctx = setupFilePanelDom({ statusImpl: () => (repo ? makeStatusResult() : noRepoStatus()) }); + try { + await openFile(ctx, 's1', 'src/a.js'); + + repo = false; + ctx.setActivity('s1', true); + ctx.setActivity('s1', false); + await flush(); + + const notice = ctx.document.getElementById('changes-diff-notice'); + assert.match(notice.textContent, /This directory is not a git repository\./); + assert.doesNotMatch(notice.textContent, /could not be refreshed/, + 'a directory that has no repository did not fail to refresh'); + assert.equal(notice.classList.contains('changes-error'), false, + 'the list below says this in a neutral note; the bar must not contradict it'); + assert.equal(notice.classList.contains('changes-diff-truncated'), true); + } finally { ctx.destroy(); } +}); + +test('a genuine refresh failure still reaches the notice bar in the failure colour', async () => { let repo = true; const ctx = setupFilePanelDom({ - confirmImpl: () => true, - statusImpl: () => (repo ? makeStatusResult() : noRepoStatus()), + statusImpl: () => (repo ? makeStatusResult() : { ok: false, error: 'could not read directory: Permission denied' }), }); try { await openFile(ctx, 's1', 'src/a.js'); - ctx.editors[0].box.text = 'my unsaved edit\n'; repo = false; ctx.setActivity('s1', true); ctx.setActivity('s1', false); await flush(); - assert.equal(ctx.calls.confirm.length, 0, 'nothing is being discarded, so nothing is asked'); - assert.equal(ctx.editors[0].box.destroyed, false); - assert.equal(ctx.editors[0].box.text, 'my unsaved edit\n'); - assert.equal(ctx.document.getElementById('file-panel').classList.contains('open'), true); + const notice = ctx.document.getElementById('changes-diff-notice'); + assert.match(notice.textContent, /The file list could not be refreshed: could not read directory/); + assert.equal(notice.classList.contains('changes-error'), true); } finally { ctx.destroy(); } });