feat(edits): support custom diff providers for edit requests#228
feat(edits): support custom diff providers for edit requests#228Norhua wants to merge 2 commits intonickjvandyke:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a configurable “edit diff renderer” abstraction for edit permission requests, keeping the existing :diffpatch flow as the default while allowing users to plug in custom renderers (including an included mini.diff helper).
Changes:
- Adds
opts.events.permissions.edits.rendererandopts.events.permissions.edits.keymapsconfiguration. - Refactors edit request handling into a renderer/session model with a shared context API.
- Exposes
require("opencode").diff_renderersand implements amini.diff-based renderer helper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents the renderer context/session API and the new keymap options, plus mini.diff setup examples. |
| plugin/events/permissions/edits.lua | Refactors edit permission handling to support custom renderers and introduces global edit keymaps and session tracking. |
| lua/opencode/integrations/diff.lua | Adds built-in mini.diff renderer helper with fallback to default :diffpatch. |
| lua/opencode/config.lua | Adds default config entries for edits.renderer and edits.keymaps. |
| lua/opencode.lua | Exposes diff renderers via require("opencode").diff_renderers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| end | ||
|
|
||
| current_edit_request_id = nil |
There was a problem hiding this comment.
accept_edit() clears current_edit_request_id before sending the permit reply. Since the permission.replied autocmd only calls close_diff() when current_edit_request_id matches the request id, the diff view will no longer auto-close after accepting, leaving stale UI/state behind. Consider keeping current_edit_request_id until the reply event closes the view, or explicitly calling close_diff() as part of the accept flow (while still sending the permit).
| current_edit_request_id = nil |
| return | ||
| end | ||
|
|
||
| current_edit_request_id = nil |
There was a problem hiding this comment.
reject_edit() clears current_edit_request_id before sending the reply, which prevents the permission.replied handler from matching and closing the diff. This likely leaves the tab/session open after rejecting. Keep the request id until permission.replied triggers close_diff(), or close the diff explicitly in the reject handler.
| current_edit_request_id = nil |
| ---@return opencode.events.permissions.edits.Session?, opencode.events.permissions.edits.ActiveRequest? | ||
| local function get_active_diff() | ||
| if not diff_session or not active_request or not current_edit_request_id then | ||
| notify_info("No active opencode edit diff") | ||
| return nil, nil | ||
| end | ||
|
|
||
| return diff_session, active_request | ||
| end |
There was a problem hiding this comment.
get_active_diff() requires current_edit_request_id to be non-nil to treat a session as active. However accept_hunk() / reject_hunk() intentionally set current_edit_request_id = nil when rejecting the request; after that, the global ]c/[c mappings will always report “No active…” and stop navigating hunks even though the diff view is still open. Consider decoupling “session is active for navigation/actions” from “request is pending for permit()”, e.g., track a separate flag for auto-close, or base activeness on diff_session/diff_bufnr validity.
| local function close_diff() | ||
| current_edit_request_id = nil | ||
| cleanup_diff() | ||
|
|
||
| if diff_tabpage and vim.api.nvim_tabpage_is_valid(diff_tabpage) then | ||
| vim.api.nvim_set_current_tabpage(diff_tabpage) | ||
| vim.cmd("tabclose") | ||
| end | ||
|
|
||
| diff_tabpage = nil | ||
| diff_tabpage_number = nil | ||
| end |
There was a problem hiding this comment.
close_diff() always does tabclose for the stored diff_tabpage. This makes the “custom renderer” API effectively tab-only: renderers that open splits/floats (or mini_diff({ open_cmd = "vsplit" })) will still close the entire tab when users press the close keymap or call ctx.close(). Consider delegating view teardown to diff_session.close() (and only tab-closing for the built-in diffpatch renderer), or tracking the window id to close instead of the whole tab.
| if vim.startswith(normalized, "/") then | ||
| return normalized | ||
| end | ||
|
|
||
| local absolute_like = vim.fs.normalize("/" .. normalized) | ||
| if vim.uv.fs_stat(absolute_like) then | ||
| return absolute_like | ||
| end | ||
|
|
||
| local cwd_relative = vim.fs.normalize(vim.fn.getcwd() .. "/" .. normalized) | ||
| if vim.uv.fs_stat(cwd_relative) then | ||
| return cwd_relative | ||
| end | ||
|
|
||
| return absolute_like |
There was a problem hiding this comment.
normalize_filepath() assumes POSIX-style absolute paths by checking vim.startswith(normalized, "/") and by manufacturing an absolute path via "/" .. normalized. This will break on Windows-style paths (e.g. C:\...), potentially making edit diffs fail to open / proposed text computation fail. Prefer vim.fs.is_absolute() or vim.fn.fnamemodify(filepath, ":p") (and avoid prepending /).
| if vim.startswith(normalized, "/") then | |
| return normalized | |
| end | |
| local absolute_like = vim.fs.normalize("/" .. normalized) | |
| if vim.uv.fs_stat(absolute_like) then | |
| return absolute_like | |
| end | |
| local cwd_relative = vim.fs.normalize(vim.fn.getcwd() .. "/" .. normalized) | |
| if vim.uv.fs_stat(cwd_relative) then | |
| return cwd_relative | |
| end | |
| return absolute_like | |
| if vim.fs.is_absolute(normalized) then | |
| return normalized | |
| end | |
| return vim.fs.normalize(vim.fn.fnamemodify(normalized, ":p")) |
| ---@return integer? | ||
| local function current_line() | ||
| if not diff_bufnr or not vim.api.nvim_buf_is_valid(diff_bufnr) then | ||
| return nil | ||
| end | ||
|
|
||
| local cursor = vim.api.nvim_win_get_cursor(0) | ||
| return cursor[1] | ||
| end | ||
|
|
There was a problem hiding this comment.
current_line() is defined but never used (only referenced at its definition). This adds dead code and can confuse future maintenance; consider removing it or wiring it into the hunk helpers if it’s intended for cursor-based actions.
| ---@return integer? | |
| local function current_line() | |
| if not diff_bufnr or not vim.api.nvim_buf_is_valid(diff_bufnr) then | |
| return nil | |
| end | |
| local cursor = vim.api.nvim_win_get_cursor(0) | |
| return cursor[1] | |
| end |
| vim.cmd(("%s %s"):format(opts.open_cmd, vim.fn.fnameescape(ctx.filepath))) | ||
| local bufnr = vim.api.nvim_get_current_buf() | ||
| local previous_state = mini_diff.get_buf_data(bufnr) | ||
| local previous_config = vim.deepcopy(vim.b[bufnr].minidiff_config) | ||
|
|
||
| pcall(mini_diff.disable, bufnr) | ||
| vim.b[bufnr].minidiff_config = { source = mini_diff.gen_source.none() } | ||
|
|
||
| local enabled = pcall(mini_diff.enable, bufnr) | ||
| local ref_ok = enabled and pcall(mini_diff.set_ref_text, bufnr, proposed) | ||
| if not ref_ok then | ||
| pcall(vim.cmd, "tabclose") | ||
| return fallback(ctx) | ||
| end |
There was a problem hiding this comment.
Failure cleanup uses tabclose unconditionally (pcall(vim.cmd, "tabclose")), but the view was opened via opts.open_cmd (which can be a split/vsplit/edit/etc). If open_cmd is not tabnew, this may close an unrelated tab and also leave the opened window behind. Track what was opened (tab vs window) and close the correct container, or constrain/document open_cmd to tab-opening commands.
| local opts = require("opencode.config").opts.events.permissions.edits or {} | ||
| local keymaps = vim.tbl_extend("force", default_keymaps, opts.keymaps or {}) | ||
| local mappings = { | ||
| { lhs = keymaps.accept, rhs = accept_edit, desc = "Accept opencode edit" }, | ||
| { lhs = keymaps.reject, rhs = reject_edit, desc = "Reject opencode edit" }, | ||
| { lhs = keymaps.close, rhs = close_diff, desc = "Close opencode edit diff" }, | ||
| { lhs = keymaps.accept_hunk, rhs = accept_hunk, desc = "Accept opencode edit hunk" }, | ||
| { lhs = keymaps.reject_hunk, rhs = reject_hunk, desc = "Reject opencode edit hunk" }, | ||
| { lhs = keymaps.next_hunk, rhs = function() goto_hunk("next") end, desc = "Next opencode edit hunk" }, | ||
| { lhs = keymaps.prev_hunk, rhs = function() goto_hunk("prev") end, desc = "Previous opencode edit hunk" }, | ||
| } | ||
|
|
||
| for _, mapping in ipairs(mappings) do | ||
| if mapping.lhs then | ||
| vim.keymap.set("n", mapping.lhs, mapping.rhs, { desc = mapping.desc }) | ||
| end | ||
| end |
There was a problem hiding this comment.
These keymaps are registered globally (vim.keymap.set without { buffer = ... }) and the defaults (q, da, dr, ]c, [c, dp, do) conflict with core Neovim behavior (e.g. q for macro recording, da{motion} delete-around text objects, ]c/[c used by built-in diff and many git plugins). Because the mappings do not fall through when there’s no active edit diff (they notify and return), they can disrupt normal editing after the first edit request. Consider making them buffer-local to the session buffer/tab, using safer default prefixes (e.g. <leader>), or using an expr mapping that passes through to the original keys when no session is active.
| ---@param session opencode.events.permissions.edits.Session | ||
| local function activate_session(session) | ||
| diff_session = session | ||
| diff_bufnr = session.bufnr or vim.api.nvim_get_current_buf() | ||
| diff_tabpage = vim.api.nvim_get_current_tabpage() | ||
| diff_tabpage_number = vim.api.nvim_tabpage_get_number(diff_tabpage) | ||
|
|
||
| local tabclosed_group = vim.api.nvim_create_augroup("OpencodeEditTabClose", { clear = false }) | ||
| vim.api.nvim_create_autocmd("TabClosed", { | ||
| group = tabclosed_group, | ||
| pattern = tostring(diff_tabpage_number), | ||
| once = true, | ||
| callback = function() | ||
| cleanup_diff() | ||
| diff_tabpage = nil | ||
| diff_tabpage_number = nil | ||
| current_edit_request_id = nil | ||
| end, | ||
| desc = "Clean up opencode edit diff state", | ||
| }) | ||
| end |
There was a problem hiding this comment.
The TabClosed autocmd uses pattern = tostring(diff_tabpage_number), but tabpage numbers are positional and can change when other tabs are opened/closed. If the diff tab’s number shifts, closing it won’t match the stored pattern and the cleanup won’t run, leaving stale diff_session/current_edit_request_id state. Prefer a TabClosed autocmd without a number pattern and, in the callback, detect cleanup based on the stored diff_tabpage handle becoming invalid (or compare the current number of the handle before it becomes invalid).
|
Please respond to the automated review comments and make sure it passes the checks 🙂 |
Closes #224
Summary
This PR adds a configurable diff renderer for edit permission requests.
By default,
opencode.nvimkeeps using the built-in:diffpatchflow, but users can now provide a custom renderer throughopts.events.permissions.edits.renderer. The renderer receives a context with the request id, filepath, unified diff, helpers to compute proposed text, send permit/reject replies, close the active view, and fall back to the default renderer.
On top of that abstraction, this PR also exposes a small
require("opencode").diff_renderers.mini_diff()helper as an optional integration example for users who prefer amini.diff-based workflow.
What changed
opts.events.permissions.edits.rendereråopts.events.permissions.edits.keymaps:diffpatchbehavior as the default rendererrequire("opencode").diff_renderersrequire("opencode").diff_renderers.mini_diff()as an optional helpermini.diffsetup in the README
Behavior
mini.diffhelper falls back to the default renderer ifmini.diffis unavailable or proposed text cannot be computed
Testing
:diffpatchedit flow still opens and closes correctlymini.diffhelper can render the proposed edit and navigate hunksmini.diffis unavailable