fix(diff): emit lifecycle events for unified layout#297
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32d0ccedf5
ℹ️ 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".
Signed-off-by: Thomas Kosiewski <tk@coder.com>
32d0cce to
f916098
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ 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". |
Summary
Fixes #296.
ClaudeCodeDiffOpenedfordiff_opts.layout = "unified"after the unified diff state is registered.ClaudeCodeDiffClosedfrom unified diff cleanup with the same payload shape as native layouts.target_window, including the terminal-only fallback case, and add focused regression coverage.Validation
mise exec -- busted -v tests/unit/diff_auto_resize_events_spec.lua— 15 successes / 0 failures.mise exec -- busted -v tests/unit/diff_inline_spec.lua tests/unit/diff_auto_resize_events_spec.lua— 41 successes / 0 failures.mise run check— 0 warnings / 0 errors in 113 files.mise run test— 705 successes / 0 failures.Dogfooding
mise exec -- nvim --headless -u NONE -l /tmp/repro_issue_296_headless.lua./tmp/issue-296-headless.typescriptand asciinema cast recorded at/tmp/issue-296-headless.cast; the run printed oneClaudeCodeDiffOpened, oneClaudeCodeDiffClosed, andHEADLESS_DOGFOOD_OK.DISPLAYis empty.Workflow verifier summary
The issue implementation workflow converged without recovery. Its verifier found no issue-scope correctness defects after the fix and confirmed unified lifecycle parity plus the terminal-only fallback coverage.
📋 Implementation Plan
Implementation Plan for #296
Issue Summary
GitHub issue
coder/claudecode.nvim#296is an accepted bug: when users setdiff_opts.layout = "unified", the unified diff UI opens and closes without firing the documentedUserautocmd lifecycle eventsClaudeCodeDiffOpenedandClaudeCodeDiffClosed.Current live issue state verified with
gh issue view 296 --repo coder/claudecode.nvim --comments:bug(diff): unified layout skips diff lifecycle User eventstriage:done,bug,acceptedVerified Repository Context
Explore agents verified these current repo facts:
lua/claudecode/diff.luaowns the native diff lifecycle orchestration and localfire_diff_event(...)helper.M._setup_blocking_diff, the unified layout branch callsrequire("claudecode.diff_inline").setup_inline_diff(params, resolution_callback, config)and returns before the existingClaudeCodeDiffOpenedemission at the end of the native path.M._cleanup_diff_state, the unified layout branch callsdiff_inline.cleanup_inline_diff(tab_name, diff_data), clearsactive_diffs[tab_name], and returns before the existingClaudeCodeDiffClosedemission.lua/claudecode/diff_inline.luaregisters unified diff state throughdiff._register_diff_state(tab_name, { ... }), includingnew_window = diff_win,layout = "unified", terminal/tab tracking fields, and local access toeditor_winanddiff_winat registration time.ClaudeCodeDiffOpenedpayloadtarget_windowas a non-nil field; only fields such asterminal_windowandtab_numberare optional/nilable. Therefore unified layout should provide a meaningfultarget_window, notnil.target_window = editor_win, the original editor window used before the unified diff split is opened. This mirrors the native path's “original editor window” meaning.Scope Boundaries
In scope:
diff_opts.layout = "unified".Out of scope:
diff_inline.luaor changing layout naming semantics.If implementation uncovers unrelated bugs or flaky behavior, follow the user's out-of-scope workflow: search existing issues first; comment with new evidence if a matching issue exists; otherwise create a
needs-triageissue with reproduction/expected/actual details and link back to #296.Recommended Implementation Strategy
Use a small TDD loop: first add a failing regression test demonstrating that unified layout emits no lifecycle events, then make the smallest code change that passes it.
Phase 1 — Add focused regression coverage
Modify
tests/unit/diff_auto_resize_events_spec.lua.Add one or two tests under the existing diff lifecycle User events coverage that:
diff_opts.layout = "unified".open_diff_tool.handler(...)with:old_file_path = test_old_filenew_file_path = test_old_filenew_file_contentstab_name, e.g.opened-unified-tabvim.api.nvim_exec_autocmdsusing the existing helper pattern.ClaudeCodeDiffOpenedevent is emitted with:tab_namefile_pathnew_file_pathis_new_file == falsefor the existing-file casediff_window, andvim.api.nvim_win_is_valid(diff_window)if the harness still has the window opentarget_window, andvim.api.nvim_win_is_valid(target_window)if the harness still has the window openterminal_window; do not require a terminal window in the unit testmodeline == false_cleanup_diff_state(...)only if that is the established pattern for this spec. Do not call both for the same active diff.ClaudeCodeDiffClosedevent is emitted with:tab_namefile_pathreason, such asdiff rejectedor the explicit test cleanup reason.Quality gate after Phase 1:
mise exec -- busted -v tests/unit/diff_auto_resize_events_spec.luaExpected before implementation: the new unified lifecycle test fails because the events are missing. If it does not fail, re-check the test harness before changing production code.
Phase 2 — Preserve unified target-window state
Modify
lua/claudecode/diff_inline.luaat the unified diff state registration inM.setup_inline_diff(...).Add the original editor window to registered state:
Place it next to
new_window = diff_winso state fields align with native event payload concepts.Rationale:
editor_winis already in scope and represents the source editor window from which the unified split is created.target_windowfor opened events.diff.luaemit the event without exposingfire_diff_eventtodiff_inline.lua.Defensive check guidance:
editor_winis valid before storing it; otherwise keep the production change minimal and let the regression test prove the invariant.Quality gate after Phase 2:
mise exec -- busted -v tests/unit/diff_inline_spec.lua tests/unit/diff_auto_resize_events_spec.luaPhase 3 — Emit
ClaudeCodeDiffOpenedfor unified layoutModify
lua/claudecode/diff.luainside thelayout == "unified"branch ofM._setup_blocking_diff.After
inline.setup_inline_diff(params, resolution_callback, config)returns and before the earlyreturn:active_diffs[tab_name].fire_diff_event("ClaudeCodeDiffOpened", { ... })helper.Implementation notes:
fire_diff_eventprivate indiff.lua; do not exposeM._fire_diff_eventunless absolutely necessary.active_diffs[tab_name]is absent because unified setup failed before registration, do not emitClaudeCodeDiffOpened; a diff that failed to open should not report opened.state.is_new_fileover recalculating if it is already stored by unified state; if the field is not present in the actual code, use the sameis_new_filelocal already computed in_setup_blocking_diff.Quality gate after Phase 3:
mise exec -- busted -v tests/unit/diff_auto_resize_events_spec.luaPhase 4 — Emit
ClaudeCodeDiffClosedfor unified layoutModify
lua/claudecode/diff.luainside thediff_data.layout == "unified"branch ofM._cleanup_diff_state.After
inline.cleanup_inline_diff(tab_name, diff_data)andactive_diffs[tab_name] = nil, but before the earlyreturn, call:Rationale:
diff_dataremains available after cleanup, so payload construction does not require any additional state.Quality gate after Phase 4:
mise exec -- busted -v tests/unit/diff_auto_resize_events_spec.lua tests/unit/diff_inline_spec.luaPhase 5 — Keep docs unchanged unless tests reveal a mismatch
The issue is that implementation does not meet the existing README lifecycle contract. The likely minimal fix is code plus regression tests only.
Only update docs if implementation reveals that README examples or field descriptions are inaccurate after the fix. Do not broaden this issue into documentation cleanup.
Acceptance Criteria
The implementation is complete when all of these are true:
diff_opts.layout = "unified", opening a proposed-edit diff fires exactly oneClaudeCodeDiffOpenedUser autocmd for that diff open.ClaudeCodeDiffOpenedpayload includes the documented fields and mirrors native payload semantics:tab_namefile_pathnew_file_pathis_new_filediff_windowthat is a valid Neovim window at emission timetarget_windowthat is a valid Neovim window at emission timeterminal_windowwhen discoverable, otherwise nil as currently allowedtab_numberonly when the diff opens in a new tabdiff_opts.layout = "unified", accepting, rejecting, or cleaning up the diff fires exactly oneClaudeCodeDiffClosedUser autocmd.ClaudeCodeDiffClosedpayload includestab_name,file_path, andreasonwith the same semantics as native layouts.Validation Plan
Run validation in this order, fixing any failures before claiming success:
Focused lifecycle test during development:
mise exec -- busted -v tests/unit/diff_auto_resize_events_spec.luaUnified diff suite and lifecycle suite together:
mise exec -- busted -v tests/unit/diff_inline_spec.lua tests/unit/diff_auto_resize_events_spec.luaStatic checks:
Full test suite before PR/readiness handoff:
mise run testOptional formatting if files are changed in a way that may need formatting:
If the repo reports a mise trust error, run
mise trustonce from the repository root and retry the command.Dogfooding / Manual Verification Plan
Because this change affects an interactive Neovim lifecycle surface, produce reviewable evidence in addition to automated tests.
Headless dogfood script, no committed fixture required
Use a temporary Lua script outside tracked files, e.g.
mktemp, that:runtimepath.claudecode.diffandclaudecode.tools.open_diff.diff.setup({ terminal = {}, diff_opts = { layout = "unified" } }).Userautocmd listeners forClaudeCodeDiffOpenedandClaudeCodeDiffClosedthat print compactvim.inspect(ev.data)payloads.open_diff_tool.handler(...)in a coroutine with changed file contents.Run it with:
Capture terminal output with either:
script -q /tmp/issue-296-headless.typescript -c 'nvim --headless -u NONE -l /path/to/temp/repro_issue_296.lua'or an equivalent terminal recording tool such as
asciinema recif available. Attach the transcript/recording or paste the relevant output in the PR notes.Interactive Neovim dogfood with visual evidence
For a reviewer-friendly UI check:
Create a temporary minimal config outside tracked files that sets:
Launch Neovim from the repo root with that config.
Trigger a unified diff through the plugin/tool path or a small local command that invokes
open_diff_tool.handler(...).Accept or reject the diff.
Capture:
ClaudeCodeDiffOpenednotification appears;ClaudeCodeDiffClosednotification or:messagesoutput after accepting/rejecting;Suggested capture options:
asciinema rec /tmp/issue-296-dogfood.castfor terminal UI recording.scripttranscript if asciinema is unavailable.If the environment is headless and screenshots are not possible, explicitly report that blocker and provide the headless transcript/recording as the reviewable evidence.
Risks and Mitigations
target_windowsemantics: Useeditor_winfromdiff_inline.setup_inline_diffbecause it is the original editor window and matches the native payload's intent. Assert it is non-nil in tests.ClaudeCodeDiffOpenedonly aftersetup_inline_diffregisters state; emitClaudeCodeDiffClosedaftercleanup_inline_diffcompletes but before returning.fire_diff_event, which already centralizes event execution behavior, rather than duplicating rawnvim_exec_autocmdscalls.lua/claudecode/diff.lua,lua/claudecode/diff_inline.lua, andtests/unit/diff_auto_resize_events_spec.luaunless validation reveals a directly related need.Implementation Checklist
tests/unit/diff_auto_resize_events_spec.lua.target_window = editor_winin unified diff state registration inlua/claudecode/diff_inline.lua.ClaudeCodeDiffOpenedin the unified branch ofM._setup_blocking_diffinlua/claudecode/diff.luaafter state registration.ClaudeCodeDiffClosedin the unified branch ofM._cleanup_diff_stateinlua/claudecode/diff.luabefore returning.Generated with
mux• Model:openai:gpt-5.5• Thinking:xhigh