Skip to content

fix(diff): emit lifecycle events for unified layout#297

Merged
ThomasK33 merged 1 commit into
mainfrom
implement-issue-296
Jun 25, 2026
Merged

fix(diff): emit lifecycle events for unified layout#297
ThomasK33 merged 1 commit into
mainfrom
implement-issue-296

Conversation

@ThomasK33

Copy link
Copy Markdown
Member

Summary

Fixes #296.

  • Emit ClaudeCodeDiffOpened for diff_opts.layout = "unified" after the unified diff state is registered.
  • Emit ClaudeCodeDiffClosed from unified diff cleanup with the same payload shape as native layouts.
  • Track a valid unified 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

  • Headless Neovim dogfood passed with mise exec -- nvim --headless -u NONE -l /tmp/repro_issue_296_headless.lua.
  • Terminal transcript recorded at /tmp/issue-296-headless.typescript and asciinema cast recorded at /tmp/issue-296-headless.cast; the run printed one ClaudeCodeDiffOpened, one ClaudeCodeDiffClosed, and HEADLESS_DOGFOOD_OK.
  • Interactive screenshots were not available in this workspace because DISPLAY is 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#296 is an accepted bug: when users set diff_opts.layout = "unified", the unified diff UI opens and closes without firing the documented User autocmd lifecycle events ClaudeCodeDiffOpened and ClaudeCodeDiffClosed.

Current live issue state verified with gh issue view 296 --repo coder/claudecode.nvim --comments:

  • Title: bug(diff): unified layout skips diff lifecycle User events
  • State: open
  • Labels: triage:done, bug, accepted
  • Impact: user autocmds for terminal resizing, statusline updates, or other review-lifecycle behavior work for native layouts but not unified layout.

Verified Repository Context

Explore agents verified these current repo facts:

  • lua/claudecode/diff.lua owns the native diff lifecycle orchestration and local fire_diff_event(...) helper.
  • In M._setup_blocking_diff, the unified layout branch calls require("claudecode.diff_inline").setup_inline_diff(params, resolution_callback, config) and returns before the existing ClaudeCodeDiffOpened emission at the end of the native path.
  • In M._cleanup_diff_state, the unified layout branch calls diff_inline.cleanup_inline_diff(tab_name, diff_data), clears active_diffs[tab_name], and returns before the existing ClaudeCodeDiffClosed emission.
  • lua/claudecode/diff_inline.lua registers unified diff state through diff._register_diff_state(tab_name, { ... }), including new_window = diff_win, layout = "unified", terminal/tab tracking fields, and local access to editor_win and diff_win at registration time.
  • README and existing tests treat ClaudeCodeDiffOpened payload target_window as a non-nil field; only fields such as terminal_window and tab_number are optional/nilable. Therefore unified layout should provide a meaningful target_window, not nil.
  • The best compatibility choice is 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:

  • Restore lifecycle event parity for diff_opts.layout = "unified".
  • Preserve existing payload shape as closely as possible to native vertical/horizontal diff layouts.
  • Add focused regression coverage for unified open and close lifecycle events.

Out of scope:

  • Reworking unified diff rendering, highlighting, folding, or accept/reject behavior.
  • Renaming diff_inline.lua or changing layout naming semantics.
  • Changing README event documentation unless implementation reveals an actual doc mismatch.
  • Adding permanent repro scripts or fixtures unless tests cannot cover the behavior cleanly.

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-triage issue 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:

  1. Configure diff handling with diff_opts.layout = "unified".
  2. Open a diff via the same tool path used by existing lifecycle tests, e.g. open_diff_tool.handler(...) with:
    • old_file_path = test_old_file
    • new_file_path = test_old_file
    • changed new_file_contents
    • unique tab_name, e.g. opened-unified-tab
  3. Capture calls to vim.api.nvim_exec_autocmds using the existing helper pattern.
  4. Assert exactly one ClaudeCodeDiffOpened event is emitted with:
    • tab_name
    • file_path
    • new_file_path
    • is_new_file == false for the existing-file case
    • non-nil diff_window, and vim.api.nvim_win_is_valid(diff_window) if the harness still has the window open
    • non-nil target_window, and vim.api.nvim_win_is_valid(target_window) if the harness still has the window open
    • optional/nilable terminal_window; do not require a terminal window in the unit test
    • modeline == false
  5. Resolve/reject or clean up the unified diff through one existing cleanup path only: prefer the same reject/accept resolver path used by existing lifecycle tests; use direct _cleanup_diff_state(...) only if that is the established pattern for this spec. Do not call both for the same active diff.
  6. Capture and assert exactly one ClaudeCodeDiffClosed event is emitted with:
    • tab_name
    • file_path
    • the expected cleanup reason, such as diff rejected or the explicit test cleanup reason.

Quality gate after Phase 1:

mise exec -- busted -v tests/unit/diff_auto_resize_events_spec.lua

Expected 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.lua at the unified diff state registration in M.setup_inline_diff(...).

Add the original editor window to registered state:

target_window = editor_win,

Place it next to new_window = diff_win so state fields align with native event payload concepts.

Rationale:

  • editor_win is already in scope and represents the source editor window from which the unified split is created.
  • Existing documentation and tests expect a non-nil target_window for opened events.
  • Storing this in state lets diff.lua emit the event without exposing fire_diff_event to diff_inline.lua.

Defensive check guidance:

  • Do not add broad runtime fallback behavior that hides bugs.
  • If a local assertion/helper is already used in nearby code, it is reasonable to assert that editor_win is 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.lua

Phase 3 — Emit ClaudeCodeDiffOpened for unified layout

Modify lua/claudecode/diff.lua inside the layout == "unified" branch of M._setup_blocking_diff.

After inline.setup_inline_diff(params, resolution_callback, config) returns and before the early return:

  1. Read the registered state from active_diffs[tab_name].
  2. If state exists, call the existing local fire_diff_event("ClaudeCodeDiffOpened", { ... }) helper.
  3. Use payload values parallel to the native path:
fire_diff_event("ClaudeCodeDiffOpened", {
  tab_name = tab_name,
  file_path = params.old_file_path,
  new_file_path = params.new_file_path,
  is_new_file = state.is_new_file,
  diff_window = state.new_window,
  target_window = state.target_window,
  terminal_window = state.terminal_win_in_new_tab or find_claudecode_terminal_window(),
  tab_number = state.created_new_tab and state.new_tab_number or nil,
})

Implementation notes:

  • Keep fire_diff_event private in diff.lua; do not expose M._fire_diff_event unless absolutely necessary.
  • If active_diffs[tab_name] is absent because unified setup failed before registration, do not emit ClaudeCodeDiffOpened; a diff that failed to open should not report opened.
  • Prefer state.is_new_file over recalculating if it is already stored by unified state; if the field is not present in the actual code, use the same is_new_file local already computed in _setup_blocking_diff.

Quality gate after Phase 3:

mise exec -- busted -v tests/unit/diff_auto_resize_events_spec.lua

Phase 4 — Emit ClaudeCodeDiffClosed for unified layout

Modify lua/claudecode/diff.lua inside the diff_data.layout == "unified" branch of M._cleanup_diff_state.

After inline.cleanup_inline_diff(tab_name, diff_data) and active_diffs[tab_name] = nil, but before the early return, call:

fire_diff_event("ClaudeCodeDiffClosed", {
  tab_name = tab_name,
  file_path = diff_data.old_file_path,
  reason = reason,
})

Rationale:

  • This preserves the existing native close payload shape.
  • Emitting after unified cleanup mirrors the current native function ordering where closed events happen at the end of cleanup.
  • The original diff_data remains 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.lua

Phase 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:

  • With diff_opts.layout = "unified", opening a proposed-edit diff fires exactly one ClaudeCodeDiffOpened User autocmd for that diff open.
  • The unified ClaudeCodeDiffOpened payload includes the documented fields and mirrors native payload semantics:
    • tab_name
    • file_path
    • new_file_path
    • is_new_file
    • non-nil diff_window that is a valid Neovim window at emission time
    • non-nil target_window that is a valid Neovim window at emission time
    • terminal_window when discoverable, otherwise nil as currently allowed
    • tab_number only when the diff opens in a new tab
  • With diff_opts.layout = "unified", accepting, rejecting, or cleaning up the diff fires exactly one ClaudeCodeDiffClosed User autocmd.
  • The unified ClaudeCodeDiffClosed payload includes tab_name, file_path, and reason with the same semantics as native layouts.
  • Native vertical/horizontal diff lifecycle event behavior remains unchanged.
  • Existing unified diff rendering and cleanup behavior remains unchanged except for lifecycle event emission.
  • Regression tests fail on the current bug and pass after the fix.
  • No unrelated issues are fixed or refactored as part of this work.

Validation Plan

Run validation in this order, fixing any failures before claiming success:

  1. Focused lifecycle test during development:

    mise exec -- busted -v tests/unit/diff_auto_resize_events_spec.lua
  2. Unified diff suite and lifecycle suite together:

    mise exec -- busted -v tests/unit/diff_inline_spec.lua tests/unit/diff_auto_resize_events_spec.lua
  3. Static checks:

    mise run check
  4. Full test suite before PR/readiness handoff:

    mise run test
  5. Optional formatting if files are changed in a way that may need formatting:

    mise run format

