refactor(styles): theme-adaptive shared code theme#8023
Conversation
github-actions
Bot
added
front-end
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
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:
📝 WalkthroughWalkthroughAdds a Storybook documentation page covering CodeHelp setup and embedded states, and fixes missing Button imports. Refactors Highlight to centralise raw HTML selection and escaping. Replaces hardcoded Highlight.js colours with design-system tokens and adds an embedded modifier. The onboarding code card now uses that modifier, while its local syntax-colour rules are removed. Estimated code review effort: 4 (Complex) | ~45 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 |
Docker builds report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 541e0da9-4ac1-4a1d-bfe5-aae4eb6f2199
📒 Files selected for processing (6)
frontend/documentation/pages/CodeHelp.stories.tsxfrontend/web/components/CodeHelp.tsxfrontend/web/components/Highlight.jsfrontend/web/components/pages/onboarding/OnboardingConnectPanel/CodeCard.tsxfrontend/web/components/pages/onboarding/OnboardingConnectPanel/OnboardingConnectPanel.scssfrontend/web/styles/3rdParty/_hljs.scss
| import Utils from 'common/utils/utils' | ||
| import Icon from './icons/Icon' | ||
| import CalloutBar from './CalloutBar' | ||
| import Button from './base/forms/Button' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the repository import alias for Button.
Change this to components/base/forms/Button rather than a relative import.
Proposed fix
-import Button from './base/forms/Button'
+import Button from 'components/base/forms/Button'As per coding guidelines, frontend files must use only common/, components/, and project/ import paths and must not use relative imports.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import Button from './base/forms/Button' | |
| import Button from 'components/base/forms/Button' |
Source: Coding guidelines
| @@ -1,5 +1,6 @@ | |||
| import React from 'react' | |||
| import hljs from 'highlight.js' | |||
| import Button from './base/forms/Button' | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the permitted component import path.
-import Button from './base/forms/Button'
+import Button from 'components/base/forms/Button'As per coding guidelines, “Use only common/, components/, and project/ import paths; do not use relative imports.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import Button from './base/forms/Button' | |
| import Button from 'components/base/forms/Button' |
Source: Coding guidelines
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Failed testsfirefox › tests/onboarding-tests.pw.ts › Onboarding › New user connects via the single-page onboarding flow @oss
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
Convert the shared highlight.js theme from its hardcoded dark palette to semantic tokens, so code blocks adapt to light/dark instead of always rendering dark. Code surface -> --color-surface-subtle; syntax colours -> the --color-text-* palette (first pass, lifted from the onboarding code card). Spike: .hljs is shared app-wide incl. the interactive value editor, so this is a visible change everywhere. Opened to see the real diff via CI visual regression before committing to a designed palette. Refs #6606 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both were already won by the cascade: .select-language span.primary follows .active at equal specificity, and code[contenteditable].hljs outranks .hljs. The remaining !important beat highlight.js's own CSS / the Button / global resets. Refs #6606 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Render CodeHelp with the real codeHelp snippets (install, init, create identity, set traits) plus collapsed-header and embedded variants, so the shared code block can be QA'd in light and dark (theme toggle) and across languages (dropdown) in one place - the safety net for the #6606 theme change. Registers window.Button in the Storybook preview, since CodeHelp uses the global Button like Select. Refs #6606 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adopt the onboarding code card's restrained palette (five semantic-token groups, unmapped tokens inherit the base) as the shared `.hljs` theme, and add a `.hljs--embedded` modifier for code that sits in a container already providing the surface (transparent, no border/radius, gutter-reserved scrollbar). Onboarding's bespoke .hljs block collapses to that one class. Refs #6606 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…the global Both reached for a bare `window.Button` (the legacy project-components.js pattern) rather than importing it, which meant Storybook had to fake the global. Import Button directly so they stand alone, and drop the shim from the Storybook preview. Also de-nests Highlight's pre-existing html ternary into a small helper so the touched file passes lint. Refs #6606 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
talissoncosta
force-pushed
the
refactor/hljs-theme-adaptive
branch
from
dba3f38 to
b1c981e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/web/components/Highlight.js (1)
3-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the permitted component import path.
Relative import still present; this was already flagged in a prior review round.
Proposed fix
-import Button from './base/forms/Button' +import Button from 'components/base/forms/Button'As per coding guidelines, "Use only
common/,components/, andproject/import paths; do not use relative imports."Source: Coding guidelines
frontend/web/styles/3rdParty/_hljs.scss (1)
86-86: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore stylelint blank-line-before-comment compliance.
Still failing
scss/double-slash-comment-empty-line-beforeat all three sites from the previous round.Proposed fix
padding: 0.5em; + // Theme-adaptive surface: light in light mode, dark in dark, via tokens.padding: 18px; + // Unmapped tokens inherit the base colour instead of the highlight.js greys;border-radius: 0; + // Reserve the scrollbar gutter so hover doesn't shift layout; !importantAlso applies to: 183-183, 290-290
Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80921548-b746-48c3-b290-c6e8aca8b379
📒 Files selected for processing (6)
frontend/documentation/pages/CodeHelp.stories.tsxfrontend/web/components/CodeHelp.tsxfrontend/web/components/Highlight.jsfrontend/web/components/pages/onboarding/OnboardingConnectPanel/CodeCard.tsxfrontend/web/components/pages/onboarding/OnboardingConnectPanel/OnboardingConnectPanel.scssfrontend/web/styles/3rdParty/_hljs.scss
| code[contenteditable='false'].hljs { | ||
| cursor: default !important; | ||
| cursor: default; | ||
| @include customScrollDark(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix mixin call lint violation.
Stylelint flags @include customScrollDark(); for unexpected parentheses on an argumentless mixin call.
Proposed fix
code[contenteditable='false'].hljs {
cursor: default;
- `@include` customScrollDark();
+ `@include` customScrollDark;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| code[contenteditable='false'].hljs { | |
| cursor: default !important; | |
| cursor: default; | |
| @include customScrollDark(); | |
| } | |
| code[contenteditable='false'].hljs { | |
| cursor: default; | |
| `@include` customScrollDark; | |
| } |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 281-281: Unexpected parentheses in argumentless mixin "customScrollDark" call (scss/at-mixin-argumentless-call-parentheses)
(scss/at-mixin-argumentless-call-parentheses)
Source: Linters/SAST tools
Pylon's support widget injects a global .hljs { background: var(--hljs-bg);
color: var(--hljs-text) } into the host document, overriding the code theme.
Define those vars on .hljs (and transparent on --embedded) so its rule paints
our tokens instead.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/web/styles/3rdParty/_hljs.scss (1)
297-302: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid forcing the scrollbar on non-hover states
overflow-x: scrollplus the unqualified::-webkit-scrollbarrule keeps the bar visible even when the code fits. Useoverflow-x: auto, addscrollbar-gutter: stable, and limitdisplay: block !importantto the:hoverselector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1505510c-624e-4fc5-8983-04c18188aa43
📒 Files selected for processing (1)
frontend/web/styles/3rdParty/_hljs.scss
Swap the SCSS-var chrome colours (language selector, collapsible header, code.txt input, invalid border) for semantic tokens so they follow the theme like the code surface. Leaves the always-dark branded callout bar and code.txt's dark fill as-is; neither maps cleanly to a token. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b68ee47-0847-4080-b705-f0952a5ce373
📒 Files selected for processing (1)
frontend/web/styles/3rdParty/_hljs.scss
| background-color: var(--color-surface-muted); | ||
| font-family: $font-family-header; | ||
| overflow: hidden; | ||
| @include transition(all 0.2s ease-in-out); | ||
| color: $btn-code-button-color; | ||
| color: var(--color-text-default); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the blank line before color.
Stylelint 17.14.0 reports declaration-empty-line-before at Line 150. Add an empty line after the transition mixin so this declaration passes lint.
Proposed fix
`@include` transition(all 0.2s ease-in-out);
+
color: var(--color-text-default);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| background-color: var(--color-surface-muted); | |
| font-family: $font-family-header; | |
| overflow: hidden; | |
| @include transition(all 0.2s ease-in-out); | |
| color: $btn-code-button-color; | |
| color: var(--color-text-default); | |
| background-color: var(--color-surface-muted); | |
| font-family: $font-family-header; | |
| overflow: hidden; | |
| `@include` transition(all 0.2s ease-in-out); | |
| color: var(--color-text-default); |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 150-150: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
Source: Linters/SAST tools
| background: transparent !important; | ||
| border: 1px solid $input-border-color; | ||
| border: 1px solid var(--color-border-default); | ||
| font-family: 'OpenSans', sans-serif; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unnecessary quotes around OpenSans.
The supplied Stylelint result reports font-family-name-quotes at Line 245. Use the unquoted family name so this file remains lint-compliant.
Proposed fix
- font-family: 'OpenSans', sans-serif;
+ font-family: OpenSans, sans-serif;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| font-family: 'OpenSans', sans-serif; | |
| font-family: OpenSans, sans-serif; |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 245-245: Expected no quotes around "OpenSans" (font-family-name-quotes)
(font-family-name-quotes)
Source: Linters/SAST tools
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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/styles/3rdParty/_hljs.scss (1)
115-124: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope the YAML token rules to
$code-roots.These shared
.hljs-*selectors remain global, allowing the application palette to leak into Pylon or other Highlight.js consumers.Proposed fix
-.yaml { - .hljs-bullet { - color: var(--color-text-default); - } - .hljs-attr { - color: var(--color-text-success); - } - .hljs-string { - color: var(--color-text-info); +#{$code-roots} { + .yaml { + .hljs-bullet { + color: var(--color-text-default); + } + .hljs-attr { + color: var(--color-text-success); + } + .hljs-string { + color: var(--color-text-info); + } } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cc26a66e-33cb-4fdd-af3b-93cea185aa00
📒 Files selected for processing (1)
frontend/web/styles/3rdParty/_hljs.scss
| // shared classes to our React roots (index.html) so our theme and Pylon's | ||
| // widget stay isolated. :where keeps specificity flat, so this only limits | ||
| // reach, it does not change the cascade within our own rules. | ||
| $code-roots: ':where(#app, #toast, #modal, #modal2, #confirm)'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'storybook-root|id=.app.|decorators' frontend/documentation .storybook 2>/dev/nullRepository: Flagsmith/flagsmith
Length of output: 11137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files\n'
git ls-files '.storybook/**' 'frontend/documentation/**' 'frontend/web/styles/3rdParty/_hljs.scss' | sed -n '1,200p'
printf '\n## Search for likely Storybook root wrappers/IDs\n'
rg -n -C2 'storybook-root|id=|createRoot|withRouter|decorators|layout:|parameters:' .storybook frontend/documentation 2>/dev/null || true
printf '\n## Relevant style file\n'
cat -n frontend/web/styles/3rdParty/_hljs.scss | sed -n '1,220p'Repository: Flagsmith/flagsmith
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## .storybook files\n'
git ls-files '.storybook/**' | sed -n '1,200p'
printf '\n## Storybook-related wrappers / root IDs\n'
rg -n -C2 'storybook-root|id=|createRoot|withRouter|decorators|layout:|parameters:' .storybook frontend/documentation 2>/dev/null || true
printf '\n## _hljs.scss\n'
cat -n frontend/web/styles/3rdParty/_hljs.scss | sed -n '1,220p'Repository: Flagsmith/flagsmith
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '### Storybook config files\n'
git ls-files | rg '(^|/)\.storybook/|storybook|preview\.(ts|tsx|js|jsx)$|decorators?\.(ts|tsx|js|jsx)$'
printf '\n### Root IDs in documentation/storybook files\n'
rg -n -C2 "storybook-root|id=['\"](app|toast|modal2?|confirm)['\"]|<div[^>]+id=['\"](app|toast|modal2?|confirm)['\"]" frontend/documentation .storybook 2>/dev/null || trueRepository: Flagsmith/flagsmith
Length of output: 745
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## Storybook preview config\n'
cat -n frontend/.storybook/preview.js | sed -n '1,220p'
printf '\n## Storybook docs container\n'
cat -n frontend/.storybook/DocsContainer.jsx | sed -n '1,220p'
printf '\n## Highlight.js styles\n'
cat -n frontend/web/styles/3rdParty/_hljs.scss | sed -n '1,220p'Repository: Flagsmith/flagsmith
Length of output: 13093
Add #storybook-root to $code-roots The CodeHelp stories render in Storybook’s default canvas, not under #app, #toast, #modal, #modal2, or #confirm, so these Highlight.js rules won’t apply there.
Pylon injects global .hljs-<type> rules (reading its own --hljs-* vars) that overrode our palette on deployed envs (e.g. .hljs-section rendered coral). Nesting our palette under .hljs (0,2,0 beats Pylon's 0,1,0) plus a [class*='hljs-'] catch-all makes our theme win, unmapped tokens included. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pylon injects --hljs-{comment,keyword,section,literal,string,attr,symbol,class}
on :root and reads them in its global rules. Redefine the whole set to our
tokens within our scope so they're neutralised on our code (and it survives
Pylon marking a rule !important), alongside the palette specificity fix.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
docs/if required so people know about the feature.Changes
Makes the shared highlight.js theme (
_hljs.scss) theme-adaptive: code blocks follow light/dark via semantic tokens instead of always rendering dark. Shared app-wide, including the value editor..hljs--embeddedfor code inside a container that owns its surface; onboarding's card uses it and drops its overridesCodeHelp/HighlightimportButtoninstead of the global; adds CodeHelp stories for light/dark QAFollow-ups: the scrollbar, header/button chrome, and value-editor input still carry dark styling.
How did you test this code?
CI visual-regression across light/dark, plus the CodeHelp stories in both themes.
lintandtypecheckpass.