Skip to content

Add CustomLegend component for interactive legend #35

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
visz11 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add CustomLegend component for interactive legend #35

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
161 changes: 161 additions & 0 deletions index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
import { Box, Flex, Text, Tooltip } from '@chakra-ui/react';
import { useEffect, useState } from 'react';
Copy link

@codeant-ai codeant-ai bot Nov 10, 2025

Choose a reason for hiding this comment

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

Suggestion: Remove the unused useEffect import from React to prevent linter warnings and keep imports minimal. [maintainability]

Severity Level: Minor ⚠️

Suggested change
import { useEffect, useState } from 'react';
import { useState } from 'react';
Why it matters? ⭐

The file index.tsx imports useEffect but there is no usage of useEffect anywhere in the file. I inspected the current file (shown in the PR) and confirmed only useState is referenced — removing useEffect will eliminate a linter warning and is a safe, minimal cleanup.

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** index.tsx
**Line:** 2:2
**Comment:**
	*Maintainability: Remove the unused `useEffect` import from React to prevent linter warnings and keep imports minimal.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

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

medium

The useEffect hook is imported but not used and should be removed. To properly type this component's props as suggested in another comment, you'll need Dispatch and SetStateAction from React, so they should be imported instead.

Suggested change
import { useEffect, useState } from 'react';
import { Dispatch, SetStateAction, useState } from 'react';


