Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughIntroduces a centralized dashboard motion system (section + element lifecycle), per-item motion sequencing, chart animation helpers, AnimatedBarFill, ChartLegend API changes, many charts migrated to helper-driven animation, numerous frontend tests, and small locale and changelog updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Section as AnimatedDashboardSection
participant Preload as onPreload Handler
participant PreloadObs as IntersectionObserver(preload)
participant RevealObs as IntersectionObserver(reveal)
participant Context as DashboardSectionMotion
participant Element as DashboardElement (Chart/Meter/Item)
Browser->>Section: mount
Section->>PreloadObs: observe (preload rootMargin)
PreloadObs-->>Section: intersect (preload)
Section->>Preload: call onPreload()
Preload-->>Section: resolve
Section->>Context: mark contentPrepared
Section->>RevealObs: observe (reveal threshold)
RevealObs-->>Section: intersect (reveal)
Section->>Context: set sectionVisible=true
Context->>Element: expose {sectionVisible, chartStartDelay, shouldReduceMotion}
Element->>Element: compute delayMs (kind + order + chartStartDelay)
Element->>Element: animate (framer-motion or AnimatedBarFill) using delayMs/runKey
Element-->>Browser: becomes visible/interactive
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/features/insights/UsageInsights.tsx (1)
96-125:⚠️ Potential issue | 🟡 MinorKeep the insight cards stretching after adding motion wrappers.
These
DashboardMotionItems are now the grid items, so the innerCardno longer gets grid stretch for free. On the 2/4-column layouts the card bottoms will go ragged once summaries wrap to different lengths. Give the wrappers andInsightCardroot a full-height class.Also applies to: 127-181, 183-229, 231-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/insights/UsageInsights.tsx` around lines 96 - 125, The DashboardMotionItem wrappers are now the grid items so the inner InsightCard no longer stretches; add full-height styling by giving the DashboardMotionItem instances a full-height class (e.g., className="h-full") and ensure the root element of InsightCard also accepts/applies a full-height class (add a h-full to its root container or support a className prop passed from its callers). Apply this change for the blocks around the InsightCard usages (the current block and the other instances you noted) so cards in 2/4-column layouts keep equal heights.src/components/charts/CorrelationAnalysis.tsx (1)
147-162:⚠️ Potential issue | 🟠 MajorDon't hide scatter data when reduced motion is enabled.
animatePointsnow controls both animation and whether any points are rendered. WhenchartMotion.shouldReduceMotionistrue, Line 219 forcesanimatePointstofalse, soCorrelationPanelfeeds[]into Recharts forever. That makes both charts blank for reduced-motion users instead of rendering them statically.♿ Suggested split between visibility and animation
function CorrelationPanel({ title, subtitle, mode, data, color, animationBegin = 0, xAxisName, xTickFormatter, yAxisName, footer, + showPoints, animatePoints, }: { title: string subtitle: string mode: 'requestCost' | 'cacheEfficiency' data: ScatterPoint[] color: string animationBegin?: number xAxisName: string xTickFormatter?: (value: number) => string yAxisName: string footer: string + showPoints: boolean animatePoints: boolean }) { - const chartData = animatePoints ? data : [] + const chartData = showPoints ? data : []- const animatePoints = !chartMotion.shouldReduceMotion && chartMotion.active + const showPoints = chartMotion.shouldReduceMotion || chartMotion.active + const animatePoints = !chartMotion.shouldReduceMotion && chartMotion.active- animatePoints={animatePoints} + showPoints={showPoints} + animatePoints={animatePoints} ... - animatePoints={animatePoints} + showPoints={showPoints} + animatePoints={animatePoints}Also applies to: 214-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/charts/CorrelationAnalysis.tsx` around lines 147 - 162, The component currently uses animatePoints to both hide points and control animation (chartData = animatePoints ? data : []), which causes charts to be empty when chartMotion.shouldReduceMotion forces animatePoints false; change this so chartData always equals data (const chartData = data) and introduce/keep a separate flag (e.g., animatePoints or animateFlag) that only controls animation props on Recharts components; update CorrelationAnalysis (and the code path where chartMotion.shouldReduceMotion sets animatePoints) so visibility uses chartData/data while animation uses the reduced-motion-aware flag.
🧹 Nitpick comments (8)
src/components/charts/chart-theme.ts (1)
44-49: Simplify duplicated delay branch logic.The
stackedand default branches currently compute the same value, so this can be reduced to a simpler conditional for readability.♻️ Suggested simplification
- const delayOffset = - role === 'secondary' - ? 140 + order * CHART_ANIMATION.stagger - : role === 'stacked' - ? order * CHART_ANIMATION.stagger - : order * CHART_ANIMATION.stagger + const delayOffset = + role === 'secondary' + ? 140 + order * CHART_ANIMATION.stagger + : order * CHART_ANIMATION.stagger🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/charts/chart-theme.ts` around lines 44 - 49, The delayOffset calculation has duplicate logic for the 'stacked' and default branches; simplify by computing the common base (order * CHART_ANIMATION.stagger) and only adding the 140 offset when role === 'secondary' so that delayOffset is set to that base plus 140 for 'secondary' otherwise just the base (refer to the delayOffset variable, role, order, and CHART_ANIMATION.stagger).src/components/dashboard/dashboard-motion.tsx (1)
1-353: Rename this file to match the component filename convention.This module exports React components, so introducing a kebab-cased filename here creates a new exception to the repo rule and propagates it through imports.
As per coding guidelines,
src/components/**/*.tsx: Use PascalCase or descriptive kebab-free names for component filenames such asDashboard.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dashboard/dashboard-motion.tsx` around lines 1 - 353, The file name uses kebab-case which violates the component filename convention; rename the file to a PascalCase name that reflects the exported symbols (for example DashboardMotion.tsx or DashboardSectionMotion.tsx) and update all imports that reference "dashboard-motion" to the new filename; ensure the exported identifiers (useDashboardSectionMotion, DashboardMotionItem, AnimatedDashboardSection, etc.) remain unchanged so consumers keep working.src/components/ui/card.tsx (1)
12-29: Consider consolidating duplicate motion configurations.The
shouldReduceMotionanddashboardSectionMotionbranches produce identical objects. This duplication could be simplified.♻️ Suggested simplification
- const motionProps = shouldReduceMotion - ? { - initial: false as const, - animate: { opacity: 1, y: 0 }, - transition: { duration: 0 }, - } - : dashboardSectionMotion - ? { - initial: false as const, - animate: { opacity: 1, y: 0 }, - transition: { duration: 0 }, - } - : { - initial: { opacity: 0, y: 14 }, - whileInView: { opacity: 1, y: 0 }, - viewport: { once: true, amount: 0.15 }, - transition: { duration: 0.35, ease: 'easeOut' as const }, - } + const skipViewportAnimation = shouldReduceMotion || dashboardSectionMotion + const motionProps = skipViewportAnimation + ? { + initial: false as const, + animate: { opacity: 1, y: 0 }, + transition: { duration: 0 }, + } + : { + initial: { opacity: 0, y: 14 }, + whileInView: { opacity: 1, y: 0 }, + viewport: { once: true, amount: 0.15 }, + transition: { duration: 0.35, ease: 'easeOut' as const }, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/card.tsx` around lines 12 - 29, The motionProps assignment duplicates the same motion object for both shouldReduceMotion and dashboardSectionMotion branches; refactor motionProps to compute the reduced/static motion object once (e.g., const staticMotion = { initial: false as const, animate: { opacity: 1, y: 0 }, transition: { duration: 0 } }) and then use that for the shouldReduceMotion and dashboardSectionMotion cases, leaving the original animated object for the fallback; update references to motionProps only (no other API changes).src/components/features/request-quality/RequestQuality.tsx (1)
78-90: Redundant CSS transition classes with framer-motion animation.The
classNameincludestransition-[width] duration-700 ease-outfor CSS transitions, butAnimatedBarFillinternally uses framer-motion'sanimateprop to animate the width. These compete, with framer-motion typically taking precedence. The CSS transition classes are effectively no-ops and add confusion.♻️ Remove redundant CSS transition classes
<AnimatedBarFill - className="h-full rounded-full transition-[width] duration-700 ease-out motion-reduce:transition-none" + className="h-full rounded-full" style={{ backgroundColor: `hsl(${item.accent})`, }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/request-quality/RequestQuality.tsx` around lines 78 - 90, The CSS width transition classes in RequestQuality.tsx are redundant because AnimatedBarFill uses framer-motion for width animation; update the JSX for AnimatedBarFill to remove the CSS transition utilities (`transition-[width]`, `duration-700`, `ease-out`) from its className and retain only non-conflicting classes like `h-full` and `rounded-full` (and keep any `motion-reduce:transition-none` if you still want to support reduced-motion). This ensures framer-motion's animate prop drives the width changes without confusing or dead CSS rules.src/components/dashboard/DashboardSections.tsx (2)
41-140: lazyWithPreload helper defined after its usage.The
lazyWithPreloadfunction is used at lines 41-140 but defined at lines 148-154. While JavaScript hoisting handles function declarations, this pattern reduces readability. Consider moving the helper definition before its usages.📦 Suggested reordering
Move lines 142-160 (type definition,
lazyWithPreload, andpreloadComponents) to before line 41, right after the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dashboard/DashboardSections.tsx` around lines 41 - 140, The helper function lazyWithPreload (and its related preloadComponents/type) is declared after it's used, hurting readability; move the type definition for LazyWithPreload, the lazyWithPreload function, and preloadComponents declaration so they appear before any imports that use them (i.e., place them above the const declarations like CostForecast, CostByModelOverTime, etc.), then run a quick build to ensure no import order issues.
285-296: Deeply nested ternary could be simplified with a lookup object.The section anchor ID mapping uses a 5-level nested ternary. A lookup object would improve readability.
♻️ Proposed refactor
+ const sectionAnchorMap: Partial<Record<DashboardSectionId, string>> = { + costAnalysis: 'charts', + currentMonth: 'current-month', + tokenAnalysis: 'token-analysis', + requestAnalysis: 'request-analysis', + advancedAnalysis: 'advanced-analysis', + } + const renderAnimatedSection = ( sectionId: DashboardSectionId, children: ReactNode, { eager = false, onPreload, }: { eager?: boolean; onPreload?: () => void | Promise<unknown> } = {}, ) => { - const sectionAnchorId = - sectionId === 'costAnalysis' - ? 'charts' - : sectionId === 'currentMonth' - ? 'current-month' - : sectionId === 'tokenAnalysis' - ? 'token-analysis' - : sectionId === 'requestAnalysis' - ? 'request-analysis' - : sectionId === 'advancedAnalysis' - ? 'advanced-analysis' - : sectionId + const sectionAnchorId = sectionAnchorMap[sectionId] ?? sectionId return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dashboard/DashboardSections.tsx` around lines 285 - 296, The nested ternary that computes sectionAnchorId is hard to read; replace it with a lookup object mapping known sectionId values to their anchor strings (e.g., map 'costAnalysis' -> 'charts', 'currentMonth' -> 'current-month', 'tokenAnalysis' -> 'token-analysis', 'requestAnalysis' -> 'request-analysis', 'advancedAnalysis' -> 'advanced-analysis') and then set sectionAnchorId = lookup[sectionId] || sectionId. Update the code that currently references sectionAnchorId to use this new mapping logic (keep the symbol names sectionAnchorId and sectionId) and ensure the fallback to the original sectionId remains.tests/frontend/provider-limits-section.test.tsx (1)
99-99: Consider if 15-second timeout is necessary.Both tests use
15_000ms timeout but appear to be synchronous renders without real timers or network calls. If there's no specific reason for this extended timeout (e.g., slow CI environments), consider using the default or a smaller value.Also applies to: 124-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/provider-limits-section.test.tsx` at line 99, Two tests in provider-limits-section.test.tsx pass an explicit 15_000 ms timeout (the numeric literal 15_000) but appear to be synchronous renders; remove the oversized timeout by deleting the third argument so the tests use Jest's default timeout, or replace 15_000 with a much smaller value like 2000 if a custom timeout is required; apply this change to both occurrences of the 15_000 timeout.src/components/features/heatmap/HeatmapCalendar.tsx (1)
380-416: Consider extracting magic numbers into named constants.The delay calculations use inline magic numbers (
12,6,90). Extracting these to named constants would improve readability and maintainability.♻️ Proposed constants
+const CELL_STAGGER_WEEK_OFFSET_MS = 12 +const CELL_STAGGER_DAY_OFFSET_MS = 6 +const TODAY_OUTLINE_DELAY_MS = 90 const cellMotionProps = shouldReduceMotion ? {} : { // ... transition: { duration: 0.28, delay: (animateCells ? cellAnimationDelayMs + - cell.week * (DASHBOARD_MOTION.itemStaggerMs + 12) + - cell.day * 6 + cell.week * (DASHBOARD_MOTION.itemStaggerMs + CELL_STAGGER_WEEK_OFFSET_MS) + + cell.day * CELL_STAGGER_DAY_OFFSET_MS : 0) / 1000, // ... }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/heatmap/HeatmapCalendar.tsx` around lines 380 - 416, Extract the inline magic numbers used in the delay calculations inside HeatmapCalendar.tsx (within the cellMotionProps and todayOutlineMotionProps blocks) into well-named constants—e.g., WEEK_STAGGER_OFFSET_MS (for 12), DAY_STAGGER_MS (for 6), and TODAY_OUTLINE_EXTRA_DELAY_MS (for 90)—and replace the literals in the delay expressions that add to cellAnimationDelayMs + cell.week * (DASHBOARD_MOTION.itemStaggerMs + 12) + cell.day * 6 and the today outline delay; keep the existing arithmetic/units (ms) and ensure the constants are defined near the top of the module (or next to DASHBOARD_MOTION) so the delay expressions in cellMotionProps and todayOutlineMotionProps read clearly.
🤖 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/cards/SecondaryMetrics.tsx`:
- Around line 95-101: The MetricCard for the dominant provider is using the
wrong tooltip key (t('metricCards.secondary.medianInfo')); update the info prop
on the MetricCard inside DashboardMotionItem (the component rendering
value={metrics.topProvider?.name ?? '–'} and subtitle via topProviderSubtitle)
to use the provider-specific translation key instead (e.g.
t('metricCards.secondary.dominantProviderInfo') or the corresponding provider
help entry).
In `@src/components/charts/CostByModel.tsx`:
- Around line 101-105: The legend renderer in renderLabel produces "Label ()"
when no matching segment is found; update renderLabel (the arrow function using
entry, data, and formatCurrency) to build the label conditionally so it only
appends ` (formattedValue)` when segment is found and non-empty—i.e., compute
value = String(entry.value ?? '') and segment = data.find(...) and return value
alone if !segment, otherwise return `${value}
(${formatCurrency(segment.value)})`.
In `@src/components/charts/RequestCacheHitRateByModel.tsx`:
- Around line 224-229: In RequestCacheHitRateByModel, the non-daily
summary/legend blocks hard-code summary.total.trailing7Rate and the label
instead of using the existing trendLabel/trendRate switch; replace the
hard-coded label and value with trendLabel for the label and use the computed
trendRate key to access the value from summary.total (e.g.,
summary.total[trendRate]) so monthly/yearly views show the same metric as
trendLabel, and apply the same replacement to the other summary/legend block
later in the file that repeats this pattern.
In `@src/components/dashboard/dashboard-motion.tsx`:
- Around line 291-317: When sectionVisible is false, replace aria-hidden usage
with the inert attribute on the container so it and its focusable descendants
are removed from the keyboard/tab order; update both the reduced-motion branch
(the plain div) and the motion.div branch to set inert={true} when
sectionVisible is false and remove aria-hidden, and keep the inline opacity
styling. Also ensure a runtime fallback for browsers that don't support
inert—either load an inert polyfill or, as an immediate fallback, add
pointer-events: none and ensure focusable children (e.g., the expand button in
ChartCard.tsx) are set to tabIndex={-1} while the section is inert and restored
when visible; use the existing symbols DashboardSectionMotionContext.Provider,
shouldReduceMotion, sectionVisible, contentClassName, motion.div and children to
locate where to apply these changes.
- Around line 198-204: The current preload chain uses
Promise.resolve(onPreload()) which does not catch synchronous throws from
onPreload; change the creation of preloadTask to wrap synchronous execution by
using Promise.resolve().then(() => onPreload()) so any synchronous exception is
routed into the .catch() branch; keep the existing .catch(() => undefined) and
.finally(() => { if (isMountedRef.current) setContentPrepared(true) }) so that
setContentPrepared is always reached even if onPreload throws, and ensure this
change is applied to the preloadTask declaration that interacts with
hasTriggeredPreloadRef and isMountedRef.
In `@tests/frontend/dashboard-motion.test.tsx`:
- Line 4: The test suite replaces globalThis.IntersectionObserver but never
restores it; add cleanup to restore the global stub after each test by invoking
vi.unstubAllGlobals() (or capture the original globalThis.IntersectionObserver
before stubbing and restore it in an afterEach) so later tests are not
order-dependent; reference the globalThis.IntersectionObserver replacement in
this file and add an afterEach hook that calls vi.unstubAllGlobals() or
reassigns the saved original.
---
Outside diff comments:
In `@src/components/charts/CorrelationAnalysis.tsx`:
- Around line 147-162: The component currently uses animatePoints to both hide
points and control animation (chartData = animatePoints ? data : []), which
causes charts to be empty when chartMotion.shouldReduceMotion forces
animatePoints false; change this so chartData always equals data (const
chartData = data) and introduce/keep a separate flag (e.g., animatePoints or
animateFlag) that only controls animation props on Recharts components; update
CorrelationAnalysis (and the code path where chartMotion.shouldReduceMotion sets
animatePoints) so visibility uses chartData/data while animation uses the
reduced-motion-aware flag.
In `@src/components/features/insights/UsageInsights.tsx`:
- Around line 96-125: The DashboardMotionItem wrappers are now the grid items so
the inner InsightCard no longer stretches; add full-height styling by giving the
DashboardMotionItem instances a full-height class (e.g., className="h-full") and
ensure the root element of InsightCard also accepts/applies a full-height class
(add a h-full to its root container or support a className prop passed from its
callers). Apply this change for the blocks around the InsightCard usages (the
current block and the other instances you noted) so cards in 2/4-column layouts
keep equal heights.
---
Nitpick comments:
In `@src/components/charts/chart-theme.ts`:
- Around line 44-49: The delayOffset calculation has duplicate logic for the
'stacked' and default branches; simplify by computing the common base (order *
CHART_ANIMATION.stagger) and only adding the 140 offset when role ===
'secondary' so that delayOffset is set to that base plus 140 for 'secondary'
otherwise just the base (refer to the delayOffset variable, role, order, and
CHART_ANIMATION.stagger).
In `@src/components/dashboard/dashboard-motion.tsx`:
- Around line 1-353: The file name uses kebab-case which violates the component
filename convention; rename the file to a PascalCase name that reflects the
exported symbols (for example DashboardMotion.tsx or DashboardSectionMotion.tsx)
and update all imports that reference "dashboard-motion" to the new filename;
ensure the exported identifiers (useDashboardSectionMotion, DashboardMotionItem,
AnimatedDashboardSection, etc.) remain unchanged so consumers keep working.
In `@src/components/dashboard/DashboardSections.tsx`:
- Around line 41-140: The helper function lazyWithPreload (and its related
preloadComponents/type) is declared after it's used, hurting readability; move
the type definition for LazyWithPreload, the lazyWithPreload function, and
preloadComponents declaration so they appear before any imports that use them
(i.e., place them above the const declarations like CostForecast,
CostByModelOverTime, etc.), then run a quick build to ensure no import order
issues.
- Around line 285-296: The nested ternary that computes sectionAnchorId is hard
to read; replace it with a lookup object mapping known sectionId values to their
anchor strings (e.g., map 'costAnalysis' -> 'charts', 'currentMonth' ->
'current-month', 'tokenAnalysis' -> 'token-analysis', 'requestAnalysis' ->
'request-analysis', 'advancedAnalysis' -> 'advanced-analysis') and then set
sectionAnchorId = lookup[sectionId] || sectionId. Update the code that currently
references sectionAnchorId to use this new mapping logic (keep the symbol names
sectionAnchorId and sectionId) and ensure the fallback to the original sectionId
remains.
In `@src/components/features/heatmap/HeatmapCalendar.tsx`:
- Around line 380-416: Extract the inline magic numbers used in the delay
calculations inside HeatmapCalendar.tsx (within the cellMotionProps and
todayOutlineMotionProps blocks) into well-named constants—e.g.,
WEEK_STAGGER_OFFSET_MS (for 12), DAY_STAGGER_MS (for 6), and
TODAY_OUTLINE_EXTRA_DELAY_MS (for 90)—and replace the literals in the delay
expressions that add to cellAnimationDelayMs + cell.week *
(DASHBOARD_MOTION.itemStaggerMs + 12) + cell.day * 6 and the today outline
delay; keep the existing arithmetic/units (ms) and ensure the constants are
defined near the top of the module (or next to DASHBOARD_MOTION) so the delay
expressions in cellMotionProps and todayOutlineMotionProps read clearly.
In `@src/components/features/request-quality/RequestQuality.tsx`:
- Around line 78-90: The CSS width transition classes in RequestQuality.tsx are
redundant because AnimatedBarFill uses framer-motion for width animation; update
the JSX for AnimatedBarFill to remove the CSS transition utilities
(`transition-[width]`, `duration-700`, `ease-out`) from its className and retain
only non-conflicting classes like `h-full` and `rounded-full` (and keep any
`motion-reduce:transition-none` if you still want to support reduced-motion).
This ensures framer-motion's animate prop drives the width changes without
confusing or dead CSS rules.
In `@src/components/ui/card.tsx`:
- Around line 12-29: The motionProps assignment duplicates the same motion
object for both shouldReduceMotion and dashboardSectionMotion branches; refactor
motionProps to compute the reduced/static motion object once (e.g., const
staticMotion = { initial: false as const, animate: { opacity: 1, y: 0 },
transition: { duration: 0 } }) and then use that for the shouldReduceMotion and
dashboardSectionMotion cases, leaving the original animated object for the
fallback; update references to motionProps only (no other API changes).
In `@tests/frontend/provider-limits-section.test.tsx`:
- Line 99: Two tests in provider-limits-section.test.tsx pass an explicit 15_000
ms timeout (the numeric literal 15_000) but appear to be synchronous renders;
remove the oversized timeout by deleting the third argument so the tests use
Jest's default timeout, or replace 15_000 with a much smaller value like 2000 if
a custom timeout is required; apply this change to both occurrences of the
15_000 timeout.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fbec493-6889-4c22-9ade-749ed63aff73
📒 Files selected for processing (44)
CHANGELOG.mdsrc/components/cards/MonthMetrics.tsxsrc/components/cards/PrimaryMetrics.tsxsrc/components/cards/SecondaryMetrics.tsxsrc/components/cards/TodayMetrics.tsxsrc/components/charts/ChartCard.tsxsrc/components/charts/ChartLegend.tsxsrc/components/charts/CorrelationAnalysis.tsxsrc/components/charts/CostByModel.tsxsrc/components/charts/CostByModelOverTime.tsxsrc/components/charts/CostByWeekday.tsxsrc/components/charts/CostOverTime.tsxsrc/components/charts/CumulativeCost.tsxsrc/components/charts/DistributionAnalysis.tsxsrc/components/charts/ModelMix.tsxsrc/components/charts/RequestCacheHitRateByModel.tsxsrc/components/charts/RequestsOverTime.tsxsrc/components/charts/TokenEfficiency.tsxsrc/components/charts/TokenTypes.tsxsrc/components/charts/TokensOverTime.tsxsrc/components/charts/chart-theme.tssrc/components/dashboard/DashboardSections.tsxsrc/components/dashboard/dashboard-motion.tsxsrc/components/features/animations/AnimatedBarFill.tsxsrc/components/features/cache-roi/CacheROI.tsxsrc/components/features/forecast/CostForecast.tsxsrc/components/features/heatmap/HeatmapCalendar.tsxsrc/components/features/insights/UsageInsights.tsxsrc/components/features/limits/ProviderLimitsSection.tsxsrc/components/features/request-quality/RequestQuality.tsxsrc/components/features/risk/ConcentrationRisk.tsxsrc/components/tables/RecentDays.tsxsrc/components/ui/card.tsxtests/frontend/cache-roi.test.tsxtests/frontend/chart-legend-integration.test.tsxtests/frontend/cost-over-time.test.tsxtests/frontend/dashboard-motion.test.tsxtests/frontend/distribution-analysis.test.tsxtests/frontend/info-heading.test.tsxtests/frontend/provider-limits-section.test.tsxtests/frontend/request-cache-hit-rate-by-model.test.tsxtests/frontend/request-quality.test.tsxtests/frontend/toast.test.tsxtests/integration/server.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/features/request-quality/RequestQuality.tsx (1)
78-89: Consider wiring barorderfor deterministic stagger.Right now each bar uses the default
order=0, so they animate simultaneously. Passing index-based order would better match orchestrated sequencing.💡 Proposed change
- {qualityMetrics.map((item) => ( + {qualityMetrics.map((item, index) => ( <div key={item.label} className="rounded-xl border border-border/50 bg-muted/15 p-3"> <div className="text-[10px] tracking-[0.14em] text-muted-foreground uppercase"> {item.label} </div> @@ <AnimatedBarFill className="h-full rounded-full" + order={index} style={{ backgroundColor: `hsl(${item.accent})`, }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/request-quality/RequestQuality.tsx` around lines 78 - 89, The bars all animate at once because no ordering is supplied; when mapping items pass a deterministic index-based order into AnimatedBarFill (e.g., add an order={index} prop or style={{ order: index }}) so each bar can stagger based on its position; update the map where AnimatedBarFill is rendered (reference AnimatedBarFill, item, metrics.hasRequestData) to use the incoming index and ensure the component/stylesheet consumes that order for staggered animation.tests/frontend/usage-insights.test.tsx (1)
65-69: Make assertions more focused than generic class-chain matching.The
.grid > .h-fullselectors are broad and can pass due to unrelated layout changes. Prefer a scoped container/test-id and assert the intended items explicitly.As per coding guidelines: "Prefer focused
*.test.tsor*.test.tsxcoverage for data transforms, hooks, or complex UI behavior".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/usage-insights.test.tsx` around lines 65 - 69, The current assertions use broad selectors ('.grid > .h-full' and '.grid > .h-full > .h-full') which can be satisfied by unrelated layout elements; update the test to target a scoped container or test ids and assert the intended items explicitly: add/use a stable data-testid on the parent/panel (or use the rendered component's semantic role) and replace motionItems/cards with queries like getAllByTestId or within(scopedContainer).getAllByTestId for the specific card/motion items, then assert on their expected count or specific content/attributes to make the test focused and resistant to layout changes.src/components/dashboard/DashboardMotion.tsx (1)
295-324: Consider simplifying duplicate style application.The
contentStyleis applied via thestyleprop, but it includesopacity: 0when not visible while theanimateprop also controls opacity. This creates redundancy—the motion.div'sanimatealready handles opacity transitions.💡 Suggested simplification
The
contentStylewithopacity: 0andpointerEvents: 'none'may be redundant when framer-motion'sanimatealready controls opacity. However, this could be intentional for the initial render before animation kicks in. Consider whether the inline style is necessary or ifanimate's initial state suffices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dashboard/DashboardMotion.tsx` around lines 295 - 324, Remove the redundant opacity in contentStyle when rendering the animated branch: for the motion.div controlled by sectionVisible and DASHBOARD_MOTION, rely on motion's initial/animate props for opacity and remove any hardcoded opacity: 0 from contentStyle so opacity is not applied twice; keep any non-opacity styles (e.g., pointerEvents) and ensure the non-animated branch (when shouldReduceMotion is true) still gets the necessary styles via contentStyle and the inert logic around sectionVisible.
🤖 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/cards/SecondaryMetrics.tsx`:
- Around line 78-92: The info prop is hardcoded to day-specific entries causing
wrong help text when viewMode is monthly/yearly; update the MetricCard usages
inside DashboardMotionItem (the card with label computed from viewMode and the
card around lines 104-120) to select the appropriate METRIC_HELP key based on
viewMode (e.g., use METRIC_HELP.mostExpensiveYear / .mostExpensiveMonth /
.mostExpensiveDay for the top-cost card and METRIC_HELP.avgCostPerYear /
.avgCostPerMonth / .avgCostPerDay for the average-cost card) so the info prop
mirrors the label logic; keep existing props like value, icon and subtitle
(topDaySubtitle/topAvgSubtitle) unchanged.
In `@src/components/charts/chart-theme.ts`:
- Around line 93-99: The scatter animation helper getScatterAnimationProps
currently sets animationDuration to CHART_ANIMATION.barDuration; change it to
use the scatter/reveal-specific constant CHART_ANIMATION.revealDuration so
scatter timing honors the new setting (keep animationBegin, animationEasing, and
the delayOffsetMs behavior as-is).
---
Nitpick comments:
In `@src/components/dashboard/DashboardMotion.tsx`:
- Around line 295-324: Remove the redundant opacity in contentStyle when
rendering the animated branch: for the motion.div controlled by sectionVisible
and DASHBOARD_MOTION, rely on motion's initial/animate props for opacity and
remove any hardcoded opacity: 0 from contentStyle so opacity is not applied
twice; keep any non-opacity styles (e.g., pointerEvents) and ensure the
non-animated branch (when shouldReduceMotion is true) still gets the necessary
styles via contentStyle and the inert logic around sectionVisible.
In `@src/components/features/request-quality/RequestQuality.tsx`:
- Around line 78-89: The bars all animate at once because no ordering is
supplied; when mapping items pass a deterministic index-based order into
AnimatedBarFill (e.g., add an order={index} prop or style={{ order: index }}) so
each bar can stagger based on its position; update the map where AnimatedBarFill
is rendered (reference AnimatedBarFill, item, metrics.hasRequestData) to use the
incoming index and ensure the component/stylesheet consumes that order for
staggered animation.
In `@tests/frontend/usage-insights.test.tsx`:
- Around line 65-69: The current assertions use broad selectors ('.grid >
.h-full' and '.grid > .h-full > .h-full') which can be satisfied by unrelated
layout elements; update the test to target a scoped container or test ids and
assert the intended items explicitly: add/use a stable data-testid on the
parent/panel (or use the rendered component's semantic role) and replace
motionItems/cards with queries like getAllByTestId or
within(scopedContainer).getAllByTestId for the specific card/motion items, then
assert on their expected count or specific content/attributes to make the test
focused and resistant to layout changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5104819f-cc13-4571-a1d5-50d8b9bd0a63
📒 Files selected for processing (28)
src/components/cards/MonthMetrics.tsxsrc/components/cards/PrimaryMetrics.tsxsrc/components/cards/SecondaryMetrics.tsxsrc/components/cards/TodayMetrics.tsxsrc/components/charts/ChartCard.tsxsrc/components/charts/CorrelationAnalysis.tsxsrc/components/charts/CostByModel.tsxsrc/components/charts/RequestCacheHitRateByModel.tsxsrc/components/charts/chart-theme.tssrc/components/dashboard/DashboardMotion.tsxsrc/components/dashboard/DashboardSections.tsxsrc/components/features/animations/AnimatedBarFill.tsxsrc/components/features/heatmap/HeatmapCalendar.tsxsrc/components/features/insights/UsageInsights.tsxsrc/components/features/limits/ProviderLimitsSection.tsxsrc/components/features/request-quality/RequestQuality.tsxsrc/components/ui/card.tsxsrc/locales/de/common.jsonsrc/locales/en/common.jsontests/frontend/correlation-analysis.test.tsxtests/frontend/dashboard-motion.test.tsxtests/frontend/drill-down-modal.test.tsxtests/frontend/heatmap-calendar.test.tsxtests/frontend/phase4-correctness.test.tsxtests/frontend/provider-limits-section.test.tsxtests/frontend/request-cache-hit-rate-by-model.test.tsxtests/frontend/sortable-tables.test.tsxtests/frontend/usage-insights.test.tsx
✅ Files skipped from review due to trivial changes (3)
- src/locales/en/common.json
- src/locales/de/common.json
- tests/frontend/dashboard-motion.test.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/cards/PrimaryMetrics.tsx
- src/components/cards/TodayMetrics.tsx
- tests/frontend/provider-limits-section.test.tsx
- src/components/ui/card.tsx
- tests/frontend/request-cache-hit-rate-by-model.test.tsx
- src/components/features/animations/AnimatedBarFill.tsx
- src/components/features/insights/UsageInsights.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/frontend/request-quality-order.test.tsx (1)
43-53: AddafterEachcleanup to prevent global stub leakage.Line 44 stubs
IntersectionObserverglobally without cleanup. AddafterEach(() => vi.unstubAllGlobals())to prevent mock state from leaking into other tests. This pattern is used consistently acrossdashboard-motion.test.tsxandmotion-accessibility.test.tsxin the codebase.Suggested patch
-import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ describe('RequestQuality bar ordering', () => { beforeEach(async () => { @@ await initI18n('en') }) + + afterEach(() => { + vi.unstubAllGlobals() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/request-quality-order.test.tsx` around lines 43 - 53, The test stubs the global IntersectionObserver in the beforeEach block which leaks into other tests; add an afterEach block that calls vi.unstubAllGlobals() to restore globals after each test. Modify the file's setup by adding afterEach(() => vi.unstubAllGlobals()) alongside the existing beforeEach that defines the IntersectionObserver stub and calls initI18n('en') so the global stub is cleaned up between tests.
🤖 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/cards/SecondaryMetrics.tsx`:
- Around line 116-132: The MetricCard inside DashboardMotionItem is passing
avgCostInfo for the info/tooltip even when viewMode === 'daily' (which shows
busiestWeek/peak7Days); update the info prop on this MetricCard to supply the
peak/peak7Days help text when viewMode === 'daily' (e.g., conditional on
viewMode or metrics.busiestWeek) instead of always using avgCostInfo so the
tooltip matches the label and value (reference symbols: DashboardMotionItem,
MetricCard, viewMode, metrics.busiestWeek, avgCostInfo,
METRIC_HELP.avgCostPerDay).
In `@src/components/dashboard/DashboardMotion.tsx`:
- Around line 133-149: Make inactive items non-interactive until reveal by using
itemMotion.active to toggle interactivity: add aria-hidden={!itemMotion.active}
and a style pointerEvents: itemMotion.active ? undefined : 'none' on the
motion.div (the element created in DashboardMotion.tsx), and implement a small
useEffect that watches itemMotion.active and, using itemRef.current, sets
tabIndex=-1 on any focusable descendants when inactive and restores their
original tabIndex when active (so keyboard users cannot tab into visually hidden
content). Reference: motion.div, itemRef, itemMotion.active.
- Around line 73-74: The element-level motion is still gated by useInView
(isInView) which defaults to false when IntersectionObserver is unavailable;
change the active calculation to apply the same fallback as sectionMotion (treat
in-view as true when IntersectionObserver is missing). Update the code
referencing isInView/active (e.g., the const isInView = useInView(...) and const
active = ...) to short-circuit when IntersectionObserver is undefined (use
typeof IntersectionObserver === 'undefined' or globalThis check) so active
becomes (sectionMotion?.sectionVisible ?? true) && (IntersectionObserver missing
? true : isInView).
In `@tests/frontend/secondary-metrics.test.tsx`:
- Around line 50-59: The test installs a global IntersectionObserver stub in the
beforeEach but never restores it, risking test pollution; update the spec to
capture the original global.IntersectionObserver before calling
vi.stubGlobal('IntersectionObserver', ...) and add an afterEach that restores
the original (or calls
vi.unstubAllGlobals()/vi.unstubGlobal('IntersectionObserver')) so the fake
observer is removed after each test; refer to the beforeEach,
IntersectionObserver stub and add a corresponding afterEach restoration.
---
Nitpick comments:
In `@tests/frontend/request-quality-order.test.tsx`:
- Around line 43-53: The test stubs the global IntersectionObserver in the
beforeEach block which leaks into other tests; add an afterEach block that calls
vi.unstubAllGlobals() to restore globals after each test. Modify the file's
setup by adding afterEach(() => vi.unstubAllGlobals()) alongside the existing
beforeEach that defines the IntersectionObserver stub and calls initI18n('en')
so the global stub is cleaned up between tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4296320-5b35-4743-9bb0-78833446fb08
📒 Files selected for processing (10)
src/components/cards/SecondaryMetrics.tsxsrc/components/charts/chart-theme.tssrc/components/dashboard/DashboardMotion.tsxsrc/components/features/insights/UsageInsights.tsxsrc/components/features/request-quality/RequestQuality.tsxsrc/lib/help-content.tstests/frontend/request-quality-order.test.tsxtests/frontend/secondary-metrics.test.tsxtests/frontend/usage-insights.test.tsxtests/unit/chart-theme.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/help-content.ts
- tests/unit/chart-theme.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/features/insights/UsageInsights.tsx
- src/components/features/request-quality/RequestQuality.tsx
- tests/frontend/usage-insights.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/frontend/dashboard-motion.test.tsx (1)
15-41: Avoid binding these tests toIntersectionObserverconstruction order.Hard-coding
instances[0],instances[1], andslice(2)makes the suite fail on harmless internal refactors that add or reorder observers. Capture the mock'soptions/observed target and select preload, reveal, and item observers by those semantics instead.♻️ Possible cleanup
class MockIntersectionObserver { static instances: MockIntersectionObserver[] = [] callback: IntersectionObserverCallback + options?: IntersectionObserverInit - constructor(callback: IntersectionObserverCallback) { + constructor(callback: IntersectionObserverCallback, options?: IntersectionObserverInit) { this.callback = callback + this.options = options MockIntersectionObserver.instances.push(this) } - observe() {} + observe() {} unobserve() {} disconnect() {} @@ } + +function getObserver(predicate: (observer: MockIntersectionObserver) => boolean) { + const observer = MockIntersectionObserver.instances.find(predicate) + expect(observer).toBeDefined() + return observer! +} @@ - act(() => { - MockIntersectionObserver.instances[0]?.trigger(true) - }) + act(() => { + getObserver((observer) => observer.options?.threshold === 0).trigger(true) + }) @@ - act(() => { - MockIntersectionObserver.instances[1]?.trigger(true) - }) + act(() => { + getObserver( + (observer) => observer.options?.threshold === 0.14, + ).trigger(true) + })Also applies to: 100-126, 147-163, 223-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/dashboard-motion.test.tsx` around lines 15 - 41, Tests rely on MockIntersectionObserver.instances index positions (e.g., instances[0], instances[1], instances.slice(2)) which is brittle; modify MockIntersectionObserver to record the constructor options and the observed targets (store the last observed element in observe()) and then update tests to locate the preload/reveal/item observers by matching those recorded semantics (e.g., matching options.root/rootMargin or the observed target element) instead of by array index; update references to use instance.find by property (e.g., instance.options or instance.lastObserved) and keep trigger/triggering behavior the same via the existing trigger method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/frontend/dashboard-motion.test.tsx`:
- Around line 15-41: Tests rely on MockIntersectionObserver.instances index
positions (e.g., instances[0], instances[1], instances.slice(2)) which is
brittle; modify MockIntersectionObserver to record the constructor options and
the observed targets (store the last observed element in observe()) and then
update tests to locate the preload/reveal/item observers by matching those
recorded semantics (e.g., matching options.root/rootMargin or the observed
target element) instead of by array index; update references to use
instance.find by property (e.g., instance.options or instance.lastObserved) and
keep trigger/triggering behavior the same via the existing trigger method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58a1d9af-ff5a-4306-aa27-4bd0b01ba4cb
📒 Files selected for processing (6)
src/components/cards/SecondaryMetrics.tsxsrc/components/dashboard/DashboardMotion.tsxsrc/lib/help-content.tstests/frontend/dashboard-motion.test.tsxtests/frontend/request-quality-order.test.tsxtests/frontend/secondary-metrics.test.tsx
✅ Files skipped from review due to trivial changes (2)
- src/lib/help-content.ts
- tests/frontend/secondary-metrics.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/frontend/request-quality-order.test.tsx
This was referenced Apr 21, 2026
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests