refactor(webview): render webview documents from TSX views - #325
Draft
X-Guardian wants to merge 2 commits into
Draft
X-Guardian wants to merge 2 commits into
X-Guardian wants to merge 2 commits into
Conversation
This branch has not been deployed
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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