Skip to content

refactor(webview): render webview documents from TSX views - #325

Draft
X-Guardian wants to merge 2 commits into
eFAILution:betafrom
X-Guardian:refactor/webview-tsx-views
Draft

X-Guardian wants to merge 2 commits into
eFAILution:betafrom
X-Guardian:refactor/webview-tsx-views

Conversation

@X-Guardian

@X-Guardian X-Guardian commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Moves every webview document out of template literals into TSX views, rendered to a string with preact-render-to-string. Now that the webview CSS and JS are external, this was the last markup that tsc and eslint saw as an opaque string.
  • The compiler now rejects unclosed and stray tags, duplicate attributes, unknown elements and string event handlers. Editors highlight the markup, and view props are typed.
  • Interpolated text and attribute values are escaped by default. The ~40 manual escapeHtml calls are gone. Raw HTML is allowed only in two helpers, which eslint enforces.
  • webviewBuilderMarkup.test.ts now renders every view instead of scanning the provider's source. It also adds a check that nothing else could do: every data-action in the markup must have a handler in its client script.
  • Link related issue(s): follows refactor(webview): move the Component Browser view's inline script, style and handlers to linted files #288

Advantages

Before (template literals) After (TSX views)
Markup errors Invisible to tsc and eslint. A duplicate class shipped 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. Compile errors: unclosed or mismatched tags, stray close tags, duplicate attributes, unknown elements, and a string where an event handler belongs.
Escaping Opt-in, one call at a time. Each of the ~40 escapeHtml calls had to be remembered, and a missed one is an XSS hole fed by publisher-controlled names, tags and descriptions. Opt-out. Every interpolation is escaped. Raw HTML means reaching for dangerouslySetInnerHTML, which lint allows only in the two helpers that emit pre-sanitised content.
CSP faults A regex test over the provider's source text caught inline scripts, styles and handlers. It could not tell a builder from any other method, and it matched text, not the rendered document. Lint rejects style=, on* handlers, and inline <script>/<style> where they are written. The tests check the rendered output, which is what the webview actually loads.
Dead buttons Nothing caught a data-action with no handler: it's valid markup and a silent no-op, found only by clicking it. A unit test fails for any data-action its client script does not handle.
Types Interpolated values were type-checked, but the markup around them was not: element names, attribute names and attribute values were all free text. Element names, and the value types of known attributes, are checked against preact's DOM typings. Unknown attribute names still pass. Each view declares its props, which separates data preparation (in the builder) from markup (in the view).
Editor support The markup was one long string: no highlighting, no completion, no go-to-definition. JSX highlighting, completion for elements and attributes, and navigation into the helper components, all with no editor extension.
Testability The builders are methods on a class that imports vscode, so the unit suite could not call them. The only coverage was text-matching the source. The views and renderDocument don't depend on vscode, so the unit suite renders each view directly from fixtures.
Structure Every builder repeated the head, the CSP meta tag, the stylesheet link and the script tag. Loops were .map(...).join('') over nested template strings. Page holds the shell once. Repeated parts are small named components (SourceSection, ComponentCard, Parameter, VersionOptions), each with JSDoc. The provider shrinks by 377 lines.

Change Type

  • feat
  • fix
  • refactor
  • docs
  • test
  • chore

Context

User-facing impact

  • None intended. Before and after output for every view differs only in ways that leave the DOM unchanged (see Validation).

GitLab scope

  • gitlab.com
  • self-managed GitLab
  • both (webview rendering only)

Affected areas

  • Component Browser
  • Hover provider (the detached details panel it opens)
  • Completion provider
  • Validation provider
  • Cache and refresh behavior
  • GitLab API calls/auth/token storage
  • Docs only

What changed by bucket

Toolchain

tsconfig.json, package.json, package-lock.json

  • jsx: react-jsx with jsxImportSource: preact. esbuild reads both from tsconfig.json, so esbuild.js is unchanged.
  • preact and preact-render-to-string are dev dependencies like everything else here. They're bundled into out/extension.js, and node_modules isn't packaged.
  • Preact was chosen over @kitajs/html because it escapes children by default. Kitajs escapes only children marked safe.

Rendering

src/webview/render.ts

  • renderDocument(View, props) puts the doctype in front, since JSX can't express one, and returns the string webview.html expects.
  • It has no dependency on vscode, so the unit suite calls it directly.

Shared shell

src/webview/views/Page.tsx

  • Page renders the head (charset, viewport, CSP, stylesheet, title), the view's body, and the nonce'd client script.
  • JsonScript emits the bootstrap <script type="application/json"> through dangerouslySetInnerHTML. Browsers don't decode entities inside a script, so normal escaping would corrupt the JSON, and serializeForScript already makes it safe to emit verbatim.
  • InlineMarkdown does the same for renderInlineMarkdown output.

