fix(diff): reject diffs on window close so :q works#266
Conversation
|
@codex review |
|
@claude review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The proposed ("new") diff buffer is created as a scratch buffer
(bufhidden="hide") and rejection was wired only to BufDelete/BufUnload/
BufWipeout. Running :q merely HIDES that still-loaded buffer, so none of those
autocmds fire and the diff is never resolved — Claude never receives
DIFF_REJECTED and (with open_in_new_tab=true) the diff tab lingers. This
affects both layouts; the README-documented ":q to reject" never actually
worked. (:ClaudeCodeDiffDeny works because it calls _resolve_diff_as_rejected
directly, bypassing the autocmds.)
Add a WinClosed autocmd in register_diff_autocmds that resolves the diff as
rejected when the proposed window closes (:q, :close, <C-w>c, tab close). It is
registered window-wide (no buffer=/pattern=) and rejects only once the proposed
buffer is displayed in no window — excluding the just-closing window via
args.match — so splitting the buffer and closing one clone does not reject
prematurely, and concurrent diffs stay isolated. It is idempotent against the
existing Buf* autocmds and teardown via the status ~= "pending" guard, and its
id is tracked in autocmd_ids so _cleanup_diff_state tears it down.
Verification/tooling:
- tests/unit/diff_reject_on_quit_spec.lua: mock-vim unit test of the handler
(reject-when-not-visible, last-window, split no-reject, unrelated-window
no-reject, accept no-op, invalid-buffer guard, multi-diff isolation,
autocmd-id cleanup, window-wide registration).
- scripts/repro_issue_238.lua: headless real-nvim regression gate (same-tab,
new-tab, multi-window split, new-file) — wired into the integration-tests CI
job (self-reports via cquit).
- scripts/repro_issue_238.sh + fixtures/issue-238/: manual MCP/websocat repro.
Fixes #238
Change-Id: I5256ec4726daa8ba43f1b46ce8b95254bfa289fc
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Thomas Kosiewski <tk@coder.com>
ThomasK33
force-pushed
the
fix/issue-238-reject-with-q
branch
from
70e9bbe to
de68a85
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| autocmd_ids[#autocmd_ids + 1] = vim.api.nvim_create_autocmd("WinClosed", { | ||
| group = get_autocmd_group(), | ||
| callback = function(args) | ||
| if not vim.api.nvim_buf_is_valid(new_buffer) then | ||
| return | ||
| end | ||
| local closing_win = tonumber(args.match) | ||
| local still_visible = false | ||
| for _, win in ipairs(vim.fn.win_findbuf(new_buffer)) do | ||
| if win ~= closing_win then | ||
| still_visible = true | ||
| break | ||
| end | ||
| end | ||
| if not still_visible then | ||
| M._resolve_diff_as_rejected(tab_name) | ||
| end | ||
| end, | ||
| }) |
There was a problem hiding this comment.
🔴 The new WinClosed handler at lua/claudecode/diff.lua:883-901 falsely rejects pending diffs whenever the proposed buffer was previously navigated away from (e.g. via :e other_file, :b other_buf, or a file picker like snacks/telescope opening a file in the proposed window). Because the proposed buffer is a scratch buffer (bufhidden="hide"), navigating away merely hides it — none of BufDelete/BufUnload/BufWipeout fire, and the diff stays status="pending". Then any later unrelated WinClosed in the editor (a help split, a sidebar, a tabclose) makes win_findbuf(new_buffer) return {}, the still_visible check fails, and _resolve_diff_as_rejected sends a spurious DIFF_REJECTED to Claude — and on the next close_tab the proposed buffer carrying Claude'''s edits is silently wiped. Fix: track whether the closing window itself was showing the proposed buffer, and reject only when it was AND no other window is.
Extended reasoning...
The bug
The WinClosed autocmd registered in register_diff_autocmds (lua/claudecode/diff.lua:883-901) is intentionally window-wide (no buffer=/pattern= scope) — every WinClosed anywhere in the editor invokes the callback for every active diff. The handler is supposed to reject the diff iff the close was the proposed buffer'''s last visible window. But the check it implements — "after excluding args.match, is the buffer still in any window?" — conflates two scenarios that win_findbuf reports identically when the result is {}:
- (a) The closing window was the last one showing the proposed buffer (legitimate reject).
- (b) The proposed buffer was already in no window because the user navigated away earlier (must not reject — the close has nothing to do with this diff).
The handler'''s own comment at diff.lua:880 says "WinClosed fires BEFORE the closing window leaves the layout, so we exclude it (args.match)". That nails down case (a)'''s semantics: for a legitimate last-window close, win_findbuf(new_buffer) returns {closing_win}. So win_findbuf returning {} is exclusively case (b) — the bug path.
How it triggers
The proposed buffer is created at diff.lua:1267 via vim.api.nvim_create_buf(false, true). The scratch flag sets bufhidden="hide", and the subsequent buf_set_option("buftype", "acwrite") does not change bufhidden — the PR'''s own fixture (fixtures/issue-238/init.lua) explicitly captures bufhidden and confirms "hide". So when the user runs :e other_file / :b other_buf in the proposed window, or a file picker (snacks-picker default, common telescope/fzf-lua configs) opens a file into the current window, the proposed buffer becomes hidden but stays loaded. None of BufDelete/BufUnload/BufWipeout fires; the diff stays status="pending". Then any unrelated WinClosed anywhere triggers the spurious rejection.
Step-by-step proof
- User opens a Claude diff. The proposed buffer is buffer 42, scratch,
bufhidden="hide", displayed in window 1001. Diff state:status="pending". - User opens a file picker that opens into the current window (e.g. snacks-picker, telescope
select_default), or just runs:b some_other_bufferin window 1001. Window 1001 now shows a different buffer; buffer 42 is hidden but still loaded. No autocmd fires; diff status stays "pending". - Later, the user closes any unrelated window — say a help window with id 9999.
WinClosedfires withargs.match="9999". - Inside the callback (the active diff'''s registered WinClosed):
nvim_buf_is_valid(42)→true(still loaded, just hidden).closing_win = 9999.vim.fn.win_findbuf(42)→{}(buffer 42 is shown in zero windows).- The
forloop body never executes;still_visibleremainsfalse. if not still_visible then M._resolve_diff_as_rejected(tab_name)runs.
_resolve_diff_as_rejectedseesstatus=="pending"and proceeds: it builds aDIFF_REJECTEDpayload and resumes the deferred coroutine with it. Claude is told the diff was rejected. On the nextclose_tabcleanup,_cleanup_diff_statewipes buffer 42 — silently destroying Claude'''s proposed edits.
Why the existing guards do not catch this
- The
status~="pending"guard inside_resolve_diff_as_rejecteddoesn'''t fire — status is still "pending" (no other resolution happened). nvim_buf_is_valid(new_buffer)only checks existence; it returnstruefor hidden-but-loaded buffers.- The PR has no other
BufHidden/BufWinLeaveautocmd to mark the diff as user-abandoned.
Why this snuck past the tests
The first unit test in tests/unit/diff_reject_on_quit_spec.lua ("rejects the diff when the proposed window closes and the buffer is no longer visible", lines 80–92) locks in this bug. It mocks win_findbuf={} while args.match="1001" and asserts rejection. But per the comment on diff.lua:880 (which the very next test cites: "WinClosed fires BEFORE the window leaves the layout, so win_findbuf still lists the closing window"), the legitimate last-window-close case is the one the second test models — win_findbuf={1001}. win_findbuf={} only models case (b) — the user-navigated-away scenario — and the test asserts that this rejects. So the test name implies case (a) but the mock encodes case (b), and that case (b) is precisely the bug.
Fix
Track whether the closing window itself was showing the proposed buffer, and reject only when it was AND no other window is:
callback = function(args)
if not vim.api.nvim_buf_is_valid(new_buffer) then return end
local closing_win = tonumber(args.match)
local was_in_closing, still_visible_elsewhere = false, false
for _, win in ipairs(vim.fn.win_findbuf(new_buffer)) do
if win == closing_win then
was_in_closing = true
else
still_visible_elsewhere = true
end
end
if was_in_closing and not still_visible_elsewhere then
M._resolve_diff_as_rejected(tab_name)
end
end,Equivalently, check vim.api.nvim_win_get_buf(closing_win) == new_buffer while the window still exists (WinClosed fires before layout change, so it does). Either approach makes the spurious-close case (b) a no-op while preserving the legitimate case (a) — and the existing tests for split-still-visible and unrelated-window-no-reject keep passing. The first unit test should then be updated to model the legitimate case (win_findbuf={1001}) and asserts the same rejection.
| autocmd_ids[#autocmd_ids + 1] = vim.api.nvim_create_autocmd("WinClosed", { | ||
| group = get_autocmd_group(), | ||
| callback = function(args) | ||
| if not vim.api.nvim_buf_is_valid(new_buffer) then | ||
| return | ||
| end | ||
| local closing_win = tonumber(args.match) | ||
| local still_visible = false | ||
| for _, win in ipairs(vim.fn.win_findbuf(new_buffer)) do | ||
| if win ~= closing_win then | ||
| still_visible = true | ||
| break | ||
| end | ||
| end | ||
| if not still_visible then | ||
| M._resolve_diff_as_rejected(tab_name) | ||
| end | ||
| end, | ||
| }) |
There was a problem hiding this comment.
🟣 Auto-teardown on client disconnect (close_diffs_for_client at diff.lua:1601) and on server stop (close_pending_diffs at diff.lua:1587) only match status=="pending", so a rejected-but-not-yet-cleaned-up diff is skipped — if Claude disconnects in the window between DIFF_REJECTED being sent and close_tab being received, the new tab + proposed buffer linger until VimLeavePre. However, this is a pre-existing design choice, not a regression introduced by this PR: deny_current_diff at diff.lua:1613-1624 has always called _resolve_diff_as_rejected directly with no UI cleanup, so a user pressing <leader>ad and then having Claude disconnect already hit the same orphan tab pre-PR. The PR makes :q consistent with <leader>ad; it does not change the disconnect-cleanup design. A separate fix relaxing the filters to status ~= "saved" would help all three rejection paths equally.
Extended reasoning...
Analysis
The synthesis describes a real behavioral gap: after this PR, :q synchronously transitions a diff to status="rejected" via the new WinClosed handler (diff.lua:883-901), but the auto-teardown filters in close_diffs_for_client (diff.lua:1601) and close_pending_diffs (diff.lua:1587) only match status=="pending". If Claude disconnects after :q but before sending close_tab, the rejected diff is skipped and the new tab + proposed buffer linger until VimLeavePre. The reporters terminal.provider="none" config makes the trigger plausible: user types :q, then closes the external Claude CLI before Claudes runtime can respond with close_tab.
Why this is pre-existing, not a regression
The refuting verifier makes a load-bearing point: deny_current_diff (diff.lua:1613-1624) has always called _resolve_diff_as_rejected directly and left UI cleanup to Claude sending close_tab. The functions explicit comment is "Do not close windows/tabs here; just mark as rejected." So pre-PR, a user pressing <leader>ad / :ClaudeCodeDiffDeny and then having Claude disconnect already hit exactly the same orphan-tab scenario the synthesis describes. The disconnect filter excluding "rejected" diffs is a pre-existing design choice, not anything this PR introduced.
The synthesis frames this as a regression by comparing against pre-PR :q, which left status="pending" precisely because it failed to reject — the very bug this PR fixes. Restoring that "accidental cleanup" path would mean re-breaking #238. This PR is purely additive on top of an existing state machine: it makes :q honor the same rejection contract <leader>ad already did, including the same disconnect gap.
Step-by-step proof (pre-existing reachability via <leader>ad)
- With this PR or without it, configure
terminal.provider="none",diff_opts.open_in_new_tab=true. - Claude calls
openDiff→ new tab with original + proposed windows. Status is "pending". - User presses
<leader>ad→deny_current_diff→_resolve_diff_as_rejected→ status="rejected", DIFF_REJECTED sent. - Before Claude sends
close_tab, user closes the external Claude CLI (a natural action). - WebSocket drops →
close_diffs_for_clientruns → filter at diff.lua:1601 requiresstatus=="pending"→ skips the rejected diff. - New tab + proposed buffer linger until VimLeavePre.
This reproduces identically on main pre-PR, demonstrating the bug is in the disconnect-cleanup filter design (or in deny_current_diff/_resolve_diff_as_rejected not eagerly cleaning up), not in the PRs new WinClosed handler.
Impact and fix
The fix described in the synthesis (relax filters to status ~= "saved", or explicitly include "rejected") would correctly address the underlying issue and benefit all three rejection paths (:w-rollback edge cases aside — the comment at diff.lua:1592-1597 only justifies excluding "saved" to preserve user :w-edits Claude hasnt written yet; "rejected" carries no accepted content). But that fix belongs to a separate PR addressing the pre-existing design, not this one. Filing it as a blocking issue against this PR overstates the change: this PR did not introduce the gap, it only made one more entry point reach it.
Flagging as pre-existing so the maintainer can prioritize the disconnect-filter fix independently if desired, without blocking the #238 fix.
1 participant
Summary
Rejecting a Claude diff with
:qdid nothing — the change was never rejected and Claude was never told (DIFF_REJECTEDwas never sent), so the proposal stayed pending and, withopen_in_new_tab = true, the tab lingered. The<leader>ad/:ClaudeCodeDiffDenypath worked, but the README-documented:qdid not. Fixes #238.Root cause
In
lua/claudecode/diff.lua, the proposed ("new") diff buffer is created as a scratch buffer (vim.api.nvim_create_buf(false, true)⇒bufhidden = "hide") and kept modified, and rejection was wired only to buffer-destruction autocmds —BufDelete/BufUnload/BufWipeout→_resolve_diff_as_rejected. Becausebufhidden = "hide",:qmerely hides the still-loaded buffer instead of destroying it, so none of those autocmds fire and the diff is never resolved. This affects bothopen_in_new_tablayouts (in same-tab mode the split just silently collapses), and it never worked — it's not a regression.:ClaudeCodeDiffDenyworks only because it calls_resolve_diff_as_rejecteddirectly, bypassing the autocmds.A naive
bufhidden = "wipe"does not fix it: the buffer is modified, so:qthen aborts withE37: No write since last changeand the window never closes.Fix
Add a
WinClosedautocmd inregister_diff_autocmdsthat resolves the diff as rejected when the proposed window closes (:q,:close,<C-w>c, tab close). It is:buffer=/pattern=) and rejects only once the proposed buffer is displayed in no window — excluding the just-closing window viaargs.match— so splitting the buffer (<C-w>v) and closing one clone does not reject prematurely, and concurrent diffs stay isolated;Buf*autocmds and against teardown, via the existingstatus ~= "pending"guard;autocmd_ids, so_cleanup_diff_state(which deletes autocmds before closing windows) tears it down.This is the same event other Neovim AI-diff plugins use for "user closed the proposed window → reject". Production change is +36 lines, purely additive.
Test plan
tests/unit/diff_reject_on_quit_spec.lua(mock vim, runs in CI): reject-when-not-visible, last-window, split-no-reject, unrelated-window-no-reject, accept-no-op, invalid-buffer guard, multi-diff isolation, autocmd-id cleanup, and window-wide registration.scripts/repro_issue_238.lua— drives a genuine:qagainst real Neovim across four scenarios (same-tab, new-tab, multi-window split, new file) and self-reports viacquit(exit 0 = fixed, 1 = regressed). Verified exit 1 onmain, exit 0 with this change. Wired into theintegration-testsCI job.scripts/repro_issue_238.sh+fixtures/issue-238/— play the role of Claude over the WebSocket and reject in Neovim.mise run allis green (luacheck + treefmt + full suite, 532/0).See the triage report on #238 for the full reproduction (agent-driven real-Neovim + WebSocket) and the investigation notes.
🤖 Generated with Claude Code