Skip to content

fix(diff): user-initiated accept/reject should clean up windows and buffers#241

Open
mightreya wants to merge 1 commit into
coder:mainfrom
mightreya:fix/user-resolve-cleanup
Open

fix(diff): user-initiated accept/reject should clean up windows and buffers#241
mightreya wants to merge 1 commit into
coder:mainfrom
mightreya:fix/user-resolve-cleanup

Conversation

@mightreya

Copy link
Copy Markdown

Problem

:ClaudeCodeDiffAccept and :ClaudeCodeDiffDeny (the accept_current_diff / deny_current_diff user commands) leave the diff buffers, split windows, and active_diffs state behind after resolving. Users have to close the leftover windows and buffers by hand.

This shows up as the symptom in #205 and as the partial behavior described in #238 (rejecting via :q closes one buffer but does nothing else).

Root cause

The two user commands call _resolve_diff_as_saved / _resolve_diff_as_rejected, which only set the diff status and resume the MCP coroutine. The actual UI cleanup lives in _cleanup_diff_state and is triggered exclusively by the close_tab MCP tool.

But close_tab has schema = nil and is documented as "Internal tool — must remain as requested by user" (see lua/claudecode/tools/close_tab.lua). The Claude CLI never invokes it. So when a user resolves a diff via the bundled commands, cleanup never fires.

This was less visible before #175 because the cleanup function did less work; with #175 expanding cleanup to close plugin-created splits and force-delete buffers, the gap between "resolve" and "cleanup" became user-facing as accumulating diff windows.

Fix

Have the user commands call _cleanup_diff_state themselves. They're not waiting for an MCP response from the CLI — they're driven by the user — so the "wait for close_tab" architecture doesn't apply to them.

This PR does not touch close_tab.lua. Its schema = nil is left intact per the existing comment.

It also extends diff_opts.keep_terminal_focus to apply at user-resolve time, not only at diff-open time, so focus returns to the Claude terminal after accept/reject.

Tests

Adds tests/unit/diff_user_command_cleanup_spec.lua covering:

  • accept_current_diff closes the diff window and removes the entry from active_diffs
  • deny_current_diff closes the diff window and removes the entry from active_diffs
  • accept_current_diff is a safe no-op when called outside a diff buffer

