feat(studio): MetricTrendPanel - #1269
Open
steramae-nvidia wants to merge 1 commit into
Open
Conversation
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Contributor
Author
|
This change is part of the following stack: Change managed by git-spice. |
Contributor
📝 WalkthroughWalkthroughAdds ChangesMetric Trend Panel
Sequence Diagram(s)sequenceDiagram
participant Consumer
participant MetricTrendPanel
participant Recharts
Consumer->>MetricTrendPanel: Provide metric series and panel props
MetricTrendPanel->>Recharts: Render active-series trend data
Recharts-->>MetricTrendPanel: Render responsive area chart
MetricTrendPanel-->>Consumer: Render values, delta, series controls, and view action
Suggested reviewers: Mergeability Score: 🔵 Low · up to The panel can mislabel an unchanged metric as increasing and can show loading indefinitely when no data is available. These are bounded presentation issues, so the PR is mergeable with explicit owner awareness or follow-up. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
web/packages/studio/src/components/charts/MetricTrendPanel/index.tsx (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
FCas a type.
FCis used only as a type.-import { FC, useId, useMemo, useState } from 'react'; +import { useId, useMemo, useState } from 'react'; +import type { FC } from 'react';As per coding guidelines, “Use
import typefor type-only imports.”🤖 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/studio/src/components/charts/MetricTrendPanel/index.tsx` at line 17, Update the React import in MetricTrendPanel to import FC using a type-only import while keeping useId, useMemo, and useState as runtime imports.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/studio/src/components/charts/MetricTrendPanel/index.tsx`:
- Around line 101-140: Update the delta styling and rendering in
MetricTrendPanel so delta === 0 uses the neutral tag color and omits the
Triangle, while preserving the existing red/downward behavior for negative
values and green/upward behavior for positive values. Add a test covering zero
delta and asserting the neutral presentation without a directional icon.
- Around line 172-175: Update the loading condition in MetricTrendPanel so an
empty series with isPending=false does not render StackedSkeleton indefinitely;
render the established empty state instead, or enforce a non-empty series
contract. Add a test covering series={[]} with isPending={false} and verify the
loading skeleton is not shown.
---
Nitpick comments:
In `@web/packages/studio/src/components/charts/MetricTrendPanel/index.tsx`:
- Line 17: Update the React import in MetricTrendPanel to import FC using a
type-only import while keeping useId, useMemo, and useState as runtime imports.
🪄 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: abb58632-2b8c-4b9b-ad33-3b77fb751498
📒 Files selected for processing (3)
web/packages/studio/src/components/charts/MetricTrendPanel/MetricTrendPanel.stories.tsxweb/packages/studio/src/components/charts/MetricTrendPanel/MetricTrendPanel.test.tsxweb/packages/studio/src/components/charts/MetricTrendPanel/index.tsx
Comment on lines
+101
to
+140
| const delta = active?.delta; | ||
| const isNegative = delta !== undefined && delta < 0; | ||
| const deltaColor = isNegative ? 'red' : 'green'; | ||
| const lineColor = isNegative ? 'var(--text-color-accent-red)' : 'var(--text-color-brand)'; | ||
| const colorMode = useNvColorMode(); | ||
| const gradient = colorMode === 'dark' ? AREA_GRADIENT.dark : AREA_GRADIENT.light; | ||
|
|
||
| return ( | ||
| <PanelRoot elevation="mid"> | ||
| <PanelHeader className="items-start"> | ||
| <Stack gap="density-xs" className="min-w-0 flex-1"> | ||
| <Text kind="label/bold/xl">{title}</Text> | ||
| {description && ( | ||
| <Text kind="body/regular/md" className="text-secondary"> | ||
| {description} | ||
| </Text> | ||
| )} | ||
| </Stack> | ||
| {onViewClick && ( | ||
| <Button kind="tertiary" size="small" className="shrink-0" onClick={onViewClick}> | ||
| {viewLabel} | ||
| </Button> | ||
| )} | ||
| </PanelHeader> | ||
|
|
||
| <PanelContent> | ||
| <Stack gap="density-lg"> | ||
| <Text kind="display/lg">{active ? formatValue(active.value) : '—'}</Text> | ||
|
|
||
| <Flex align="center" gap="density-lg" wrap="wrap"> | ||
| {delta !== undefined && ( | ||
| <Flex align="center" gap="density-sm"> | ||
| <Tag readOnly color={deltaColor} density="compact"> | ||
| <Triangle | ||
| size={12} | ||
| className={`fill-current ${isNegative ? 'rotate-180' : ''}`} | ||
| aria-hidden | ||
| /> | ||
| {formatDelta(delta)} | ||
| </Tag> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render zero delta as neutral.
When delta === 0, the panel shows a green upward triangle. This reports no change as an increase. Use a neutral tag and omit the directional icon for zero.
Add a zero-delta test.
🤖 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/studio/src/components/charts/MetricTrendPanel/index.tsx` around
lines 101 - 140, Update the delta styling and rendering in MetricTrendPanel so
delta === 0 uses the neutral tag color and omits the Triangle, while preserving
the existing red/downward behavior for negative values and green/upward behavior
for positive values. Add a test covering zero delta and asserting the neutral
presentation without a directional icon.
Comment on lines
+172
to
+175
| <div className="-mx-density-2xl -mb-density-2xl overflow-hidden rounded-b-density-xl"> | ||
| {isPending || !active ? ( | ||
| <StackedSkeleton count={1} height={chartHeight} className="w-full" /> | ||
| ) : ( |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not show loading for empty data.
When series is empty and isPending is false, !active renders StackedSkeleton indefinitely. Render an empty state, or require a non-empty series contract.
Add a test for series={[]} with isPending={false}.
🤖 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/studio/src/components/charts/MetricTrendPanel/index.tsx` around
lines 172 - 175, Update the loading condition in MetricTrendPanel so an empty
series with isPending=false does not render StackedSkeleton indefinitely; render
the established empty state instead, or enforce a non-empty series contract. Add
a test covering series={[]} with isPending={false} and verify the loading
skeleton is not shown.
Contributor
|
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.
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