Repository navigation
Conversation
- Keep up to 50 HTML outputs per session while the Viewer is open - Add Back/Forward buttons, keyboard shortcuts, and session info - Isolate widget content in an iframe - Handle stale loads, missing files, and Viewer disposal - Add navigation, session ownership, and browser rendering tests
- Retain session history and selection after the Viewer tab closes - Persist history across VS Code reloads and session reconnects - Add R: Restore HTML Viewer and reuse existing session panels - Clear saved history when the originating R process exits - Add lifecycle, persistence, and browser restoration tests
- Share viewer, page_viewer, and local browser HTML in session history - Include profvis outputs in navigation and restoration - Normalise tab names to HTML Viewer - Preserve output titles and existing HTTP/HTTPS browser routing
Fred-Wu
changed the title
eitsupi
left a comment
eitsupi
left a comment
There was a problem hiding this comment.
ChatGPT-assisted review
I reviewed this PR with ChatGPT, focusing primarily on separation of concerns and maintainability.
Overall, the session-scoped HTML Viewer, retained history, and lifecycle handling look well thought out. The new "ViewerSessionContext" is also a useful improvement.
I have a few concerns about the implementation boundaries:
-
Session management and Viewer responsibilities
"webViewer/index.ts" imports "activeSession" and "getViewerSessionContext" from "session.ts", while "session.ts" already imports the Viewer, creating a circular dependency.
Ideally, the Viewer should manage presentation and history without knowing how sessions are discovered or resolved. Could we pass the necessary session resolver and active-session accessor when initializing the Viewer manager? This seems like a small improvement in responsibility separation rather than a need for a larger abstraction.
-
Reliance on VS Code internals
"webViewer/webview/toolbar.ts" constructs a "fake.html" iframe URL using VS Code-specific internal parameters such as "id" and "vscode-coi".
This concerns me because the rendering implementation now depends on undocumented VS Code behavior. Could we investigate an approach based on supported Webview APIs? If this workaround is unavoidable, its assumptions and compatibility risks should at least be documented clearly.
-
Session identity consistency (minor)
"WidgetHistoryStore.remember()" distinguishes processes using host/PID, but "HtmlWidgetViewerManager.open()" reuses panels based only on "sessionId". It would be good to clarify the identity contract or ensure these two components behave consistently if a session ID is reused for a different process.
These concerns are about the current design rather than hypothetical future backends. I would avoid introducing additional abstractions solely for future arf integration (YAGNI). The existing "ViewerSessionContext" is a reasonable starting point, and keeping responsibilities clearly separated should be sufficient for now.
Thanks for the substantial work on this feature!
Thanks for reviewing this @eitsupi. I agree that this could cause compatibility issues if VS Code changes those parameters in the future. Using the native editor toolbar would be an alternative, but we would just have less control over its layout and behaviour. Also, given the unknown number of extensions users may have installed that add toolbar buttons, the toolbar could become crowded. Here is what it looks like if using the native toolbar Screen.Recording.2026-10-08.at.11.08.31.pm.mov |
- Remove the custom toolbar and internal iframe workaround - Show session information through the tab title - Preserve process identity, exit handling, and history across reloads
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed the latest revision, 6c1422d. The injected session access and native editor toolbar address the earlier circular dependency and private iframe concerns. I found two rendering regressions and left inline comments with concrete reproductions.
Validation: built and typechecked an isolated snapshot; focused lint passed; 22 Node/content tests and 62 extension tests passed locally (VS Code 1.110.0). Additional browser reproductions failed for inline script/tag preservation and an existing resource base: a valid script containing '</body>' stops executing, SVG/script <title> content is removed, and <base href="assets/"> makes fetch('data.json') request the output directory instead of assets/data.json. These cases are not covered by the passing tests.
| let html = /<head\b[^>]*>/i.test(source) | ||
| ? source.replace(/<title\b[^>]*>[\s\S]*?<\/title\s*>/gi, '').replace(/<head\b[^>]*>/i, match => match + head) | ||
| : `<!doctype html><html><head>${head}</head><body>${source}</body></html>`; | ||
| const script = `<script src="${escapeHtml(String(scriptUri))}" data-generation="${generation}" data-session-owned="${sessionOwned}" data-viewer-state="${escapeHtml(JSON.stringify(state ?? null))}"></script>`; | ||
| html = /<\/body\s*>/i.test(html) ? html.replace(/<\/body\s*>/i, script + '</body>') : html + script; |
There was a problem hiding this comment.
[P2] Restrict document edits to actual head/body elements
These regex replacements also match tag-shaped text inside scripts and SVGs. For example, a valid standalone page containing <script>const example = '</body>'; window.example = example;</script> gets the viewer script inserted into that string, including a closing </script> that terminates the original script; I reproduced the page's scripts failing in a real VS Code webview. The global title replacement likewise removes an SVG's accessible <title> and alters strings such as '<svg><title>Accessible chart</title></svg>' (Plotly's JS bundle contains this pattern). Please locate the actual document head/title and body boundary with an HTML-aware parser or tokenizer, preserving script text and SVG titles, and cover these cases in the rendering tests.
| worker-src https: data: filesystem: blob:; | ||
| frame-src https: data: blob:; | ||
| `; | ||
| const head = `<meta http-equiv="Content-Security-Policy" content="${CSP}"><base href="${escapeHtml(baseUri)}/"><title>${escapeHtml(title)}</title><link rel="stylesheet" href="${escapeHtml(String(styleUri))}">`; |
There was a problem hiding this comment.
[P2] Preserve an output's existing resource base
The new base is prepended before the output's own <base>, so the browser ignores the authored base and resolves every relative resource against the HTML file's directory. I reproduced this with index.html containing <base href="assets/">, an existing assets/data.json, and fetch('data.json'): the webview requests .../data.json and fails instead of loading .../assets/data.json. Absolute authored bases are overridden too. Please inject the directory base only when no effective base is present; when an existing base is relative, resolve it against the original file location and convert local targets to webview URIs. Add a rendering test with an authored base and a relative resource.
3 participants
Closes #1421
Summary
HTML outputs previously opened a new tab each time, without session-specific navigation or restoration. This PR introduces one reusable HTML Viewer per R session, covering widgets such as Plotly, flextable, and profvis, plus other standalone HTML pages.
Changes
viewer,page_viewer, andbrowseURL()in the same session history.Existing viewer placement and disable settings are preserved. Shiny applications and other HTTP/HTTPS pages continue to use their existing browser route.
Scope
History stores file references rather than copies of HTML outputs or dependencies; those files must remain available. R Markdown Preview and Interactive’s expanded-output panels retain their separate implementations.
Validation
Build, typecheck, lint, and focused tests passed. Coverage includes session ownership, concurrent loads, navigation controls, persistence and restoration, process exit cleanup, viewer routing, and real VS Code iframe rendering with relative dependencies.