refactor(code-help): escape snippets centrally instead of in source strings - #8062
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change makes Estimated code review effort: 3 (Moderate) | ~20 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Keep this PR theming-only. preventEscape removal + code-help source de-escaping move to #8062. Highlight keeps embedded + the de-nested getRawHtml (retaining preventEscape); escaping behaviour is unchanged here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-16 — run #18683 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18683 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18683 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18683 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18681 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18681 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18681 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18681 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18680 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18679 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18679 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18680 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
2a4a72c to
33075d5
Compare
|
@Zaimwa9 can I get a new stamp? I had to rebase it |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/web/components/Highlight.js (1)
106-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the new render inputs in
shouldComponentUpdate.When
disabledchanges for an empty snippet,getRawHtml()switches placeholders but the component returnsfalsebecause the children and tracked state are unchanged. Likewise, changingembeddedwill not add or removehljs--embedded. Add both prop comparisons to prevent stale rendering.Suggested fix
shouldComponentUpdate(nextProps, nextState) { + if (nextProps.disabled !== this.props.disabled) return true + if (nextProps.embedded !== this.props.embedded) return trueAlso applies to: 163-166
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aecd899d-7f9d-4747-8746-2cba94f7768a
📒 Files selected for processing (10)
frontend/common/code-help/create-user/create-user-next.jsfrontend/common/code-help/create-user/create-user-react.jsfrontend/common/code-help/init/init-next-app-router.jsfrontend/common/code-help/init/init-next-pages-router.jsfrontend/common/code-help/init/init-react.jsfrontend/common/code-help/traits/traits-next.jsfrontend/common/code-help/traits/traits-react.jsfrontend/web/components/CodeHelp.tsxfrontend/web/components/Highlight.jsfrontend/web/components/integrations/mcp/MCPSnippet.tsx
💤 Files with no reviewable changes (1)
- frontend/web/components/CodeHelp.tsx
Highlight now always escapes (dropped the preventEscape branch); MCPSnippet and CodeHelp no longer pass preventEscape. Code-help sources use raw JSX (<FlagsmithProvider>) instead of hand-escaped entities (<), so they're readable. Rendered output is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
They escaped their XML with Utils.escapeHtml at render-build time, which the entity sweep missed (nothing literal in the source). With Highlight escaping centrally they double-escaped, showing <dependency> on screen, and Copy Code shipped entities. The onboarding sdkSnippets unescape workaround is dead now the sources are clean, so it's gone too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0e62939 to
88dd8d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/common/code-help/create-user/create-user-next.js (1)
26-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winImport
FlagsmithProviderin both generated Next.js templates.
frontend/common/code-help/create-user/create-user-next.jsandfrontend/common/code-help/traits/traits-next.jsboth render<FlagsmithProvider>from Option 2 but continue to only importflagsmithplus React hooks, so the emitted examples are missing the provider import and will fail to compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 91488edd-9f05-43cf-8705-4535c9113763
📒 Files selected for processing (13)
frontend/common/code-help/create-user/create-user-next.jsfrontend/common/code-help/create-user/create-user-react.jsfrontend/common/code-help/init/init-next-app-router.jsfrontend/common/code-help/init/init-next-pages-router.jsfrontend/common/code-help/init/init-react.jsfrontend/common/code-help/install/install-dotnet.jsfrontend/common/code-help/install/install-java.jsfrontend/common/code-help/traits/traits-next.jsfrontend/common/code-help/traits/traits-react.jsfrontend/web/components/CodeHelp.tsxfrontend/web/components/Highlight.jsfrontend/web/components/integrations/mcp/MCPSnippet.tsxfrontend/web/components/pages/onboarding/OnboardingConnectPanel/sdkSnippets.ts
💤 Files with no reviewable changes (1)
- frontend/web/components/CodeHelp.tsx
Highlight is the single escape point now; keeping this helper around invites pre-escaping content again, the pattern this branch removes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
With Highlight escaping everything, the <a> tags in the curl, Flutter and Rust install notes showed up as raw markup. The links were already dead on prod (the highlighter strips the tags) and Copy Code shipped the markup, so keep the same text prod effectively renders, minus the tags. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
docs/if required so people know about the feature.Changes
Split out of #8023 to keep that PR to theming only.
Copy Code was shipping HTML entities (
<FlagsmithProvider) to the clipboard: snippet sources were pre-escaped becauseCodeHelprendered them as raw HTML. Escape centrally inHighlightinstead.< >(literal entities removed; Java/.NET drop their runtimeUtils.escapeHtml);Highlightalways escapes at render,preventEscapeis gone.<a>tags: the links were already dead on prod (the highlighter strips them) and would have shown as raw markup here; same text, no markup.How did you test this code?
<FlagsmithProvider>, Java shows the Maven<dependency>XML, .NET shows<PackageReference>(not<)/getting-started(behind ffonboarding_quickstart_flow): code cards render and copy clean<a href>markup, same text as prod< >still edits and renders (path unchanged, quick sanity)lint+typecheckpass.Screenshots