[DRAFT][RUI-236] Add comparison line chart with tabs#153
Draft
alicjakujawa wants to merge 5 commits intomainfrom
Draft
[DRAFT][RUI-236] Add comparison line chart with tabs#153alicjakujawa wants to merge 5 commits intomainfrom
alicjakujawa wants to merge 5 commits intomainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MeasureTabs
participant LineChartTabbedPro
participant LineChart
User->>MeasureTabs: Clicks measure tab
MeasureTabs->>LineChartTabbedPro: onTabClick(measureIndex)
LineChartTabbedPro->>LineChartTabbedPro: Update activeMeasureIndex state
LineChartTabbedPro->>LineChartTabbedPro: Recompute data & options<br/>for active measure
LineChartTabbedPro->>LineChart: Render with new dataset
LineChart->>User: Display selected measure
MeasureTabs->>MeasureTabs: Highlight active tab
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/components/charts/lines/LineChartTabbedPro/components/useHorizontalScroll.ts (2)
15-21: Consider memoizing scroll handlers.
handleScrollLeftandhandleScrollRightare recreated on every render. If they're passed to memoized child components, this could cause unnecessary re-renders.♻️ Wrap handlers in useCallback
- const handleScrollRight = () => { + const handleScrollRight = useCallback(() => { scrollRef.current?.scrollBy({ left: 200, behavior: 'smooth' }); - }; + }, []); - const handleScrollLeft = () => { + const handleScrollLeft = useCallback(() => { scrollRef.current?.scrollBy({ left: -200, behavior: 'smooth' }); - }; + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/charts/lines/LineChartTabbedPro/components/useHorizontalScroll.ts` around lines 15 - 21, handleScrollLeft and handleScrollRight are recreated on every render which can cause unnecessary re-renders in memoized children; wrap both handlers in useCallback (referencing scrollRef) so they are stable across renders, e.g., create memoized functions for handleScrollLeft and handleScrollRight using useCallback with an appropriate dependency array that includes scrollRef.current or scrollRef if needed.
34-34: Spreadingdepsinto useEffect dependencies may trigger linting warnings.The
eslint-plugin-react-hooksexhaustive-deps rule doesn't handle spread arrays well. If you encounter lint issues, consider restructuring or using a disable comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/charts/lines/LineChartTabbedPro/components/useHorizontalScroll.ts` at line 34, The dependency array currently spreads ...deps into the useEffect call in useHorizontalScroll which can trigger the react-hooks/exhaustive-deps linter; change the dependency strategy so the hook doesn't spread the array: either enumerate the explicit dependencies used by updateScrollState and the effect, or pass deps as a single dependency (e.g. deps) or use a stable serialization (e.g. JSON.stringify(deps)) to detect changes, or add a targeted eslint-disable-next-line comment if you intentionally need the spread; update the useEffect call and its dependency list accordingly and keep references to updateScrollState and deps (from useHorizontalScroll) so the linter is satisfied.src/components/charts/lines/LineChartTabbedPro/components/MeasureTabs.module.css (1)
23-65: Consider consolidating scroll button styles.The
.scrollLeftButtonand.scrollRightButtonselectors are identical. You could use a shared.scrollButtonclass and apply it to both buttons, reducing duplication.♻️ Optional consolidation
-.scrollLeftButton { +.scrollButton { flex-shrink: 0; box-sizing: border-box; cursor: pointer; display: flex; align-items: center; border: none; background: var(--em-tabbedlinechart-scroll-button-background, transparent); padding: var(--em-tabbedlinechart-scroll-button-padding, 0.25rem); color: var(--em-tabbedlinechart-scroll-button-color, `#5c5c66`); } -.scrollLeftButton:hover { +.scrollButton:hover { background: var(--em-tabbedlinechart-scroll-button-background--hover, `#ededf1`); } -.scrollLeftButton svg { +.scrollButton svg { flex-shrink: 0; width: var(--em-tabbedlinechart-scroll-button-icon-size, 1rem); height: var(--em-tabbedlinechart-scroll-button-icon-size, 1rem); } - -.scrollRightButton { - flex-shrink: 0; - box-sizing: border-box; - cursor: pointer; - display: flex; - align-items: center; - border: none; - background: var(--em-tabbedlinechart-scroll-button-background, transparent); - padding: var(--em-tabbedlinechart-scroll-button-padding, 0.25rem); - color: var(--em-tabbedlinechart-scroll-button-color, `#5c5c66`); -} - -.scrollRightButton:hover { - background: var(--em-tabbedlinechart-scroll-button-background--hover, `#ededf1`); -} - -.scrollRightButton svg { - flex-shrink: 0; - width: var(--em-tabbedlinechart-scroll-button-icon-size, 1rem); - height: var(--em-tabbedlinechart-scroll-button-icon-size, 1rem); -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/charts/lines/LineChartTabbedPro/components/MeasureTabs.module.css` around lines 23 - 65, The .scrollLeftButton and .scrollRightButton rules are duplicate; create a shared .scrollButton class containing the common declarations and replace the duplicated rule blocks by using .scrollButton for both buttons (leave the existing :hover and svg rules consolidated under .scrollButton:hover and .scrollButton svg or keep specific overrides only if needed), then update the components that render the buttons to apply the .scrollButton class alongside any directional class if present (refer to .scrollLeftButton and .scrollRightButton to locate where to change).src/components/charts/lines/LineChartTabbedPro/components/MeasureTab.tsx (1)
29-38: Consider addingaria-pressedfor accessibility.Since this is a toggle-like tab button, adding
aria-pressed={isActive}would improve screen reader support.♿ Accessibility enhancement
<button className={clsx(styles.tab, isActive && styles.tabActive)} onClick={onClick} type="button" + aria-pressed={isActive} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/charts/lines/LineChartTabbedPro/components/MeasureTab.tsx` around lines 29 - 38, The button in MeasureTab.tsx should expose its toggle state to assistive tech: update the JSX for the <button> rendered by the MeasureTab component to include aria-pressed={isActive} (using the existing isActive prop) so screen readers know when the tab is active; keep the rest of the props (className, onClick, type) unchanged and ensure the prop value is a boolean.src/components/charts/lines/LineChartTabbedPro/definition.ts (1)
62-74: Metadata and event type alignment is consistent across the codebase, but worth documenting.The pattern of declaring event properties as
type: 'string'in metadata while emittingValue.noFilter()(and exportingstring | nullin TypeScript) is used consistently throughout all chart and editor components. Since this pattern works across the entire codebase, it's likely intentional. A brief doc note or a single shared utility comment explaining why metadata types don't exactly match the runtime contract could help future contributors understand the design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/charts/lines/LineChartTabbedPro/definition.ts` around lines 62 - 74, The event metadata declares axisDimensionValue as type: 'string' while the runtime emits Value.noFilter() and the TypeScript exports allow string | null; add a short shared comment or utility doc explaining this intentional mismatch so future contributors understand the design. Specifically, update the area around the events declaration (the onLineClicked event and its axisDimensionValue property) to reference the runtime contract (Value.noFilter and exported string | null) and link to a single shared comment or README entry describing why metadata uses 'string' for tooling/compat reasons while runtime allows null; ensure the note is concise and colocated near other chart/editor event definitions for discoverability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/charts/lines/LineChartTabbedPro/index.tsx`:
- Around line 58-59: The code assumes measures non-empty; when measures.length
=== 0 safeIndex becomes -1 and activeMeasure is undefined, causing failures. In
the component where safeIndex and activeMeasure are computed (using
activeMeasureIndex and measures), add a guard that checks if measures.length ===
0 and handle it early — e.g., return a null/empty-state render or a fallback
measure before using activeMeasure — so downstream logic (references to
activeMeasure) never runs with undefined. Update the logic around safeIndex and
activeMeasure to only compute/consume activeMeasure when measures.length > 0.
---
Nitpick comments:
In `@src/components/charts/lines/LineChartTabbedPro/components/MeasureTab.tsx`:
- Around line 29-38: The button in MeasureTab.tsx should expose its toggle state
to assistive tech: update the JSX for the <button> rendered by the MeasureTab
component to include aria-pressed={isActive} (using the existing isActive prop)
so screen readers know when the tab is active; keep the rest of the props
(className, onClick, type) unchanged and ensure the prop value is a boolean.
In
`@src/components/charts/lines/LineChartTabbedPro/components/MeasureTabs.module.css`:
- Around line 23-65: The .scrollLeftButton and .scrollRightButton rules are
duplicate; create a shared .scrollButton class containing the common
declarations and replace the duplicated rule blocks by using .scrollButton for
both buttons (leave the existing :hover and svg rules consolidated under
.scrollButton:hover and .scrollButton svg or keep specific overrides only if
needed), then update the components that render the buttons to apply the
.scrollButton class alongside any directional class if present (refer to
.scrollLeftButton and .scrollRightButton to locate where to change).
In
`@src/components/charts/lines/LineChartTabbedPro/components/useHorizontalScroll.ts`:
- Around line 15-21: handleScrollLeft and handleScrollRight are recreated on
every render which can cause unnecessary re-renders in memoized children; wrap
both handlers in useCallback (referencing scrollRef) so they are stable across
renders, e.g., create memoized functions for handleScrollLeft and
handleScrollRight using useCallback with an appropriate dependency array that
includes scrollRef.current or scrollRef if needed.
- Line 34: The dependency array currently spreads ...deps into the useEffect
call in useHorizontalScroll which can trigger the react-hooks/exhaustive-deps
linter; change the dependency strategy so the hook doesn't spread the array:
either enumerate the explicit dependencies used by updateScrollState and the
effect, or pass deps as a single dependency (e.g. deps) or use a stable
serialization (e.g. JSON.stringify(deps)) to detect changes, or add a targeted
eslint-disable-next-line comment if you intentionally need the spread; update
the useEffect call and its dependency list accordingly and keep references to
updateScrollState and deps (from useHorizontalScroll) so the linter is
satisfied.
In `@src/components/charts/lines/LineChartTabbedPro/definition.ts`:
- Around line 62-74: The event metadata declares axisDimensionValue as type:
'string' while the runtime emits Value.noFilter() and the TypeScript exports
allow string | null; add a short shared comment or utility doc explaining this
intentional mismatch so future contributors understand the design. Specifically,
update the area around the events declaration (the onLineClicked event and its
axisDimensionValue property) to reference the runtime contract (Value.noFilter
and exported string | null) and link to a single shared comment or README entry
describing why metadata uses 'string' for tooling/compat reasons while runtime
allows null; ensure the note is concise and colocated near other chart/editor
event definitions for discoverability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2968495-8a24-4ef2-999b-108f99936e3e
📒 Files selected for processing (10)
src/components/charts/lines/LineChartTabbedPro/LineChartTabbedPro.emb.tssrc/components/charts/lines/LineChartTabbedPro/LineChartTabbedPro.utils.tssrc/components/charts/lines/LineChartTabbedPro/components/MeasureTab.module.csssrc/components/charts/lines/LineChartTabbedPro/components/MeasureTab.tsxsrc/components/charts/lines/LineChartTabbedPro/components/MeasureTabs.module.csssrc/components/charts/lines/LineChartTabbedPro/components/MeasureTabs.tsxsrc/components/charts/lines/LineChartTabbedPro/components/useHorizontalScroll.tssrc/components/charts/lines/LineChartTabbedPro/definition.tssrc/components/charts/lines/LineChartTabbedPro/index.tsxsrc/index.ts
Comment on lines
+58
to
+59
| const safeIndex = Math.min(activeMeasureIndex, measures.length - 1); | ||
| const activeMeasure = measures[safeIndex]!; |
There was a problem hiding this comment.
Guard the empty-measure case.
When measures.length is 0, safeIndex becomes -1 and activeMeasure is undefined, so the chart setup blows up instead of falling back cleanly.
Small guard
const [activeMeasureIndex, setActiveMeasureIndex] = useState(0);
- const safeIndex = Math.min(activeMeasureIndex, measures.length - 1);
- const activeMeasure = measures[safeIndex]!;
+ if (measures.length === 0) {
+ return (
+ <ChartCard
+ data={props.results}
+ dimensionsAndMeasures={[xAxis]}
+ errorMessage={props.results.error ?? 'At least one measure is required.'}
+ description={description}
+ title={title}
+ tooltip={tooltip}
+ hideMenu={hideMenu}
+ />
+ );
+ }
+
+ const safeIndex = Math.min(activeMeasureIndex, measures.length - 1);
+ const activeMeasure = measures[safeIndex];📝 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
| const safeIndex = Math.min(activeMeasureIndex, measures.length - 1); | |
| const activeMeasure = measures[safeIndex]!; | |
| const [activeMeasureIndex, setActiveMeasureIndex] = useState(0); | |
| if (measures.length === 0) { | |
| return ( | |
| <ChartCard | |
| data={props.results} | |
| dimensionsAndMeasures={[xAxis]} | |
| errorMessage={props.results.error ?? 'At least one measure is required.'} | |
| description={description} | |
| title={title} | |
| tooltip={tooltip} | |
| hideMenu={hideMenu} | |
| /> | |
| ); | |
| } | |
| const safeIndex = Math.min(activeMeasureIndex, measures.length - 1); | |
| const activeMeasure = measures[safeIndex]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/charts/lines/LineChartTabbedPro/index.tsx` around lines 58 -
59, The code assumes measures non-empty; when measures.length === 0 safeIndex
becomes -1 and activeMeasure is undefined, causing failures. In the component
where safeIndex and activeMeasure are computed (using activeMeasureIndex and
measures), add a guard that checks if measures.length === 0 and handle it early
— e.g., return a null/empty-state render or a fallback measure before using
activeMeasure — so downstream logic (references to activeMeasure) never runs
with undefined. Update the logic around safeIndex and activeMeasure to only
compute/consume activeMeasure when measures.length > 0.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this pull-request needed?
Provide a high-level pull-request description of why this pull request is needed. This information will be used
by reviewers to understand the context of your pull-request.
Main changes
Describe the main changes in this pull-request in more detail.
Test evidence
Provide test evidence that the pull-request changes work as expected.
Summary by CodeRabbit