export default function CustomLegend({
barProps,
setBarProps,
keys,
w,
...props
}: any) {
Comment on lines +4 to +10
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

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

high

The component props are typed as any, which undermines the benefits of TypeScript. You should define specific types for the props to improve type safety and code clarity. Additionally, ...props are unused and should be removed, and a default value can be set for w.

First, define these interfaces and constants at the top of your file (or in a separate types file):

const DEFAULT_LEGEND_WIDTH = 220;

interface LegendKey {
  key: string;
  name: string;
  color: string;
  chartType: string;
}

interface BarProps {
  [key: string]: boolean | string | null;
  hover: string | null;
}

interface CustomLegendProps {
  barProps: BarProps;
  setBarProps: Dispatch<SetStateAction<BarProps>>;
  keys: LegendKey[];
  w?: number;
}

Then, apply the type to your component props. You should also replace all other occurrences of any with the appropriate specific types (e.g., in reduce callbacks and useState setters).

Suggested change
export default function CustomLegend({
barProps,
setBarProps,
keys,
w,
...props
}: any) {
export default function CustomLegend({
barProps,
setBarProps,
keys,
w = DEFAULT_LEGEND_WIDTH,
}: CustomLegendProps) {

Comment on lines +4 to +10
Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

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

Missing Type Safety

Using 'any' type eliminates TypeScript benefits and reduces maintainability. Props lack proper interface definition making component contract unclear and future modifications risk runtime errors due to missing compile-time validation.

Standards
  • Clean-Code-Meaningful-Names
  • Maintainability-Quality-Analyzability

Comment on lines +4 to +10
Copy link

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Define proper TypeScript types instead of using any.

The component signature uses any for all parameters, which disables TypeScript's type checking and eliminates the benefits of static typing. This can lead to runtime errors and makes the component's contract unclear.

Define proper interfaces:

+interface LegendKey {
+  key: string;
+  name: string;
+  color: string;
+  chartType: 'line_chart' | 'dashed_line_chart' | 'bar_chart' | 'dashed_bar_chart';
+}
+
+interface BarProps {
+  [key: string]: boolean;
+  hover: string | null;
+}
+
+interface CustomLegendProps {
+  barProps: BarProps;
+  setBarProps: React.Dispatch<React.SetStateAction<BarProps>>;
+  keys: LegendKey[];
+  w?: number;
+  [key: string]: any;
+}
+
 export default function CustomLegend({
   barProps,
   setBarProps,
   keys,
   w,
   ...props
-}: any) {
+}: CustomLegendProps) {
📝 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.

Suggested change
export default function CustomLegend({
barProps,
setBarProps,
keys,
w,
...props
}: any) {
interface LegendKey {
key: string;
name: string;
color: string;
chartType: 'line_chart' | 'dashed_line_chart' | 'bar_chart' | 'dashed_bar_chart';
}
interface BarProps {
[key: string]: boolean;
hover: string | null;
}
interface CustomLegendProps {
barProps: BarProps;
setBarProps: React.Dispatch<React.SetStateAction<BarProps>>;
keys: LegendKey[];
w?: number;
[key: string]: any;
}
export default function CustomLegend({
barProps,
setBarProps,
keys,
w,
...props
}: CustomLegendProps) {
🤖 Prompt for AI Agents 
In index.tsx around lines 4 to 10, the component currently types all params as
`any`; replace that with a proper typed props interface (e.g. export an
interface CustomLegendProps) and use it as the function parameter type. Define
barProps with a concrete shape (or a Record<string, number|string> if dynamic),
type setBarProps as React.Dispatch<React.SetStateAction<ThatBarPropsType>>, type
keys as string[] (or string[]|number[] if needed), type w as number, and type
the rest props as React.HTMLAttributes<HTMLElement> or a more specific prop
type; update the function signature to use CustomLegendProps instead of `any`
and import React types if necessary.

const showReset = !keys.every((item: any) => barProps[item.key]);
const [hasShownTooltip, setHasShownTooltip] = useState(false);
const [showTooltip, setShowTooltip] = useState(false);

const resetSelected = () => {
setBarProps(
keys?.reduce(
(a: any, { key }: any) => {
a[key] = true;
return a;
},
{
hover: null,
}
)
);
Comment on lines +16 to +26
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

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

medium

Using reduce to build this object is a bit verbose. You can achieve the same result more concisely and readably with Object.fromEntries.

    setBarProps({
      ...Object.fromEntries(keys.map(({ key }) => [key, true])),
      hover: null,
    });

Comment on lines +16 to +26
Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

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

Inefficient Array Reduction

Array.reduce() creates new object on every resetSelected call with O(n) complexity. For frequent legend resets with large key arrays, this causes unnecessary object allocation and GC pressure affecting performance.

Standards
  • ISO-IEC-25010-Performance-Efficiency-Resource-Utilization
  • Optimization-Pattern-Memoization
  • Algorithmic-Complexity-Linear-Optimization

};

return (
<Box flexDirection={'column'} padding={'0 20px'} w={w ? w : 220}>
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

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

medium

Since a default value for w is now provided in the component signature, you can simplify this to just w={w}.

    <Box flexDirection={'column'} padding={'0 20px'} w={w}>

<Tooltip
label={'Double-click on legend to isolate a single metric.'}
isOpen={showTooltip}
placement="top"
w={170}
fontSize={'xs'}
>
<Text
fontWeight={600}
fontSize={'xs'}
textDecor={'underline'}
w={'fit-content'}
mb={1}
onClick={() => {
if (!showReset) {
return;
}
resetSelected();
}}
visibility={showReset ? 'visible' : 'hidden'}
cursor={'pointer'}
>
Reset
</Text>
Comment on lines +38 to +54
Copy link

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add keyboard accessibility to interactive elements.

The Reset button and legend items are not keyboard-accessible. Users who rely on keyboard navigation cannot interact with the legend, which is an accessibility violation.

Add keyboard support:

 <Text
   fontWeight={600}
   fontSize={'xs'}
   textDecor={'underline'}
   w={'fit-content'}
   mb={1}
+  tabIndex={showReset ? 0 : -1}
+  role="button"
+  aria-label="Reset legend selection"
   onClick={() => {
     if (!showReset) {
       return;
     }
     resetSelected();
   }}
+  onKeyDown={(e) => {
+    if ((e.key === 'Enter' || e.key === ' ') && showReset) {
+      e.preventDefault();
+      resetSelected();
+    }
+  }}
   visibility={showReset ? 'visible' : 'hidden'}
   cursor={'pointer'}
 >
   Reset
 </Text>

 ...

 <Flex
   key={entry?.key}
   align={'center'}
   w={'fit-content'}
   cursor={'pointer'}
+  tabIndex={0}
+  role="button"
+  aria-label={`Toggle ${entry?.name} visibility`}
+  aria-pressed={barProps[entry?.key]}
   onDoubleClick={() => { ... }}
   onClick={() => { ... }}
+  onKeyDown={(e) => {
+    if (e.key === 'Enter' || e.key === ' ') {
+      e.preventDefault();
+      // Trigger click behavior
+      if (!hasShownTooltip) {
+        setHasShownTooltip(true);
+        setShowTooltip(true);
+        setTimeout(() => setShowTooltip(false), 1500);
+      }
+      setBarProps((barProps: any) => ({
+        ...barProps,
+        [entry?.key]: !barProps[entry?.key],
+        hover: null,
+      }));
+    }
+  }}
   onMouseEnter={() => { ... }}
   onMouseLeave={() => { ... }}
 >

Also applies to: 61-108

</Tooltip>
{keys?.map((entry: any) => {
const isLine = ['line_chart', 'dashed_line_chart'].includes(
entry?.chartType
);
Comment on lines +57 to +59
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

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

medium

The strings 'line_chart' and 'dashed_line_chart' are magic strings. It's better to define them as constants or as part of an enum. This prevents typos and makes the code easier to maintain.

For example, you could define:

const CHART_TYPE = {
  LINE: 'line_chart',
  DASHED_LINE: 'dashed_line_chart',
  // ... other types
}

And then use [CHART_TYPE.LINE, CHART_TYPE.DASHED_LINE].includes(entry.chartType).

return (
<Flex
key={entry?.key}
align={'center'}
w={'fit-content'}
cursor={'pointer'}
onDoubleClick={() => {
setBarProps(
keys?.reduce(
(a: any, { key }: any) => {
a[key] = entry?.key === key ? true : false;
return a;
},
{
hover: null,
}
)
);
Comment on lines +67 to +77
Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

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

medium

Using reduce here to build the new props object is verbose and can be harder to read. Consider refactoring for clarity using Object.fromEntries, which is more declarative for this use case.

              setBarProps({
                ...Object.fromEntries(
                  keys.map(({ key }) => [key, entry.key === key])
                ),
                hover: null,
              });

}}
onClick={() => {
if (!hasShownTooltip) {
setHasShownTooltip(true);
setShowTooltip(true);
setTimeout(() => {
setShowTooltip(false);
}, 1500);
Comment on lines +83 to +85
Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

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

Memory Leak Risk

setTimeout creates timer without cleanup mechanism in component lifecycle. Component unmounting before timeout completion leaves dangling timer reference causing memory leaks and accumulated timers degrade application performance.

              const timeoutId = setTimeout(() => {
                setShowTooltip(false);
              }, 1500);
              return () => clearTimeout(timeoutId);
Commitable Suggestion
Suggested change
setTimeout(() => {
setShowTooltip(false);
}, 1500);
const timeoutId = setTimeout(() => {
setShowTooltip(false);
}, 1500);
return () => clearTimeout(timeoutId);
Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness

Copy link

@gemini-code-assist gemini-code-assist bot Nov 10, 2025

Choose a reason for hiding this comment

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

medium

The timeout duration 1500 is a magic number. It's better to define it as a named constant at the top of the component. This improves readability and makes it easier to change the value if needed.

Example:

const TOOLTIP_DISPLAY_DURATION = 1500;
                }, TOOLTIP_DISPLAY_DURATION);

}
Comment on lines +80 to +86
Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

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

Tooltip State Race

Tooltip logic creates potential race condition where rapid clicks can trigger multiple setTimeout callbacks. State updates are not synchronized, allowing overlapping timeout executions that cause inconsistent tooltip visibility state.

Standards
  • Algorithm-Correctness-State-Management
  • Business-Rule-UI-Consistency
  • Logic-Verification-Race-Conditions


setBarProps((barProps: any) => {
return {
...barProps,
[entry?.key]: !barProps[entry?.key],
hover: null,
};
});
}}
Comment on lines +66 to +95
Copy link

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Click and double-click handlers may conflict.

When a user double-clicks a legend item, the onClick handler fires twice before the onDoubleClick handler executes. This causes the item to toggle on/off twice before being isolated, which may result in visual flickers or unexpected intermediate states.

Consider using a click delay pattern or debouncing to distinguish between single and double clicks:

const clickTimeoutRef = useRef<NodeJS.Timeout | null>(null);

// In the JSX:
onClick={(e) => {
  if (clickTimeoutRef.current) {
    // This is part of a double-click, ignore
    clearTimeout(clickTimeoutRef.current);
    clickTimeoutRef.current = null;
    return;
  }
  
  clickTimeoutRef.current = setTimeout(() => {
    // This is a single click
    if (!hasShownTooltip) {
      setHasShownTooltip(true);
      setShowTooltip(true);
      setTimeout(() => setShowTooltip(false), 1500);
    }
    setBarProps((barProps: any) => ({
      ...barProps,
      [entry?.key]: !barProps[entry?.key],
      hover: null,
    }));
    clickTimeoutRef.current = null;
  }, 250); // Delay to detect double-click
}}
🤖 Prompt for AI Agents 
In index.tsx around lines 66 to 95, the onClick and onDoubleClick handlers
conflict because onClick fires twice before onDoubleClick, causing flicker; fix
this by implementing a click-delay pattern: add a ref (e.g. clickTimeoutRef) to
hold a timeout ID, change onClick to start a short timeout (≈250ms) which
performs the single-click logic (tooltip and toggling barProps) and then clears
the ref, and change onDoubleClick to clear any existing click timeout and
immediately run the double-click isolation logic; also add cleanup to clear the
timeout on unmount and type the ref appropriately so clicks that are part of a
double-click are ignored and single-clicks execute only after the delay.

Comment on lines +79 to +95
Copy link

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Memory leak: setTimeout not cleaned up on unmount.

The setTimeout on line 83 will attempt to call setShowTooltip(false) even if the component has unmounted before 1.5 seconds elapse, causing a React warning and potential memory leak.

Store the timeout ID and clean it up:

+import { useEffect, useState, useRef } from 'react';

 export default function CustomLegend({
   ...
 }: any) {
+  const tooltipTimeoutRef = useRef<NodeJS.Timeout | null>(null);
   const showReset = !keys.every((item: any) => barProps[item.key]);
   const [hasShownTooltip, setHasShownTooltip] = useState(false);
   const [showTooltip, setShowTooltip] = useState(false);
+
+  useEffect(() => {
+    return () => {
+      if (tooltipTimeoutRef.current) {
+        clearTimeout(tooltipTimeoutRef.current);
+      }
+    };
+  }, []);

   ...
   
   onClick={() => {
     if (!hasShownTooltip) {
       setHasShownTooltip(true);
       setShowTooltip(true);
-      setTimeout(() => {
+      tooltipTimeoutRef.current = setTimeout(() => {
         setShowTooltip(false);
+        tooltipTimeoutRef.current = null;
       }, 1500);
     }
     ...
   }}
📝 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.

Suggested change
onClick={() => {
if (!hasShownTooltip) {
setHasShownTooltip(true);
setShowTooltip(true);
setTimeout(() => {
setShowTooltip(false);
}, 1500);
}
setBarProps((barProps: any) => {
return {
...barProps,
[entry?.key]: !barProps[entry?.key],
hover: null,
};
});
}}
onClick={() => {
if (!hasShownTooltip) {
setHasShownTooltip(true);
setShowTooltip(true);
tooltipTimeoutRef.current = setTimeout(() => {
setShowTooltip(false);
tooltipTimeoutRef.current = null;
}, 1500);
}
setBarProps((barProps: any) => {
return {
...barProps,
[entry?.key]: !barProps[entry?.key],
hover: null,
};
});
}}
🤖 Prompt for AI Agents 
In index.tsx around lines 79 to 95, the onClick handler creates a setTimeout
that calls setShowTooltip(false) but never clears the timer, risking a memory
leak and React state update after unmount; store the timeout ID (useRef<number |
null> or similar), assign the ID when creating setTimeout, clear any existing
timeout before setting a new one, and call clearTimeout in a useEffect cleanup
(or componentWillUnmount equivalent) so the timer is cancelled if the component
unmounts or a new timeout is created.

onMouseEnter={() => {
if (!barProps[entry?.key]) {
return;
}
setBarProps((barProps: any) => {
return { ...barProps, hover: entry.key };
});
}}
onMouseLeave={() => {
setBarProps((barProps: any) => {
return { ...barProps, hover: null };
});
}}
>
{isLine ? (
<svg width="20" height="3" style={{ marginRight: '5px' }}>
<line
x1="0"
y1="1.5"
x2="20"
y2="1.5"
stroke={barProps[entry?.key] ? entry?.color : 'grey'}
strokeWidth="3"
strokeDasharray={
entry?.chartType === 'dashed_line_chart' ? '2 2' : 'none'
}
/>
</svg>
) : (
<svg width="20" height="20" style={{ marginRight: '5px' }}>
<circle
cx="10"
cy="10"
r="5"
fill={barProps[entry?.key] ? entry?.color : 'grey'}
fillOpacity={
entry?.chartType === 'dashed_bar_chart' ? 0.2 : 1
}
stroke={
entry?.chartType === 'dashed_bar_chart'
? entry?.color
: 'none'
}
strokeDasharray={
entry?.chartType === 'dashed_bar_chart' ? '2 2' : 'none'
}
/>
</svg>
)}
<Text
_hover={
barProps[entry?.key]
? { color: entry?.color, fontWeight: 600 }
: { color: 'grey' }
}
color={barProps[entry?.key] ? entry?.color : 'grey'}
decoration={barProps[entry?.key] ? 'auto' : 'line-through'}
>
{entry?.name}
Copy link

@refacto-visz refacto-visz bot Nov 10, 2025

Choose a reason for hiding this comment

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

DOM-based XSS Risk

User-controlled entry name data rendered directly in JSX without sanitization enables DOM-based XSS attacks. If entry.name contains malicious script content from external sources, it will execute in user browsers.

            <Text
              _hover={
                barProps[entry?.key]
                  ? { color: entry?.color, fontWeight: 600 }
                  : { color: 'grey' }
              }
              color={barProps[entry?.key] ? entry?.color : 'grey'}
              decoration={barProps[entry?.key] ? 'auto' : 'line-through'}
            >
              {entry?.name ? String(entry.name).replace(/<[^>]*>/g, '') : ''}
            </Text>
Commitable Suggestion
Suggested change
{entry?.name}
<Text
_hover={
barProps[entry?.key]
? { color: entry?.color, fontWeight: 600 }
: { color: 'grey' }
}
color={barProps[entry?.key] ? entry?.color : 'grey'}
decoration={barProps[entry?.key] ? 'auto' : 'line-through'}
>
{entry?.name ? String(entry.name).replace(/<[^>]*>/g, '') : ''}
</Text>
Standards
  • CWE-79
  • OWASP-A03
  • NIST-SSDF-PW.1

</Text>
Comment on lines +145 to +155
Copy link

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix incorrect prop name: decoration should be textDecoration.

Chakra UI's Text component does not recognize the decoration prop. It should be textDecoration to apply the line-through style to inactive legend items.

Apply this diff:

 <Text
   _hover={
     barProps[entry?.key]
       ? { color: entry?.color, fontWeight: 600 }
       : { color: 'grey' }
   }
   color={barProps[entry?.key] ? entry?.color : 'grey'}
-  decoration={barProps[entry?.key] ? 'auto' : 'line-through'}
+  textDecoration={barProps[entry?.key] ? 'none' : 'line-through'}
 >
   {entry?.name}
 </Text>
📝 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.

Suggested change
<Text
_hover={
barProps[entry?.key]
? { color: entry?.color, fontWeight: 600 }
: { color: 'grey' }
}
color={barProps[entry?.key] ? entry?.color : 'grey'}
decoration={barProps[entry?.key] ? 'auto' : 'line-through'}
>
{entry?.name}
</Text>
<Text
_hover={
barProps[entry?.key]
? { color: entry?.color, fontWeight: 600 }
: { color: 'grey' }
}
color={barProps[entry?.key] ? entry?.color : 'grey'}
textDecoration={barProps[entry?.key] ? 'none' : 'line-through'}
>
{entry?.name}
</Text>
🤖 Prompt for AI Agents 
In index.tsx around lines 145 to 155, the Chakra UI Text prop `decoration` is
incorrect and should be `textDecoration`; update the prop name to
`textDecoration` and keep the same conditional value (`barProps[entry?.key] ?
'auto' : 'line-through'`) so the inactive legend items render with a
line-through.

</Flex>
);
})}
</Box>
);
}