Skip to content

Conversation

@ThomasK33
Copy link
Member

Fixes #155.

This PR prevents leftover diff splits by tracking which windows are created by the diff UI and cleaning them up deterministically when close_tab is invoked.

Summary

  • Track when the original-side window was created via an automatic split, and close it during cleanup.
  • When open_diff_blocking() is called with an existing tab_name, resolve/cleanup the prior diff state to avoid leaked windows.
  • Add unit tests for both the split-created and reuse-existing-window cases.
  • Update docs to prefer modern diff_opts keys and clarify legacy aliases.

Testing

  • make check / make test could not be run in this environment because luacheck / busted are not installed.
  • Lua syntax checks were run for lua/ and tests/.

📋 Implementation Plan

Plan: Fix issue #155 (diff splits left behind)

Context / Why

Issue #155 reports that during Claude-driven edits the plugin frequently opens a 2-pane diff (original + proposed) as vertical splits, but after accepting the change one split remains. Over time, repeated diffs accumulate these leftover windows, forcing users to manually :q them.

Goal: make diff UI cleanup deterministic so that, when a diff is closed (via the close_tab MCP tool), the plugin closes any windows it created for the diff and does not leak extra splits.

Evidence (what I inspected)

  • Issue text + comments via GitHub API (6 comments). Key repro note: when the file is not already open, the plugin opens it in a new split and then splits again for the diff; on accept it returns to the extra split (comment by ehaynes99, 2025-11-14).
  • lua/claudecode/diff.lua
    • Window creation: _create_diff_view_from_window() calls choose_original_window(...) and, when decision == "split", runs create_split() and uses that new window for the original side (around lines ~933–941).
    • Cleanup: _cleanup_diff_state() closes diff_data.new_window but only runs :diffoff in diff_data.target_window (around lines ~1029–1039). If target_window was created by create_split(), it stays open.
    • Replacement: open_diff_blocking() calls _resolve_diff_as_rejected() when an active diff with the same tab_name already exists, but does not clean up its windows (around ~1262–1265).
  • lua/claudecode/config.lua: legacy options vertical_split and open_in_current_tab are mapped onto modern diff_opts.layout and diff_opts.open_in_new_tab (lines ~205–215).
  • Docs drift:
    • README.md still documents legacy keys like vertical_split / open_in_current_tab and mentions auto_close_on_accept (which is validated but not implemented).
    • CLAUDE.md documents open_in_new_tab but includes an example that also sets legacy keys, which overrides the new setting.

Plan

1) Track which diff windows were created by the plugin

Why: cleanup must differentiate between windows that were part of the user’s pre-existing layout vs windows created solely to render the diff.

  • Extend the diff layout/state structures to record window ownership:
    • Add target_window_created_by_plugin: boolean (or original_window_created_by_plugin) to the object returned by _create_diff_view_from_window().
    • Persist the flag into active_diffs[tab_name] in _setup_blocking_diff().
  • Set the flag conservatively:
    • If choose_original_window(...).decision == "split", then target_window_created_by_plugin = true because we just created original_window via create_split().
    • Otherwise, false (reused an existing window).
Optional hardening (if you want to cover more layouts)

There is another window-creation path when _create_diff_view_from_window() is called with target_window == nil and it needs to create_split() away from a terminal/sidebar buffer (around ~908–927). If this path is contributing to “mystery splits”, consider tracking a small list like created_windows = { ... } instead of a single boolean, so cleanup can close all plugin-created windows deterministically.

2) Close the plugin-created original-side window during cleanup

  • Update _cleanup_diff_state(tab_name, reason) (non-new-tab branch):
    • Keep existing behavior of closing diff_data.new_window (proposed side).
    • For diff_data.target_window:
      • If diff_data.target_window_created_by_plugin == true and the window is still valid, close it with nvim_win_close(..., true).
      • Else, preserve current behavior (vim.cmd("diffoff")) so we don’t destroy a user’s pre-existing window.

Safety: only close windows we explicitly created.

3) Clean up when replacing an active diff with the same tab_name

Why: avoids accumulating stale diff windows/state when the CLI requests a new diff before the old one is fully closed.

  • In open_diff_blocking():
    • If active_diffs[tab_name] exists, call _resolve_diff_as_rejected(tab_name) and then _cleanup_diff_state(tab_name, "replaced by new diff") before starting the new diff.
    • This is safe because the close_tab tool already treats “diff not found” as success.

4) Add/extend unit tests to prevent regressions

Add a focused spec that reproduces issue #155 using the existing vim mock (which simulates :vsplit and window closing).

  • Test: closes plugin-created original window after accept

    1. Create a real temp file (old file) and set current window to a different named buffer so choose_original_window must split.
    2. Call diff._setup_blocking_diff(...).
    3. Capture target_window + new_window from diff._get_active_diffs()[tab_name].
    4. Call diff._resolve_diff_as_saved(tab_name, new_buffer) then diff.close_diff_by_tab_name(tab_name).
    5. Assert both new_window and target_window are no longer valid, and the original user window is still valid.
  • Test: does not close reused existing window

    • Set current window to already display the old file (or an empty unmodified buffer) so decision == "reuse".
    • Ensure cleanup closes only new_window, and keeps the reused window alive.

5) Documentation cleanup (reduce user confusion)

  • Update README.md diff section to document modern keys:
    • diff_opts.layout = "vertical"|"horizontal"
    • diff_opts.open_in_new_tab
    • diff_opts.keep_terminal_focus, diff_opts.hide_terminal_in_new_tab, diff_opts.on_new_file_reject
    • Mark vertical_split / open_in_current_tab as legacy aliases (or remove them from examples).
  • Fix CLAUDE.md example so it doesn’t set both open_in_new_tab and legacy open_in_current_tab at the same time.
  • Either remove or explicitly label auto_close_on_accept / show_diff_stats as legacy/no-op until implemented.

Acceptance Criteria

  • Repro from issue [BUG] Claude Code spawns a lot of splited windows #155 no longer leaves an extra split after accept (and likewise after reject for existing files).
  • Repeated diffs do not accumulate leftover diff windows.
  • Unit tests covering window ownership + cleanup pass (make test).
  • Docs reflect the actual supported configuration knobs.

Generated with mux • Model: openai:gpt-5.2 • Thinking: xhigh

Change-Id: Ib857da86fd15fe097651a50680d7d1639097905f
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33
Copy link
Member Author

@codex review

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. Nice work!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

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] Claude Code spawns a lot of splited windows

1 participant