From a80e54000ded8f52f2b57067980037944fba0826 Mon Sep 17 00:00:00 2001 From: Barrett Ruth Date: Fri, 10 Jul 2026 10:59:32 -0500 Subject: [PATCH] fix(review): label skipped files in review surfaces --- doc/diffs.nvim.txt | 8 +-- lua/diffs/commands.lua | 117 ++++++++++++++++++++++++++++++++--------- lua/diffs/lists.lua | 26 ++++++++- lua/diffs/review.lua | 10 +++- spec/commands_spec.lua | 99 ++++++++++++++++++++++++++++------ 5 files changed, 211 insertions(+), 49 deletions(-) diff --git a/doc/diffs.nvim.txt b/doc/diffs.nvim.txt index 90ce3a0..f44ff38 100644 --- a/doc/diffs.nvim.txt +++ b/doc/diffs.nvim.txt @@ -1271,6 +1271,7 @@ A `diffs.ReviewFile` describes one changed file in a review: ~ {section_label} (string|nil) Display label, e.g. `Unstaged`. {added} (integer) Added line count. {removed} (integer) Removed line count. + {skipped} (boolean) True when split review navigation skips this file. review_files({bufnr}) *diffs.nvim.review_files()* List the changed files in the review split {bufnr} belongs to, as @@ -1290,7 +1291,7 @@ review_current({bufnr}) *diffs.nvim.review_current()* review_goto({target}, {bufnr}) *diffs.nvim.review_goto()* Switch the review split {bufnr} belongs to so it shows {target}, swapping both panes in place. Returns `true` on success, `false` when {bufnr} is not - a review split or no file matches. Never opens a split. + a review split, no file matches, or {target} is skipped. Never opens a split. Parameters: ~ {target} (string) A |diffs.ReviewFile| `key` (preferred) or path. @@ -1299,12 +1300,13 @@ review_goto({target}, {bufnr}) *diffs.nvim.review_goto()* review_next_file() *diffs.nvim.review_next_file()* review_prev_file() *diffs.nvim.review_prev_file()* Step the current review split to the next/previous changed file, wrapping - at the ends. Backing functions for `]f`/`[f`, + at the ends and skipping unsupported files. Backing functions for `]f`/`[f`, |(diffs-review-next-file)|, and |(diffs-review-prev-file)|. select_review_file({bufnr}) *diffs.nvim.select_review_file()* Prompt with |vim.ui.select()| over the review split's files and jump to the - chosen one. Backing function for |(diffs-review-select-file)| (`gO`). + chosen one. Skipped files are shown as `(skipped)` and do not jump. Backing + function for |(diffs-review-select-file)| (`gO`). It uses whatever |vim.ui.select()| backend you have configured, so no extra dependency is required. To build a bespoke picker, call the data and action functions directly, capturing the review buffer before the picker opens: >lua diff --git a/lua/diffs/commands.lua b/lua/diffs/commands.lua index 2d2a967..b757467 100644 --- a/lua/diffs/commands.lua +++ b/lua/diffs/commands.lua @@ -1159,6 +1159,7 @@ local function review_generated_list_opts(list_opts) metadata_for_line = list_opts and list_opts.metadata_for_line, sections = list_opts and list_opts.sections, store_hunks = list_opts and list_opts.store_hunks, + is_skipped = list_opts and list_opts.is_skipped, } end @@ -1166,27 +1167,34 @@ end ---@param repo_root string ---@param review_lines string[] ---@param list_opts table? ----@return diffs.GeneratedFileSelection?, diffs.DiffSpec?, string[]?, string?, integer? +---@return diffs.GeneratedFileSelection?, diffs.DiffSpec?, string[]?, string?, integer?, diffs.GeneratedFileSelection[] local function first_renderable_review_file(review_spec, repo_root, review_lines, list_opts) local last_err local last_level + local skipped = {} for _, selected in ipairs(lists.generated_files(review_lines, review_generated_list_opts(list_opts))) do - local diff_spec, diff_lines, err, level = - render_review_file_selection(review_spec, repo_root, selected) - if diff_spec and diff_lines then - return selected, diff_spec, diff_lines, nil, nil + if selected.skipped then + skipped[#skipped + 1] = selected + else + local diff_spec, diff_lines, err, level = + render_review_file_selection(review_spec, repo_root, selected) + if diff_spec and diff_lines then + return selected, diff_spec, diff_lines, nil, nil, skipped + end + skipped[#skipped + 1] = selected + last_err = err + last_level = level end - last_err = err - last_level = level end return nil, nil, nil, last_err or 'no renderable review file selected', - last_level or vim.log.levels.INFO + last_level or vim.log.levels.INFO, + skipped end ---@param state diffs.ReviewSplitState @@ -1366,17 +1374,22 @@ local function step_review_split_file(state, delta) local skipped = {} for offset = 1, count - 1 do local target = ((current - 1 + delta * offset) % count) + 1 - local switched, unsupported = switch_review_split_file(state, files[target], { quiet = true }) - if switched then - notify_skipped_review_files(skipped) - announce_review_file(files[target], target, count) - return - end - if not unsupported then - notify_skipped_review_files(skipped) - return + local selected = files[target] + if selected.skipped then + skipped[#skipped + 1] = selected + else + local switched, unsupported = switch_review_split_file(state, selected, { quiet = true }) + if switched then + notify_skipped_review_files(skipped) + announce_review_file(selected, target, count) + return + end + if not unsupported then + notify_skipped_review_files(skipped) + return + end + skipped[#skipped + 1] = selected end - skipped[#skipped + 1] = files[target] end notify_skipped_review_files(skipped) end @@ -1404,6 +1417,7 @@ end ---@field section_label? string ---@field added integer ---@field removed integer +---@field skipped boolean ---@param selection diffs.GeneratedFileSelection ---@return diffs.ReviewFile @@ -1415,6 +1429,7 @@ local function project_review_file(selection) section_label = selection.section_label, added = selection.added or 0, removed = selection.removed or 0, + skipped = selection.skipped == true, } end @@ -1467,12 +1482,58 @@ function M.review_goto(target, bufnr) if selection.key == state.selected_key or selection.file == state.selected_file then return true end + if selection.skipped then + notify_skipped_review_files({ selection }) + return false + end return goto_selection(state, selection, i, #files) end end return false end +---@param file diffs.ReviewFile +---@return string +local function review_file_label(file) + local path = file.skipped and (file.path .. ' (skipped)') or file.path + if file.section_label and file.section_label ~= '' then + return ('[%s] %s'):format(file.section_label, path) + end + return path +end + +---@param files diffs.ReviewFile[] +---@return fun(file: diffs.ReviewFile): string +local function review_file_formatter(files) + local labels = {} + local max_label, max_add, max_del = 0, 0, 0 + for _, file in ipairs(files) do + local label = review_file_label(file) + labels[file.key] = label + max_label = math.max(max_label, #label) + if file.added > 0 then + max_add = math.max(max_add, #tostring(file.added) + 1) + end + if file.removed > 0 then + max_del = math.max(max_del, #tostring(file.removed) + 1) + end + end + return function(file) + local label = labels[file.key] or review_file_label(file) + local parts = { label .. string.rep(' ', max_label - #label) } + if max_add > 0 then + parts[#parts + 1] = file.added > 0 and string.format('%' .. max_add .. 's', '+' .. file.added) + or string.rep(' ', max_add) + end + if max_del > 0 then + parts[#parts + 1] = file.removed > 0 + and string.format('%' .. max_del .. 's', '-' .. file.removed) + or string.rep(' ', max_del) + end + return (table.concat(parts, ' '):gsub('%s+$', '')) + end +end + ---@param bufnr integer? Defaults to the current buffer. function M.select_review_file(bufnr) bufnr = bufnr or vim.api.nvim_get_current_buf() @@ -1482,12 +1543,7 @@ function M.select_review_file(bufnr) end vim.ui.select(files, { prompt = 'Review file:', - format_item = function(file) - local label = (file.section_label and file.section_label ~= '') - and ('[%s] %s'):format(file.section_label, file.path) - or file.path - return ('%s +%d -%d'):format(label, file.added, file.removed) - end, + format_item = review_file_formatter(files), }, function(choice) if choice then M.review_goto(choice.key, bufnr) @@ -1535,7 +1591,12 @@ open_review_split = function(spec, opts) local review_spec = stored_review_spec(normalized) local first = opts.selection local diff_spec, diff_lines + local skipped = {} if first then + if first.skipped then + notify_skipped_review_files({ first }) + return nil + end local err, level diff_spec, diff_lines, err, level = render_review_file_selection(review_spec, normalized.repo_root, first) @@ -1545,10 +1606,13 @@ open_review_split = function(spec, opts) end else local err, level - first, diff_spec, diff_lines, err, level = + first, diff_spec, diff_lines, err, level, skipped = first_renderable_review_file(review_spec, normalized.repo_root, review_lines, list_opts) if not first or not diff_spec or not diff_lines then - notify(err or 'no review file selected', level or vim.log.levels.INFO) + notify_skipped_review_files(skipped) + if #skipped == 0 then + notify(err or 'no review file selected', level or vim.log.levels.INFO) + end return nil end end @@ -1608,6 +1672,7 @@ open_review_split = function(spec, opts) break end end + notify_skipped_review_files(skipped) announce_review_file(first, index, #files) dbg('opened review split %d/%d (%s)', opened.left_buf, opened.right_buf, normalized.display) diff --git a/lua/diffs/lists.lua b/lua/diffs/lists.lua index 431b4b3..ba58bb0 100644 --- a/lua/diffs/lists.lua +++ b/lua/diffs/lists.lua @@ -18,6 +18,7 @@ local group = vim.api.nvim_create_augroup('diffs_generated_lists', { clear = fal ---@field sections? table[] ---@field store_hunks? boolean ---@field quickfix? boolean +---@field is_skipped? fun(entry: table): boolean ---@param bufnr integer ---@param name string @@ -100,6 +101,7 @@ local function ensure_file_entry(entries, by_file, file, lnum, meta) local entry = by_file[key] if entry then entry.lnum = math.min(entry.lnum, lnum) + entry.skipped = entry.skipped or meta.skipped == true return entry end @@ -113,6 +115,7 @@ local function ensure_file_entry(entries, by_file, file, lnum, meta) section = meta.section, section_label = meta.section_label, diff_spec = meta.diff_spec, + skipped = meta.skipped == true, } by_file[key] = entry entries[#entries + 1] = entry @@ -139,10 +142,11 @@ end ---@param entry table ---@return string local function entry_display_file(entry) + local file = entry.skipped and (entry.file .. ' (skipped)') or entry.file if type(entry.section_label) == 'string' and entry.section_label ~= '' then - return ('[%s] %s'):format(entry.section_label, entry.file) + return ('[%s] %s'):format(entry.section_label, file) end - return entry.file + return file end ---@param hunk diffs.DiffHunk? @@ -186,6 +190,18 @@ local function file_entries(hunks, diff_lines, opts) return entries end +---@param entries table[] +---@param opts? table +local function apply_skipped(entries, opts) + local is_skipped = opts and opts.is_skipped + if type(is_skipped) ~= 'function' then + return + end + for _, entry in ipairs(entries) do + entry.skipped = is_skipped(entry) == true + end +end + ---@param entries table[] local function format_file_text(entries) local max_fname = 0 @@ -236,6 +252,7 @@ local function quickfix_items(bufnr, entries) section = entry.section, section_label = entry.section_label, diff_spec = entry.diff_spec, + skipped = entry.skipped == true, }, }, } @@ -399,6 +416,7 @@ local function selection_from_entry(state, entry) hunk_index = file_hunk_index(state.hunks, hunk), added = entry.adds, removed = entry.dels, + skipped = entry.skipped == true, } end @@ -443,6 +461,7 @@ local function selection_from_item(item) lnum = item.lnum, hunk = hunk, hunk_index = state and file_hunk_index(state.hunks, hunk) or nil, + skipped = data.skipped == true, }, nil end @@ -482,6 +501,7 @@ local function list_state(diff_lines, opts) local diff_spec = opts.diff_spec local hunks = hunk_model.parse(diff_lines, diff_spec) local entries = file_entries(hunks, diff_lines, opts) + apply_skipped(entries, opts) local title = opts.title or 'diffs' return { hunks = hunks, @@ -650,6 +670,7 @@ end ---@field hunk_index? integer ---@field added? integer ---@field removed? integer +---@field skipped? boolean ---@param opts? diffs.GeneratedFileSelectionOpts ---@return diffs.GeneratedFileSelection?, string? @@ -690,6 +711,7 @@ function M.selected_generated_file(opts) lnum = lnum, hunk = hunk, hunk_index = file_hunk_index(state.hunks, hunk), + skipped = entry.skipped == true, }, nil end diff --git a/lua/diffs/review.lua b/lua/diffs/review.lua index 2fea73f..d7394dc 100644 --- a/lua/diffs/review.lua +++ b/lua/diffs/review.lua @@ -11,6 +11,12 @@ local render = require('diffs.render') local dbg = log.dbg local notify = log.notify +---@param entry table +---@return boolean +local function is_skipped_entry(entry) + return type(entry.hunks) == 'table' and #entry.hunks == 0 +end + ---@class diffs.ReviewSpec ---@field base? string ---@field target? string @@ -639,6 +645,7 @@ local function run_current_state(review, deps) metadata_for_line = metadata_for_records(state.records), sections = state.sections, store_hunks = true, + is_skipped = is_skipped_entry, } end @@ -769,7 +776,7 @@ function M.render(review, deps) end local result, err = M.run(review, deps) - return result, err, nil + return result, err, { is_skipped = is_skipped_entry } end ---@param spec? diffs.ReviewSpec @@ -824,6 +831,7 @@ function M.open(spec, deps) metadata_for_line = list_opts and list_opts.metadata_for_line, sections = list_opts and list_opts.sections, store_hunks = list_opts and list_opts.store_hunks, + is_skipped = list_opts and list_opts.is_skipped, quickfix = true, }) diff --git a/spec/commands_spec.lua b/spec/commands_spec.lua index e765e6e..dc237be 100644 --- a/spec/commands_spec.lua +++ b/spec/commands_spec.lua @@ -3397,7 +3397,7 @@ describe('commands', function() assert.is_true(right_row >= 1) end - local function create_mode_first_review_repo() + local function create_binary_first_review_repo() local repo_root = vim.fn.tempname() vim.fn.mkdir(repo_root, 'p') test_repos[#test_repos + 1] = repo_root @@ -3406,19 +3406,18 @@ describe('commands', function() assert.are.equal(0, vim.v.shell_error) git_cmd(repo_root, { 'config', 'user.email', 'test@example.com' }) git_cmd(repo_root, { 'config', 'user.name', 'Test' }) - git_cmd(repo_root, { 'config', 'core.filemode', 'true' }) - write_repo_file(repo_root, 'aaa-mode.sh', { '#!/bin/sh', 'echo mode' }) + write_binary_file(repo_root .. '/aaa-bin.dat', 'binary\\000old') write_repo_file(repo_root, 'zzz-changed.lua', { 'old' }) - git_cmd(repo_root, { 'add', 'aaa-mode.sh', 'zzz-changed.lua' }) + git_cmd(repo_root, { 'add', 'aaa-bin.dat', 'zzz-changed.lua' }) git_cmd(repo_root, { 'commit', '-qm', 'base' }) - git_cmd(repo_root, { 'branch', 'mode-base' }) + git_cmd(repo_root, { 'branch', 'binary-first-base' }) - git_cmd(repo_root, { 'checkout', '-qb', 'mode-topic' }) - git_cmd(repo_root, { 'update-index', '--chmod=+x', 'aaa-mode.sh' }) + git_cmd(repo_root, { 'checkout', '-qb', 'binary-first-topic' }) + write_binary_file(repo_root .. '/aaa-bin.dat', 'binary\\000new') write_repo_file(repo_root, 'zzz-changed.lua', { 'new' }) - git_cmd(repo_root, { 'add', 'aaa-mode.sh', 'zzz-changed.lua' }) - git_cmd(repo_root, { 'commit', '-qm', 'mode and content' }) + git_cmd(repo_root, { 'add', 'aaa-bin.dat', 'zzz-changed.lua' }) + git_cmd(repo_root, { 'commit', '-qm', 'binary and content' }) return repo_root end @@ -3633,23 +3632,49 @@ describe('commands', function() end) it('skips unsupported default review entries when opening direct split workspaces', function() - local repo_root = create_mode_first_review_repo() + local repo_root = create_binary_first_review_repo() edit_file(repo_root .. '/zzz-changed.lua') + local notifications = capture_notifications() mock_runtime_attach(function() end) - local left_buf = commands.review_command('++layout=split mode-base..mode-topic') + local left_buf = + commands.review_command('++layout=split binary-first-base..binary-first-topic') local panes = track_panes(left_buf) assert.are.same( - diffspec.rev_to_rev('mode-base', 'mode-topic', 'zzz-changed.lua'), + diffspec.rev_to_rev('binary-first-base', 'binary-first-topic', 'zzz-changed.lua'), vim.api.nvim_buf_get_var(panes.left_buf, 'diffs_spec') ) assert.are.same( - diffspec.rev_to_rev('mode-base', 'mode-topic', 'zzz-changed.lua'), + diffspec.rev_to_rev('binary-first-base', 'binary-first-topic', 'zzz-changed.lua'), vim.api.nvim_buf_get_var(panes.right_buf, 'diffs_spec') ) assert.are.same({ 'old' }, vim.api.nvim_buf_get_lines(panes.left_buf, 0, -1, false)) assert.are.same({ 'new' }, vim.api.nvim_buf_get_lines(panes.right_buf, 0, -1, false)) + assert.are.equal('[diffs]: review skipped 1 file(s): aaa-bin.dat', notifications[1].message) + end) + + it('marks skipped review files in the quickfix file index', function() + local repo_root = create_binary_review_repo() + mock_runtime_attach(function() end) + + local bufnr = commands.review({ + base = 'binary-base', + target = 'binary-topic', + mode = 'direct', + repo = repo_root, + }) + assert.is_not_nil(bufnr) + table.insert(test_buffers, bufnr) + + local qf = quickfix_items() + assert.are.equal(3, #qf) + assert.is_true(qf[1].text:match('^aaa%-one%.lua%s+%+1%s+%-1$') ~= nil) + assert.are.equal('bbb-bin.dat (skipped)', qf[2].text) + assert.is_true(qf[3].text:match('^ccc%-two%.lua%s+%+1%s+%-1$') ~= nil) + assert.is_false(qf[1].user_data.diffs.skipped) + assert.is_true(qf[2].user_data.diffs.skipped) + assert.is_false(qf[3].user_data.diffs.skipped) end) it('routes current-state duplicate paths by section in the two-surface workspace', function() @@ -3821,6 +3846,43 @@ describe('commands', function() assert.are.equal('[diffs]: review skipped 1 file(s): bbb-bin.dat', notifications[1].message) end) + it('marks skipped review files for the API and picker', function() + local repo_root = create_binary_review_repo() + edit_file(repo_root .. '/aaa-one.lua') + local notifications = capture_notifications() + mock_runtime_attach(function() end) + + local left_buf = commands.review_command('++layout=split binary-base..binary-topic') + local panes = track_panes(left_buf) + local files = commands.review_files(panes.left_buf) + assert.are.equal(3, #files) + assert.is_false(files[1].skipped) + assert.is_true(files[2].skipped) + assert.is_false(files[3].skipped) + + local formatted = {} + local original = vim.ui.select + vim.ui.select = function(items, opts, on_choice) + formatted[1] = opts.format_item(items[1]) + formatted[2] = opts.format_item(items[2]) + formatted[3] = opts.format_item(items[3]) + on_choice(items[2]) + end + local ok, err = pcall(commands.select_review_file, panes.left_buf) + vim.ui.select = original + assert.is_true(ok, err) + + assert.is_true(formatted[1]:match('^aaa%-one%.lua%s+%+1%s+%-1$') ~= nil) + assert.are.equal('bbb-bin.dat (skipped)', formatted[2]) + assert.is_true(formatted[3]:match('^ccc%-two%.lua%s+%+1%s+%-1$') ~= nil) + assert.are.same( + diffspec.rev_to_rev('binary-base', 'binary-topic', 'aaa-one.lua'), + vim.api.nvim_buf_get_var(panes.left_buf, 'diffs_spec') + ) + assert.are.equal(1, #notifications) + assert.are.equal('[diffs]: review skipped 1 file(s): bbb-bin.dat', notifications[1].message) + end) + it('exposes review_files/current/goto, the gO map, and the b:diffs_review marker', function() local repo = create_review_repo() edit_file(repo.repo_root .. '/lua/one.lua') @@ -4038,7 +4100,7 @@ describe('commands', function() ) end) - it('reports unsupported selected review files without opening a workspace', function() + it('reports skipped selected review files without opening a workspace', function() local repo = create_mode_only_review_repo() local notifications = capture_notifications() mock_runtime_attach(function() end) @@ -4059,8 +4121,9 @@ describe('commands', function() local opened = commands.review_split({ bufnr = bufnr }) assert.is_nil(opened) - assert.is_true( - notifications[#notifications].message:find('mode-only changes', 1, true) ~= nil + assert.are.equal( + '[diffs]: review skipped 1 file(s): scripts/tool.sh', + notifications[1].message ) assert.is_nil(commands._test.review_split_state(bufnr)) end) @@ -4106,10 +4169,12 @@ describe('commands', function() assert.are.equal(2, #qf) assert.are.equal(bufnr, qf[1].bufnr) assert.are.equal(1, qf[1].lnum) - assert.is_true(qf[1].text:find('lua/mode.lua', 1, true) ~= nil) + assert.is_true(qf[1].text:find('lua/mode.lua (skipped)', 1, true) ~= nil) + assert.is_true(qf[1].user_data.diffs.skipped) assert.are.equal(bufnr, qf[2].bufnr) assert.are.equal(4, qf[2].lnum) assert.is_true(qf[2].text:find('lua/changed.lua', 1, true) ~= nil) + assert.is_false(qf[2].user_data.diffs.skipped) assert.are.equal(0, #loclist_items())