feat(ui): D.5a migrate Results header + 4-card metric strip - #71
Conversation
slittycode
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
Pure UI layer component substitution — <h1> → <SectionHeader>, four <AccentMetricCard> → four <MetricTile size="lg"> inside a <DeviceRack>, two <button> → <Button variant>. No logic changed. Tests pass (645/649, 4 skipped), lint clean, build stable at the sizes the PR description quotes (AnalysisResults chunk 255 KB / 63 KB gz). All data-testid hooks preserved.
Findings
Worth considering (not blocking):
MetricTile renders its value prop inside a <span className="... tabular-nums ...">. For the CHARACTER genre string that's harmless, but tabular-nums on a word is semantically odd. Not a bug — just something to be aware of if that value ever needs proportional spacing.
The CHARACTER "SCANNING..." fallback passes <span className="text-base ..."> as the value prop, which correctly overrides the parent text-2xl from size="lg". That works, but it's a fragile coupling: the sizing inside MetricTile is being cancelled by a class on the child rather than a supported prop. Again not blocking — the visual result is identical to the old code — but worth knowing for when this tile gets standardised in D.5b+.
Test results
645 passed / 4 skipped (0 failures). No new non-trivial logic was introduced; the existing data-testid selectors in smoke tests cover the preserved hooks.
Phase boundary check
Clean. AnalysisResults.tsx reads phase1.* values identically to before — phase1.bpmConfidence, phase1.keyConfidence, phase1.genreDetail.confidence etc. are passed through to child components as props. No assignment, no re-derivation.
Generated by Claude Code
PR 6 of 16 in the UI consistency overhaul. Promotes the AnalysisResults page header to <SectionHeader> and wraps the top TEMPO/KEY/METER/ CHARACTER metric strip in a <DeviceRack name="Measurement Summary"> with <MetricTile> cards inside. Stacked on PR #69 (D.4 FileUpload). Will rebase onto main after #69 merges. Header (AnalysisResults.tsx:835-880 → :835-878): - <h1> + Download buttons row → <SectionHeader size="lg" variant="inline" eyebrow="ASA Results" titleRole="page-title"> with both Download buttons in the action slot. - The Activity icon prefix on the h1 disappears — the SectionHeader LED dot already carries the "results are live" semantic. - The subtitle (sourceFileName · interpretationSubtitle) stays as a separate <p> sibling, preserving data-testid="analysis-results- subtitle" and the existing text-role-meta styling. Indented pl-4 to align under the title after the LED. - Download data: <button> → <Button variant="secondary" leadingIcon=FileJson>. Preserved data-testid="analysis-export-json". - Download report: <button> → <Button variant="primary" leadingIcon=FileText>. Preserved data-testid="analysis-export-markdown". Metric strip (AnalysisResults.tsx:884-1012 → :882-1010): - Outer <div className="grid grid-cols-2 md:grid-cols-4 gap-3"> wrapped in <DeviceRack name="Measurement Summary" density="dense" status="success">. The grid layout stays as-is inside the rack body so the responsive 2-col / 4-col break still works. - 4 × AccentMetricCard → 4 × <MetricTile size="lg" accent="accent">. API parity drop-in: • label (was a flex-span with icon+text) → icon prop + label string. Cleaner separation; the MetricTile primitive places the icon in the eyebrow row. • value / unit / headerRight / footer pass through unchanged. • The hand-rolled text-[1.5rem] override on the CHARACTER value disappears — MetricTile size="lg" already renders text-2xl which is the same 1.5rem. Imports: - AccentMetricCard stays imported from ./MeasurementPrimitives. Other usages of it remain at :1249-1258 and :1444-1448 (Patches / Sonic Elements sections — migrated in later PRs D.5b/c/d). - Added Button / DeviceRack / MetricTile / SectionHeader from ./ui. Preserved verbatim: - All data-testid attributes (analysis-results-root, analysis-results- subtitle, analysis-export-json, analysis-export-markdown). - All visible text strings: "Analysis Results", "Download data", "Download report", "TEMPO", "BPM", "KEY SIG", "METER", "CHARACTER", "SCANNING...". - The PhaseSourceBadge + ConfidenceBandBadge + MetricBar + StatusBadge + TokenBadgeList rendering inside each tile is untouched. - Audit Finding #4 commentary preserved (canonical band-pill vocabulary). - Audit Finding #1 ordering comment preserved (MeasurementDashboard rendering at the bottom of the scroll). - The lowConfidenceIndicator + characteristicPills logic untouched. Verified: lint clean, 628/628 unit tests passing, build clean (AnalysisResults chunk 255 KB / 63 KB gz; index chunk 323 KB / 97 KB gz — both stable vs PR 5). https://claude.ai/code/session_01WNtYcWXwn4mVnnjxqT7uAe
d049e34 to
2accb2b
Compare
PR 7 of 16 in the UI consistency overhaul. Mix & Master Chain groups (DRUM PROCESSING, BASS PROCESSING, HIGH-END DETAIL, MASTER BUS) now each render as a <DeviceRack> with the group emoji + name on the title strip. Branched off main (PRs #66/#67/#69/#71 squash-merged). Single-file PR — AnalysisResults.tsx. Migration: - Per-group <section> + <h3> heading → <DeviceRack name={emoji + group.name}>. The DeviceRack title strip displays the emoji and uppercase group name; the DOM text content is preserved exactly so analysisResultsUi.test.ts:441-450 (toContain('🥁 DRUM PROCESSING') etc.) continue to pass. - group.annotation <p> kept inside the rack body with data-text-role="meta", margin-bottom restored. - Cards grid div retains its `grid gap-4 grid-cols-1 sm:grid-cols-2` className verbatim — analysisResultsUi.test.ts:440 expects ≥2 occurrences of this exact string (Mix Chain + Patches). - mix-chain-applied-progress badge: hand-rolled span → <Pill tone="success" size="sm">. Preserved data-testid="mix-chain- applied-progress". The Pill emits the canonical `bg-success/20 text-success border-success/30` class string the test at :507 already asserts is present in the rendered HTML. - SourcesToggle <button> → <Button variant="link">. Used by both Mix Chain and other sections; small ergonomic upgrade. Deferred (out of D.5b scope): - Per-card <div> outer wrapper NOT migrated to <Panel variant= "surface">. The card carries rich state-dependent styling (bg-bg-card with hover transitions, conditional border-l-2 for applied state, overflow-hidden for collapsible) that would require either extensive className overrides on Panel or extending Panel with new variants. Per-card migration deferred to a focused follow-up if/when Panel needs the additional variants. - AppliedCheckbox NOT migrated to the Checkbox primitive. The existing implementation is already a <button role="checkbox"> with aria-checked + aria-label + data-testid + green-tinted applied state. Migrating to Radix Checkbox would change the styling palette (success-green → accent-orange) and require Checkbox primitive extensions (tone prop). Existing implementation has equivalent a11y semantics and passes the applied-checkbox smoke test. Preserved verbatim: - All data-testid attributes (mix-chain-applied-progress, applied-checkbox, mix-chain-citation-*, mix-chain-headline-*). - All visible text strings (group names + emojis, "applied", "Sources", PRO TIP, etc.). - The exact `grid gap-4 grid-cols-1 sm:grid-cols-2` className the brittle assertion locks. - All data-text-role attributes on h2/h3/h4/p elements (section- title, item-title, meta, body). - The characteristicPillClass function which produces the `bg-{tone}/20 text-{tone} border-{tone}/30` strings asserted at :507-509 — untouched. - CitationBlock + CitationHeadline internals — untouched. - AppliedCheckbox tracker + audioContentHash gating — untouched. Verified: lint clean, 628/628 unit tests passing (including analysisResultsUi.test.ts which directly asserts the preserved class strings + text + data-text-role attributes), build clean (AnalysisResults chunk 254 KB / 63 KB gz — slightly smaller than PR 6 due to removed hand-rolled span styling). https://claude.ai/code/session_01WNtYcWXwn4mVnnjxqT7uAe Co-authored-by: Claude <noreply@anthropic.com>
tests/smoke/responsive-layout.spec.ts:192 fails at the 375px mobile
viewport with `expect(getByText('Analysis Results')).toBeVisible()`
resolving to a hidden h2. Root cause: my D.5a migration replaced the
original `flex flex-col md:flex-row` layout for the Analysis Results
page header with the SectionHeader primitive which used
`flex items-center justify-between` unconditionally. At 375px the
two Download buttons (~280px combined) squeeze the title to zero
width and the parent `.ableton-shell overflow-hidden` clips the
title off-screen, so Playwright sees it as hidden.
Restore the responsive collapse: mobile-first
`flex flex-col items-start gap-2`, then `md:flex-row md:items-center
md:justify-between md:gap-3` at the 768px+ breakpoint. Matches the
pre-D.5a Analysis Results header behavior. Other consumers of
SectionHeader (the many ResultsSectionHeader call sites) have small
rightSlot content (badges, single pills, icons) — stacking those
below the title on mobile is a tiny visual improvement, not a
regression.
This is the failure that's been blocking #79, #80, #81, #82 — none
of those PRs introduced the bug; they all inherit it from main
because D.5a (#71) landed this regression. The fact that #71 itself
merged green is either CI luck (the test passed on its first run)
or the test was added later and not re-checked.
Landing this hotfix lets the four open PRs rebase and pick up the
fix to clear their CI.
Verified: lint clean, 628/628 unit tests passing, build clean.
https://claude.ai/code/session_01WNtYcWXwn4mVnnjxqT7uAe
Co-authored-by: Claude <noreply@anthropic.com>
PR 14 of 16 in the UI consistency overhaul. Final cleanup pass. Removes three CSS utility classes from index.css: - .ableton-panel - .ableton-device - .ableton-header All three were retired by D.* migrations — every former consumer now uses the canonical primitives from components/ui/ (Panel, DeviceRack, SectionHeader). `grep -rn` against apps/ui/src/ confirms zero remaining usages. .ableton-shell and .ableton-toolbar STAY because App.tsx:952,956 still consumes them for the outer app chrome (the load-bearing section-grid shell). A focused follow-up could migrate those two to DeviceRack / a future AppShell primitive; out of scope here. Deferred (preserved as-is): - components/MeasurementPrimitives.tsx cannot be fully deleted yet — 5 consumers still depend on timeline/lane primitives (LaneContainer, LaneRow, StatsBar, TimeRuler) that the plan explicitly preserved. The shipped primitives (StatusBadge, DeltaBadge, MetricBar, MetricBarRow, OutlinePillButton, TokenBadgeList, StyledDataTable, AccentMetricCard) also still have consumers in MeasurementDashboard and AnalysisResults sections that weren't migrated (StructureLanes, HarmonyLanes, AnalysisResults remaining sections). Keep the file intact. - components/Tooltip.tsx stays as a re-export shim; the file isn't causing any drift and external imports may still target it. Verified: lint clean, 628/628 unit tests passing, build clean. D.* migrations summary (all merged or open at time of this PR): - D.0 Foundation (#64) ✓ - D.1 Primitives complete (#65) ✓ - D.2 SamplePlayback (#66) ✓ - D.3 SignalChain marquee (#67) ✓ - D.4 FileUpload + Input Source (#69) ✓ - D.5a Results header + metric strip (#71) ✓ - D.5b Mix Chain (#73) ✓ - D.5c Patches (#74) ✓ - D.5d Section headers (#75) ✓ - D.6a Dashboard structure (#76) ✓ - D.6b Dashboard sections (#77) ✓ - D.6c Dashboard headers (#78) ✓ - D.7 SessionMusicianPanel + MixDoctorPanel (#79) — open - D.8 Signal Monitor + SpectrogramViewer drift (#80) — open - D.9 DenseDawConcept drift (#81) — open - D.10 Cleanup (this PR) ✓ https://claude.ai/code/session_01WNtYcWXwn4mVnnjxqT7uAe
…82) PR 14 of 16 in the UI consistency overhaul. Final cleanup pass. Removes three CSS utility classes from index.css: - .ableton-panel - .ableton-device - .ableton-header All three were retired by D.* migrations — every former consumer now uses the canonical primitives from components/ui/ (Panel, DeviceRack, SectionHeader). `grep -rn` against apps/ui/src/ confirms zero remaining usages. .ableton-shell and .ableton-toolbar STAY because App.tsx:952,956 still consumes them for the outer app chrome (the load-bearing section-grid shell). A focused follow-up could migrate those two to DeviceRack / a future AppShell primitive; out of scope here. Deferred (preserved as-is): - components/MeasurementPrimitives.tsx cannot be fully deleted yet — 5 consumers still depend on timeline/lane primitives (LaneContainer, LaneRow, StatsBar, TimeRuler) that the plan explicitly preserved. The shipped primitives (StatusBadge, DeltaBadge, MetricBar, MetricBarRow, OutlinePillButton, TokenBadgeList, StyledDataTable, AccentMetricCard) also still have consumers in MeasurementDashboard and AnalysisResults sections that weren't migrated (StructureLanes, HarmonyLanes, AnalysisResults remaining sections). Keep the file intact. - components/Tooltip.tsx stays as a re-export shim; the file isn't causing any drift and external imports may still target it. Verified: lint clean, 628/628 unit tests passing, build clean. D.* migrations summary (all merged or open at time of this PR): - D.0 Foundation (#64) ✓ - D.1 Primitives complete (#65) ✓ - D.2 SamplePlayback (#66) ✓ - D.3 SignalChain marquee (#67) ✓ - D.4 FileUpload + Input Source (#69) ✓ - D.5a Results header + metric strip (#71) ✓ - D.5b Mix Chain (#73) ✓ - D.5c Patches (#74) ✓ - D.5d Section headers (#75) ✓ - D.6a Dashboard structure (#76) ✓ - D.6b Dashboard sections (#77) ✓ - D.6c Dashboard headers (#78) ✓ - D.7 SessionMusicianPanel + MixDoctorPanel (#79) — open - D.8 Signal Monitor + SpectrogramViewer drift (#80) — open - D.9 DenseDawConcept drift (#81) — open - D.10 Cleanup (this PR) ✓ https://claude.ai/code/session_01WNtYcWXwn4mVnnjxqT7uAe Co-authored-by: Claude <noreply@anthropic.com>
Summary
PR 6 of 16 in the UI consistency overhaul / Live 12 restyle stack. Promotes the
AnalysisResultspage header to<SectionHeader>and wraps the top TEMPO/KEY/METER/CHARACTER metric strip in a<DeviceRack name="Measurement Summary">with<MetricTile>cards inside.Stacked on #69 (D.4 FileUpload). Base will be retargeted to
mainafter #69 merges.Header (
AnalysisResults.tsx:835-880→:835-878)<h1>+ Download buttons row →<SectionHeader size="lg" variant="inline" eyebrow="ASA Results" titleRole="page-title">with both Download buttons in theactionslot.sourceFileName · interpretationSubtitle) stays as a separate<p>sibling, preservingdata-testid="analysis-results-subtitle"and the existingtext-role-metastyling. Indentedpl-4to align under the title after the LED.<button>→<Button variant="secondary" leadingIcon={FileJson}>. Preserveddata-testid="analysis-export-json".<button>→<Button variant="primary" leadingIcon={FileText}>. Preserveddata-testid="analysis-export-markdown".Metric strip (
AnalysisResults.tsx:884-1012→:882-1010)<div className="grid grid-cols-2 md:grid-cols-4 gap-3">wrapped in<DeviceRack name="Measurement Summary" density="dense" status="success">. The grid layout stays as-is inside the rack body so the responsive 2-col / 4-col break still works.AccentMetricCard→ 4 ×<MetricTile size="lg" accent="accent">. API parity drop-in:label(was a flex-span with icon+text) →iconprop +labelstring. Cleaner separation; the MetricTile primitive places the icon in the eyebrow row.value/unit/headerRight/footerpass through unchanged.text-[1.5rem]override on the CHARACTER value disappears —MetricTile size="lg"already renderstext-2xlwhich is the same 1.5rem.Imports
AccentMetricCardstays imported from./MeasurementPrimitives. Other usages of it remain at:1249-1258and:1444-1448(Patches / Sonic Elements sections — migrated in later PRs D.5b/c/d).Button/DeviceRack/MetricTile/SectionHeaderfrom./ui.Preserved verbatim
data-testidattributes:analysis-results-root,analysis-results-subtitle,analysis-export-json,analysis-export-markdown.PhaseSourceBadge+ConfidenceBandBadge+MetricBar+StatusBadge+TokenBadgeListrendering inside each tile is untouched.lowConfidenceIndicator+characteristicPillslogic untouched.Test plan
npm run lint— cleannpm run test:unit— 628/628 passingnpm run build— clean (AnalysisResults chunk 255 KB / 63 KB gz; index chunk 323 KB / 97 KB gz — stable vs PR 5)npm run test:smoke— sandbox network blocks Playwright browser; smoke selectors target text +data-testidwhich are all preserved (Download data,Download report,analysis-results-subtitle, etc.)Up next (PR 7)
D.5b Mix Chain. Each group →
<DeviceRack>. Cards →<Panel variant="surface">.AppliedCheckbox(AnalysisResults.tsx:189-220) uses<Checkbox>internally. Preservedata-testid="mix-chain-applied-progress"and CitationBlock/CitationHeadline internals verbatim. UpdateanalysisResultsUi.test.tsbrittle assertions per plan §F.5.https://claude.ai/code/session_01WNtYcWXwn4mVnnjxqT7uAe
Generated by Claude Code