Fix gr.Dataframe shift+click range selecting rows hidden by the search - #13829
Open
hysts wants to merge 15 commits into
Open
Fix gr.Dataframe shift+click range selecting rows hidden by the search#13829hysts wants to merge 15 commits into
gr.Dataframe shift+click range selecting rows hidden by the search#13829hysts wants to merge 15 commits into
Conversation
The range was built by walking the numeric range between the two row indices. Both ends index `values`, but the rows on screen come from the filtered and sorted row model, so with a search active the range pulled in rows that were never rendered. Delete then cleared them. Walk the view instead and map each position back to a data index, the same shape #13729 used for the column select path. When the anchor has dropped out of the view, select just the clicked cell.
Collaborator
🪼 branch checks and previews
Install Gradio from this PR pip install https://huggingface.co/buckets/gradio/pypi-previews/resolve/dac57f250da34f16b80d90539b8c0ee0b82d3bb1/gradio-6.26.0-py3-none-any.whlInstall Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@dac57f250da34f16b80d90539b8c0ee0b82d3bb1#subdirectory=client/python"Import Gradio JS Client from this PR via CDN import { Client } from "https://huggingface.co/buckets/gradio/npm-previews/resolve/dac57f250da34f16b80d90539b8c0ee0b82d3bb1/browser.js"; |
Collaborator
🦄 change detectedThis Pull Request includes changes to the following packages.
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The PR adds a hand-authored .changeset/*.md file, which violates the repository’s documented PR rules and should be removed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes gr.Dataframe shift+click range selection so it operates in visible row (filtered/sorted) order rather than raw data-index order, preventing hidden-by-search rows from being unintentionally included in the selection (and then affected by Delete/Copy).
Changes:
- Update
handle_cell_clickrange selection to walk the current TanStack row model (view space) and map back to data indices viarows[p].original._index. - Add a regression test ensuring shift+click selection under
show_search="search"does not affect rows hidden by the search filter. - Add a
.changesetentry (but this repository’s contributor rules indicate changesets should not be hand-authored).
File summaries
| File | Description |
|---|---|
js/dataframe/shared/Table.svelte |
Builds shift+click ranges by iterating the filtered/sorted view rows and translating back to data indices, avoiding selection of hidden rows. |
js/dataframe/Dataframe.test.ts |
Adds a regression test that verifies Delete after shift+click does not clear search-hidden rows. |
.changeset/crazy-aliens-type.md |
Adds a hand-authored changeset file (repo guidance indicates these should be generated by CI, not committed manually). |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1
to
+6
| --- | ||
| "@gradio/dataframe": patch | ||
| "gradio": patch | ||
| --- | ||
|
|
||
| fix:Fix `gr.Dataframe` shift+click range selecting rows hidden by the search |
Removing the search-hidden rows from the range left no two members adjacent in the data, so `is_cell_selected` stopped suppressing the inner edges and the range drew as separate boxes instead of one outlined block. Take the neighbouring rows from the caller, which is rendering the view and knows them; that also closes the same gap on the column-select path from #13729. Also drop the dead `⋮` arm of the Delete assertion (the cell menu needs a single-cell selection) and cover the two behaviours the description calls out: the sorted-order span and the fallback when the anchor has dropped out of the view.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It adds a hand-authored .changeset/*.md file, which conflicts with the repository’s documented contribution workflow.
Review details
Suppressed comments (1)
.changeset/crazy-aliens-type.md:6
- This PR adds a hand-authored Changeset file, but this repository’s contributor guidelines explicitly say not to write
.changeset/*.mdfiles because a GitHub Action generates them from the PR title and a manual file can override the changelog entry. This file should be removed from the PR branch.
---
"@gradio/dataframe": patch
"gradio": patch
---
fix:Fix `gr.Dataframe` shift+click range selecting rows hidden by the search
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
- Hoist the neighbour row lookups to `{@const}` beside `row_idx`. They
depend only on the row, but sat in the per-column loop and so were
re-derived for every rendered cell, and the call site read as four
positional arguments.
- Take `null` rather than `-1` for "no row on that side". `-1` is not
distinguishable from a row index, so a caller passing `0` for absent
would have merged the edge into data row 0. Still required, so view
order cannot be opted out of by omission.
- Pin the other direction of the merge: a selected row that is adjacent
in the data but not on screen must not merge. Only the merging
direction was covered.
- Assert that the top of a sorted block draws its top edge. This is the
outline assertion that actually fails on `main`; the one in the search
test passes there, because the hidden rows sit in the selection and
make data adjacency hold anyway.
- Extract `sort_column` from the two copies of the header-menu sort
sequence, keeping its existence checks.
- Stop calling the previously selected cell "the anchor". It moves to
every click, so the word promised a spreadsheet behaviour this does
not have.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The PR adds a hand-authored .changeset/*.md file, which AGENTS.md explicitly disallows because the changelog entry is generated by automation from the PR title.
Review details
Suppressed comments (1)
.changeset/crazy-aliens-type.md:6
- This repository’s PR rules say not to add
.changeset/*.mdfiles manually; an Action generates them from the PR title, and a hand-authored file can override the intended changelog entry. Please remove this changeset file from the PR (and if a changeset is still required for some reason, ensure the entry text is correctly formatted).
---
"@gradio/dataframe": patch
"gradio": patch
---
fix:Fix `gr.Dataframe` shift+click range selecting rows hidden by the search
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Building the range in view order only covers the moment the range is made. Select a range while everything is on screen, then search, and the hidden rows stay in `selected_cells`: Delete blanked them and Ctrl+C copied them, with nothing on screen to say they were still in the selection. Both consumers now skip rows that are not in the row model, the same gate `handle_keydown` already applies to the cell the keyboard is on. The selection is left as it was, so it still survives a search and comes back when the query is cleared. Also pin the ring merge in the sort test. It asserted only which cells were selected and that the top edge was drawn, all of which still held with the old data-space `is_cell_selected`, so it never guarded the outline it was added for.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new “skip hidden rows” checks call a linear rows.findIndex once per selected cell, which can cause avoidable O(n²) slowdowns for large selections during Copy/Delete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
js/dataframe/shared/Table.svelte:1026
- In the Delete/Backspace handler, checking
visible_row_position(selected_row) !== -1inside the loop performs a linear scan ofrowsfor every selected cell. Precomputing aSetof visible row indices once avoids an O(selected_cells × visible_rows) slowdown for large selections.
const new_values = values.map((value_row) => [...value_row]);
selected_cells.forEach(([selected_row, selected_col]) => {
if (
!is_static_column(selected_col) &&
visible_row_position(selected_row) !== -1
) {
new_values[selected_row][selected_col] = "";
}
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
The visibility gate added to Delete and copy called `visible_row_position` once per selected cell, and that is a `rows.findIndex`. Selecting a whole column and then deleting is a realistic thing to do, so on a large table it went from one pass over the selection to a quadratic one. Build the set of visible row indices once per sweep instead.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There is at least one confirmed behavioral bug in the new copy-path (hidden-only selection can copy the entire table) and the PR adds a hand-written changeset file contrary to repo policy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.changeset/crazy-aliens-type.md:6
- Repository policy is to not add hand-written
.changeset/*.mdfiles because a GitHub Action generates them from the PR title; committing one here will override the automated changelog entry.
---
"@gradio/dataframe": patch
"gradio": patch
---
fix:Fix `gr.Dataframe` shift+click range selecting rows hidden by the search
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
`copy_table_data` reads a null selection as "copy the whole table", so filtering the visible rows out of `selected_cells` and handing the result straight over turned "copy one hidden cell" into "copy the entire dataframe". The toolbar Copy button reaches that path without the gate `handle_keydown` applies to Ctrl+C, and it still shows its copied state, so nothing said what had landed on the clipboard. Bail out instead when there is a selection but none of it is on screen. Delete had a quieter version of the same thing: with every selected row hidden it wrote nothing and still replaced `values` and pushed, firing change and input for an identical table. Only write when a cell was actually cleared. The search test named for the range fix had also stopped covering it, since the visibility gate on Delete satisfies its assertions on its own. Assert the selection instead: clear the query afterwards, so the skipped rows render again, and check they were never in it.
The guard added for a fully hidden selection returned without touching the clipboard, but `Toolbar.handle_copy` flips to "Copied to clipboard" whether or not anything was written, so the button claimed a copy that never happened. `handle_copy` now reports whether it copied and the toolbar only shows its copied state on true. The no-selection branch was still inconsistent with the rest of the rule: it passed null, and `copy_table_data` reads that as "copy every row in `values`", hidden ones included. Spell the cells out from `rows` instead, so both branches stay inside the view and null never goes over that boundary. Take the clicked cell's view position as an argument rather than looking it up again. The template knows it, the second `findIndex` scanned the whole filtered row model on every click, and its -1 case was unreachable, so the guard now reads as the one thing it is for: the previously selected row may be gone. Covers the two gates that had no test: Delete firing change and input for a table it did not touch, and the copy button reporting success.
Spelling the cells out to keep the copy inside the view took the column count from `resolved_headers`, but only the headers are padded out to the column count on the way in: `Dataframe.postprocess` normalizes `headers` to `len(data[0])` and leaves the rows as they came. A row shorter than the header row therefore produced coordinates that are not in `values`, and `copy_table_data` dereferences those without a guard, so copying with nothing selected threw. The rejection left the toolbar waiting, so the button simply did nothing. Take the bound from each row's own length, which is what `copy_table_data`'s null case did before. The row axis still comes from `rows`, so a search still limits the copy to what is on screen. Selecting a cell past the end of a short row and copying still throws, because `copy_table_data` has no bounds check at all. That predates this branch and reads as its own decision, so it is left alone.
`copy_table_data` dereferences `data[row][col].value` with no guard, but a selection can hold a cell that is not in `values`. Only the headers are padded out to the column count on the way in, the rows are left as they came, and every row renders a cell per header, so the phantom cells of a short row are clickable. A plain click on one, or the `Select row` button, which takes its columns from `resolved_headers`, then makes Ctrl+C or the copy button throw inside the reduce. This predates the branch: `main` hands `selected_cells` straight to `copy_table_data` and lands in the same place. The no-selection path was bounded at the call site instead, which left the two branches inconsistent, so the guard belongs in `copy_table_data`, where it covers every caller. The guard covers the missing cell and nothing else. A cell that is present still goes through `String`, so a `null` value keeps copying as "null" the way it does today, rather than being swallowed by a `??` that cannot tell the two apart. The comment on the ragged copy-all test described an intermediate state of this branch rather than `main`, where the null case was already bounded by each row's own length. Reword it, and add the case that is genuinely live.
`copy_table_data` throws when `navigator.clipboard.writeText` rejects, which happens on an insecure origin or without permission, and `handle_copy` awaited it with nothing around it. The toolbar reads the return value now, so that rejection propagates out of `Toolbar.handle_copy` and off the end of an event handler: no copied state, which is right, but also an unhandled rejection and no other sign that anything went wrong. The Ctrl+C path discards the promise outright, so it is unhandled there too. Catch it and return false, which is the same answer the guards above already give for a selection with nothing on screen. The test asserts the absence of an `unhandledrejection` rather than the absence of the copied state, since a rejection skips `copy_feedback()` on its own and the two are indistinguishable from the label alone. It stubs the clipboard, so the describe now restores mocks after every test: without that, a failure here leaves the stub in place and the next copy test reads it.
The guard added for a selection the view hides entirely set `cleared` on every write, a write of "" over an already empty cell included, so Delete on blank cells still replaced `values` and pushed. That fires `change` and `input` for a table nobody touched, which is the case the comment beside the guard claims to cover. Set `cleared` only when the cell held something, so the comment describes what the code does. `main` pushed unconditionally here, so this drops a redundant pair of events rather than one anything depended on.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It adds a hand-authored .changeset/*.md file, which the repo’s contributor guidance explicitly says not to include because the changelog entry is generated by GitHub Actions.
Review details
Suppressed comments (1)
.changeset/crazy-aliens-type.md:6
- This repository’s contributor guidance says not to add hand-written
.changeset/*.mdfiles because a GitHub Action generates them from the PR title, and an existing changeset will override the changelog entry (AGENTS.md:39-40). This changeset should be removed from the PR (and its summary text also looks malformed:fix:Fix ...).
---
"@gradio/dataframe": patch
"gradio": patch
---
fix:Fix `gr.Dataframe` shift+click range selecting rows hidden by the search
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Five small things, all in code added on this branch rather than in what it set out to fix. `copy_table_data` still took `selected_cells: [...] | null` and read `null` as "every row in `data`". The only caller spells the cells out in both branches now, so that path is dead, and leaving it in the signature keeps a way to reach past the view one argument away. The Delete guard compared against `""` alone. A cell past the end of a short row reads as `undefined`, so it was written and pushed, which fired change and input for a table that looks the same and quietly grew the row by a column it never had. Treat nullish as already blank, which also leaves a `null` as it was rather than normalising it to "". The `catch` around the clipboard write swallowed everything, not just a refusal, and returned the same `false` the "nothing on screen" guard returns, so a TypeError on the way there would have been invisible. Log it. The ragged copy test asserted only that the copied label appeared, which it does for any copy that did not throw, so it said nothing about what reached the clipboard. Assert the text. The `unhandledrejection` listener was removed on the last line of its test, so a failing assertion above would have leaked it into every later test, the same shape as the clipboard stub that leaked last round. Register the removal with `onTestFinished`.
Three fixes, two of them to conditions written wider than the case they were verified against. Hoisting the Delete read to a `const` put it ahead of the `visible.has` guard that used to short-circuit it, so a selection naming a row a shrunk table no longer has threw a TypeError inside the forEach and left the rows that were still there untouched. Read through `?.`. `current != null` skipped a `null` cell as well as a cell past the end of a short row. A `null` renders blank but it is a value the backend sent, and clearing it has always written "" and pushed, so the change was silent and unasked for. `!== undefined` covers the case the comment describes and nothing else. `copy_table_data` took its output columns from the lowest-indexed selected row alone, so a selection that is not rectangular there lost the rest of its cells with no error: Ctrl+click [0,0] and [1,1] and the clipboard held "a\n", "d" gone. Take the union of the columns every selected row contributes. Gaps were already filled with "" for the columns that did survive, so nothing new is being decided about how to render one. That last one is reachable with a rectangular table and two clicks, not only on the ragged data it was first spotted with.
No behaviour change. `Table.svelte` on `main` carries 19 comment lines in 1723; this branch had been adding 41 in 115, which is a diff a reviewer has to read twice to find the code in. Three kinds went: Stale. The note about spelling the cells out "rather than leaning on `copy_table_data`'s null case" outlived the null case itself, and the header comment on `copy_table_data` explained what a parameter it no longer takes used to mean. Duplicated. `handle_copy` and `copy_table_data` both explained that only the headers are padded out to the column count. Long. The clipboard `catch`, the Delete guard, the deref guard and the `row_above`/`row_below` note ran five lines each, and the last of those spent two of them arguing against making the parameters optional. Two or three lines each now. What is left is the part that is not in the code: why a Set sits beside `visible_row_position`, why the range guards its anchor, what the boolean return means. Two tests went the same way. The ragged copy-all case stopped discriminating once the deref guard moved into `copy_table_data`: the enumeration it was written against no longer throws, and produces the same text. And the two Delete no-op cases were the same `if` seen twice, so they are one test over a fixture that is blank both ways.
hysts
marked this pull request as ready for review
September 8, 2026 05:17
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.
Description
With
show_search="search"filtering the table, shift-clicking two visible cells selected every data row between them, including the rows the search was hiding. Those rows are never rendered, so they could not be highlighted and there was no way to see them before pressing Delete, which then cleared them.handle_cell_clickbuilt the range by walking the numeric range between the two row indices. Both ends are indices intovalues(the template passesrow.original._index), but the rows on screen come from the filtered and sorted TanStack row model, so the two coincide only when nothing is filtering or sorting. The range now walks the view and converts each position back to a data index withrows[p].original._index, the same shape #13729 used when it fixed this confusion in the column select path.The selection ring needed a matching change, to keep it looking the way it does today.
is_cell_selectedblanks out a range's inner edges by looking forrow ± 1in the selection, which is data adjacency. Onmainthat happens to work: the hidden rows are in the range, so the lookup finds them and the ring merges into one block. Take those rows out and the reason goes with them, and the range draws as separate boxes. Sois_cell_selectednow takes the neighbouring rows from the caller, which is rendering the view and knows which they are. Nothing about the outline changes for the user; this only stops the range fix from regressing it. It does close the same gap on #13729's column-select path, where a filtered column selection is non-contiguous in the data for the same reason.selected_cellsitself stays in data space, and building the range in view order only constrains the moment the range is made. Selecting a range and then searching leaves the hidden rows in the selection, and Delete blanked them and Ctrl+C copied them, with nothing on screen to say they were still in there. So both consumers now skip rows the view is not showing, which is the gatehandle_keydownalready applies to the cell the keyboard is on. The selection itself is left alone, so it still survives a search and comes back when the query is cleared.Once Delete and copy skip the rows the view is hiding, a selection can have nothing left to act on, so both need a guard for that case. Delete no longer assigns
valuesor pushes a change when it cleared nothing, since an identical table would still firechangeandinput. Copy stops rather than falling back oncopy_table_data's null case, which means the whole table, andhandle_copynow reports whether anything reached the clipboard so the toolbar shows "Copied to clipboard" only when it did.The copy button carries the same confusion on a second path: with nothing selected, copy passed
nullas well, so it copied every row invalues, hidden ones included. It now enumerates the rows the view is showing. To see it, search with no cell selected and press the toolbar's copy button. With no search active the copied text is unchanged, sincerowsis then every row.Routing every copy onto the explicit-cells argument brought out one more thing in
copy_table_data, and this one is worth stating because it changes what a copy contains. It took its output columns from the lowest-indexed selected row alone, so a selection that is not rectangular in that row lost the rest of its cells with no error. Ctrl+click[0, 0]and[1, 1]on a two-by-two table and the clipboard held"a\n", with"d"simply gone. The columns are now the union of what every selected row contributes, so that copy is"a,\n,d". Gaps in the columns that did survive were already filled with an empty string, so this decides nothing new about how to render one.One fix here is not about the view at all.
copy_table_datadereferenceddata[row][col].valuewith no guard, and since only the headers are padded out to the column count, a row shorter than the header row still renders a cell per header. Clicking one of those and copying threw inside the reduce, onmainas much as here. The guard now lives incopy_table_data, so both of this component's copy paths and any other caller are covered.Two further effects worth calling out:
The loop being changed here dates from #13150, but the same index-range enumeration predates it in
shared/utils/selection_utils.ts(no longer wired up to the component), so this path has never worked correctly under a filter.copy_table_datastill emits the selected rows in ascending data-index order, so under a sort the clipboard order does not match the screen. That is unchanged from before and only easier to notice now that the selected set is the visual span; it deserves its own issue rather than widening this one.Closes: #13791
Before / after Spaces
The same app on
mainand on this PR, if you would rather not check anything out. The before Space is not a released build: it runsmainas of 82ca180, so #13729 is already in it and what you see there is what survives that PR.main, #13729 included)Each Space has two tables, because the two cases show up differently.
The sort table is where the outline differs. Sort
agedescending so the screen readsD B C A, then shift-click fromD's name cell toB's. On the before SpaceDis missing its top edge, a line runs betweenDandB, andCis selected too, one row below where the drag ended; after, it is a closed block aroundDandBalone.The search table is where nothing differs to look at. Search
target, click thetagcell ofidx0 and shift-clickidx4: the highlight is identical on both, which is the point. Press Delete and clear the search, and on the before Spaceidx1 and 3 have lost their values as well.AI Disclosure
We encourage the use of AI tooling in creating PRs, but the any non-trivial use of AI needs be disclosed. E.g. if you used Claude to write a first draft, you should mention that. Trivial tab-completion doesn't need to be disclosed. You should self-review all PRs, especially if they were generated with AI.
🎯 PRs Should Target Issues
Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.
Not adhering to this guideline will result in the PR being closed.
Testing and Formatting Your Code
PRs will only be merged if tests pass on CI. We recommend at least running the backend tests locally, please set up your Gradio environment locally and run the backed tests:
bash scripts/run_backend_tests.shPlease run these bash scripts to automatically format your code:
bash scripts/format_backend.sh, and (if you made any changes to non-Python files)bash scripts/format_frontend.sh