refactor: consolidate duplicated chart rendering into reusable components#174
refactor: consolidate duplicated chart rendering into reusable components#174hessius merged 6 commits intofeat/desktop-two-columnfrom
Conversation
Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the Shot History UI to reduce duplicated Recharts JSX by extracting the replay/compare/analyze charts into reusable components, aiming to keep mobile and desktop chart behavior in sync and easier to maintain.
Changes:
- Added
ReplayChart,CompareChart, andAnalyzeChartreusable components inShotCharts.tsx. - Replaced multiple duplicated chart-rendering blocks in
ShotHistoryView.tsxwith component calls (withvariant="mobile" | "desktop"styling). - Cleaned up
apps/web/package-lock.jsonby removing thebundependency entries.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| apps/web/src/components/ShotHistoryView.tsx | Uses extracted chart components in place of some duplicated chart JSX blocks. |
| apps/web/src/components/ShotCharts.tsx | New reusable chart components encapsulating Recharts rendering logic and styling variants. |
| apps/web/package-lock.json | Removes bun from lockfile dependency graph. |
Files not reviewed (1)
- apps/web/package-lock.json: Language not supported
| const isMobile = variant === 'mobile' | ||
| const chartHeight = isMobile ? 'h-64' : 'h-[60vh] min-h-[400px]' | ||
| const padding = isMobile ? 'p-1' : 'p-2' | ||
| const rightMargin = isMobile ? 0 : 5 | ||
| const wrapperClass = isMobile ? 'space-y-2' : '' | ||
|
|
||
| return ( | ||
| <div className={wrapperClass}> | ||
| <div className="flex items-center justify-between"> | ||
| <Label className="text-sm font-semibold tracking-wide text-primary flex items-center gap-2"> |
There was a problem hiding this comment.
ReplayChart wraps all content in a single <div>, but on desktop the parent layout in ShotHistoryView uses space-y-2 to create spacing between the header/chart/legend siblings. With a single wrapper element, that spacing is lost and the sections will render flush together. Consider returning a fragment (like CompareChart/AnalyzeChart) or applying a space-y-* wrapper for the desktop variant as well to preserve the previous layout.
There was a problem hiding this comment.
Fixed in 778e401. ReplayChart now returns a fragment for desktop variant, allowing the parent's space-y-2 to properly apply spacing between header, chart, and legend elements. Mobile variant still wraps content in a div with space-y-2 since it doesn't have a parent container with spacing.
| <CompareChart | ||
| combinedData={combinedData} | ||
| dataMaxTime={dataMaxTime} | ||
| leftDomain={leftDomain} | ||
| rightDomain={rightDomain} | ||
| isShowingReplay={isShowingReplay} | ||
| comparisonCurrentTime={comparisonCurrentTime} | ||
| comparisonIsPlaying={comparisonIsPlaying} | ||
| comparisonPlaybackSpeed={comparisonPlaybackSpeed} | ||
| variant="desktop" | ||
| /> |
There was a problem hiding this comment.
This refactor introduces CompareChart/AnalyzeChart, but ShotHistoryView still contains inline Recharts render blocks for the mobile comparison chart and the small “Shot vs Profile Target” chart inside the analysis summary. That leaves chart rendering logic duplicated across files and makes it easier for mobile/desktop to drift again. Consider swapping those remaining inline charts to use CompareChart/AnalyzeChart with variant="mobile" (and then removing any now-unused Recharts JSX in ShotHistoryView).
There was a problem hiding this comment.
Partially addressed in 778e401:
✅ Mobile comparison chart (lines 1656-1738) replaced with CompareChart component with variant="mobile"
The small "Shot vs Profile Target" chart in the analysis summary is a unique one-off display (h-48 height, custom header positioning, embedded in summary card). Extracting it would require significant component customization for this single use case. Kept as inline to avoid over-engineering.
| // Chart colors matching Meticulous app style (muted to fit dark theme) | ||
| const CHART_COLORS = { | ||
| pressure: '#4ade80', // Green (muted) | ||
| flow: '#67e8f9', // Light cyan/blue (muted) | ||
| weight: '#fbbf24', // Amber/Yellow (muted) | ||
| gravimetricFlow: '#c2855a', // Brown-orange (muted to fit dark theme) | ||
| // Profile target curves (lighter/dashed versions of main colors) | ||
| targetPressure: '#86efac', // Lighter green for target pressure | ||
| targetFlow: '#a5f3fc' // Lighter cyan for target flow | ||
| } | ||
|
|
||
| // Stage colors for background areas (matching tag colors) | ||
| const STAGE_COLORS = [ | ||
| 'rgba(239, 68, 68, 0.25)', // Red | ||
| 'rgba(249, 115, 22, 0.25)', // Orange | ||
| 'rgba(234, 179, 8, 0.25)', // Yellow | ||
| 'rgba(34, 197, 94, 0.25)', // Green | ||
| 'rgba(59, 130, 246, 0.25)', // Blue | ||
| 'rgba(168, 85, 247, 0.25)', // Purple | ||
| 'rgba(236, 72, 153, 0.25)', // Pink | ||
| 'rgba(20, 184, 166, 0.25)', // Teal | ||
| ] | ||
|
|
||
| const STAGE_BORDER_COLORS = [ | ||
| 'rgba(239, 68, 68, 0.5)', | ||
| 'rgba(249, 115, 22, 0.5)', | ||
| 'rgba(234, 179, 8, 0.5)', | ||
| 'rgba(34, 197, 94, 0.5)', | ||
| 'rgba(59, 130, 246, 0.5)', | ||
| 'rgba(168, 85, 247, 0.5)', | ||
| 'rgba(236, 72, 153, 0.5)', | ||
| 'rgba(20, 184, 166, 0.5)', | ||
| ] | ||
|
|
||
| // Darker text colors for stage pills — legible on both light and dark backgrounds | ||
| const STAGE_TEXT_COLORS_LIGHT = [ | ||
| 'rgb(153, 27, 27)', // Red-800 | ||
| 'rgb(154, 52, 18)', // Orange-800 | ||
| 'rgb(133, 77, 14)', // Yellow-800 | ||
| 'rgb(22, 101, 52)', // Green-800 | ||
| 'rgb(30, 64, 175)', // Blue-800 | ||
| 'rgb(107, 33, 168)', // Purple-800 | ||
| 'rgb(157, 23, 77)', // Pink-800 | ||
| 'rgb(17, 94, 89)', // Teal-800 | ||
| ] | ||
| const STAGE_TEXT_COLORS_DARK = [ | ||
| 'rgb(252, 165, 165)', // Red-300 | ||
| 'rgb(253, 186, 116)', // Orange-300 | ||
| 'rgb(253, 224, 71)', // Yellow-300 | ||
| 'rgb(134, 239, 172)', // Green-300 | ||
| 'rgb(147, 197, 253)', // Blue-300 | ||
| 'rgb(216, 180, 254)', // Purple-300 | ||
| 'rgb(249, 168, 212)', // Pink-300 | ||
| 'rgb(94, 234, 212)', // Teal-300 | ||
| ] | ||
|
|
||
| // Comparison chart colors | ||
| const COMPARISON_COLORS = { | ||
| pressure: '#4ade80', | ||
| flow: '#67e8f9', | ||
| weight: '#fbbf24' | ||
| } |
There was a problem hiding this comment.
CHART_COLORS, STAGE_COLORS, STAGE_BORDER_COLORS, and stage text color arrays are duplicated here and in ShotHistoryView (same values, different modules). With charts now split across both files, it’s easy for a future change to update one set and not the other. To avoid divergence, consider moving these constants to a shared chartStyles module (or exporting them from one place) and importing them where needed.
There was a problem hiding this comment.
Fixed in 778e401. All chart color and stage style constants extracted to /apps/web/src/lib/chartStyles.ts and imported by both ShotCharts.tsx and ShotHistoryView.tsx. This ensures a single source of truth for chart styling across the app.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
- Extract chart constants to shared chartStyles module to avoid duplication - Fix ReplayChart to use fragment for desktop to preserve parent spacing - Replace mobile comparison inline chart with CompareChart component - Remove duplicated chart color/stage constants from ShotHistoryView and ShotCharts Co-authored-by: hessius <1499030+hessius@users.noreply.github.com>
Applied all review feedback in commit 778e401: ✅ Fixed:
Note: The small "Shot vs Profile Target" chart in the analysis summary is a unique embedded chart (custom height, positioning) with no duplication elsewhere, so kept as inline to avoid over-engineering. Build verified: TypeScript compilation passes ✅, Vite production build succeeds ✅ |
Note: Analysis summary "Shot vs Profile Target" chart intentionally kept inline as it's a unique one-off embedded chart with custom styling (h-48 height) and no duplication elsewhere.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.