Views

src/webview/views/ — LoadingView, NoSourcesView, ErrorsView, ErrorView, ComponentBrowserView, ComponentDetailsView

  • One view per former builder. Every id, class and data-* attribute the client scripts use is unchanged.
  • Where JSX would drop a space between inline elements outside a flex container, an explicit {' '} puts it back.
  • The example JSON on the no-sources page is a string literal, so its <pre> keeps its indentation.

Builders

src/providers/componentBrowserProvider.ts

  • Each get…Html method still prepares its data: version data, classifySourceError summaries, URL validation, and the template-file URL. It then calls renderDocument.
  • renderVersionOptions moves into the browser view. The private escapeHtml and renderInlineMarkdown wrappers are removed.
  • 1,663 → 1,286 lines.

CSP

src/webview/csp.ts, src/webview/webviewHtml.ts, tests/unit/csp.test.ts

  • cspPolicy() returns the policy string that Page sets on its meta tag.
  • cspMetaTag has no callers left and is removed. Its tests now target cspPolicy.

Lint

eslint.config.js

  • .tsx joins the TypeScript block.
  • For src/webview/views/**, no-restricted-syntax bans:
    • style=, the one CSP fault the compiler can't see
    • on* attributes
    • inline <script> and <style>
    • dangerouslySetInnerHTML outside Page.tsx

Tests

tests/unit/webviewBuilderMarkup.test.ts, tests/unit/loadingView.test.ts

  • Every view is rendered from fixtures that include hostile publisher text. The tests assert:
    • there's no inline code the CSP would block;
    • every script carries the CSP nonce;
    • publisher text stays inert;
    • the bootstrap JSON round-trips, even with </script> in the data;
    • no metadata URL reaches an href;
    • every data-action has a handler.
  • The duplicate-attribute test and NOT_YET_EXTRACTED go. The compiler now rejects duplicate attributes, and NOT_YET_EXTRACTED has nothing left to track.
  • Every tag-matching regex is case-insensitive, so <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 compile
  • npm test: 525 passing. npm run lint is clean (eslint and stylelint).
  • Extension-host suite: 25 passing
  • Manual verification in VS Code Extension Host: the loading view renders styled, with no CSP violations in the webview console
  • Manual verification of the remaining views: outstanding

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 beta and from this branch with the same fixtures, including hostile names, tags and descriptions, using a stubbed vscode. Both outputs were then normalised (entities decoded, whitespace collapsed) and diffed. What remains is DOM-equivalent:

  • class="" and data-description="" now render as bare attributes, which have the same empty value.
  • Attribute order changed, and so did the order of <link> and <title> in the head.
  • The old <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:

  • A deleted <head> and a misspelt <butto> fail compilation.
  • A duplicate class fails with TS17001, and onClick="…" as a string is a type error.
  • style=, onclick=, dangerouslySetInnerHTML, inline <script> and inline <style> each fail lint.
  • A data-action misspelt as viewDetials fails with no handler for: viewDetials.

Manual test notes

  • In the Extension Development Host, with Developer: Open Webview Developer Tools open for CSP violations:
    • Component Browser. Test Load Versions → dropdown → switch version → Details and Insert. Also test search, expanding and collapsing a source and a project, Refresh, Update Cache, Reset Cache, the version dropdown's context menu, and a failing source's error banner with Show Details.
    • Details panel. Open it from both the browser's Details button and a hover's View Full Component Details. Then switch version, toggle raw YAML, tick individual inputs and Select All, use Refresh Versions, and Insert Component.
    • No sources, errors and error views. Test Open Settings, Try Again, Update Token and Show details.

Breaking Changes

  • No breaking changes
  • Breaking changes (describe below)

Risk and Rollback

  • Main risks: low to medium. The equivalence check covers the rendered output, and the handler check covers the most common silent fault. What neither covers is layout: a dropped space in a container that is not flex would be visible but not caught. The second equivalence pass found none.
  • dangerouslySetInnerHTML is confined to JsonScript and InlineMarkdown by lint. Both emit values that serializeForScript and renderInlineMarkdown have already made safe, and both helpers have their own tests.
  • The bundle grows by the size of preact and its string renderer, which are small.
  • Rollback strategy: revert the commit.

Release Notes Draft

  • Internal: webview markup is now written as type-checked TSX views, escaped by default.

Checklist

  • Branch is up to date with target branch
  • Commit messages follow conventional commits
  • Added/updated docs for behavior or settings changes (this PR description)
  • Added/updated tests for new behavior (rendered-output tests for every view, including the data-action handler check)
  • No secrets or tokens in code, logs, screenshots, or test fixtures

🤖 Generated with Claude Code

Comment thread tests/unit/loadingView.test.ts Fixed
Comment thread tests/unit/webviewBuilderMarkup.test.ts Fixed

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants