fix(diff): user-initiated accept/reject should clean up windows and buffers#241
Open
mightreya wants to merge 1 commit intocoder:mainfrom
Open
fix(diff): user-initiated accept/reject should clean up windows and buffers#241mightreya wants to merge 1 commit intocoder:mainfrom
mightreya wants to merge 1 commit intocoder:mainfrom
Conversation
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
:ClaudeCodeDiffAcceptand:ClaudeCodeDiffDeny(theaccept_current_diff/deny_current_diffuser commands) leave the diff buffers, split windows, andactive_diffsstate 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
:qcloses 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_stateand is triggered exclusively by theclose_tabMCP tool.But
close_tabhasschema = niland is documented as "Internal tool — must remain as requested by user" (seelua/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_statethemselves. They're not waiting for an MCP response from the CLI — they're driven by the user — so the "wait forclose_tab" architecture doesn't apply to them.This PR does not touch
close_tab.lua. Itsschema = nilis left intact per the existing comment.It also extends
diff_opts.keep_terminal_focusto 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.luacovering:accept_current_diffcloses the diff window and removes the entry fromactive_diffsdeny_current_diffcloses the diff window and removes the entry fromactive_diffsaccept_current_diffis a safe no-op when called outside a diff bufferI was unable to run
make testlocally (the Nix-based dev shell wasn't available on my machine). I confirmed Lua syntax vialoadfile. Please run the suite in CI / locally before merging.Closes / refs
:qdoes not work #238