If the repo reports a mise trust error, run mise trust once 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:

  1. Prepends the repo root to runtimepath.
  2. Requires claudecode.diff and claudecode.tools.open_diff.
  3. Creates a temporary file with simple Lua content.
  4. Calls diff.setup({ terminal = {}, diff_opts = { layout = "unified" } }).
  5. Registers User autocmd listeners for ClaudeCodeDiffOpened and ClaudeCodeDiffClosed that print compact vim.inspect(ev.data) payloads.
  6. Starts open_diff_tool.handler(...) in a coroutine with changed file contents.
  7. Rejects or cleans up the diff.
  8. Exits non-zero if either event was not observed.

Run it with:

nvim --headless -u NONE -l /path/to/temp/repro_issue_296.lua

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 rec if 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:

  1. Create a temporary minimal config outside tracked files that sets:

    vim.opt.runtimepath:prepend(vim.fn.getcwd())
    require("claudecode").setup({
      diff_opts = { layout = "unified" },
    })
    vim.api.nvim_create_autocmd("User", {
      pattern = "ClaudeCodeDiffOpened",
      callback = function(ev)
        vim.notify("ClaudeCodeDiffOpened " .. vim.inspect(ev.data), vim.log.levels.INFO)
      end,
    })
    vim.api.nvim_create_autocmd("User", {
      pattern = "ClaudeCodeDiffClosed",
      callback = function(ev)
        vim.notify("ClaudeCodeDiffClosed " .. vim.inspect(ev.data), vim.log.levels.INFO)
      end,
    })
  2. Launch Neovim from the repo root with that config.

  3. Trigger a unified diff through the plugin/tool path or a small local command that invokes open_diff_tool.handler(...).

  4. Accept or reject the diff.

  5. Capture:

    • screenshot of the unified diff UI after ClaudeCodeDiffOpened notification appears;
    • screenshot of the ClaudeCodeDiffClosed notification or :messages output after accepting/rejecting;
    • short terminal/video recording showing the open → event → close → event sequence.

Suggested capture options:

  • asciinema rec /tmp/issue-296-dogfood.cast for terminal UI recording.
  • script transcript if asciinema is unavailable.
  • If a GUI/display is available, take screenshots and attach them to the PR using the repository's normal image attachment flow.

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

  • Duplicate event emission: Add tests that capture event count or use unique tab names so the implementation proves exactly one open and one close event for unified layout.
  • Incorrect target_window semantics: Use editor_win from diff_inline.setup_inline_diff because it is the original editor window and matches the native payload's intent. Assert it is non-nil in tests.
  • Event emitted before state/window is usable: Emit ClaudeCodeDiffOpened only after setup_inline_diff registers state; emit ClaudeCodeDiffClosed after cleanup_inline_diff completes but before returning.
  • User autocmd errors affecting diff flow: Reuse existing fire_diff_event, which already centralizes event execution behavior, rather than duplicating raw nvim_exec_autocmds calls.
  • Over-broad changes: Keep edits limited to lua/claudecode/diff.lua, lua/claudecode/diff_inline.lua, and tests/unit/diff_auto_resize_events_spec.lua unless validation reveals a directly related need.

Implementation Checklist

  • Add failing unified lifecycle regression test(s) in tests/unit/diff_auto_resize_events_spec.lua.
  • Store target_window = editor_win in unified diff state registration in lua/claudecode/diff_inline.lua.
  • Emit ClaudeCodeDiffOpened in the unified branch of M._setup_blocking_diff in lua/claudecode/diff.lua after state registration.
  • Emit ClaudeCodeDiffClosed in the unified branch of M._cleanup_diff_state in lua/claudecode/diff.lua before returning.
  • Run focused and full validation commands.
  • Dogfood with headless and, if possible, interactive evidence; attach transcript/screenshots/recording to the PR notes.

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

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread lua/claudecode/diff_inline.lua Outdated
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33 ThomasK33 force-pushed the implement-issue-296 branch from 32d0cce to f916098 Compare June 25, 2026 15:44
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

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

Reviewed commit: f916098470

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

@ThomasK33 ThomasK33 added this pull request to the merge queue Jun 25, 2026
Merged via the queue into main with commit 2390c6e Jun 25, 2026
4 checks passed
@ThomasK33 ThomasK33 deleted the implement-issue-296 branch June 25, 2026 17:31
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): unified layout skips diff lifecycle User events

1 participant