fix: replace dangerouslySetInnerHTML with safe React text rendering i… - #327
Conversation
|
🎉 Thank you @Tirthpanchori for submitting a Pull Request! We're excited to review your contribution. Before Review✅ Ensure all CI checks pass ⚡ Want faster reviews and contributor support? Join our Discord community: 🔗 https://discord.gg/FcXuyw2Rs Maintainers and mentors are active there and can help resolve blockers quickly. Happy Contributing! 🚀 |
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
arpit2006
left a comment
There was a problem hiding this comment.
PR #327 Review — dangerouslySetInnerHTML Removal in ChartStyle
Branch: fix/chartstyle-css-injection
Claimed scope: Single-file frontend fix (chart.tsx)
Verdict: ❌ REQUEST CHANGES
🚩 Critical Issue: Scope Mismatch (Same Pattern as Prior Rejected PRs)
The PR description claims it modifies one file (frontend/src/app/components/ui/chart.tsx). The actual branch diff against main touches 49 files with 3,132 insertions and 1,500 deletions — including massive changes to backend/app/main.py (+776 lines), backend/app/db.py (+192 lines), frontend/src/app/pages/dashboard.tsx (-903 lines refactored), and dozens of other unrelated files.
49 files changed, 3132 insertions(+), 1500 deletions(-)
The contributor branched from a point in history that already includes a pile of merged PRs (#295, #245, #266, and others). The branch is not rebased onto current main, so all of those upstream merges appear as the PR's own diff.
The contributor's actual commit is exactly one commit (
7ff5f53) touching onlychart.tsx, which is correct in isolation. The problem is the branch base.
Technical Review of the Actual Change (7ff5f53)
What the PR claims vs. what it delivers
| Claim | Reality |
|---|---|
| "plain JSX text rendering" | ❌ Not implemented. The PR does NOT pass CSS as a JSX text child. |
Removes dangerouslySetInnerHTML |
✅ Correct — removed |
| "simpler than useEffect+ref" approach | ❌ Uses useEffect + ref anyway — the same approach the PR claims to avoid |
| No visual regression | |
| Eliminates the injection surface | ✅ Technically yes, but see caveats |
The Implementation Actually Shipped
// What the PR description says it does (plain JSX child):
return <style>{cssText}</style>; // ← NOT what's in the code
// What it actually does (useEffect + ref):
const styleRef = React.useRef<HTMLStyleElement>(null);
React.useEffect(() => {
if (styleRef.current) {
styleRef.current.textContent = cssText; // ← DOM mutation post-render
}
}, [cssText]);
return <style ref={styleRef} />;The PR description is factually wrong about its own approach. It explicitly distances itself from the useEffect + DOM ref + textContent pattern that prior PRs (#5, #285, #291) were closed for — then implements exactly that pattern.
Bug: Logic Order Inversion
There is a structural ordering bug. The early-return guard appears after the hooks:
// Line 100-104: useEffect hook
React.useEffect(() => {
if (styleRef.current) {
styleRef.current.textContent = cssText;
}
}, [cssText]);
// Line 106-108: early return ← too late, hooks already called above
if (!colorConfig.length) {
return null; // <style ref={styleRef} /> is never mounted, effect is a no-op
}
return <style ref={styleRef} />;This doesn't violate the Rules of Hooks (hooks are always called unconditionally), but the guard at line 106 means styleRef.current will be null for the empty-config case (component returns null → no DOM node → ref not attached). The useEffect fires with cssText = "" but styleRef.current is null, so nothing happens — harmless in this case, but wasteful and confusing.
The correct structure should be:
// Guard BEFORE hooks OR restructure so the style element always mounts
// Option A (cleanest — plain JSX child, as the PR actually claims):
if (!colorConfig.length) return null;
return <style>{cssText}</style>;Hydration / SSR Flash (The Exact Reason Prior PRs Were Rejected)
The PR's own description acknowledges that prior PRs using useEffect + textContent were closed partly because of "SSR/client-only-effect hydration flash". The implemented approach has this same flaw:
- On SSR (or initial paint before hydration),
<style ref={styleRef} />renders as an empty<style>tag. useEffectonly runs client-side, after paint.- This means charts will render without CSS custom properties for one frame during hydration, causing a visible flash if the app ever moves to SSR or strict hydration.
The simple fix (plain JSX child) avoids this entirely:
return <style>{cssText}</style>;React renders text children of <style> as unescaped text (style content is not HTML-escaped by the browser's CSS parser regardless), so this works correctly and safely.
useMemo Indentation
Minor: the colorConfig useMemo has inconsistent indentation (the callback and dependency array are at column 0 instead of being indented inside the function body).
// Current (misaligned):
const colorConfig = React.useMemo(
() => Object.entries(config).filter(([, c]) => c.theme || c.color),
[config],
);
// Should be:
const colorConfig = React.useMemo(
() => Object.entries(config).filter(([, c]) => c.theme || c.color),
[config],
);Summary of Required Changes
| # | Severity | Issue | Required Action |
|---|---|---|---|
| 1 | 🔴 Blocker | Branch not rebased — 49 files of unrelated noise in diff | Rebase onto current main so the PR diff shows only chart.tsx |
| 2 | 🔴 Blocker | PR description claims "plain JSX child" but implements useEffect + ref (the pattern it claims to avoid) |
Either implement the simple <style>{cssText}</style> approach as described, or update the description to accurately reflect the implementation |
| 3 | 🟡 Medium | useEffect + ref approach introduces the SSR hydration flash that closed prior PRs |
Switch to <style>{cssText}</style> (plain text child) to eliminate flash and match the stated approach |
| 4 | 🟢 Minor | useMemo indentation misalignment |
Fix formatting |
Recommended Fix (15 lines)
The cleanest correct implementation — matching what the PR description actually claims:
const ChartStyle = ({ id, config }: { id: string; config: ChartConfig }) => {
const colorConfig = React.useMemo(
() => Object.entries(config).filter(([, c]) => c.theme || c.color),
[config],
);
const cssText = React.useMemo(() => {
if (!colorConfig.length) return "";
return Object.entries(THEMES)
.map(([theme, prefix]) => `
${prefix} [data-chart=${id}] {
${colorConfig
.map(([key, itemConfig]) => {
const color =
itemConfig.theme?.[theme as keyof typeof itemConfig.theme] ||
itemConfig.color;
return color ? ` --color-${key}: ${color};` : null;
})
.join("\n")}
}
`)
.join("\n");
}, [id, colorConfig]);
if (!colorConfig.length) return null;
return <style>{cssText}</style>; // ← plain text child, no dangerouslySetInnerHTML, no useEffect
};This is SSR-safe, has no hydration flash, needs no ref or effect, and matches the PR description exactly.
7ff5f53 to
0a66419
Compare
|
✅ PR template check passed! @arpit2006 this PR is ready for your review. 🚀 |
arpit2006
left a comment
There was a problem hiding this comment.
LGTM! 🚀
Thank you for updating the implementation. All requested changes have been addressed, and this PR is approved for merge.
Verification Details:
- Clean Diff (Rebase): The branch has been rebased/reset onto the latest
main. The unrelated file changes and upstream merge noise have been removed, leaving only the targeted fix inchart.tsx. - Implementation Alignment: The
useEffecthook,useRef, and post-render DOM manipulations have been fully removed. The component now safely renders the styles as a plain JSX text child (<style>{cssText}</style>) as originally proposed. - SSR / Hydration Flash: The JSX plain text child approach is fully SSR-compatible and successfully eliminates the client-side hydration flash.
- Formatting: The minor indentation misalignment on the
colorConfiguseMemohas been fixed. - Build Status: Verified that the frontend application builds cleanly without errors.
|
@ionfwsrijan , Merge Ready! |
Linked issue
Closes #249
What this PR does
Replaces the unsafe use of
dangerouslySetInnerHTMLin theChartStylecomponent with React's built-in JSX text-node rendering, which is automatically HTML-escaped. This removes the raw HTML/CSS injection surface while preserving identical chart theming behavior (per-theme CSS custom properties, including dark mode overrides).Type of change
ML tier (if applicable)
Stack affected
Changes
Backend
Frontend
dangerouslySetInnerHTMLfromChartStyle(frontend/src/app/components/ui/chart.tsx).[data-chart=id]/.dark [data-chart=id]) and--color-*custom properties.<style>instead of viadangerouslySetInnerHTML, so it's rendered as an escaped text node rather than parsed as raw HTML.New dependencies
Database / schema changes
Testing
How did you test this?
npm run build) — passes cleanly with no new errors or warnings.ChartStyle/ChartContainerwith a temporary isolated test render (with bothcolorandtheme-based config entries) to verify output without relying on the app's currently-broken scan/backend pipeline (unrelated pre-existing issue, confirmed via cleangit log main..origin/main— not caused by or fixed in this PR).<style>tag directly in DevTools and confirmed both the light and.darkselector blocks render with correct--color-*values, matching pre-fix output.dangerouslySetInnerHTMLremains anywhere in the modified component.Checklist
<style>output for both light and dark theme blocks)console.erroror unhandled exceptions introducedrequirements.txt/package.jsonupdated if new dependencies added — N/A, no new dependencies.pkl,.pt, etc.) are gitignored, not committed — N/AAnything reviewers should focus on
Three prior PRs against this issue (#5, #285, #291) used a
useEffect+ DOM ref +textContentapproach and were closed. This PR takes a simpler route — plain JSX text rendering — which avoids the extra lifecycle complexity and the SSR/client-only-effect hydration flash that approach would introduce. Please confirm this satisfies the security requirement and that no visual regression exists versus the original.Screenshots (if UI changed)
No visible UI changes — chart styling output is byte-for-byte equivalent to the original, verified via direct
<style>tag inspection (see Testing section).