Repository navigation
Feature/implement sess-aware html viewer - #1852
Conversation
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 |
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.
- 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
- Remove the custom toolbar and internal iframe workaround - Show session information through the tab title - Preserve process identity, exit handling, and history across reloads
6c1422d to
71b794a
Compare
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Follow-up review of 71b794a against the previously reviewed 6c1422d. The feature has been rebased onto current main, but the HTML rendering implementation is unchanged. Both earlier P2 findings remain unresolved: tag/script preservation and authored resource bases.
The rebase also introduces a P1 build blocker, detailed inline: main removed fs-extra and its declarations, but this PR still imports it in the viewer and two test files. I reproduced compilation failing with Could not resolve "fs-extra" and typecheck reporting TS2307 for all three imports in an isolated snapshot using only dependencies declared by the latest manifest. The latest packaging and all three platform test jobs fail on that same compile error; lint also fails. The earlier 84 passing tests apply to 6c1422d, not this revision.
| import { readContent, UriIcon } from '../util'; | ||
| import { extensionContext } from '../extension'; | ||
| import { randomUUID } from 'crypto'; | ||
| import { readFile } from 'fs-extra'; |
There was a problem hiding this comment.
[P1] Migrate the remaining fs-extra imports after rebasing
Current main removed fs-extra and @types/fs-extra from package.json and the lockfile, so this import prevents the extension from building with the latest dependency set. I reproduced node esbuild.js failing at this line with Could not resolve "fs-extra"; the packaging job and macOS/Linux/Windows test jobs fail for the same reason. Typecheck also reports TS2307 here and in src/test/suite/htmlWidgetViewer.test.ts:2 and src/test/suite/dataViewerSessions.test.ts:2. Please use node:fs/promises for this read and migrate the two tests' imports/stubs to match before rerunning the checks. A local installation retaining the removed package can mask the failure.
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed the latest head, 6296d15, including the changes since 71b794a. All three findings from my earlier reviews are addressed: the undeclared fs-extra imports are removed, HTML source splicing preserves authored scripts and SVG titles, and local authored base URLs resolve correctly with the required resource roots.
Validation in an isolated snapshot with frozen-lockfile dependencies: build, TypeScript compilation, and whole-source lint passed; 22 Node/content tests and 79 focused extension tests passed on VS Code 1.110.0. An additional reproduction through RMarkdownKnitManager exposed one new P2 regression from the js-yaml upgrade, detailed inline: merged frontmatter silently loses its custom knit function because the Knit parser still uses the new default schema.
eitsupi
left a comment
eitsupi
left a comment
There was a problem hiding this comment.
I haven't looked into it in detail, but could you revert version updates for unrelated dependent packages?
|
I have restored package dependencies. |
eitsupi
left a comment
eitsupi
left a comment
There was a problem hiding this comment.
ChatGPT-assisted follow-up review (752621b)
I found one concern:
Local resource access scope: In getWebviewHtml(), an authored <base href="/"> causes the filesystem root to be added to localResourceRoots. This behavior is also explicitly covered by a test. Since the displayed HTML can execute scripts, allowing arbitrary document-provided base paths to expand the Webview's filesystem access seems unsafe. Could we restrict additional resource roots to trusted directories while preserving support for legitimate relative dependencies?
One non-blocking UI suggestion: all four toolbar actions are still in the navigation group. I would prefer keeping Back/Forward visible and moving Remove/Info into the overflow menu. The Info command could also be named more explicitly to indicate that it toggles session information in the tab title.
- Keep the current output visible while the next page loads - Disable navigation buttons until the selected page is ready - Preserve resource-access restrictions - Add regression tests for smooth transitions and button states
eitsupi
left a comment
eitsupi
left a comment
There was a problem hiding this comment.
ChatGPT-assisted follow-up review (27cdc1f)
One remaining concern is the new widget/loaded handshake. If the injected script cannot run (e.g., an HTML document with script-src 'none') or the window load event never completes, entry.loading remains true indefinitely, disabling Back, Forward, and Remove.
Could we provide a fallback so navigation remains usable even when the acknowledgement cannot arrive?
- Show loading progress in the VS Code status bar - Unlock navigation after five seconds while retaining the loading message - Clear progress on completion, failure, replacement, or disposal - Cover timeouts, stale loads, and CSP-blocked confirmations
eitsupi
left a comment
eitsupi
left a comment
There was a problem hiding this comment.
ChatGPT-assisted follow-up review (e2c6b5a)
startLoading() leaves the window.withProgress() task pending after the five-second fallback. For HTML with script-src 'none', widget/loaded cannot arrive, so navigation recovers but the window-level “Still loading” progress indicator remains until the output is replaced or the panel is closed.
Please end the progress task when the fallback releases navigation, reusing entry.finishLoading?.() and then updating the toolbar. Ending the wait does not need to imply that the HTML loaded successfully.
The timeout test should also assert that progress completes without receiving widget/loaded, rather than expecting task.done === false.
I intentionally kept this behaviour as during loading a large html or an unknown successfulness loading users could navigate away, delete or close the window to dismiss the progress indicator. |
|
I see. |
- Show “Load not confirmed. Please wait or retry.” after ten seconds - Keep the five-second navigation fallback after HTML assignment - Clear loading status on completion, replacement, or viewer closure - Remove unused test configuration and obsolete test mocks - Update loading and CSP regression tests
Updated with a 10s final state static message if load is still not confirmed. User actions such as navigating away, rendering again, removing or closing would dismiss the message. Switching to another tab hides the message as well. Comments are also added to the codes. |
eitsupi
left a comment
eitsupi
left a comment
There was a problem hiding this comment.
One minor documentation note: the PR description still mentions iframe-based rendering, although the implementation now uses the native editor toolbar without that iframe.
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.Add Back/Forward, deletion, and session-info icons in the VS Code native editor toolbar.Isolate complete widget documents in an iframe so their styles do not affect the toolbar.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.Build, typecheck, lint, and tests passed on Windows, macOS, and Linux. Coverage includes session ownership, concurrent loads, navigation controls, persistence and restoration, process exit cleanup, viewer routing, loading timeouts, CSP-blocked acknowledgements, resource access restrictions, and real VS Code Webview rendering with relative dependencies.