Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean feature addition. The code is correct, handles edge cases well (null cost/duration, empty runs), and all CI checks pass. One design concern worth noting around dark mode color consistency.
Should Fix
-
web/src/lib/chart-colors.ts:17-24 — The hardcoded hex palette only approximates the light-mode
oklchchart colors fromindex.css. The CSS defines completely different chart colors for dark mode (e.g.,chart-1is orange in light mode but blueoklch(0.488 0.243 264.376)in dark mode), yet these chart components always use the same static hex values regardless of theme. The comment on lines 7-8 says "both light and dark theme colors" but only light-mode equivalents are actually defined in the palette.Consider one of:
- Reading the CSS custom properties at runtime via
getComputedStyle(document.documentElement).getPropertyValue('--color-chart-1') - Defining a separate dark-mode palette and switching based on the current theme (available via
next-themes'suseThemehook) - Choosing a single set of colors that work acceptably in both themes (and updating the comment to not claim theme-awareness)
This isn't blocking since the chosen colors are still visible in dark mode, but they won't match the theme's intended chart color scheme.
- Reading the CSS custom properties at runtime via
The JSDoc claimed the palette covered both light and dark themes, but only light-mode oklch approximations were defined. Update the comment to accurately state the palette is static and visible in both modes but not theme-adaptive. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM - Clean, well-structured addition of recharts visualizations. The components are properly isolated, type-safe against the existing tRPC query data, and handle edge cases (null/zero values, empty data) gracefully. CI passes including lint, typecheck, and all 4618 tests.
Should Fix (non-blocking)
-
Dark mode chart colors (web/src/lib/chart-colors.ts line 19): The hardcoded hex palette approximates light-mode oklch chart colors but will not match the dark-mode theme, which defines a completely different set of chart colors (e.g., dark chart-1 is blue/purple, not orange). The comment on lines 8-9 documents this honestly, but users on dark mode will see visually mismatched charts. Consider reading the computed CSS variable values at render time so charts adapt to the active theme, or check if recharts v3 supports CSS custom properties directly.
-
Bundle size: recharts brings a significant transitive dependency tree (redux, react-redux, @reduxjs/toolkit, immer, d3-*, victory-vendor, es-toolkit). This is ~377 new lockfile entries. For an internal dashboard this is acceptable, but worth being aware of for build/load time. No action needed now, just noting it.
Summary
web/src/lib/chart-colors.ts) mapping agent types to consistent chart colors with human-readable labelsWorkItemDurationChart) showing per-run durations color-coded by agent typeWorkItemCostChart) showing cost composition aggregated by agent type with center total label/work-items/:projectId/:workItemId) and PR Runs (/prs/:projectId/:prNumber) pagesTrello card: https://trello.com/c/69b44daedfaa20a09e71e2f0
Test plan
🤖 Generated with Claude Code