I was unable to run make test locally (the Nix-based dev shell wasn't available on my machine). I confirmed Lua syntax via loadfile. Please run the suite in CI / locally before merging.

Closes / refs

…uffers

The user commands :ClaudeCodeDiffAccept and :ClaudeCodeDiffDeny call
_resolve_diff_as_saved/_rejected, which only mark the diff status. Real
cleanup (close windows, force-delete buffers, remove from active_diffs)
is performed by _cleanup_diff_state, currently triggered exclusively by
the close_tab MCP tool.

But close_tab has schema = nil (it is intentionally internal-only, as
documented in the source), so the Claude CLI never invokes it. The result
is that user-initiated accept/reject leaves the diff windows and buffers
behind, requiring manual cleanup.

The fix: have the user commands call _cleanup_diff_state themselves,
since they are not waiting for an MCP response from the CLI. Also
restore terminal focus when diff_opts.keep_terminal_focus is set
(previously this only applied at diff-open time).

Adds unit tests covering both commands and the no-marker no-op case.

Closes coder#205, partially addresses coder#238.
@ThomasK33

Copy link
Copy Markdown
Member

Note

This triage report is AI-generated using Claude Code

This PR was looked at while triaging #205. The cleanup it adds is a genuine improvement, so this is not a request to close the PR — but the evidence gathered during triage indicates that #205 would not actually be resolved by this change, so decoupling the Closes #205 link is worth considering.

The change itself looks reasonable

Having :ClaudeCodeDiffAccept / :ClaudeCodeDiffDeny (accept_current_diff / deny_current_diff) call _cleanup_diff_state directly is a sensible fix: when a diff is resolved through those user commands, the windows, buffers, and active_diffs entry should be torn down rather than left behind. Extending keep_terminal_focus to resolve-time is a nice touch too. This overlaps the user-resolve / :q family described in #238.

The premise behind "Closes #205" does not match the observed behaviour

The PR describes the root cause as close_tab being internal-only and therefore never invoked by the Claude client. That was checked behaviourally during triage and does not hold.

A reproduction was set up with Neovim running the plugin (terminal.provider = "none", matching the two-pane layout) and a Claude session connected to it over the plugin's WebSocket server. The plugin's own MCP tool dispatch was instrumented to record every tools/call request received from the connected client. With an edit made and then accepted in the editor, the recorded requests were:

→ closeAllDiffTabs
→ getDiagnostics
→ openDiff     tab_name = "✻ [Claude Code] demo.txt (f59b63) ⧉"
→ close_tab    tab_name = "✻ [Claude Code] demo.txt (f59b63) ⧉"
→ close_tab    tab_name = "✻ [Claude Code] demo.txt (f59b63) ⧉"
→ getDiagnostics

So the connected client does send close_tab — carrying the same tab_name as the preceding openDiff — as soon as the diff resolves, plus a closeAllDiffTabs at the start of the turn. Independently, the plugin's tracked-diff count went 1 → 0 after acceptance; a diff in the saved state is removed only by a close_tab, which corroborates that the request arrived and was acted upon.

In other words, in the connected case the diff teardown already happens over the protocol. The immediate cleanup this PR adds is valuable mainly as a safety net for paths where that round-trip does not occur — for example a diff resolved via the user commands when no client will follow up with close_tab.

Why #205 specifically would remain

Across the scenarios that could be exercised in a headless harness — including the reporter's plugin commit with their Neovim version — diffs were torn down correctly: both the internal window state and the on-screen content were correct, whether or not Neovim held focus. The #205 symptom is a previous diff remaining painted on an unfocused split pane until focus returns, which points at a terminal-side repaint behaviour (consistent with the Ghostty split-pane repaint reports linked on #205). Since this PR changes when internal cleanup runs but adds no redraw, it would not change what an unfocused pane displays.

Suggestion

The fix is worth keeping. Re-scoping it to the user-command / #238 cleanup and dropping the Closes #205 reference would be more accurate, leaving #205 to be confirmed as a terminal-side (Ghostty) rendering issue. Full triage details, including the deterministic reproduction scripts, are on #205.

fixture used for the protocol observation (provider=none + env-gated MCP-call log)

The diff lifecycle was observed by pointing a connected Claude session at this fixture and logging the tools/call requests the plugin's own server received ($ISSUE205_TRACE). No edit was made to the plugin's behaviour; the hook only records inbound calls.

-- Fixture for issue #205:
--   "[BUG] Diff panels not cleared when new suggestion is shown — stale diffs
--    accumulate until window focus change"
--   https://github.com/coder/claudecode.nvim/issues/205
--
-- Reporter setup: Ghostty tab split into two panes — Claude Code CLI in one,
-- Neovim (LazyVim) in the other. Claude connects to the plugin's WebSocket
-- server (so terminal.provider = "none": Claude lives OUTSIDE Neovim). When
-- Claude proposes a second change, the previous diff panes stay on screen and
-- only disappear once the user focuses the Neovim pane.
--
-- This fixture mirrors that: provider="none", auto_start, plus a window-state
-- probe so a script can compare Neovim's INTERNAL window state against what is
-- actually rendered on screen (the focus-dependent symptom):
--   nvim --server <sock> --remote-expr 'v:lua.Repro205State()'
--   nvim --server <sock> --remote-expr 'v:lua.Repro205DiffWinCount()'
--
-- Usage (from repo root):
--   source fixtures/nvim-aliases.sh && vv issue-205
-- or scripted:
--   scripts/repro_issue_205.sh

local config_dir = vim.fn.stdpath("config")
local repo_root = vim.fn.fnamemodify(config_dir, ":h:h")
vim.opt.rtp:prepend(repo_root)

vim.g.mapleader = " "
vim.g.maplocalleader = "\\"

-- ---------------------------------------------------------------------------
-- claudecode.nvim (dev version from this repo)
-- ---------------------------------------------------------------------------
local ok, claudecode = pcall(require, "claudecode")
assert(ok, "Failed to load claudecode.nvim from repo root: " .. tostring(claudecode))

claudecode.setup({
  auto_start = true, -- server + lock file immediately, so scripts can connect
  -- "warn", not "debug": multi-line debug echoes trip nvim's hit-enter prompt,
  -- which blocks --remote-expr probes in the scripted repro.
  log_level = "warn",
  terminal = {
    -- Reporter runs Claude in a SEPARATE Ghostty pane, not inside Neovim.
    provider = "none",
  },
})

vim.o.showtabline = 2
vim.o.laststatus = 2

-- ---------------------------------------------------------------------------
-- Optional MCP trace: when $ISSUE205_TRACE is a writable path, log every
-- tools/call the connected client makes (name + tab_name) to that file. Used to
-- verify empirically whether the real Claude CLI sends `close_tab` after a diff
-- resolves (PR #241 claims it does not). Wrapping the module field works because
-- the server looks up handle_invoke on the shared tools table each dispatch.
-- ---------------------------------------------------------------------------
do
  local trace_path = vim.env.ISSUE205_TRACE
  if trace_path and trace_path ~= "" then
    -- Use the EXACT module key the server uses ("claudecode.tools.init"); Lua
    -- caches "claudecode.tools" and "claudecode.tools.init" as separate tables,
    -- so wrapping the wrong one is a silent no-op.
    local ok_tools, tools = pcall(require, "claudecode.tools.init")
    if ok_tools and type(tools.handle_invoke) == "function" then
      local orig = tools.handle_invoke
      tools.handle_invoke = function(client, params)
        pcall(function()
          local name = params and params.name or "?"
          local tab = params and params.arguments and params.arguments.tab_name or ""
          local fh = io.open(trace_path, "a")
          if fh then
            fh:write(string.format("%d\t%s\t%s\n", vim.fn.localtime(), name, tab))
            fh:close()
          end
        end)
        return orig(client, params)
      end
    end
  end
end

-- ---------------------------------------------------------------------------
-- Window-state probe (for --remote-expr / on-screen verification)
-- ---------------------------------------------------------------------------

---Compact state of every window across all tabpages.
---@return string JSON: [{win,tab,name,buftype,diff}...]
function _G.Repro205State()
  local out = {}
  for _, win in ipairs(vim.api.nvim_list_wins()) do
    local buf = vim.api.nvim_win_get_buf(win)
    local name = vim.api.nvim_buf_get_name(buf)
    out[#out + 1] = {
      win = win,
      tab = vim.api.nvim_tabpage_get_number(vim.api.nvim_win_get_tabpage(win)),
      name = vim.fn.fnamemodify(name, ":t") ~= "" and vim.fn.fnamemodify(name, ":~:.") or "[No Name]",
      buftype = vim.bo[buf].buftype,
      diff = vim.wo[win].diff,
    }
  end
  return vim.json.encode(out)
end

---WebSocket endpoint of the running claudecode server ("port token", or "" if
---not started yet). Lets scripts connect without scanning ~/.claude/ide.
---@return string
function _G.Repro205Server()
  local cc = require("claudecode")
  if cc.state.port and cc.state.auth_token then
    return cc.state.port .. " " .. cc.state.auth_token
  end
  return ""
end

---Count of windows currently in diff mode (quick assertion helper).
---@return integer
function _G.Repro205DiffWinCount()
  local n = 0
  for _, win in ipairs(vim.api.nvim_list_wins()) do
    if vim.wo[win].diff then
      n = n + 1
    end
  end
  return n
end

---Number of distinct diffs the plugin is actively tracking.
---@return integer
function _G.Repro205ActiveDiffs()
  local ok_diff, diff = pcall(require, "claudecode.diff")
  if not ok_diff or not diff._get_active_diffs then
    return -1
  end
  local n = 0
  for _ in pairs(diff._get_active_diffs()) do
    n = n + 1
  end
  return n
end

---Accept every still-pending diff (mirrors the user pressing :w / accepting in
---the IDE). Returns FILE_SAVED to the connected Claude client, which then sends
---close_tab. Used by the scripted end-to-end repro so diffs don't block forever.
---@return integer accepted Number of diffs accepted
function _G.Repro205AcceptAll()
  local ok_diff, diff = pcall(require, "claudecode.diff")
  if not ok_diff or not diff._get_active_diffs then
    return 0
  end
  local accepted = 0
  for tab_name, diff_data in pairs(diff._get_active_diffs()) do
    if diff_data.status == "pending" and diff_data.new_buffer and vim.api.nvim_buf_is_valid(diff_data.new_buffer) then
      pcall(diff._resolve_diff_as_saved, tab_name, diff_data.new_buffer)
      accepted = accepted + 1
    end
  end
  return accepted
end

vim.api.nvim_create_user_command("ReproState", function()
  vim.notify(_G.Repro205State())
end, { desc = "Show issue-205 window state" })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Diff panels not cleared when new suggestion is shown — stale diffs accumulate until window focus change

2 participants