Fix blank Data Explorer grid for matrices with row names - #1357
Merged
Conversation
juliasilge
added a commit
to posit-dev/positron
that referenced
this pull request
Jul 22, 2026
…5050) This is the Positron-side hardening half of the fix for #12547. The Ark root-cause fix is posit-dev/ark#1357. ### Summary When the Data Explorer opens, the first cache update can run before the viewport rows are laid out, so it carries an empty row selection. For a backend that reports row labels (an R matrix with row names does), that produced a malformed `get_row_labels` request that the backend rejected. `TableDataCache.update()` set its in-progress flag with no `try/finally`, so the rejected request threw out of the method before the flag was cleared. Every subsequent update (scrolling, resizing, sorting) then parked itself behind the in-progress guard and returned, so the grid never repainted: it stayed blank even though the next request would have succeeded. This PR hardens `TableDataCache.update()` so a single failed backend request degrades one paint instead of permanently wedging the viewer: - Wrap the update body in `try/catch/finally` so the in-progress flag is always cleared and the pending-update descriptor still drains, even when a backend request rejects. This mirrors the same hardening already applied to `TableSummaryCache.update()`. This makes the git diff look bigger than the substance of the change here! - Skip the `get_row_labels` request entirely when the row selection is empty, so the malformed request is never sent. - Decouple row-label loading from cell-data loading. On an invalidating refresh (sort or filter), row labels are now fetched after the data cache is cleared, refetched, and repopulated, and a row-labels failure is isolated so it degrades to missing labels instead of aborting the data refresh and leaving stale cells on screen. Either side alone unblanks the viewer. The two changes are independent and can land in either order. ### Release Notes #### New Features - N/A #### Bug Fixes - Fixed the Data Explorer grid remaining blank after a failed backend request (#12547). ### Validation Steps @:data-explorer Automated: `npx vitest run src/vs/workbench/services/positronDataExplorer/test/common/tableDataCache.vitest.ts` covers flag-clear on rejection, pending-descriptor drain on failure, the empty-selection skip, and data-refresh isolation from row-label failures. Manual (the un-blanking is verifiable against the current vendored Ark): 1. In an R session, confirm the grid renders instead of showing blank: ```r m <- matrix(1:4, nrow = 2) rownames(m) <- c("A", "B") View(m) ``` <img width="1402" height="1003" alt="Screenshot 2026-07-21 at 7 09 26 PM" src="https://github.com/user-attachments/assets/d01af619-5780-461c-bbe6-fd959d833252" /> 2. Scroll after opening a larger matrix and confirm it keeps repainting: ```r big <- matrix(1:2000, ncol = 2) rownames(big) <- paste0("r", 1:1000) View(big) ``` 3. Confirm regressions are unaffected: `View(matrix(1:4, nrow = 2))` (no row names), `View(mtcars)`, and `View(iris)` all render normally.
thomasp85
approved these changes
Jul 23, 2026
thomasp85
left a comment
thomasp85
left a comment
Collaborator
There was a problem hiding this comment.
LGTM - a small non-blocking comment
| }, | ||
| // Defense in depth: `row.names()` returns `NULL` for a table with no | ||
| // row names, so treat that as "no labels" rather than an error. | ||
| NILSXP => Ok(vec![]), |
Collaborator
There was a problem hiding this comment.
I get the wish for added defensive coding but I don't think this path is ever possible to reach. We test for whether row names are null (line 1017) before asking for them so it should never be able to resolve to null with the new fix.
Not blocking, but maybe worth a mention as you could otherwise assume this was an actual possibility
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Positron Release Notes
New Features
Bug Fixes
Description
View()on an R matrix with row names opened the Data Explorer but showed a blank grid. Data frames were fine, and matrices without row names were fine.The root cause is in
get_row_labels. When the Data Explorer opens, Positron's first cache update runs before the viewport rows are laid out, so it sendsget_row_labelswith an empty row selection. Ark implemented that request by subsetting the table to the selected rows and callingrow.names()on the result. A zero-row subset of a matrix drops its row names toNULL(unlike a data.frame, which returnscharacter(0)), and the strict match on the result then errored with "row.namesshould be strings, got 0". That single failed RPC was enough to leave the grid blank.This PR fixes the Ark side:
get_row_labelsnow early-returns an empty result when the resolved row selection is empty, skipping the R subset call entirely.NILSXPresult fromrow.names()is treated as "no labels" rather than an error. The error is kept for other unexpected kinds.Manual verification
Build a dev Ark (
cargo build), point Positron at it with thepositron.r.kernel.pathsetting (target/debug/ark), run Developer: Reload Window, and start a fresh R session.Minimal repro renders with row labels
AandB, norow.nameserror in the console:Scrolling after open still repaints correctly with the right row labels:
The
as.matrix(data.frame)variant from the issue renders with its row names:View(matrix(1:4, nrow = 2))(no row names),View(mtcars), andView(iris)all render normally.Automated:
cargo testcovers the newtest_row_names_matrix_empty_selectionintegration test alongside the existing matrix row-label tests.A follow-up as well
This Ark fix removes the failing RPC, which is enough to unblank the viewer on its own. I'm going to do a separate Positron PR to harden
TableDataCache.update()so that any future failing backend request degrades a single paint rather than permanently wedging the viewer. We'll wrap the update body in try/finally so_updatingis always cleared and the pending-descriptor drain still runs, and skip the row-labels RPC entirely when the row selection is empty. That change is independent of this one and will ship with its own vitest coverage.