feat(studio): Comparison Line Chart common component - #1244
Merged
Conversation
Contributor
Author
|
This change is part of the following stack: Change managed by git-spice. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a reusable ChangesComparison line chart
Sequence Diagram(s)sequenceDiagram
participant ComparisonLineChart
participant useComparisonChartModel
participant ComparisonLegend
participant Recharts
ComparisonLineChart->>useComparisonChartModel: Build chart rows and resolve chart state
ComparisonLineChart->>ComparisonLegend: Render legend items and callbacks
ComparisonLineChart->>Recharts: Render axes, lines, reference lines, annotations, and tooltip
ComparisonLegend->>ComparisonLineChart: Toggle or hover a series
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
web/packages/common/src/components/ComparisonLineChart/types.ts (1)
14-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake chart input contracts readonly.
The components and utilities do not mutate these inputs. Mark immutable properties and collections as
readonly. Update utility parameters to accept readonly collections.
web/packages/common/src/components/ComparisonLineChart/types.ts#L14-L25: makeComparisonSeriesimmutable, includingdata.web/packages/common/src/components/ComparisonLineChart/types.ts#L59-L90: make chart input collections and immutable properties readonly.web/packages/common/src/components/ComparisonLineChart/ComparisonLegend.tsx#L8-L22: accept readonly legend items.web/packages/common/src/components/ComparisonLineChart/utils.ts#L21-L25: accept a readonly x axis.web/packages/common/src/components/ComparisonLineChart/utils.ts#L36-L39: accept readonly series and x-axis collections.web/packages/common/src/components/ComparisonLineChart/utils.ts#L62-L62: accept a readonly x axis.web/packages/common/src/components/ComparisonLineChart/utils.ts#L88-L91: accept readonly series and x-axis collections.web/packages/common/src/components/ComparisonLineChart/utils.ts#L122-L124: accept readonly series and x-axis collections.As per coding guidelines, use
readonlyfor immutable properties.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/ComparisonLineChart/types.ts` around lines 14 - 25, Make the ComparisonLineChart input contracts readonly: update ComparisonSeries and the chart input types in web/packages/common/src/components/ComparisonLineChart/types.ts (lines 14-25 and 59-90), accept readonly legend items in ComparisonLegend.tsx (lines 8-22), and change the utility parameters in utils.ts (lines 21-25, 36-39, 62, 88-91, and 122-124) to accept readonly x-axis and series collections, including readonly data and immutable properties.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/packages/common/src/components/ComparisonLineChart/index.tsx`:
- Around line 101-109: Move the onVisibleSeriesChange invocation out of the
setHiddenIds functional updater in the comparison chart toggle handler,
computing the next hidden IDs once before updating state and notifying the
parent once per toggle. Preserve the existing visible-series filtering, and add
a Strict Mode test covering a toggle with exactly one callback notification.
In `@web/packages/common/src/components/ComparisonLineChart/utils.ts`:
- Around line 36-45: Validate all series IDs before constructing rows in
buildChartRows: reject the reserved "x" ID and any duplicate IDs, before
assigning values onto ComparisonChartRow. Preserve the existing row-building
behavior for valid, unique series IDs.
- Around line 121-124: Update hasPlottableData to inspect only series values
corresponding to xAxis indices, matching the points emitted by buildChartRows.
Limit each series scan to xAxis.length (or reject mismatched lengths) so
trailing values cannot make an otherwise empty chart appear plottable.
---
Nitpick comments:
In `@web/packages/common/src/components/ComparisonLineChart/types.ts`:
- Around line 14-25: Make the ComparisonLineChart input contracts readonly:
update ComparisonSeries and the chart input types in
web/packages/common/src/components/ComparisonLineChart/types.ts (lines 14-25 and
59-90), accept readonly legend items in ComparisonLegend.tsx (lines 8-22), and
change the utility parameters in utils.ts (lines 21-25, 36-39, 62, 88-91, and
122-124) to accept readonly x-axis and series collections, including readonly
data and immutable properties.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e1aae3b0-e505-457d-b6be-6dc2b2ca475f
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
web/.gitignoreweb/packages/common/package.jsonweb/packages/common/src/components/ComparisonLineChart/ComparisonAnnotationLabel.tsxweb/packages/common/src/components/ComparisonLineChart/ComparisonLegend.tsxweb/packages/common/src/components/ComparisonLineChart/ComparisonLineChart.stories.tsxweb/packages/common/src/components/ComparisonLineChart/ComparisonLineChartEmpty.tsxweb/packages/common/src/components/ComparisonLineChart/ComparisonLineChartSkeleton.tsxweb/packages/common/src/components/ComparisonLineChart/ComparisonTooltip.tsxweb/packages/common/src/components/ComparisonLineChart/consts.tsweb/packages/common/src/components/ComparisonLineChart/index.test.tsxweb/packages/common/src/components/ComparisonLineChart/index.tsxweb/packages/common/src/components/ComparisonLineChart/types.tsweb/packages/common/src/components/ComparisonLineChart/utils.tsweb/packages/common/src/components/index.ts
Contributor
|
Contributor
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 (3)
web/packages/common/src/components/ComparisonLineChart/index.tsx (3)
156-157: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
classNameduring loading.The loading branch drops the public
className. The ready and empty branches apply it to the outerStack, so the loading state can lose consumer styling or change layout. Wrap the skeleton in the same outer container or passclassNameto it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/ComparisonLineChart/index.tsx` around lines 156 - 157, Update the loading branch of ComparisonLineChart to preserve the public className, matching the ready and empty states by either applying it to an equivalent outer Stack or passing it to ComparisonLineChartSkeleton. Keep the existing loading skeleton and height behavior unchanged.
236-236: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse unique keys for overlay entries.
The keys use coordinates and labels only. Two reference lines with the same
yand label, or two annotations with the samexand label, produce duplicate React keys. Use a stable unique ID from each item, with an index fallback only when the contract has no ID.Also applies to: 249-249
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/ComparisonLineChart/index.tsx` at line 236, Update the overlay entry keys in the comparison chart render near the reference-line and annotation entries to use each item’s stable unique ID, falling back to the mapped index only when no ID exists. Apply this to both the `ref-` and annotation key paths, replacing coordinate-and-label-only keys while preserving the existing prefixes.
279-279: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear
hoveredIdwhen hiding a series.If the user hides the currently hovered series,
hoveredIdstill names the hidden series. This fades every remaining line until pointer leave. ClearhoveredIdwhen hiding that ID, or apply fading only when the hovered ID remains visible. Add a regression test for this interaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/ComparisonLineChart/index.tsx` at line 279, Update the series-hiding logic in ComparisonLineChart so hiding the series identified by hoveredId clears that state, or ensure fading only applies when hoveredId still refers to a visible series. Preserve hover fading for visible series and add a regression test covering hiding the currently hovered series.
🧹 Nitpick comments (1)
web/packages/common/src/components/ComparisonLineChart/index.tsx (1)
52-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the public component return type.
ComparisonLineChartis a public API, but its return type is inferred. Add the repository’s preferred explicit React return type.As per coding guidelines: use explicit return types for public APIs and complex functions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/ComparisonLineChart/index.tsx` around lines 52 - 77, Update the public ComparisonLineChart component declaration to include the repository-preferred explicit React return type, while preserving its existing props, defaults, and rendering behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/packages/common/src/components/ComparisonLineChart/index.tsx`:
- Around line 156-157: Update the loading branch of ComparisonLineChart to
preserve the public className, matching the ready and empty states by either
applying it to an equivalent outer Stack or passing it to
ComparisonLineChartSkeleton. Keep the existing loading skeleton and height
behavior unchanged.
- Line 236: Update the overlay entry keys in the comparison chart render near
the reference-line and annotation entries to use each item’s stable unique ID,
falling back to the mapped index only when no ID exists. Apply this to both the
`ref-` and annotation key paths, replacing coordinate-and-label-only keys while
preserving the existing prefixes.
- Line 279: Update the series-hiding logic in ComparisonLineChart so hiding the
series identified by hoveredId clears that state, or ensure fading only applies
when hoveredId still refers to a visible series. Preserve hover fading for
visible series and add a regression test covering hiding the currently hovered
series.
---
Nitpick comments:
In `@web/packages/common/src/components/ComparisonLineChart/index.tsx`:
- Around line 52-77: Update the public ComparisonLineChart component declaration
to include the repository-preferred explicit React return type, while preserving
its existing props, defaults, and rendering behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df361643-ed7d-462b-a487-932441ef0dc3
📒 Files selected for processing (3)
web/packages/common/src/components/ComparisonLineChart/index.test.tsxweb/packages/common/src/components/ComparisonLineChart/index.tsxweb/packages/common/src/components/ComparisonLineChart/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/packages/common/src/components/ComparisonLineChart/index.test.tsx
- web/packages/common/src/components/ComparisonLineChart/utils.ts
marcusds
reviewed
Aug 11, 2026
marcusds
approved these changes
Aug 11, 2026
steramae-nvidia
force-pushed
the
steramae/comparison-line-chart
branch
from
August 12, 2026 17:11
d467212 to
51531d4
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/packages/common/src/components/ComparisonLineChart/ComparisonChartHeader.tsx (1)
7-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark immutable contract fields as
readonly.
web/packages/common/src/components/ComparisonLineChart/ComparisonChartHeader.tsx#L7-L10: markProps.titleandProps.legendas readonly.web/packages/common/src/components/ComparisonLineChart/chartLayers.tsx#L19-L21: markColoredSeries.resolvedColoras readonly.web/packages/common/src/components/ComparisonLineChart/chartLayers.tsx#L64-L69: markSeriesLineOptionsfields as readonly.As per coding guidelines, “Use
readonlyfor immutable properties.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/ComparisonLineChart/ComparisonChartHeader.tsx` around lines 7 - 10, Mark the immutable contract fields as readonly: Props.title and Props.legend in web/packages/common/src/components/ComparisonLineChart/ComparisonChartHeader.tsx (lines 7-10), ColoredSeries.resolvedColor in web/packages/common/src/components/ComparisonLineChart/chartLayers.tsx (lines 19-21), and every field of SeriesLineOptions in web/packages/common/src/components/ComparisonLineChart/chartLayers.tsx (lines 64-69).Source: Coding guidelines
web/packages/common/src/components/ComparisonLineChart/chartFrame.ts (1)
11-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare return types for exported APIs.
web/packages/common/src/components/ComparisonLineChart/chartFrame.ts#L11-L16: declare the structural margin return type.web/packages/common/src/components/ComparisonLineChart/index.tsx#L39-L64: declare the component return type.web/packages/common/src/components/ComparisonLineChart/useComparisonChartModel.ts#L39-L48: declare a model interface and use it as the hook return type.As per coding guidelines, “Use explicit return types for public APIs and complex functions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/ComparisonLineChart/chartFrame.ts` around lines 11 - 16, Declare explicit public return types for all three APIs: in web/packages/common/src/components/ComparisonLineChart/chartFrame.ts lines 11-16, add a structural margin return type to chartMargin; in web/packages/common/src/components/ComparisonLineChart/index.tsx lines 39-64, annotate the component return type; and in web/packages/common/src/components/ComparisonLineChart/useComparisonChartModel.ts lines 39-48, define a model interface and use it as the hook return type.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/packages/common/src/components/ComparisonLineChart/chartFrame.ts`:
- Around line 11-16: Declare explicit public return types for all three APIs: in
web/packages/common/src/components/ComparisonLineChart/chartFrame.ts lines
11-16, add a structural margin return type to chartMargin; in
web/packages/common/src/components/ComparisonLineChart/index.tsx lines 39-64,
annotate the component return type; and in
web/packages/common/src/components/ComparisonLineChart/useComparisonChartModel.ts
lines 39-48, define a model interface and use it as the hook return type.
In
`@web/packages/common/src/components/ComparisonLineChart/ComparisonChartHeader.tsx`:
- Around line 7-10: Mark the immutable contract fields as readonly: Props.title
and Props.legend in
web/packages/common/src/components/ComparisonLineChart/ComparisonChartHeader.tsx
(lines 7-10), ColoredSeries.resolvedColor in
web/packages/common/src/components/ComparisonLineChart/chartLayers.tsx (lines
19-21), and every field of SeriesLineOptions in
web/packages/common/src/components/ComparisonLineChart/chartLayers.tsx (lines
64-69).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee72bbf2-577c-4e96-b4d5-0d345e5fe321
📒 Files selected for processing (6)
web/packages/common/src/components/ComparisonLineChart/ComparisonChartHeader.tsxweb/packages/common/src/components/ComparisonLineChart/ComparisonLineChartEmpty.tsxweb/packages/common/src/components/ComparisonLineChart/chartFrame.tsweb/packages/common/src/components/ComparisonLineChart/chartLayers.tsxweb/packages/common/src/components/ComparisonLineChart/index.tsxweb/packages/common/src/components/ComparisonLineChart/useComparisonChartModel.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/packages/common/src/components/ComparisonLineChart/ComparisonLineChartEmpty.tsx
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
steramae-nvidia
force-pushed
the
steramae/comparison-line-chart
branch
from
August 12, 2026 17:15
51531d4 to
80eae8c
Compare
15 tasks
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
2 participants
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.
feat(studio): Comparison Line Chart common component
Signed-off-by: Sean Teramae steramae@nvidia.com
lock file
Signed-off-by: Sean Teramae steramae@nvidia.com
Summary
Related Issue
Changes
Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests
Chores