fix(review): skip unsupported files during file navigation (#417)

Author: barrettruth

lua/diffs/commands.lua

@@ -1248,19 +1248,23 @@ end
 
 ---@param state diffs.ReviewSplitState
 ---@param selected diffs.GeneratedFileSelection
----@return boolean
-local function switch_review_split_file(state, selected)
+---@param opts? { quiet?: boolean }
+---@return boolean, boolean
+local function switch_review_split_file(state, selected, opts)
+  opts = opts or {}
   local diff_spec, diff_lines, err, level =
     render_review_file_selection(state.review, state.repo_root, selected)
   if not diff_spec or not diff_lines then
-    notify(err or 'cannot render review split file', level or vim.log.levels.WARN)
-    return false
+    if not opts.quiet then
+      notify(err or 'cannot render review split file', level or vim.log.levels.WARN)
+    end
+    return false, true
   end
 
   local left_win = first_window_for_buffer(state.left_buf)
   local right_win = first_window_for_buffer(state.right_buf)
   if not left_win or not right_win then
-    return false
+    return false, false
   end
 
   local opened, split_err = split.open({
@@ -1276,7 +1280,7 @@ local function switch_review_split_file(state, selected)
   })
   if not opened then
     notify(split_err or 'cannot open review split', vim.log.levels.ERROR)
-    return false
+    return false, false
   end
 
   forget_review_split(state)
@@ -1290,7 +1294,7 @@ local function switch_review_split_file(state, selected)
   review_split_states[state.left_buf] = state
   review_split_states[state.right_buf] = state
   attach_review_split_autocmds(state)
-  return true
+  return true, false
 end
 
 ---@param state diffs.ReviewSplitState
@@ -1322,6 +1326,21 @@ local function announce_review_file(selection, index, count)
   vim.api.nvim_echo({ { ('[diffs]: (%d of %d): %s'):format(index, count, entry) } }, false, {})
 end
 
+---@param skipped diffs.GeneratedFileSelection[]
+local function notify_skipped_review_files(skipped)
+  if #skipped == 0 then
+    return
+  end
+  local paths = {}
+  for _, selection in ipairs(skipped) do
+    paths[#paths + 1] = selection.file
+  end
+  notify(
+    ('review skipped %d file(s): %s'):format(#skipped, table.concat(paths, ', ')),
+    vim.log.levels.INFO
+  )
+end
+
 ---@param state diffs.ReviewSplitState
 ---@param selection diffs.GeneratedFileSelection
 ---@param index integer
@@ -1344,8 +1363,22 @@ local function step_review_split_file(state, delta)
     return
   end
   local current = review_index_of_current(files, state)
-  local target = ((current - 1 + delta) % count) + 1
-  goto_selection(state, files[target], target, count)
+  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
+    end
+    skipped[#skipped + 1] = files[target]
+  end
+  notify_skipped_review_files(skipped)
 end
 
 ---@param delta integer

spec/commands_spec.lua

@@ -3423,6 +3423,42 @@ describe('commands', function()
       return repo_root
     end
 
+    local function create_binary_review_repo(opts)
+      opts = opts or {}
+      local repo_root = vim.fn.tempname()
+      vim.fn.mkdir(repo_root, 'p')
+      test_repos[#test_repos + 1] = repo_root
+
+      vim.fn.systemlist({ 'git', 'init', '-q', repo_root })
+      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' })
+
+      write_repo_file(repo_root, 'aaa-one.lua', { 'old one' })
+      write_binary_file(repo_root .. '/bbb-bin.dat', 'binary\\000old')
+      if opts.trailing ~= false then
+        write_repo_file(repo_root, 'ccc-two.lua', { 'old two' })
+        git_cmd(repo_root, { 'add', 'aaa-one.lua', 'bbb-bin.dat', 'ccc-two.lua' })
+      else
+        git_cmd(repo_root, { 'add', 'aaa-one.lua', 'bbb-bin.dat' })
+      end
+      git_cmd(repo_root, { 'commit', '-qm', 'base' })
+      git_cmd(repo_root, { 'branch', 'binary-base' })
+
+      git_cmd(repo_root, { 'checkout', '-qb', 'binary-topic' })
+      write_repo_file(repo_root, 'aaa-one.lua', { 'new one' })
+      write_binary_file(repo_root .. '/bbb-bin.dat', 'binary\\000new')
+      if opts.trailing ~= false then
+        write_repo_file(repo_root, 'ccc-two.lua', { 'new two' })
+        git_cmd(repo_root, { 'add', 'aaa-one.lua', 'bbb-bin.dat', 'ccc-two.lua' })
+      else
+        git_cmd(repo_root, { 'add', 'aaa-one.lua', 'bbb-bin.dat' })
+      end
+      git_cmd(repo_root, { 'commit', '-qm', 'target' })
+
+      return repo_root
+    end
+
     it('opens review layout split as exactly two visible surfaces', function()
       local repo_root = create_repo()
       vim.fn.writefile({ 'line 1', 'line 2 changed' }, repo_root .. '/file.txt')
@@ -3719,6 +3755,72 @@ describe('commands', function()
       assert_target_at_hunk(panes, 1)
     end)
 
+    it('skips unsupported files when switching review files', 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)
+      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')
+      )
+
+      vim.api.nvim_set_current_win(panes.right_win)
+      commands.review_next_file()
+      panes = track_panes(panes.state.left_buf)
+      assert.are.same(
+        diffspec.rev_to_rev('binary-base', 'binary-topic', 'ccc-two.lua'),
+        vim.api.nvim_buf_get_var(panes.left_buf, 'diffs_spec')
+      )
+      assert.are.equal(vim.log.levels.INFO, notifications[#notifications].level)
+      assert.are.equal(
+        '[diffs]: review skipped 1 file(s): bbb-bin.dat',
+        notifications[#notifications].message
+      )
+
+      vim.api.nvim_set_current_win(panes.right_win)
+      commands.review_prev_file()
+      panes = track_panes(panes.state.left_buf)
+      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(vim.log.levels.INFO, notifications[#notifications].level)
+      assert.are.equal(
+        '[diffs]: review skipped 1 file(s): bbb-bin.dat',
+        notifications[#notifications].message
+      )
+    end)
+
+    it('keeps the skipped-file message when no candidate switches', function()
+      local repo_root = create_binary_review_repo({ trailing = false })
+      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)
+      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')
+      )
+
+      vim.api.nvim_set_current_win(panes.right_win)
+      commands.review_next_file()
+      panes = track_panes(panes.state.left_buf)
+
+      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(vim.log.levels.INFO, notifications[1].level)
+      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')