Repository navigation
refactor(webview): render webview documents from TSX views - #325
Conversation
bef9d63 to
c426d41
Compare
eFAILution
left a comment
There was a problem hiding this comment.
Ran through the manual test notes in a real VS Code with the packaged build. Browser, details panel from both entry points, no-sources and errors views all render and every button works, no CSP violations. Thanks Simon!
|
Ran the manual test notes against the packaged build in a real VS Code. The browser, the details panel (from both the Details button and the hover link), and the no-sources and errors views all render with their stylesheets. Every control I tried works: search, expand/collapse, the error banner details, the version dropdown and its context menu, Insert, Select All, Refresh Versions, switching versions, Refresh, Update Cache and Reset Cache. No CSP violations in the webview console. I also injected an inline script to confirm the check would catch one, and it did. A few things I couldn't reach: Update Token (couldn't get GitLab to return an auth error), the single fatal error view (same stylesheet and script as the errors view, which passed), Load Versions (every card already had versions), and the raw YAML toggle (sast has none). The unit tests cover their markup, so I'm not worried about them. Two things I noticed that aren't from this PR. Insert from the details panel drops the component onto the current line when the cursor sits at the end of an existing component line. And a source with an unreachable host doesn't show up on the errors view at all. Probably worth opening issues for both. |
Summary
preact-render-to-string. Now that the webview CSS and JS are external, this was the last markup thattscand eslint saw as an opaque string.escapeHtmlcalls are gone. Raw HTML is allowed only in two helpers, which eslint enforces.webviewBuilderMarkup.test.tsnow renders every view instead of scanning the provider's source. It also adds a check that nothing else could do: everydata-actionin the markup must have a handler in its client script.Advantages
tscand eslint. A duplicateclassshipped in the fatal-error view, and only a purpose-built regex added in #310 caught it, because HTML5 silently drops the second one. A stray</head>renders fine in a browser, so nothing ever flagged it.escapeHtmlcalls had to be remembered, and a missed one is an XSS hole fed by publisher-controlled names, tags and descriptions.dangerouslySetInnerHTML, which lint allows only in the two helpers that emit pre-sanitised content.style=,on*handlers, and inline<script>/<style>where they are written. The tests check the rendered output, which is what the webview actually loads.data-actionwith no handler: it's valid markup and a silent no-op, found only by clicking it.data-actionits client script does not handle.vscode, so the unit suite could not call them. The only coverage was text-matching the source.renderDocumentdon't depend onvscode, so the unit suite renders each view directly from fixtures..map(...).join('')over nested template strings.Pageholds the shell once. Repeated parts are small named components (SourceSection,ComponentCard,Parameter,VersionOptions), each with JSDoc. The provider shrinks by 377 lines.Change Type
Context
User-facing impact
GitLab scope
Affected areas
What changed by bucket
Toolchain
tsconfig.json,package.json,package-lock.jsonjsx: react-jsxwithjsxImportSource: preact. esbuild reads both fromtsconfig.json, soesbuild.jsis unchanged.preactandpreact-render-to-stringare dev dependencies like everything else here. They're bundled intoout/extension.js, andnode_modulesisn't packaged.@kitajs/htmlbecause it escapes children by default. Kitajs escapes only children markedsafe.Rendering
src/webview/render.tsrenderDocument(View, props)puts the doctype in front, since JSX can't express one, and returns the stringwebview.htmlexpects.vscode, so the unit suite calls it directly.Shared shell
src/webview/views/Page.tsxPagerenders the head (charset, viewport, CSP, stylesheet, title), the view's body, and the nonce'd client script.JsonScriptemits the bootstrap<script type="application/json">throughdangerouslySetInnerHTML. Browsers don't decode entities inside a script, so normal escaping would corrupt the JSON, andserializeForScriptalready makes it safe to emit verbatim.InlineMarkdowndoes the same forrenderInlineMarkdownoutput.Views
src/webview/views/—LoadingView,NoSourcesView,ErrorsView,ErrorView,ComponentBrowserView,ComponentDetailsViewid,classanddata-*attribute the client scripts use is unchanged.{' '}puts it back.<pre>keeps its indentation.Builders
src/providers/componentBrowserProvider.tsget…Htmlmethod still prepares its data: version data,classifySourceErrorsummaries, URL validation, and the template-file URL. It then callsrenderDocument.renderVersionOptionsmoves into the browser view. The privateescapeHtmlandrenderInlineMarkdownwrappers are removed.CSP
src/webview/csp.ts,src/webview/webviewHtml.ts,tests/unit/csp.test.tscspPolicy()returns the policy string thatPagesets on its meta tag.cspMetaTaghas no callers left and is removed. Its tests now targetcspPolicy.Lint
eslint.config.js.tsxjoins the TypeScript block.src/webview/views/**,no-restricted-syntaxbans:style=, the one CSP fault the compiler can't seeon*attributes<script>and<style>dangerouslySetInnerHTMLoutsidePage.tsxTests
tests/unit/webviewBuilderMarkup.test.ts,tests/unit/loadingView.test.ts</script>in the data;href;data-actionhas a handler.NOT_YET_EXTRACTEDgo. The compiler now rejects duplicate attributes, andNOT_YET_EXTRACTEDhas nothing left to track.<SCRIPT>or<Style>can't slip past a check. CodeQL flagged two of them as "Bad HTML filtering regexp", and the other four had the same gap.Validation
Local checks
npm run compilenpm test: 525 passing.npm run lintis clean (eslint and stylelint).Equivalence check
The move is meant to be mechanical, so I checked it directly rather than rely on review alone. Every builder was rendered from
betaand from this branch with the same fixtures, including hostile names, tags and descriptions, using a stubbedvscode. Both outputs were then normalised (entities decoded, whitespace collapsed) and diffed. What remains is DOM-equivalent:class=""anddata-description=""now render as bare attributes, which have the same empty value.<link>and<title>in the head.<option value="…" >had a stray space inside the tag.A second pass compared whitespace between adjacent inline elements. The spaces JSX drops all sit inside flex containers with
gap: the header buttons, the source and project header spans, the card actions and title, the version control row, and the checkbox and label pairs. Flex layout ignores that whitespace.Checks that were shown to fire
Each was tested by adding a deliberate fault and then reverting it:
<head>and a misspelt<butto>fail compilation.classfails with TS17001, andonClick="…"as a string is a type error.style=,onclick=,dangerouslySetInnerHTML, inline<script>and inline<style>each fail lint.data-actionmisspelt asviewDetialsfails withno handler for: viewDetials.Manual test notes
Developer: Open Webview Developer Toolsopen for CSP violations:Breaking Changes
Risk and Rollback
dangerouslySetInnerHTMLis confined toJsonScriptandInlineMarkdownby lint. Both emit values thatserializeForScriptandrenderInlineMarkdownhave already made safe, and both helpers have their own tests.Release Notes Draft
Checklist
🤖 Generated with Claude Code