fix(tui): yank cell/row no longer reports "No row selected" - #41
Merged
Conversation
Yank resolved its row from Buffer::get_selected_row(), which was backed by ratatui's TableState - a second source of truth for the crosshair that only row navigation (sync_row_state) and query execution (reset_navigation_state) ever wrote. Loading a file never initialised it, so on a view narrowed to a single row - where j/k can never fire - `yv` and `yy` refused to copy a perfectly visible cell. The `f` text filter made it worse: apply_text_filter_with_refs applied the filter and set a status message, nothing else, unlike the fuzzy filter which resets selection, scroll and the viewport crosshair to the first match. - get_selected_row() now derives from view_state.crosshair_row, the position the table actually renders. None means "no visible rows", and the row is clamped to the view - so a filter that shrinks the results can no longer leave yank reading past the end and copying "NULL". TableState is untouched and still drives rendering. - apply_text_filter_with_refs now resets to the first match like the fuzzy filter does, which is also the better behaviour on its own. Two existing tests asserted the old contract against buffers holding no data at all; they now install data, since an empty buffer having no selection is correct under the new semantics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Filter down to a single row (
f+ a term), navigate to a cell, pressyv— the status bar says "No row selected" and nothing is copied. Reported from real use: pulling version numbers out of a TeamCity CSV, cell by cell.Root cause
Yank resolves its row from
Buffer::get_selected_row()(src/handlers/yank.rs:43), which was backed by ratatui'sTableState— a second source of truth for the crosshair, separate from theViewportManager/view_state.crosshair_rowposition the table actually renders.Only two things ever wrote it:
sync_row_state()(src/ui/traits/navigation.rs:37), i.e.j/k/Greset_table_state()→reset_navigation_state()Loading a file does neither:
new_with_dataviewandadd_dataview_with_refsinstall the DataView and the ViewportManager but never initialise the selection. So it stayedNoneuntil you moved the cursor — and on a view narrowed to one row,j/kcan never fire. Hence "consistent with a single row in the view".The two filter halves had also drifted apart:
apply_fuzzy_filter_with_refsShift+Fset_selected_row(Some(0))+vm.set_crosshair_row(0)apply_text_filter_with_refsfSecond bug, same root
No clamping either. Navigate to row 57, then filter down to one row: the selection stayed at
Some(57),dataview.get_cell_value(57, col)returnedNone, and yank silently copied the stringNULL.The fix
get_selected_row()derives from the crosshair. It now readsview_state.crosshair_row, returnsNoneonly when there are genuinely no visible rows, and clamps to the view's row count. Kills both failure modes.TableStateis untouched and still drives rendering; the only functional caller ofget_selected_row()was yank (the rest are debug dumps and oneunwrap_or(0)).The
ftext filter resets to the first match, mirroring the fuzzy filter — viewport ref threaded in fromenhanced_tui::apply_filter. Better behaviour in its own right, independent of the yank bug.Tests
New
tests/test_yank_single_row_repro.rs— fresh load has a selection, empty results don't, both filters keep a selection at a single row, and the selection is clamped so yank reads the visible cell.Two existing tests asserted the old contract using buffers with no data at all (
test_buffer::test_buffer_navigation,test_buffer_state_refactor::test_direct_buffer_viewstate_access); they now install data, since an empty buffer having no selection is correct under the new semantics.cargo test— 688 + 412 + 1 passed, 0 failedcargo fmt/cargo clippy— clean, no new warnings.exe-suffix noise, green in CIVerified by hand against the original CSV that triggered the report.
Follow-up, not in this PR
fis the text filter andShift+Fthe fuzzy filter perKeyMapper(src/ui/key_handling/mapper.rs:222), butsrc/config/key_bindings.rs:365anddispatcher.rscarry the opposite mapping. They're dead duplicate tables that KeyMapper overrides — worth pruning so the code and the help agree.🤖 Generated with Claude Code