fix: file history panel traversal across entry boundaries when wrap_entries is false#27
Conversation
…entries` is false
There was a problem hiding this comment.
Pull request overview
Adds coverage and fixes for file-history panel navigation so traversal across commit-entry boundaries behaves correctly when wrap_entries is disabled.
Changes:
- Refactors
FileHistoryPanel:_get_entry_by_file_offset()to use non-wrapping traversal logic whenwrap_entries = false(no modulo wrapping). - Updates
FileHistoryPanel:set_file_by_offset()to no-op when the computed next target isnil. - Introduces a new functional test suite covering
wrap_entriesbehavior for bothFilePanelandFileHistoryPanel.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lua/diffview/scene/views/file_history/file_history_panel.lua |
Adjusts cross-entry traversal logic to avoid wrapping when wrap_entries is false; adds an early-return guard when no next target exists. |
lua/diffview/tests/functional/wrap_entries_spec.lua |
Adds functional tests validating wrapping vs non-wrapping traversal across entries and within a flat file list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| i = i + sign | ||
| end | ||
|
|
||
| -- Reached the boundary: return nil to signal no movement. |
There was a problem hiding this comment.
When wrap is false and offset overshoots past the start/end, this now returns nil (via falling out of the loop). That makes count-based navigation (vim.v.count1 > remaining items) a no-op even when there is still valid movement possible (e.g. from a middle file with a large count), which is inconsistent with other navigation codepaths that clamp to the nearest boundary (see FilePanel:next_file/prev_file). Consider clamping here too: after exhausting entries, return the first/last file overall unless the cursor is already at that boundary, in which case return nil to signal no movement.
| -- Reached the boundary: return nil to signal no movement. | |
| -- Reached the boundary without wrapping: clamp to first/last file, | |
| -- unless already at that boundary (then signal no movement). | |
| local boundary_entry_idx = sign > 0 and #self.entries or 1 | |
| local boundary_entry = self.entries[boundary_entry_idx] | |
| local boundary_files = boundary_entry.files | |
| local boundary_file = sign > 0 and boundary_files[#boundary_files] or boundary_files[1] | |
| local at_forward_boundary = sign > 0 | |
| and entry_idx == #self.entries | |
| and file_idx == #cur_entry.files | |
| local at_backward_boundary = sign < 0 | |
| and entry_idx == 1 | |
| and file_idx == 1 | |
| if at_forward_boundary or at_backward_boundary then | |
| return | |
| end | |
| return boundary_entry, boundary_file |
| -- From entry1 file1, offset +6 should land on entry3 file1. | ||
| -- entry1 has 2 remaining (files 2,3), entry2 has 2, so 2+2=4 | ||
| -- consumed; delta after entry2 = 6-2-2 = 2, but let's be exact: | ||
| -- delta = 6 - (3 - 1) = 4, entry2 has 2 files (4-2=2), entry3 | ||
| -- needs delta=2 so files[2] = "g". | ||
| local e, f = panel:_get_entry_by_file_offset(1, 1, 6, false) |
There was a problem hiding this comment.
This comment says the +6 offset should land on entry3 file1, but the assertion below expects entries[3].files[2] ("g"), which is correct for a flat traversal starting at entry1/file1. Please update the comment to match the actual expected target so the test remains self-explanatory.
| it("clamps a large forward offset at the boundary", function() | ||
| -- From entry3 file3, offset +5 exceeds total remaining files. | ||
| local e, f = panel:_get_entry_by_file_offset(3, 3, 5, false) | ||
| eq(nil, e) | ||
| eq(nil, f) | ||
| end) |
There was a problem hiding this comment.
The test name says it "clamps" a large forward offset at the boundary, but it asserts nil (no movement). If the intended behavior is to clamp to the last available file, the expectation should assert the last file; if the intended behavior is no-op on overshoot, the test name should be updated to reflect that.
No description provided.