Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR extends the CodeBlock component system to support multi-axis tabs. It adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes introduce interconnected multi-axis tab logic across several components. While each individual piece is reasonably clear, the review requires understanding how variant state flows through the system, verifying the 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
carlagn
requested review from
AmanVarshney01,
ArthurGamby,
aidankmcalister,
ankur-arch,
mhartington and
mhessdev
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/eclipse/src/components/codeblock.tsx`:
- Around line 275-279: The debug console.log in the tab change handler should be
removed: delete the console.log call inside handleTabChange (which references
tab, activeTab, activeVariant) and ensure the function simply calls
setInternalActiveTab(tab) and onValueChange?.(tab); also remove the other stray
debug logs around lines handling the same tab/variant logic (the
console.debug/console.log calls mentioned at 287-292) so the component emits no
runtime console output during normal tab interactions.
- Around line 254-256: Normalize variant handling by deriving a single boolean
hasVariants = (variants?.length ?? 0) > 0 and use it to gate the tabs and
dropdown in CodeBlockTabsList; ensure activeVariant (and activeTab if present)
is initialized and revalidated to a valid value by falling back to the first
available variant when defaultVariant is missing or not in variants (e.g., set
activeVariant to defaultVariant if it exists in variants, otherwise to
variants[0] when hasVariants is true, otherwise to ""), and update any places
checking truthiness of variants to check hasVariants instead so an empty array
no longer hides the dropdown while disabling triggers.
- Around line 361-397: The variant Select is currently nested inside the ARIA
TabsList (TabsList), which breaks tablist semantics; move the Select (including
Select, SelectTrigger, SelectValue, SelectContent, SelectItem and their props)
out of the TabsList so TabsList only contains the tab pills div
(props.children). Wrap TabsList and the Select in a parent flex container (use
the existing hasVariants check) to preserve the layout (left: TabsList with the
"flex items-center overflow-x-auto" div; right: Select as a sibling), remove the
conditional "items-center justify-between pr-2" class from TabsList and apply
spacing/alignment to the parent container instead, and keep ctx and ctx.* usage
unchanged.
- Around line 453-459: In the CodeBlockTab rendering path, avoid creating the
Radix TabsContent wrapper for non-matching variants: compute the variant match
(using variant, ctx?.variants and ctx.activeVariant) and if it does not match,
return null immediately instead of rendering <TabsContent> with null children;
only render TabsContent when the variantMatch is true so you do not produce
extra tabpanel wrappers for the same tab value (adjust the CodeBlockTab
component around the variantMatch, TabsContent, children, and ctx symbols).
- Around line 130-134: The current rendering of the public prop "icon" in
Codeblock component uses dangerouslySetInnerHTML which can execute unsanitized
markup; update the JSX that handles typeof icon === "string" to render the
string as plain text (e.g., children or text node) instead of injecting HTML,
and if you intend to accept raw SVG markup add a distinct prop (e.g., svgIcon)
and require sanitization before using dangerouslySetInnerHTML; locate the JSX
handling "icon" in the Codeblock component (the typeof icon === "string" branch)
and replace the dangerous injection with text rendering or refactor the prop API
to clearly separate sanitized/raw SVG usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8f8a775-f6a9-490b-929b-f3e45c638762
📒 Files selected for processing (2)
apps/eclipse/content/design-system/components/codeblock.mdxpackages/eclipse/src/components/codeblock.tsx
| {typeof icon === "string" ? ( | ||
| <div | ||
| className="[&_svg]:size-3.5" | ||
| dangerouslySetInnerHTML={{ | ||
| __html: icon, | ||
| }} | ||
| dangerouslySetInnerHTML={{ __html: icon }} | ||
| /> |
There was a problem hiding this comment.
Avoid interpreting icon strings as raw HTML.
icon is a public prop and the docs already use plain strings like "📦". Sending string values through dangerouslySetInnerHTML turns any unsanitized input into executable markup. Render strings as text; if raw SVG markup is intentional, narrow the API and sanitize it first.
🛡️ Safer rendering
- {typeof icon === "string" ? (
- <div
- className="[&_svg]:size-3.5"
- dangerouslySetInnerHTML={{ __html: icon }}
- />
- ) : (
- icon
- )}
+ {typeof icon === "string" ? (
+ <span className="leading-none">{icon}</span>
+ ) : (
+ icon
+ )}🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 132-132: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eclipse/src/components/codeblock.tsx` around lines 130 - 134, The
current rendering of the public prop "icon" in Codeblock component uses
dangerouslySetInnerHTML which can execute unsanitized markup; update the JSX
that handles typeof icon === "string" to render the string as plain text (e.g.,
children or text node) instead of injecting HTML, and if you intend to accept
raw SVG markup add a distinct prop (e.g., svgIcon) and require sanitization
before using dangerouslySetInnerHTML; locate the JSX handling "icon" in the
Codeblock component (the typeof icon === "string" branch) and replace the
dangerous injection with text rendering or refactor the prop API to clearly
separate sanitized/raw SVG usage.
| const [activeVariant, setActiveVariant] = useState<string>( | ||
| defaultVariant ?? variants?.[0] ?? "", | ||
| ); |
There was a problem hiding this comment.
Normalize the “variants enabled” state before using it to gate tabs.
CodeBlockTabsList treats variants={[]} as “no variants” via .length, but the availability checks only test truthiness, so the dropdown disappears while every trigger becomes disabled. Separately, activeVariant is never revalidated, so a valid prop combination like defaultValue="npm" plus defaultVariant="Python" starts blank until the user manually recovers. Drive all of this from one hasVariants = variants?.length > 0 flag and fall back to the first available variant for activeTab.
Also applies to: 260-311, 357-359, 412-415, 456-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eclipse/src/components/codeblock.tsx` around lines 254 - 256,
Normalize variant handling by deriving a single boolean hasVariants =
(variants?.length ?? 0) > 0 and use it to gate the tabs and dropdown in
CodeBlockTabsList; ensure activeVariant (and activeTab if present) is
initialized and revalidated to a valid value by falling back to the first
available variant when defaultVariant is missing or not in variants (e.g., set
activeVariant to defaultVariant if it exists in variants, otherwise to
variants[0] when hasVariants is true, otherwise to ""), and update any places
checking truthiness of variants to check hasVariants instead so an empty array
no longer hides the dropdown while disabling triggers.
| function handleTabChange(tab: string) { | ||
| console.log("handleTabChange called:", { tab, activeTab, activeVariant }); | ||
| setInternalActiveTab(tab); | ||
| onValueChange?.(tab); | ||
| } |
There was a problem hiding this comment.
Remove the debug logging before this ships.
These console.log calls run during normal tab interactions and render paths, so every consumer gets noisy production console output from the design-system component.
🧹 Minimal cleanup
function handleTabChange(tab: string) {
- console.log("handleTabChange called:", { tab, activeTab, activeVariant });
setInternalActiveTab(tab);
onValueChange?.(tab);
}
@@
const availableVariants = useMemo<Set<string>>(() => {
if (!variants) return new Set();
const available = new Set(
variants.filter((v) => existingCombos.has(`${activeTab}__${v}`)),
);
- console.log("availableVariants calculation:", {
- activeTab,
- variants,
- existingCombos: Array.from(existingCombos),
- available: Array.from(available),
- });
return available;
}, [variants, activeTab, existingCombos]);Also applies to: 287-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eclipse/src/components/codeblock.tsx` around lines 275 - 279, The
debug console.log in the tab change handler should be removed: delete the
console.log call inside handleTabChange (which references tab, activeTab,
activeVariant) and ensure the function simply calls setInternalActiveTab(tab)
and onValueChange?.(tab); also remove the other stray debug logs around lines
handling the same tab/variant logic (the console.debug/console.log calls
mentioned at 287-292) so the component emits no runtime console output during
normal tab interactions.
| <TabsList | ||
| {...props} | ||
| className={cn( | ||
| "flex flex-row px-2 overflow-x-auto text-fd-muted-foreground bg-background-default", | ||
| hasVariants && "items-center justify-between pr-2", | ||
| props.className, | ||
| )} | ||
| > | ||
| {props.children} | ||
| {/* Tab pills — left side */} | ||
| <div className="flex items-center overflow-x-auto">{props.children}</div> | ||
| {/* Variant dropdown — right side, only rendered when variants exist */} | ||
| {hasVariants && ctx && ( | ||
| <Select value={ctx.activeVariant} onValueChange={ctx.setActiveVariant}> | ||
| <SelectTrigger | ||
| className={cn( | ||
| "h-7 min-w-[7rem] shrink-0 border-none bg-transparent px-2 py-0", | ||
| "text-fd-muted-foreground type-text-sm focus:ring-0 focus:ring-offset-0", | ||
| "hover:text-fd-accent-foreground", | ||
| )} | ||
| > | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent align="end"> | ||
| {ctx.variants!.map((v) => ( | ||
| <SelectItem | ||
| key={v} | ||
| value={v} | ||
| disabled={!ctx.availableVariants?.has(v)} | ||
| className="type-text-sm" | ||
| > | ||
| {v} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| )} | ||
| </TabsList> |
There was a problem hiding this comment.
Keep the variant selector outside the tablist.
TabsList renders the ARIA tablist. Nesting a second focusable control inside it breaks the tabs structure and can confuse keyboard and screen-reader navigation. The Select should sit next to the tablist, not inside it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eclipse/src/components/codeblock.tsx` around lines 361 - 397, The
variant Select is currently nested inside the ARIA TabsList (TabsList), which
breaks tablist semantics; move the Select (including Select, SelectTrigger,
SelectValue, SelectContent, SelectItem and their props) out of the TabsList so
TabsList only contains the tab pills div (props.children). Wrap TabsList and the
Select in a parent flex container (use the existing hasVariants check) to
preserve the layout (left: TabsList with the "flex items-center overflow-x-auto"
div; right: Select as a sibling), remove the conditional "items-center
justify-between pr-2" class from TabsList and apply spacing/alignment to the
parent container instead, and keep ctx and ctx.* usage unchanged.
| // If this tab has a variant declared, only render its children when the | ||
| // active variant matches. The TabsContent visibility is still controlled | ||
| // by Radix (active tab), so we only gate the inner content. | ||
| const variantMatch = | ||
| !variant || !ctx?.variants || ctx.activeVariant === variant; | ||
|
|
||
| return <TabsContent {...props}>{variantMatch ? children : null}</TabsContent>; |
There was a problem hiding this comment.
Filter non-matching variants before creating TabsContent.
packages/eclipse/src/components/tabs.tsx force-mounts TabsContent, so every CodeBlockTab with the same value still renders an active tabpanel wrapper; only its children disappear. That leaves multiple tabpanels associated with one tab when variants are enabled. Return null for non-matching variants instead of rendering an empty TabsContent.
♿ Simpler panel rendering
export function CodeBlockTab({
variant,
children,
...props
}: CodeBlockTabProps) {
const ctx = use(TabsContext);
-
- // If this tab has a variant declared, only render its children when the
- // active variant matches. The TabsContent visibility is still controlled
- // by Radix (active tab), so we only gate the inner content.
- const variantMatch =
- !variant || !ctx?.variants || ctx.activeVariant === variant;
-
- return <TabsContent {...props}>{variantMatch ? children : null}</TabsContent>;
+ if (ctx?.variants && variant && ctx.activeVariant !== variant) return null;
+ return <TabsContent {...props}>{children}</TabsContent>;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eclipse/src/components/codeblock.tsx` around lines 453 - 459, In the
CodeBlockTab rendering path, avoid creating the Radix TabsContent wrapper for
non-matching variants: compute the variant match (using variant, ctx?.variants
and ctx.activeVariant) and if it does not match, return null immediately instead
of rendering <TabsContent> with null children; only render TabsContent when the
variantMatch is true so you do not produce extra tabpanel wrappers for the same
tab value (adjust the CodeBlockTab component around the variantMatch,
TabsContent, children, and ctx symbols).
1 participant
Summary by CodeRabbit