-
Notifications
You must be signed in to change notification settings - Fork 196
fix(diff): open diffs when the Claude terminal is the only window #260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3205b4f
b92f195
604c461
a9df0b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1063,6 +1063,12 @@ function M._cleanup_diff_state(tab_name, reason) | |
| pcall(vim.api.nvim_win_close, diff_data.new_window, true) | ||
| end | ||
|
|
||
| -- Close the fallback window we created when no editor window existed (issue #231); it's reused | ||
| -- as the original pane, so target_window_created_by_plugin below doesn't cover it. | ||
| if diff_data.fallback_window and vim.api.nvim_win_is_valid(diff_data.fallback_window) then | ||
| pcall(vim.api.nvim_win_close, diff_data.fallback_window, false) | ||
| end | ||
|
|
||
| -- If we created an extra window/split for the diff, close it. Otherwise just disable diff mode. | ||
| if diff_data.target_window and vim.api.nvim_win_is_valid(diff_data.target_window) then | ||
| if diff_data.target_window_created_by_plugin then | ||
|
|
@@ -1132,6 +1138,12 @@ function M._setup_blocking_diff(params, resolution_callback) | |
| local tab_name = params.tab_name | ||
| logger.debug("diff", "Setting up diff for:", params.old_file_path) | ||
|
|
||
| -- Hoisted so the error handler can clean them up if setup fails before the diff state is | ||
| -- registered: otherwise the terminal-only fallback split and the proposed buffer are stranded | ||
| -- (the state-based cleanup is gated on a registered diff). Issue #231. | ||
| local fallback_window = nil | ||
| local new_buffer = nil | ||
|
|
||
| -- Wrap the setup in error handling to ensure cleanup on failure | ||
| local setup_success, setup_error = pcall(function() | ||
| local old_file_exists = vim.fn.filereadable(params.old_file_path) == 1 | ||
|
|
@@ -1197,17 +1209,26 @@ function M._setup_blocking_diff(params, resolution_callback) | |
| target_window = find_main_editor_window() | ||
| end | ||
| end | ||
| -- If created_new_tab is true, target_window stays nil and will be created in the new tab | ||
| -- If we still can't find a suitable window AND we're not in a new tab, error out | ||
| -- If created_new_tab is true, target_window stays nil and will be created in the new tab. | ||
| -- Otherwise, if no editor window is suitable (e.g. the Claude terminal is the only window -- | ||
| -- issue #231), create one by splitting the current window instead of erroring out, mirroring | ||
| -- the fallback in lua/claudecode/tools/open_file.lua. | ||
| if not target_window and not created_new_tab then | ||
| error({ | ||
| code = -32000, | ||
| message = "No suitable editor window found", | ||
| data = "Could not find a main editor window to display the diff", | ||
| }) | ||
| create_split() | ||
| local scratch_buf = vim.api.nvim_create_buf(false, true) -- unlisted, scratch | ||
| if scratch_buf ~= 0 then | ||
| -- wipe it once it leaves the window so it isn't leaked when the diff reuses it (new file) | ||
| -- or :edit replaces it with the real file (existing file) | ||
| vim.api.nvim_buf_set_option(scratch_buf, "bufhidden", "wipe") | ||
| vim.api.nvim_win_set_buf(vim.api.nvim_get_current_win(), scratch_buf) | ||
| end | ||
| target_window = vim.api.nvim_get_current_win() | ||
| -- Track it so _cleanup_diff_state closes it; the reused scratch buffer means it won't be | ||
| -- flagged target_window_created_by_plugin. | ||
| fallback_window = target_window | ||
|
claude[bot] marked this conversation as resolved.
|
||
| end | ||
|
|
||
| local new_buffer = vim.api.nvim_create_buf(false, true) -- unlisted, scratch | ||
| new_buffer = vim.api.nvim_create_buf(false, true) -- unlisted, scratch (hoisted above the pcall) | ||
| if new_buffer == 0 then | ||
| error({ | ||
| code = -32000, | ||
|
|
@@ -1253,6 +1274,7 @@ function M._setup_blocking_diff(params, resolution_callback) | |
| new_window = diff_info.new_window, | ||
| target_window = diff_info.target_window, | ||
| target_window_created_by_plugin = diff_info.target_window_created_by_plugin, | ||
| fallback_window = fallback_window, | ||
| original_buffer = diff_info.original_buffer, | ||
| original_buffer_created_by_plugin = diff_info.original_buffer_created_by_plugin, | ||
| original_cursor_pos = original_cursor_pos, | ||
|
|
@@ -1288,6 +1310,16 @@ function M._setup_blocking_diff(params, resolution_callback) | |
| -- Clean up any partial state that might have been created | ||
| if active_diffs[tab_name] then | ||
| M._cleanup_diff_state(tab_name, "setup failed") | ||
| else | ||
| -- Errored before the diff state was registered, so the state-based cleanup can't run. Close | ||
| -- the fallback split we may have created (its bufhidden=wipe scratch self-cleans) and delete | ||
| -- the proposed buffer; neither is owned by a registered diff. | ||
| if fallback_window and vim.api.nvim_win_is_valid(fallback_window) then | ||
| pcall(vim.api.nvim_win_close, fallback_window, true) | ||
| end | ||
| if new_buffer and vim.api.nvim_buf_is_valid(new_buffer) then | ||
| pcall(vim.api.nvim_buf_delete, new_buffer, { force = true }) | ||
| end | ||
| end | ||
|
ThomasK33 marked this conversation as resolved.
Comment on lines
+1313
to
1323
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Two more resources slip past the new pre-registration error handler at lua/claudecode/diff.lua:1313-1323, completing the symmetric pattern from 17626a3 / bf30c36 / #262: (1) Extended reasoning...The two missed resourcesThe pre-registration error handler added in 17626a3 and extended in bf30c36 currently handles (1)
|
||
|
|
||
| -- Re-throw the error for MCP compliance | ||
|
ThomasK33 marked this conversation as resolved.
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| -- Reproduction / verification for issue #231: | ||
| -- "When the Claude Code terminal is the only window (no other splits), an | ||
| -- error is generated when Claude tries to suggest changes." | ||
| -- https://github.com/coder/claudecode.nvim/issues/231 | ||
| -- | ||
| -- The bug: with a single `buftype=terminal` window, diff.lua's | ||
| -- find_main_editor_window() returns nil (it correctly excludes terminals). The | ||
| -- fix makes M._setup_blocking_diff create a split to host the diff instead of | ||
| -- erroring with "No suitable editor window found". | ||
| -- | ||
| -- This script drives the REAL diff.lua against a terminal-only layout, with no | ||
| -- WebSocket/Claude CLI needed. It exercises the exact code path the openDiff MCP | ||
| -- tool uses (M._setup_blocking_diff), so it both reproduces the original bug (on | ||
| -- unfixed code) and verifies the fix. | ||
| -- | ||
| -- Run from the repo root: | ||
| -- nvim --headless -u NONE -l scripts/repro_issue_231.lua | ||
| -- | ||
| -- Exit code: 0 if the diff opens (fixed), 1 if the #231 error is reproduced. | ||
| -- The detailed verdict is printed to stdout either way. | ||
|
|
||
| local script_path = debug.getinfo(1, "S").source:sub(2) | ||
| local repo_root = vim.fn.fnamemodify(script_path, ":h:h") | ||
| vim.opt.rtp:prepend(repo_root) | ||
|
|
||
| local function out(msg) | ||
| io.stdout:write(msg .. "\n") | ||
| end | ||
|
|
||
| local diff = require("claudecode.diff") | ||
|
|
||
| ---Make a `buftype=terminal` window the ONLY window (the issue #231 layout). | ||
| local function make_terminal_only_window() | ||
| vim.cmd("silent! only") | ||
| vim.cmd("enew!") | ||
| -- jobstart({term=true}) (Neovim 0.11+) / fallback to termopen on older versions. | ||
| if vim.fn.has("nvim-0.11") == 1 then | ||
| vim.fn.jobstart({ "cat" }, { term = true }) | ||
| else | ||
| vim.fn.termopen({ "cat" }) | ||
| end | ||
| vim.cmd("silent! only") | ||
| return #vim.api.nvim_list_wins(), vim.api.nvim_buf_get_option(0, "buftype") | ||
| end | ||
|
|
||
| ---Run M._setup_blocking_diff for a brand-new file and capture the outcome. | ||
| ---@return boolean ok, string detail | ||
| local function try_open_diff() | ||
| local new_file = repo_root .. "/__issue_231_repro__.md" | ||
| os.remove(new_file) -- ensure is_new_file = true (matches: Claude proposing a new file) | ||
| local ok, err = pcall(function() | ||
| diff._setup_blocking_diff({ | ||
| old_file_path = new_file, | ||
| new_file_path = new_file, | ||
| new_file_contents = "# Proposed by Claude\n\nhello\n", | ||
| tab_name = "✻ [Claude Code] __issue_231_repro__.md (445ca6) ⧉", | ||
| }, function() end) | ||
| end) | ||
| -- Best-effort cleanup of any windows/diff state the setup created. | ||
| pcall(function() | ||
| diff._cleanup_all_active_diffs("repro cleanup") | ||
| end) | ||
| if ok then | ||
| return true, "setup SUCCEEDED (a window was found or created)" | ||
| end | ||
| local msg = type(err) == "table" and (tostring(err.message) .. " - " .. tostring(err.data)) or tostring(err) | ||
| return false, msg | ||
| end | ||
|
|
||
| out("== issue #231 reproduction ==") | ||
| out(("Neovim: %s"):format(vim.version and tostring(vim.version()) or vim.fn.execute("version"):match("NVIM[^\n]*"))) | ||
|
|
||
| -- Scenario A: default diff_opts (open_in_new_tab = false) -- the path that regressed in #231. | ||
| -- This exercises the actual fix (find_main_editor_window -> nil -> create a split fallback). | ||
| diff.setup({ diff_opts = { layout = "vertical", open_in_new_tab = false } }) | ||
| local wins, bt = make_terminal_only_window() | ||
| out(("\n[A] default config | precondition: windows=%d, only buftype=%q"):format(wins, bt)) | ||
| local a_ok, a_detail = try_open_diff() | ||
| out(("[A] result: %s -> %s"):format(a_ok and "OK" or "ERROR", a_detail)) | ||
|
|
||
| -- Scenario B: open_in_new_tab = true -- a pre-existing WORKAROUND. NOTE: this does NOT exercise | ||
| -- the #231 fix path; the new-tab path creates its own window and never calls | ||
| -- find_main_editor_window, so it succeeds even on unfixed code. Included only to confirm the | ||
| -- documented workaround still works; scenario A is the real regression signal. | ||
| diff.setup({ diff_opts = { layout = "vertical", open_in_new_tab = true } }) | ||
| wins, bt = make_terminal_only_window() | ||
| out(("\n[B] open_in_new_tab=true | precondition: windows=%d, only buftype=%q"):format(wins, bt)) | ||
| local b_ok, b_detail = try_open_diff() | ||
| out(("[B] result: %s -> %s"):format(b_ok and "OK" or "ERROR", b_detail)) | ||
|
|
||
| out("\n== verdict ==") | ||
| local bug_reproduced = (not a_ok) and a_detail:match("No suitable editor window found") ~= nil | ||
| if bug_reproduced then | ||
| out("BUG REPRODUCED: default config errors with 'No suitable editor window found' (issue #231).") | ||
| else | ||
| out("FIXED: default config opens the diff in a terminal-only layout (scenario A).") | ||
| end | ||
| if b_ok then | ||
| out("WORKAROUND OK: diff_opts.open_in_new_tab=true opens the diff in a new tab (does not exercise the fix).") | ||
| else | ||
| out("NOTE: open_in_new_tab=true did NOT open the diff in this environment: " .. b_detail) | ||
| end | ||
|
|
||
| io.stdout:flush() | ||
| vim.cmd("cquit " .. (bug_reproduced and 1 or 0)) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| -- Regression test for issue #231: | ||
| -- "When the Claude Code terminal is the only window (no other splits), an | ||
| -- error is generated when Claude tries to suggest changes." | ||
| -- https://github.com/coder/claudecode.nvim/issues/231 | ||
| -- | ||
| -- Before the fix, M._setup_blocking_diff errored with "No suitable editor window | ||
| -- found" whenever find_main_editor_window() returned nil (e.g. the only window | ||
| -- is a terminal). The fix creates a split + fresh buffer to host the diff, tracks | ||
| -- that window as `fallback_window`, and closes it on cleanup so it is not leaked. | ||
| require("tests.busted_setup") | ||
|
|
||
| -- Build a consistent mock window model where the ONLY window (1000) is a terminal, | ||
| -- so find_main_editor_window() returns nil -- the issue #231 layout. (_next_winid is | ||
| -- advanced past 1000 so create_split() allocates fresh window ids without colliding.) | ||
| local function reset_to_terminal_only() | ||
| assert(vim and vim._mock and vim._mock.reset, "expected vim mock with _mock.reset()") | ||
|
|
||
| vim._mock.reset() | ||
| vim._tabs = { [1] = true } | ||
| vim._current_tabpage = 1 | ||
| vim._current_window = 1000 | ||
| vim._next_winid = 1001 | ||
|
|
||
| vim._mock.add_buffer(1, "term://fake/claude", "", { buftype = "terminal", modified = false }) | ||
| vim._mock.add_window(1000, 1, { 1, 0 }) | ||
| vim._win_tab[1000] = 1 | ||
| vim._tab_windows[1] = { 1000 } | ||
| end | ||
|
|
||
| describe("Diff with the Claude terminal as the only window (issue #231)", function() | ||
| local diff | ||
|
|
||
| before_each(function() | ||
| reset_to_terminal_only() | ||
|
|
||
| package.loaded["claudecode.logger"] = { | ||
| debug = function() end, | ||
| error = function() end, | ||
| info = function() end, | ||
| warn = function() end, | ||
| } | ||
|
|
||
| package.loaded["claudecode.diff"] = nil | ||
| diff = require("claudecode.diff") | ||
|
|
||
| diff.setup({ | ||
| diff_opts = { | ||
| layout = "vertical", | ||
| open_in_new_tab = false, -- default: the path that used to error | ||
| keep_terminal_focus = false, | ||
| on_new_file_reject = "keep_empty", | ||
| }, | ||
| terminal = {}, | ||
| }) | ||
| end) | ||
|
|
||
| after_each(function() | ||
| if diff and diff._cleanup_all_active_diffs then | ||
| diff._cleanup_all_active_diffs("test teardown") | ||
| end | ||
| package.loaded["claudecode.diff"] = nil | ||
| package.loaded["claudecode.logger"] = nil | ||
| end) | ||
|
|
||
| it("has no suitable editor window in this layout (root-cause precondition)", function() | ||
| expect(diff._find_main_editor_window()).to_be(nil) | ||
| end) | ||
|
|
||
| it("creates a window for the diff instead of erroring, then cleans it up (new file)", function() | ||
| local tab_name = "✻ [Claude Code] INSTALL.md (445ca6) ⧉" | ||
| local params = { | ||
| old_file_path = "/nonexistent/INSTALL.md", -- new file (matches the issue) | ||
| new_file_path = "/nonexistent/INSTALL.md", | ||
| new_file_contents = "# Install\n\nproposed by Claude\n", | ||
| tab_name = tab_name, | ||
| } | ||
|
|
||
| -- The regression: this used to raise "No suitable editor window found". | ||
| local setup_ok, setup_err = pcall(function() | ||
| diff._setup_blocking_diff(params, function() end) | ||
| end) | ||
| assert.is_true(setup_ok, "diff setup should not error in a terminal-only layout: " .. tostring(setup_err)) | ||
|
|
||
| local state = diff._get_active_diffs()[tab_name] | ||
| assert.is_table(state) | ||
| assert.is_true(vim.api.nvim_buf_is_valid(state.new_buffer)) | ||
|
|
||
| -- The plugin had to create a window to host the diff (none existed). It must be recorded as | ||
| -- `fallback_window` (distinct from the terminal) so cleanup can close it. | ||
| assert.is_number(state.fallback_window) | ||
| assert.are_not.equal(1000, state.fallback_window) | ||
| assert.is_true(vim.api.nvim_win_is_valid(state.fallback_window)) | ||
| local fallback_buf = vim.api.nvim_win_get_buf(state.fallback_window) | ||
|
|
||
| -- Cleanup must close the plugin-created fallback window (leaving the terminal, 1000) and wipe | ||
| -- its throwaway scratch buffer. Regression guard for the window + buffer leak (the host window | ||
| -- was left open and the scratch buffer left behind on every terminal-only diff). | ||
| diff._cleanup_diff_state(tab_name, "test cleanup") | ||
| assert.is_false(vim.api.nvim_win_is_valid(state.fallback_window)) | ||
| assert.is_false(vim.api.nvim_buf_is_valid(fallback_buf)) | ||
| assert.is_true(vim.api.nvim_win_is_valid(1000)) | ||
| end) | ||
|
|
||
| -- If setup errors after the fallback window + proposed buffer are created but before the diff | ||
| -- state is registered, the error handler must still clean them up (not covered by state cleanup). | ||
| it("cleans up the fallback window and proposed buffer when setup errors before registration", function() | ||
| -- Force a failure after the fallback split is created (winid 1001) but before registration. | ||
| diff._create_diff_view_from_window = function() | ||
| error({ code = -32000, message = "boom" }) | ||
| end | ||
|
|
||
| local bufs_before = #vim.api.nvim_list_bufs() | ||
| local tab_name = "✻ [Claude Code] err.md ⧉" | ||
| local ok = pcall(function() | ||
| diff._setup_blocking_diff({ | ||
| old_file_path = "/nonexistent/err.md", | ||
| new_file_path = "/nonexistent/err.md", | ||
| new_file_contents = "x\n", | ||
| tab_name = tab_name, | ||
| }, function() end) | ||
| end) | ||
|
|
||
| assert.is_false(ok) -- setup is expected to fail | ||
| assert.is_nil(diff._get_active_diffs()[tab_name]) -- no diff state registered | ||
| assert.is_false(vim.api.nvim_win_is_valid(1001)) -- the stranded fallback split was closed | ||
| assert.is_true(vim.api.nvim_win_is_valid(1000)) -- the terminal window survives | ||
| assert.equals(bufs_before, #vim.api.nvim_list_bufs()) -- proposed buffer + scratch not leaked | ||
| end) | ||
| end) |
Uh oh!
There was an error while loading. Please reload this page.