Feature/list viewer 1399 - #1741
Conversation
|
I will leave this to 3.x milestone. Here is the current behavior. Screen.Recording.2026-10-03.at.7.03.23.pm.mov |
3973a3b to
5796fdf
Compare
renkun-ken
left a comment
There was a problem hiding this comment.
I reproduced five issues in revision 34bf0c134386187927c20eba6f7aa638ad8ae7f3: repeated evaluation of active root bindings, incorrect reconstruction of custom $ expressions, missing parent breadcrumbs when opening vectors from the List Viewer, loss of named-vector element names, and unbounded expansion of scalar POSIXlt values. The inline comments include reproduction details and suggested fixes.
Validation: TypeScript compilation, ESLint, 17 webview tests, and 212 R assertions passed. Targeted reproductions exposed these gaps; comparisons against the base revision confirmed the previous behavior for active bindings, custom $ extraction, named vectors, and POSIXlt values.
| } | ||
| if (!is.symbol(expression)) return(NULL) | ||
| name <- as.character(expression) | ||
| root <- listview_state(get(name, envir = envir, inherits = TRUE), name, owner) |
There was a problem hiding this comment.
[P2] Avoid evaluating the root binding twice
View(x) first forces x to determine its viewer type, then this get() evaluates the binding again. An active binding returning list(value = counter) runs twice, and the viewer displays the second value. The base revision evaluates it once. Reuse the evaluated argument, or skip breadcrumb reconstruction when it would force an active binding again.
| value <- expression[[3L]] | ||
| if (operator %in% c("$", "@") && is.symbol(value)) value <- as.character(value) | ||
| if (length(value) != 1L || !(is.character(value) || is.numeric(value))) return(NULL) | ||
| selectors <- c(list(list(kind = "index", value = value)), selectors) |
There was a problem hiding this comment.
[P2] Preserve the original extraction operator
Every expression selector becomes an index selector, so View(x$a) reconstructs its value using x[[index]]. For an S3 list class with a custom $ method, these can return different objects: I reproduced View(x$a) displaying 1 although x$a returns 42. Preserve the extraction semantics or fall back to the evaluated argument when the reconstructed value differs.
| if (listview_is_list(location$data)) { | ||
| return(listview_navigation(state, c(path, list(index)))) | ||
| } | ||
| workspace_show_view(location$data, location$title, state$owner %||% state$title) |
There was a problem hiding this comment.
[P2] Retain parent breadcrumbs when opening vectors
For x <- list(a = list(values = 1:3)), opening values through the List Viewer calls workspace_show_view without context. The Vector Viewer consequently retains only the vector, with path [] and one breadcrumb labelled x$a$values. Users cannot navigate to a or x. Pass the retained root and descendant path, as the workspace entry point does.
| label <- if (!is.null(view_id) && isTRUE(vector_view)) { | ||
| paste0("[", index, "]") |
There was a problem hiding this comment.
[P2] Preserve named vector element names
View(c(one = 1, two = 2)) now displays only [1] and [2]. child_name is available here but discarded, while scalar extraction also removes the name from the displayed value. The previous viewer preserved both names. Include element names alongside their indices so named vectors remain interpretable.
| if (!is.null(view_id)) { | ||
| list( | ||
| label = label, str = summary, viewable = !isTRUE(vector_view), index = index, | ||
| has_children = !isTRUE(vector_view) && workspace_child_count(child) > 0L |
There was a problem hiding this comment.
[P2] Terminate expansion for scalar POSIXlt values
View(as.POSIXlt(c('2025-01-01', '2025-01-02'))) produces expandable timestamp rows. POSIXlt is internally a list, but scalar[[1]] returns an equivalent scalar, so each expansion creates another identical expandable row indefinitely. The previous viewer showed finite timestamp values. Treat POSIXlt elements as leaves or route them through a value formatter.
Thanks @renkun-ken. Some of those are being addressed right now. |
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed the latest revision, 5c807f67be2d5d3609b88ba6d47b428923ade087. Four previous findings are fixed: the active-binding, custom-extraction, vector-breadcrumb, and POSIXlt reproductions now pass.
Two issues remain: named vectors still lose their element names, and difftime vectors lose their units during scalar extraction. The inline comments describe the remaining behavior and suggested fixes.
Validation: build, TypeScript compilation, ESLint, 27 viewer tests (including nine VS Code panel and session-ownership tests), and 417 R assertions passed. Targeted reproductions confirmed the two display issues; the base revision preserved duration units.
| label <- if (vector_rows) { | ||
| paste0("[", index, "]") |
There was a problem hiding this comment.
[P2] Named vectors still lose their names
The previous finding remains unresolved: View(c(one = 1, two = 2)) displays only [1] and [2], with values 1 and 2. child_name is available but discarded. Include names alongside indices so users can interpret named vectors.
| child <- switch(kind, | ||
| name = get(child_name, envir = object, inherits = FALSE), | ||
| slot = methods::slot(object, child_name), | ||
| index = object[[index]] |
There was a problem hiding this comment.
[P2] Preserve difftime units when formatting values
Extracting object[[index]] strips the difftime class and units before formatting. View(as.difftime(c(1, 2), units = 'hours')) and the equivalent vector in minutes both display identical bare values 1 and 2. The base revision preserved units. Use a class-preserving slice for vector formatting or explicitly include duration units.
07f39de to
997730c
Compare
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed revision 90ad9c34797333331799a18a554b6cd0aac5da64. All previous findings are resolved: active bindings are evaluated once, custom extraction displays the evaluated value, vector navigation retains parent breadcrumbs, named vectors retain their labels, POSIXlt expansion terminates, and difftime values retain their units. The original targeted reproductions now pass.
I found no remaining blocking issues. There is one nonblocking test correction inline: the pending table-request test needs to include the document generation so it reaches the asynchronous disposal path.
Validation: TypeScript compilation, ESLint, build, 28 viewer tests (including 10 VS Code panel/session tests), and 819 R assertions passed. I also reran the 10 panel/session tests with the missing generation field added in the temporary review checkout; they passed.
| const listener = receive.firstCall.args[0] as (message: unknown) => Promise<void>; | ||
| // With no R session, requests settle asynchronously with an unavailable response. | ||
| const pending = source === 'table' | ||
| ? [listener({ message: 'dataview/request', action: 'page', requestId: 1 })] |
There was a problem hiding this comment.
[P3] Include the document generation in the pending table request
The table request omits documentGeneration, so attachDynamicDataViewBridge rejects it at its new validation guard before calling sessionRequest. This makes the table version of the panel-close test pass without exercising a pending request during disposal. Add the captured documentGeneration to this message, as the list branch already does, so the test covers the intended race. I verified that the panel/session tests still pass with this field included.
eitsupi
left a comment
There was a problem hiding this comment.
ChatGPT review found one remaining concern: this PR changes the sess IPC contract for list/data viewers, but protocol_version is still 1.
Since old/new extension and sess combinations are no longer compatible for these messages, I think this should bump the protocol version and update the sess protocol documentation accordingly.
Also, the existing non-blocking comment about adding documentGeneration to the pending table-request test looks worth fixing at the same time.
eitsupi
left a comment
There was a problem hiding this comment.
ChatGPT review: small nit — do we need to vendor these Codicon SVGs?
Since these are built-in VS Code icons, it might be cleaner to use ThemeIcon for the panel icon and @vscode/codicons inside the webview instead of committing copies of the SVGs and maintaining their attribution separately.
Not blocking, but I think using the standard Codicon assets would be simpler and more theme-friendly.
Good point. There is no strong reason to vendor the SVGs. I'll switch the icons to use @vscode/codicons directly where possible. However, the codicon CSS and font used in the webview are still bundled from the package as support for Tab 's original UriIcon with ThemeIcon seems only announced from VS Code 1.110. Maybe we should add a third-party notice in the source to cover those assets and the R icon. |
43b78ca to
109dc25
Compare
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed revision 109dc25d8399e2227197f62bac9a0207967e868a. The pending-request test correction is fixed, the protocol-2 contract is documented, and the packaged Codicons CSS, font, and licenses are included in the generated VSIX. The previous viewer reproductions still pass, including named vectors and duration units.
One upgrade issue remains: protocol 2 is required, but the bundled sess package still has the same version as the protocol-1 package. The version-only installation checks therefore skip the update. I reproduced the generated Attach script skipping an existing sess 3.0.9000.9000 installation and then receiving Cannot attach R session: unsupported sess protocol version 1; this extension requires protocol version 2. The inline comment describes the fix.
Validation: TypeScript compilation, ESLint with no errors, production build/VSIX packaging, 42 viewer and integration tests, and 994 R assertions passed. These include the new Interactive nested-table session routing and pending table-request tests. A separate targeted upgrade reproduction confirmed the installation gap.
| if (is.null(host) || is.na(host)) host <- "" | ||
| list( | ||
| protocol_version = 1L, | ||
| protocol_version = 2L, |
There was a problem hiding this comment.
[P2] Make the protocol bump trigger a sess package update
The protocol changes to 2 while sess/DESCRIPTION remains 3.0.9000.9000, the same version as the protocol-1 package on the base revision. Both promptToInstallSessPackage and the generated Attach script compare only package versions, so an existing installation is considered current and never replaced. I reproduced the generated Attach script skipping the old installation, followed by the extension rejecting its handshake with unsupported sess protocol version 1; this extension requires protocol version 2. Bump the sess package version together with this protocol change, or check protocol compatibility in both installation paths, so existing users can attach after upgrading.
…names retain [index] labels.
- reuse data viewer formatting for list and vector values - route scalar value to the shared list viewer for consistency
Replace copied List Viewer control SVGs with @vscode/codicons. Bundle its CSS, font, and licenses. Move attribution from README into third-party notices covering Codicons, existing viewer tab icons, and the R logo.
e8ddfb3 to
5fd29ce
Compare
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed revision 5fd29ce0c0d27a8f999e735284622ba2f36e8947. The previous upgrade finding is resolved: the package version is bumped to 3.0.9000.9001, and the rebased source-identity checks also detect an old installation. I reran the generated Attach command against the prior protocol-1 package in a temporary library; it installed the bundled copy and connected with protocol 2.
One packaging issue remains: .vscodeignore contains unresolved rebase conflicts. Both sides' inclusion rules are active, so the VSIX ships the raw, unstamped sess/ tree and R/tests/ in addition to the prepared package. The inline comment describes the required conflict resolution.
Validation: TypeScript compilation, ESLint with no errors, production build/VSIX packaging, packaged source-identity verification, 53 targeted viewer/integration/installation tests, and 994 R assertions passed. The separate upgrade reproduction passed as well. macOS CI remains red on the existing workspace-focus assertion; that test passed in the local targeted run, so I have not established a PR runtime regression from that failure.
| !R/** | ||
| !sess/** | ||
| >>>>>>> 109dc25 (refactor(viewer): use packaged codicons and consolidate icon notices) |
There was a problem hiding this comment.
[P2] Resolve the rebase conflicts in the packaging allowlist
Both conflict blocks are still committed. vsce treats the markers as ordinary patterns and applies both sides, so !R/** and !sess/** undo the base's selective exclusions. I inspected the generated VSIX: it contains 47 raw sess/ files with the unexpanded source-revision placeholder, plus R/tests/sess_source.R, alongside the prepared dist/resources/sess/ package. Resolve both blocks by keeping the base's selective R paths and prepared sess inclusion, then add the notices, Codicons font, and license entries. I verified that this removes the 48 unintended files while retaining all required resources.
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed revision b470e57b6e4e3edc40e883b4035855c4ba84e050. The previous packaging finding is resolved: both conflict blocks are removed, the selective R inclusion rules are restored, and the prepared sess package, Codicons font/CSS, and license notices remain included. I inspected the generated VSIX and confirmed that the raw sess/ tree and R/tests/ are excluded, while the bundled sess DESCRIPTION has the correct source identity and bootstrap disabled.
This revision changes only .vscodeignore; the previously reviewed runtime code is unchanged. Local production build, VSIX packaging, packaged source-identity verification, and archive-content checks passed. All current CI checks are green, including build, lint, and macOS/Linux/Windows tests. No further actionable findings.
6b044fe to
246cb7f
Compare
Requested protocol version update has been resolved.
Sorry, unreleased version yet, so version 1 is ok.
Closes #1399
Closes #1687
Closes #1576
Summary
Adds dedicated viewers for R lists and vectors, allowing large and deeply nested objects to be inspected without serialising the complete object when the viewer opens.
List Viewer
View()opens lists, environments, and S4 objects in a dedicated List Viewer.str()summary.$name,[[index]], and@slotas appropriate.deparse()of large vectors.Nested navigation
View()calls for descendants of the same top-level object reuse the corresponding viewer.On-demand loading
Session-aware viewers
Viewer lifecycle fixes
state_generationto viewer state so delayed disposal requests cannot remove a newer viewer state that reused the sameview_id.documentGenerationto List Viewer and Data Viewer webview requests/responses so messages from a previous webview document are ignored after the panel is reloaded.state_generation) from webview document lifetime (documentGeneration).plot_backendargument and the requiredstate_generationwhen disposing viewer state.Tests
Adds regression coverage for: