Skip to content

feat(chart) : Add Scatter and Bubble Chart Components #9151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Harshul23 wants to merge 5 commits into
base: main
Choose a base branch
from
+1,316 −46
Open

feat(chart) : Add Scatter and Bubble Chart Components #9151

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4cd3559
feat(charts): add scatter and bubble chart components
Harshul23 Dec 19, 2025
5664571
fix some console errors.
Harshul23 Dec 19, 2025
b0aa8b4
refactor: improve clipboard copy functionality with enhanced fallback…
Harshul23 Dec 21, 2025
a7b416d
revert : remove hydration mismatch workaround in ChartCodeViewer
Harshul23 Dec 22, 2025
e51edea
fix: add setTimeout cleanup in chart-copy-button
Harshul23 Dec 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/v4/app/(app)/charts/[type]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const chartTypes = [
"pie",
"radar",
"radial",
"scatter",
"tooltip",
] as const
type ChartType = (typeof chartTypes)[number]
Expand Down Expand Up @@ -50,7 +51,7 @@ export default async function ChartPage({ params }: ChartPageProps) {
{type.charAt(0).toUpperCase() + type.slice(1)} Charts
</h2>
<div className="grid flex-1 scroll-mt-20 items-stretch gap-10 md:grid-cols-2 md:gap-6 lg:grid-cols-3 xl:gap-10">
{Array.from({ length: 12 }).map((_, index) => {
{Array.from({ length: 9 }).map((_, index) => {
Copy link

@alamenai alamenai Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We changed the array’s initial length from 12 to 9, but there’s no comment explaining the reason for this change.

If the value is meant to be dynamic, we could derive it from chartList.length instead of hard-coding it.

Copy link
Contributor Author

@Harshul23 Harshul23 Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! The change from 12 to 9 was intentional to better match our scatter chart collection.

Context:

  • This PR adds 6 new scatter chart variants

  • The page previously rendered 12 total slots (actual charts + empty placeholders)

  • With only 6 scatter charts, this created 6 empty placeholder boxes, which looked sparse

Why 9:

  • Changing to 9 gives us a cleaner 3×3 grid on larger screens

  • Shows 6 actual scatter charts + 3 empty placeholders

  • The empty placeholders indicate room for future chart additions

  • Your suggestion about deriving from chartList.length:

I considered this, but the current implementation intentionally shows a few empty slots to indicate that more charts can be added. However, I'm open to either approach:

  1. Keep it at 9 (current) - Shows intentional space for future charts

  2. Use chartList.length - Dynamically adjusts, no empty slots

  3. Use Math.max(chartList.length, 9) Shows charts + minimum 9 slots

Which approach would you prefer for consistency across the codebase?

Copy link

@alamenai alamenai Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You hit it!

I noticed before these empty zones and slots and I was wondering why they were there.

The core team can decide on that.

const chart = chartList[index]
return chart ? (
<ChartDisplay
Expand Down
21 changes: 21 additions & 0 deletions apps/v4/app/(app)/charts/charts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ import { ChartRadialShape } from "@/registry/new-york-v4/charts/chart-radial-sha
import { ChartRadialSimple } from "@/registry/new-york-v4/charts/chart-radial-simple"
import { ChartRadialStacked } from "@/registry/new-york-v4/charts/chart-radial-stacked"
import { ChartRadialText } from "@/registry/new-york-v4/charts/chart-radial-text"
import { ChartScatterBubble } from "@/registry/new-york-v4/charts/chart-scatter-bubble"
import { ChartScatterDefault } from "@/registry/new-york-v4/charts/chart-scatter-default"
import { ChartScatterLabel } from "@/registry/new-york-v4/charts/chart-scatter-label"
import { ChartScatterLegend } from "@/registry/new-york-v4/charts/chart-scatter-legend"
import { ChartScatterMultiple } from "@/registry/new-york-v4/charts/chart-scatter-multiple"
import { ChartScatterShape } from "@/registry/new-york-v4/charts/chart-scatter-shape"
import { ChartTooltipAdvanced } from "@/registry/new-york-v4/charts/chart-tooltip-advanced"
import { ChartTooltipDefault } from "@/registry/new-york-v4/charts/chart-tooltip-default"
import { ChartTooltipFormatter } from "@/registry/new-york-v4/charts/chart-tooltip-formatter"
Expand All @@ -86,6 +92,7 @@ interface ChartGroups {
pie: ChartItem[]
radar: ChartItem[]
radial: ChartItem[]
scatter: ChartItem[]
tooltip: ChartItem[]
}

Expand Down Expand Up @@ -178,6 +185,14 @@ export const charts: ChartGroups = {
{ id: "chart-radial-shape", component: ChartRadialShape },
{ id: "chart-radial-stacked", component: ChartRadialStacked },
],
scatter: [
{ id: "chart-scatter-default", component: ChartScatterDefault },
{ id: "chart-scatter-bubble", component: ChartScatterBubble },
{ id: "chart-scatter-multiple", component: ChartScatterMultiple },
{ id: "chart-scatter-shape", component: ChartScatterShape },
{ id: "chart-scatter-label", component: ChartScatterLabel },
{ id: "chart-scatter-legend", component: ChartScatterLegend },
],
tooltip: [
{ id: "chart-tooltip-default", component: ChartTooltipDefault },
{
Expand Down Expand Up @@ -263,6 +278,12 @@ export {
ChartRadialText,
ChartRadialShape,
ChartRadialStacked,
ChartScatterDefault,
ChartScatterBubble,
ChartScatterMultiple,
ChartScatterShape,
ChartScatterLabel,
ChartScatterLegend,
ChartTooltipDefault,
ChartTooltipIndicatorLine,
ChartTooltipIndicatorNone,
Expand Down
27 changes: 27 additions & 0 deletions apps/v4/components/chart-code-viewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,13 @@ export function ChartCodeViewer({
}: {
chart: Chart
} & React.ComponentProps<"div">) {
const [mounted, setMounted] = React.useState(false)
const isDesktop = useMediaQuery("(min-width: 768px)")

React.useEffect(() => {
setMounted(true)
}, [])

const button = (
<Button
size="sm"
Expand Down Expand Up @@ -79,6 +84,28 @@ export function ChartCodeViewer({
</div>
)

// Prevent hydration mismatch by rendering a default state during SSR
Copy link

@alamenai alamenai Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @Harshul23, for the fix.

Would you mind sharing or reproducing the issue that was occurring in the previous code before this change?

Copy link
Contributor Author

@Harshul23 Harshul23 Dec 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alamenai for asking! Here's the issue and how I fixed it:

The Problem:

The ChartCodeViewer component uses a useMediaQuery hook to conditionally render either a Sheet (desktop) or Drawer (mobile) component. This caused a hydration mismatch because:

  1. Server-Side Render (SSR): The server has no access to window.matchMedia, so useMediaQuery returns a default value (typically false)

  2. Client-Side Render: On the client, useMediaQuery accesses the actual browser window and returns the real media query result

  3. Mismatch: React detects different HTML structures between server and client, triggering the hydration error

How to Reproduce:

  1. Load any chart page (e.g., /charts/scatter) in a fresh browser tab

  2. Open the browser console

  3. You'd see errors like: Warning: Text content does not match server-rendered HTML or Hydration failed because the initial UI does not match what was rendered on the server

The Fix:

I added a mounted state that:

  • During SSR: Always renders the Sheet component (desktop version) with a stable default state

  • After client mount: Re-evaluates isDesktop and renders the correct component based on actual screen size

  • This ensures server and client HTML match initially, preventing hydration mismatch

Code changes:

const [mounted, setMounted] = React.useState(false)

React.useEffect(() => {
  setMounted(true)
}, [])

// During SSR, always render Sheet. After mount, render based on actual media query
if (!mounted) {
  return <Sheet>...</Sheet>
}

// After mounting, render the appropriate component
return isDesktop ? <Sheet>...</Sheet> : <Drawer>...</Drawer>

This pattern is a common solution for SSR hydration issues with dynamic/client-only values in Next.js + Radix Ul applications.

Copy link

@alamenai alamenai Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Harshul23 ,

Thank you for the explanation.

I tried to reproduce it the issue as you mentioned but no warning or errors appears in the browser log regarding the SSR hydration.

Please, could you share a reproduction link so I check that?

Heads up

This kind of patterns is likely implemented by AI.

Copy link
Contributor Author

@Harshul23 Harshul23 Dec 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alamenai,

You're absolutely right to question this. After further investigation, I couldn't consistently reproduce the hydration error across different environments. It appears to have been device-specific or possibly caused by browser extensions in my local setup.

Since this doesn't appear to be a genuine issue with the codebase, I'm going to revert this change and keep the PR focused on the scatter chart additions.

Thanks for catching this!

if (!mounted) {
return (
<Sheet>
<SheetTrigger asChild>{button}</SheetTrigger>
<SheetContent
side="right"
className={cn(
"flex flex-col gap-0 border-l-0 p-0 sm:max-w-sm md:w-[700px] md:max-w-[700px] dark:border-l",
className
)}
>
<SheetHeader className="sr-only">
<SheetTitle>Code</SheetTitle>
<SheetDescription>View the code for the chart.</SheetDescription>
</SheetHeader>
{content}
</SheetContent>
</Sheet>
)
}

if (!isDesktop) {
return (
<Drawer>
Expand Down
49 changes: 39 additions & 10 deletions apps/v4/components/chart-copy-button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,44 @@ export function ChartCopyButton({
}, 2000)
}, [hasCopied])

const copyToClipboard = async (text: string) => {
Copy link

@alamenai alamenai Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function references event, name and setHasCopied in from the closure but it's not memoized.

We could wrap it with useCallback to avoid the creation of the object references between re-renders:

useCallback(()=>{},[event,name])

Copy link
Contributor Author

@Harshul23 Harshul23 Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!
I've wrapped copyToClipboardinuseCallbackwith the correct dependencies[event, name]` to prevent unnecessary function recreations on each render. This improves performance and follows React best practices.

try {
Copy link

@diffray-bot diffray-bot Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 HIGH - Memory leak: setTimeout not cleaned up in useEffect
Category: bug

Description:
The setTimeout in useEffect lacks a cleanup function. If the component unmounts before the 2-second timeout completes, it will attempt to update state on an unmounted component.

Suggestion:
Return a cleanup function from useEffect:

React.useEffect(() => {
  if (!hasCopied) return
  
  const timeoutId = setTimeout(() => {
    setHasCopied(false)
  }, 2000)
  
  return () => clearTimeout(timeoutId)
}, [hasCopied])

Confidence: 95%
Rule: bug_missing_cleanup

Copy link

@alamenai alamenai Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a pull request for this fix.

// Try using the modern clipboard API first
if (navigator.clipboard && window.isSecureContext) {
await navigator.clipboard.writeText(text)
} else {
// Fallback for older browsers or non-secure contexts
const textArea = document.createElement("textarea")
textArea.value = text
textArea.style.position = "fixed"
textArea.style.left = "-999999px"
textArea.style.top = "-999999px"
document.body.appendChild(textArea)
textArea.focus()
textArea.select()

try {
document.execCommand("copy")
Copy link

@alamenai alamenai Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execCommand is a deprecated and it's no longer recommended.Though some browsers might still support it, it may have already been removed from the relevant web standards.

Copy link
Contributor Author

@Harshul23 Harshul23 Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right that execCommand is deprecated. I've acknowledged this in the code comments and marked it clearly as a legacy fallback.

The implementation prioritizes the modern navigator.clipboard API and only falls back to execCommand when there's no alternative (older browsers or HTTP contexts). This ensures maximum compatibility while following best practices where possible.

As browser support continues to improve and HTTPS becomes universal, this fallback will naturally become less necessary over time.

textArea.remove()
} catch (err) {
console.error("Fallback: Could not copy text", err)
textArea.remove()
throw err
}
}

trackEvent({
name: event,
properties: {
name,
},
})
setHasCopied(true)
} catch (err) {
console.error("Failed to copy text: ", err)
}
}

return (
<Tooltip>
<TooltipTrigger asChild>
Expand All @@ -41,16 +79,7 @@ export function ChartCopyButton({
"[&_svg]-h-3.5 h-7 w-7 rounded-[6px] [&_svg]:w-3.5",
className
)}
onClick={() => {
navigator.clipboard.writeText(code)
trackEvent({
name: event,
properties: {
name,
},
})
setHasCopied(true)
}}
onClick={() => copyToClipboard(code)}
Copy link

@alamenai alamenai Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this refactoring, please could you add more details about the older-browsers and the non-secure contexts?

Copy link
Contributor Author

@Harshul23 Harshul23 Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I've added detailed documentation explaining both paths:

Modern path (navigator.clipboard):

  • Used in secure contexts (HTTPS, localhost)
  • Supported in all modern browsers (Chrome 63+, Firefox 53+, Safari 13.1+, Edge 79+)
  • Async and follows current web standards

Fallback path (execCommand):

  • Handles older browsers (IE 11, older Android browsers)
  • Works in non-secure contexts (HTTP-served pages)
  • Needed for environments where clipboard API is unavailable or disabled
  • Includes scenarios like certain iframe configurations

I've added inline comments explaining the use cases for each path.

Copy link

@alamenai alamenai Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, However execCommand is deprecated.

{...props}
>
<span className="sr-only">Copy</span>
Expand Down
9 changes: 9 additions & 0 deletions apps/v4/components/chart-toolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
MousePointer2Icon,
PieChartIcon,
RadarIcon,
ScatterChartIcon,
} from "lucide-react"

import { cn } from "@/lib/utils"
Expand Down Expand Up @@ -94,6 +95,14 @@ function ChartTitle({ chart }: { chart: Chart }) {
)
}

if (chart.name.includes("chart-scatter")) {
return (
<>
<ScatterChartIcon /> Scatter Chart
</>
)
}

if (chart.name.includes("chart-tooltip")) {
return (
<>
Expand Down
4 changes: 4 additions & 0 deletions apps/v4/components/charts-nav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ const links = [
name: "Radial Charts",
href: "/charts/radial#charts",
},
{
name: "Scatter Charts",
href: "/charts/scatter#charts",
},
{
name: "Tooltips",
href: "/charts/tooltip#charts",
Copy link

@diffray-bot diffray-bot Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM - Potentially fragile active link detection logic
Category: bug

Description:
The condition link.href.startsWith(pathname) works for the current use case where links contain hash fragments, but could be fragile if link structure changes or pathname includes query params.

Suggestion:
Consider using pathname.startsWith(link.href.split('#')[0]) for more robust route matching that handles edge cases.

Confidence: 60%
Rule: qual_inverted_logic_js

Expand Down
2 changes: 1 addition & 1 deletion apps/v4/public/r/styles/base-lyra/input-group-example.json
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion apps/v4/public/r/styles/base-lyra/item-example.json
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion apps/v4/public/r/styles/base-lyra/toggle-example.json
View file
Open in desktop

Large diffs are not rendered by default.

Loading
Loading