Skip to content

fix(desktop): isolate artifact comment action styles - #80243

Merged
trunk-io[bot] merged 2 commits into
masterfrom
posthog-code/fix-isolate-html-comment-action
Aug 9, 2026
Merged

fix(desktop): isolate artifact comment action styles#80243
trunk-io[bot] merged 2 commits into
masterfrom
posthog-code/fix-isolate-html-comment-action

Conversation

@puemos

@puemos puemos commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Problem

People commenting on HTML artifacts can see the Comment action inherit artifact button styles, making the action unreadable or oversized.

The action shared the artifact document and CSS cascade with generated HTML.

Changes

  • Render the HTML artifact comment action inside a shadow root.
  • Scope theme variables to the isolated button while preserving live theme changes.
  • Leave canvas comments unchanged because their action already renders outside the canvas iframe.
Before After
Comment action inherits artifact styles Comment action keeps its own styles

How did you test this code?

  • Added a regression case where artifact CSS resets every button with !important.
  • Ran the focused comment bridge and placement tests.
  • Ran the @posthog/ui typecheck and the full desktop build.
  • Verified the synthetic conflict in headless Chromium for both versions.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Automatic notifications

  • Publish to changelog?

Docs update

None. This restores the existing comment design without changing a documented workflow.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Pi used repository tools and the PostHog MCP task context. It invoked /working-with-task-comments, /triaging-visual-review-runs, /writing-tests, /writing-code-comments, and /writing-pr-descriptions.

Render the HTML artifact comment action inside a shadow root so artifact CSS cannot restyle it. Keep theme updates scoped to the isolated button.

Generated-By: PostHog Code
Task-Id: dbf0f11d-bf57-440c-8749-541f85a1513d
@puemos puemos self-assigned this Aug 9, 2026
@trunk-io

trunk-io Bot commented Aug 9, 2026

Copy link
Copy Markdown

😎 Merged directly without going through the merge queue, as the queue was empty and the PR was up to date with the target branch - details.

@github-actions github-actions Bot added the feature/desktop Feature Tag: Desktop label Aug 9, 2026
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit aa0f275.

@puemos puemos left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge Confidence: 4.0/5 - Confident

"Looks good - minor concerns only"

Assessment
  • ✓ Correct fix for a real problem: artifact HTML is model-generated and a rule like button{all:unset!important} could previously destroy the comment affordance. Moving the button into a shadow root with :host{all:initial!important} is the standard remedy, and the !important is load-bearing — per CSS Scoping, important declarations from the inner tree beat the outer document's important declarations on the host.
  • ✓ Event retargeting was handled correctly, which is the subtle part. Moving data-selection-comment-overlay onto the host means the document-level gesture guard at selectionCommentAction.ts:238 (target.closest(...)) keeps suppressing hide() on presses over the button. The new pressAction helper dispatching with composed: true proves it rather than assuming it.
  • ✓ Tests were upgraded from string-matching the generated script text to driving a real jsdom document and asserting observable DOM state — the it.each(['light','dark']) theme test and the before/after-creation theme-message test are both meaningful contracts, not restatements of the implementation.
  • ✓ Theme vars applied inline to the button instead of the artifact's :root stops leaking five --ph-comment-action-* tokens into the sandboxed page's namespace, and commentActionButtonCss() is now embedded via JSON.stringify() rather than raw interpolation into a JS string literal.
  • ⚠ The shadow host is a plain <span> appended to document.documentElement, so artifact CSS selecting the host itself (span{display:none!important}, html > *{...}) still reaches it and would remove the affordance. Not covered by the new isolation test; pinning the host with inline styles would close it.
  • ⚠ No test exercises the button's mousedownsend('selection', {..., triggerRect}) path, which AnnotatedArtifactHtml.tsx:170-181 depends on to place the host-side popover. Pre-existing gap, but the shadow-DOM relocation is precisely what makes it worth locking down.
  • ⚠ jsdom does no layout or cascade resolution, so the tests cannot validate what this PR is actually about — that position:fixed placement survives the host becoming display:inline via all:initial, and that the artifact CSS is genuinely blocked. Needs one manual pass in a real Electron window before merge.
  • ⚠ Minor cleanup left behind: commentActionButtonCssVars now has no production callers (only its own unit test), and the new target = document.documentElement default is never exercised in production since the bridge always passes an explicit target.

Review: Isolate the artifact comment action button in a shadow root

Moves the selection-comment action button in the artifact HTML bridge out of the artifact's DOM/CSS reach: it now lives inside a <span data-selection-comment-overlay> shadow host with :host{all:initial!important} and the component CSS scoped inside the shadow root, instead of being a bare <button> on documentElement styled by a document-level <style>. Theming follows the same move — setCommentActionTheme gains a target parameter so the --ph-comment-action-* vars are set inline on the button rather than on the artifact's :root, with state.theme seeded at injection and re-applied on host theme messages.

The right fix for a real problem, and the subtle part (event retargeting keeping the target.closest("[data-selection-comment-overlay]") gesture guard working) is handled and explicitly tested. Tests also improved from string-matching the generated script to driving a real jsdom document.

Main risks: the host is a plain <span>, so artifact CSS selecting the host itself still reaches it; jsdom can't validate the layout/cascade behavior this PR actually turns on, so it needs one manual pass in Electron; and nothing tests the mousedownselection/triggerRect message that the host-side popover positioning depends on. Two now-dead exports (commentActionButtonCssVars, the target default) are worth cleaning up.

Protect the shadow host from artifact CSS, cover the selection message contract, and remove the obsolete root-variable helper.

Generated-By: PostHog Code
Task-Id: dbf0f11d-bf57-440c-8749-541f85a1513d
@trunk-io

trunk-io Bot commented Aug 9, 2026

Copy link
Copy Markdown

Static BadgeStatic BadgeStatic Badge

View Full Report ↗︎Docs

@puemos puemos added the stamphog Request AI approval (no full review) label Aug 9, 2026
@puemos
puemos marked this pull request as ready for review August 9, 2026 18:42

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contained frontend fix isolating a comment button into a shadow DOM to prevent artifact CSS bleed-through; no auth, billing, migration, dependency, or CI surface touched. All flagged review concerns were resolved with matching diff changes, and test coverage was substantially expanded.

  • Author wrote 100% of the modified lines and has 8 merged PRs in these paths (familiarity STRONG).
  • 👍 on the PR from hex-security-app[bot].
Gate mechanics and policy version
Gate Result
prerequisites all clear
deny-list no deny categories matched
size 19L, 2F substantive, 219L/4F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1c-medium (219L, 4F, single-area, fix)
stamphog 2.0.0b4 .stamphog/policy.yml @ 29880c9 · reviewed head aa0f275

@trunk-io
trunk-io Bot merged commit 25142c0 into master Aug 9, 2026
333 checks passed
@trunk-io
trunk-io Bot deleted the posthog-code/fix-isolate-html-comment-action branch August 9, 2026 18:49
@deployment-status-posthog

deployment-status-posthog Bot commented Aug 9, 2026

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-08-09 19:15 UTC Run
prod-us ✅ Deployed 2026-08-09 19:30 UTC Run
prod-eu ✅ Deployed 2026-08-09 19:32 UTC Run

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

Labels

feature/desktop Feature Tag: Desktop stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant