Skip to content

fix: preserve radical rendering in experiment intros - #200

Draft
16th-admin wants to merge 1 commit into
mainfrom
codex/fix-experiment-intro-radical
Draft

fix: preserve radical rendering in experiment intros#200
16th-admin wants to merge 1 commit into
mainfrom
codex/fix-experiment-intro-radical

Conversation

@16th-admin

Copy link
Copy Markdown
Collaborator

What changed

  • stop the experiment intro layout from applying responsive image sizing to every inline SVG
  • keep Mermaid diagrams responsive through their dedicated styles
  • add a browser regression test that renders \sqrt{x} in an experiment description and verifies the radical has a visible height
  • mock summary images in the regression test so external image requests cannot create false failures

Root cause

KaTeX draws the radical with an inline SVG. The experiment intro's broad .intro svg rule overrode the SVG dimensions, while comments did not have that override.

Validation

  • npm.cmd run build
  • npm.cmd run eslint (0 errors; 18 existing warnings)
  • Chromium targeted Playwright regression: 1 passed
  • git diff --check

Firefox and WebKit executables are absent locally; GitHub Actions will run the repository's configured browser matrix.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

🤖 Codex-Style Code Review by OpenCode (Vue Best Practice Enabled)

💡 Autonomous AI Reviewer inspecting git commit history and Vue code quality.

I'll start by inspecting the PR context and changes.
Let me identify the PR's base branch and inspect the commit history.
The PR contains one commit: d862711 fix: preserve radical rendering in experiment intros, touching 2 files. Let me examine the diff.
Now let me examine the full context of the modified Vue file and related code.
Let me look at the test file and understand how the content is parsed/rendered (KaTeX usage).
Now let me understand the parse function and how rich text/KaTeX is rendered.
Let me check the test setup, package scripts, and verify the build/lint status of changed files.
Let me check for similar CSS patterns elsewhere and the history of this rule.
Let me verify the KaTeX DOM structure assumption and check for style/lint issues in the changed files.
Let me check the TypeScript config for tests and verify the KaTeX selector assumption in the test.
Let me check the tab default and how the intro content renders (to validate the test's selector assumptions).
Let me check for similar SVG-constraining rules elsewhere that could have the same KaTeX bug, and verify the KaTeX radical DOM structure.
I have enough context now. Let me verify one more detail — whether the tsconfig.tests.json typechecks the new test code and confirm the changed selector's impact scope.
I now have a complete picture. Let me write the review report.

PR Review: fix: preserve radical rendering in experiment intros (#200)

Base: main (f394e54) · Head: d862711 · Scope: 2 files (+74 / −40)

Summary

A focused bugfix: the CSS rule .intro :deep(img), .intro :deep(svg) { max-width: 100%; height: auto; } was forcing height: auto onto KaTeX radical (\sqrt{}) SVGs, collapsing the radical sign in experiment descriptions. The PR removes svg from that selector and adds a Playwright regression test (plus a createSummary() helper to DRY the mock).

What's good 🎉

  • Correct, minimal fix. KaTeX renders radicals as <svg> elements whose height comes from a viewBox/attribute pair; height: auto overrode that and destroyed the layout. Scoping the constraint to img only is the right call.
  • No mermaid regression. Mermaid SVGs are still constrained by .intro :deep(.mermaid-diagram svg) (scoped) and .mermaid-diagram svg { height: auto; max-width: none; } (global rendering.css), so removing the generic svg rule doesn't break diagrams.
  • Good test hygiene. The new test intercepts avatar/image requests with a transparent pixel — important because the shared fixture auto-fails on unexpected console errors (404s). String.raw correctly preserves \sqrt so the $…$ KaTeX delimiter path is exercised.
  • Helper extraction (createSummary()) removes meaningful duplication across the spec and keeps the Data-shape comment in one place.
  • Only one other rich-text container in the app (MessageItem.vue:178) constrains svg, and it already uses :deep(img) only — so the fix is complete for this codebase.

Comments

🟡 [important] — Removing svg entirely drops max-width: 100% for arbitrary inline SVGs

By dropping the whole :deep(svg) selector, any user-embedded inline <svg> in an intro (DOMPurify allows svg: true, advancedParser.ts:117) loses the max-width: 100% guard and can overflow the .gray container on narrow screens. The bug is specifically about height: auto, not max-width. A more surgical fix that preserves overflow protection would be:

.intro :deep(img) { max-width: 100%; height: auto; }
.intro :deep(svg):not(.katex svg) { max-width: 100%; height: auto; }

or, keeping the original rule and excluding KaTeX explicitly:

.intro :deep(.katex svg) { max-width: none; height: auto; }

If embedded SVGs in content are not a supported scenario, the current change is acceptable — but worth a conscious decision.

🟢 [nit] — Test relies on KaTeX's internal DOM

The selector .intro .katex .sqrt svg pins the test to KaTeX's markup structure (<span class="sqrt">…<svg>). It's a stable internal detail, so low risk; just noting the coupling.

🟢 [nit]bounds!.height non-null assertion

expect(bounds).not.toBeNull() doesn't narrow the type, hence the !. A cleaner pattern:

expect(bounds?.height).toBeGreaterThan(5)

Not blocking either way.

Decision

Approve — correct, well-scoped fix with a meaningful regression test. The 🟡 item is a trade-off discussion (targeted KaTeX exclusion vs. broad svg removal), not a blocker.

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.

1 participant