fix(chart): remove unwanted borders and overlays from bar chart#47
Merged
pineapplestrikesback merged 2 commits intomainfrom Jan 24, 2026
Merged
fix(chart): remove unwanted borders and overlays from bar chart#47pineapplestrikesback merged 2 commits intomainfrom
pineapplestrikesback merged 2 commits intomainfrom
Conversation
- Remove browser focus outline on chart elements (yellow border on mobile Safari) - Remove selection stroke border from selected bars - Remove hover cursor overlay (blue hue on bars) Selected bars now indicate selection through color change only (cyan-400 vs cyan-500). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR introduces a comprehensive mobile-first heatmap refactor, adding a centralized color-scale utility with perceptually uniform volume-to-color mapping, three new mobile-specific components (heatmap, muscle list, carousel), device detection and session-state hooks, simplified desktop components, and extensive multi-phase planning documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dashboard
participant useIsMobileDevice
participant MobileCarousel
participant MobileHeatmap
participant useScientificMuscleVolume
participant color-scale
User->>Dashboard: Load app
Dashboard->>useIsMobileDevice: Check device type
useIsMobileDevice-->>Dashboard: isMobile boolean
alt Mobile Device
Dashboard->>MobileCarousel: Render with profileId
MobileCarousel->>MobileHeatmap: Initialize slide 1
MobileHeatmap->>useScientificMuscleVolume: Fetch volume data
useScientificMuscleVolume-->>MobileHeatmap: VolumeStatItem[]
MobileHeatmap->>color-scale: getVolumeColor(percentage)
color-scale-->>MobileHeatmap: oklch color string
MobileHeatmap-->>MobileCarousel: Rendered heatmap with colors
User->>MobileCarousel: Swipe/tap to slide 2
MobileCarousel->>MobileHeatmap: toggleView or navigate
else Desktop Device
Dashboard->>MuscleHeatmap: Render with profileId
MuscleHeatmap->>useScientificMuscleVolume: Fetch volume data
useScientificMuscleVolume-->>MuscleHeatmap: VolumeStatItem[]
MuscleHeatmap->>color-scale: getVolumeColor(percentage)
color-scale-->>MuscleHeatmap: oklch color string
MuscleHeatmap-->>Dashboard: Rendered heatmap
end
sequenceDiagram
participant User
participant MobileHeatmap
participant useSessionState
participant sessionStorage
participant MobileBodyHighlighter
participant color-scale
User->>MobileHeatmap: View loaded
MobileHeatmap->>useSessionState: Initialize view state (front)
useSessionState->>sessionStorage: Read view preference
sessionStorage-->>useSessionState: 'front' | 'back'
useSessionState-->>MobileHeatmap: view state
MobileHeatmap->>MobileBodyHighlighter: Render anterior body
MobileBodyHighlighter->>color-scale: getVolumeColor per region
color-scale-->>MobileBodyHighlighter: oklch colors
MobileBodyHighlighter-->>MobileHeatmap: Rendered body diagram
User->>MobileHeatmap: Tap toggle button
MobileHeatmap->>useSessionState: Update view to 'back'
useSessionState->>sessionStorage: Persist 'back'
MobileHeatmap->>MobileBodyHighlighter: Render posterior body
MobileBodyHighlighter->>color-scale: getVolumeColor per region
color-scale-->>MobileBodyHighlighter: oklch colors
MobileBodyHighlighter-->>MobileHeatmap: Rendered back diagram
User->>User: View persists across navigation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR spans 40+ files with heterogeneous changes: extensive planning documentation (low complexity but high volume), new color system with interpolation logic and ~20 tests (medium complexity), three new mobile components with state management and carousel integration (medium-high complexity), significant refactoring of existing components, taxonomy restructuring, and dependency additions. The diversity of concerns—color utilities, React hooks, mobile-specific UX, component simplification—demands careful review of logic, type safety, and integration points despite some repetitive planning artifacts. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
![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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/components/MuscleHeatmap.tsx (1)
124-130: Zero‑progress and no‑goal states are currently indistinguishable.Both totalGoal=0 and totalGoal>0 with zero volume produce
percentage = 0, sogetFrequencyLevelreturns 0 for both. WithbodyColornow set togetNoTargetColor, regions that have a goal but no progress render the same as no‑goal regions. Consider encoding “no target” separately so 0% progress still maps into the lowest highlighted bucket.✅ Suggested fix
- * `@param` percentage - A percentage value between 0 and 100 + * `@param` percentage - A percentage value between 0 and 100; use -1 when no goal exists @@ - if (percentage === 0) return 0; + if (percentage < 0) return 0; + if (percentage === 0) return 1; @@ - const percentage = totalGoal > 0 ? (totalVolume / totalGoal) * 100 : 0; + const percentage = totalGoal > 0 ? (totalVolume / totalGoal) * 100 : -1;Also applies to: 168-173, 295-299
🤖 Fix all issues with AI agents
In @.planning/codebase/STACK.md:
- Around line 1-160: The PR accidentally includes an unrelated STACK.md instead
of the chart styling changes noted in the review; replace or supplement this
commit with the actual chart fixes by updating the BarChart component and
styles: modify the BarChart (e.g., src/ui/charts/BarChart.tsx or any
BarChart/Bar component) to remove or disable the focus outline/overlay logic
(look for identifiers like BarChart, Bar, handleBarFocus, onBarHover or
useBarHover) and remove the corresponding CSS/Tailwind classes (e.g.,
bar-focus-overlay, bar-hover-overlay, .bar:focus, .bar:hover) from the component
stylesheet or global tailwind rules (src/index.css / tailwind.config.js) so bars
no longer show focus borders or hover overlays; then amend the PR to remove the
unrelated STACK.md changes or move STACK.md to a separate docs-only commit so
the diff contains only the BarChart/style updates.
In @.planning/codebase/TESTING.md:
- Line 224: Replace the bare URL in the markdown with a proper markdown link by
wrapping the link text and URL using the [text](URL) syntax (e.g., replace plain
"https://example.com" with "[Testing guide](https://example.com)"); ensure the
link text is descriptive and update any surrounding sentence punctuation so
linting and readability are satisfied.
In @.planning/phases/01-component-foundation/01-01-SUMMARY.md:
- Around line 1-128: The planning document
.planning/phases/01-component-foundation/01-01-SUMMARY.md describes a mobile
heatmap refactor while the PR objectives claim fixes to bar chart focus
borders/overlays on mobile Safari; reconcile by either removing this planning
file from the PR if it was added accidentally, or updating the PR
summary/objectives to match this file. Specifically: check the git stage/commit
for this file and either (a) revert/unstage/exclude it from the PR if it’s
unrelated to the bar-chart/Safari fixes, or (b) if the PR is meant to include
the mobile heatmap work, update the PR title/description and any CI/planning
metadata to reference useIsMobileDevice, MobileHeatmap, and Dashboard changes so
the PR objectives and .planning summary align; also verify the actual code
changes for bar-chart focus-border fixes (search for bar chart components and
Safari-specific CSS/JS) and include or remove those commits as appropriate.
In @.planning/phases/01-component-foundation/01-RESEARCH.md:
- Around line 23-35: Add language identifiers to the fenced code blocks in
.planning/phases/01-component-foundation/01-RESEARCH.md so markdownlint stops
flagging them; for example, mark the React hook snippet with ```typescript (the
block containing function useIsMobile()) and mark the directory listings with
```text (the blocks showing src/ui/components/ and the mobile/ fragment); update
the other fenced blocks referenced in the comment (the ones around the directory
trees and any additional code fences) to include appropriate languages.
- Around line 220-230: Replace emphasis-only lines used as section titles (e.g.,
"**Option A: Lightweight navigator check**" and "**Option B: react-device-detect
library**") with proper Markdown headings (e.g., "### Option A: Lightweight
navigator check" and "### Option B: react-device-detect library") so
markdownlint stops flagging them; update any nearby emphasis-only title lines in
the same file around the isMobileDevice() snippet to use the heading syntax and
ensure consistent heading levels for the component options.
In @.planning/phases/02-visual-system/02-RESEARCH.md:
- Around line 358-367: The listed resources contain bare URLs that trigger
markdownlint MD034; update each list item in the 02-RESEARCH.md section to use
Markdown link syntax or angle-bracketed URLs—e.g., replace "Tailwind CSS 4
`@theme` docs - https://tailwindcss.com/docs/theme" with a descriptive link like
"Tailwind CSS 4 `@theme` docs - [Tailwind theme
docs](https://tailwindcss.com/docs/theme)" (apply the same change for the Oklab
page, MDN oklch reference, react-body-highlighter GitHub, chroma.js docs,
CSS-Tricks oklch guide, and the Evil Martians OKLCH article), ensuring no bare
URLs remain and link text is descriptive.
- Around line 47-56: The fenced code block containing the project tree is
missing a language identifier which triggers markdownlint; update the fenced
block in the documentation by adding a language specifier (e.g., "text") to the
opening ``` fence for the directory tree so the lint rule is satisfied—locate
the fenced block that shows the src/ → core/ → ui/ tree in the RESEARCH.md file
and change the opening fence to include the language identifier.
In @.planning/phases/03-heatmap-core/03-01-PLAN.md:
- Around line 1-159: The plan file
.planning/phases/03-heatmap-core/03-01-PLAN.md is inconsistent with the PR
objective: the document instructs removing floating cards/leader lines from
MuscleHeatmap (references: LEADER_LINE_OFFSETS, CARD_POSITIONS, MuscleCard,
SplitView, "Show All / Hide All") but the PR description and changes target bar
chart focus/border styling (Safari yellow border, cyan stroke, blue hover
overlay). Update the plan to match the PR objective by replacing the heatmap
refactor tasks with tasks that identify and fix the bar chart focus/hover issues
(mention the affected component/files that render the bar chart and CSS
rules/classes responsible for focus/outline/stroke), remove or mark obsolete
heatmap removal steps (LEADER_LINE_OFFSETS, CARD_POSITIONS, MuscleCard,
SplitView) and add verification steps for cross-browser focus styles (Safari,
Chrome) and visual tests to confirm the yellow border/cyan/blue overlays are
resolved.
In @.planning/phases/03-heatmap-core/03-01-SUMMARY.md:
- Around line 42-45: Replace the emphasized line "**Simplified MuscleHeatmap to
color-only body visualization without floating cards, leader lines, or toggle
buttons**" with a proper Markdown heading (e.g., "### Simplified MuscleHeatmap
to color-only body visualization without floating cards, leader lines, or toggle
buttons"), removing the surrounding ** markers so the section is a heading and
not emphasized text (this resolves MD036).
In @.planning/phases/04-front-back-toggle/04-01-SUMMARY.md:
- Around line 1-107: The planning summary for phase: 04-front-back-toggle/plan:
01 currently documents a 3D flip toggle and useSessionState hook which conflicts
with the PR goal to remove unwanted borders/overlays from a bar chart; update
this document so the title, summary, provides/affects, tags, key-files,
key-decisions, patterns-established and accomplishments describe the bar-chart
cleanup (removing stroke borders, focus outlines, hover overlays), list the
actual modified files (e.g., the bar chart component and related CSS), remove
references to useSessionState and 3D transforms, and ensure metrics/commits
reflect the bar-chart change instead of the flip toggle.
In @.planning/phases/04-front-back-toggle/04-RESEARCH.md:
- Around line 1-474: The file titled "Phase 4: Front/Back Toggle - Research"
contains an unrelated 3D flip/sessionStorage research doc instead of the work
required by the PR (fixing bar chart focus borders: yellow border on mobile
Safari, cyan stroke, blue hover overlay); remove or replace this document so its
content matches the PR objectives (address bar chart focus/hover/focus-visible
styles) and eliminate the stray marker [inconsistent_summary]; ensure the
document references the actual CSS selectors/classes used in the chart (e.g.,
the bar element class or focus-visible rule names) and lists specific fixes to
implement (mobile Safari outline override, correct stroke color rules, hover
overlay z-index/opacity) so the research file aligns with the PR rather than the
flip-toggle topic.
In @.planning/phases/05-muscle-list/05-02-PLAN.md:
- Around line 100-121: The fenced JSX examples in the markdown lack language
identifiers causing MD040; update each fenced code block containing JSX/TSX (the
blocks showing the component markup that references muscle, getVolumeColor,
formatVolume, percentage, groupPercentage, totalVolume) to include a language
tag like "tsx" (e.g., ```tsx) so markdownlint recognizes the code language;
apply this change to the blocks shown and the repeated block around lines
referencing groupPercentage/totalVolume.
In @.planning/phases/05-muscle-list/05-VERIFICATION.md:
- Around line 1-159: The verification file currently validates
MobileMuscleList.tsx (collapsible muscle list) but the PR objectives are about
removing focus borders and hover overlays from a bar chart; replace or rewrite
this phase verification so it targets the correct artifact(s) and truths:
identify and verify the bar chart component(s) (e.g., BarChart,
HorizontalBarSeries, or the component that renders the chart hover/focus
layers), update must_haves to include "no focus outlines on bars", "no hover
overlay layer", "keyboard/accessibility preserved", and link key_links to the
actual chart module(s) (e.g., the BarChart component, any CSS modules or
getBarColor/getVolumeColor helper), and ensure artifacts point to the chart
file(s) with correct min_lines and evidence lines referencing the
functions/classes (BarChart, renderBars, handleMouseEnter/handleFocus, focus
styles) so the verification reflects the PR objectives rather than
MobileMuscleList.
In @.planning/phases/06-carousel-navigation/06-RESEARCH.md:
- Around line 27-61: Add blank lines before and after each Markdown table to
satisfy MD058/MD040 and change the directory tree code fence to declare a
language (e.g., use ```text) so the block passes linting; update the tables
under "### Core" and "### Supporting" and the "Alternatives Considered" section
to have a blank line above and below each table, and replace the triple-backtick
fence around the project structure with ```text and a blank line before and
after that fence (references: file
.planning/phases/06-carousel-navigation/06-RESEARCH.md and symbols
MobileHeatmap.tsx, MobileMuscleList.tsx, MobileCarousel.tsx in the directory
tree).
In @.planning/phases/06-carousel-navigation/06-VERIFICATION.md:
- Line 83: Replace the bare URL in the sentence "Open http://localhost:3000 in
Chrome DevTools mobile emulator (iPhone 12 or similar)" with a markdown link to
satisfy linting and improve readability; for example, change it to use a link
like [http://localhost:3000](http://localhost:3000) or a descriptive label such
as [Localhost:3000](http://localhost:3000) while keeping the rest of the
sentence unchanged.
In `@src/ui/components/mobile/MobileCarousel.tsx`:
- Around line 64-101: Buttons in the carousel tablist are missing type="button"
and ARIA associations; update the tab buttons (the element that calls scrollTo
and reads selectedIndex) to include type="button", an id (e.g.,
`carousel-tab-${index}`), and aria-controls pointing to the corresponding panel
id, and update the slide panels that render MobileHeatmap and MobileMuscleList
to include role="tabpanel", an id matching aria-controls (e.g.,
`carousel-panel-${index}`) and aria-labelledby pointing back to the tab id;
ensure ids are stable per index and that the button's aria-selected reflects
selectedIndex so tabs and panels are fully wired for WAI-ARIA compliance.
In `@src/ui/components/mobile/MobileHeatmap.tsx`:
- Around line 239-243: The toggle button in MobileHeatmap.tsx (the <button>
element with onClick={toggleView} and aria-label={`Show ${view === 'front' ?
'back' : 'front'} view`}) should include type="button" to avoid acting as a
submit button when rendered inside a form; update that button element to add the
type attribute while leaving the existing onClick, className, and aria-label
intact.
In `@src/ui/components/mobile/MobileMuscleList.tsx`:
- Around line 4-6: The doc comment atop the MobileMuscleList component is
inaccurate (says 7 anatomical groups while the UI grouping source uses 6);
update the header comment in MobileMuscleList (the top-level comment above the
MobileMuscleList component and any related grouping constant/array such as the
muscle group array or GROUPS variable) to reflect the actual taxonomy (6 groups)
or adjust the grouping data to match the comment—ensure the number in the
comment matches the length of the grouping array used by the component.
🧹 Nitpick comments (6)
src/ui/components/WeeklyActivityChart.tsx (1)
235-235: Avoid blanket outline removal to preserve focus visibility.
[&_*]:outline-nonestrips outlines for all descendants, which can erase focus-visible styles if any interactive elements appear in this container. Consider scoping the rule to SVG elements (or:focus:not(:focus-visible)) instead.♻️ Suggested scope reduction
- <div className="p-4 [&_*]:outline-none [&_*:focus]:outline-none [&_*:focus-visible]:outline-none [&_svg]:outline-none"> + <div className="p-4 [&_svg]:outline-none [&_rect]:outline-none">.planning/codebase/STRUCTURE.md (1)
7-38: Add a fenced-block language and avoid emphasis-as-heading.This aligns with markdownlint (MD040, MD036) if those checks are enforced.
✏️ Suggested markdown tweak
-``` +```text gym-analytics/ ├── src/ │ ├── core/ # Pure business logic (NO React) @@ -``` +``` -_Structure analysis: 2026-01-18_ +**Structure analysis:** 2026-01-18Also applies to: 256-258
src/index.css (1)
47-51: Duplicate color values for accent-cyan and accent-blue.
--color-accent-cyanand--color-accent-blueboth resolve to#22d3ee. If these are intentionally the same, consider removing one to avoid confusion. If they should be distinct, update--color-accent-blueto a different value.Suggested fix if blue should be distinct
--color-accent-cyan: `#22d3ee`; --color-accent-orange: `#f97316`; --color-accent-red: `#ef4444`; --color-accent-yellow: `#facc15`; - --color-accent-blue: `#22d3ee`; + --color-accent-blue: `#3b82f6`;src/ui/components/mobile/MobileMuscleList.tsx (1)
92-115: Add button type + aria state for safer embedding and accessibility.Explicit
type="button"prevents accidental form submits, andaria-expandedimproves screen reader behavior.♿ Suggested tweak
- <button + <button + type="button" onClick={() => toggleGroup(group.name)} - className="w-full flex items-center gap-3 p-3 bg-primary-800 transition-colors active:bg-primary-700" + aria-expanded={isExpanded} + className="w-full flex items-center gap-3 p-3 bg-primary-800 transition-colors active:bg-primary-700" >src/core/__tests__/color-scale.test.ts (1)
1-2: Use@corealias for color-scale import.Switching to the alias keeps TS/TSX imports consistent and resilient to refactors. As per coding guidelines, use
@core/aliases instead of relative paths.♻️ Proposed change
-import { getVolumeColor, getVolumeOpacity, getNoTargetColor } from '../color-scale'; +import { getVolumeColor, getVolumeOpacity, getNoTargetColor } from '@core/color-scale';src/ui/components/mobile/MobileCarousel.tsx (1)
13-14: Use@uialias for mobile component imports.This keeps imports consistent with the project alias convention. As per coding guidelines, use
@ui/aliases instead of relative paths.♻️ Proposed change
-import { MobileHeatmap } from './MobileHeatmap'; -import { MobileMuscleList } from './MobileMuscleList'; +import { MobileHeatmap } from '@ui/components/mobile/MobileHeatmap'; +import { MobileMuscleList } from '@ui/components/mobile/MobileMuscleList';
Comment on lines
+1
to
+160
| # Technology Stack | ||
|
|
||
| **Analysis Date:** 2026-01-18 | ||
|
|
||
| ## Languages | ||
|
|
||
| **Primary:** | ||
|
|
||
| - TypeScript 5.9.3 - All source code in `src/` | ||
| - Strict mode enabled with `noUncheckedIndexedAccess` | ||
|
|
||
| **Secondary:** | ||
|
|
||
| - JavaScript - Config files (`eslint.config.js`, `tailwind.config.js`, `postcss.config.js`) | ||
|
|
||
| ## Runtime | ||
|
|
||
| **Environment:** | ||
|
|
||
| - Node.js 22.20.0 | ||
| - Browser (PWA with IndexedDB) | ||
|
|
||
| **Package Manager:** | ||
|
|
||
| - npm 10.9.3 | ||
| - Lockfile: `package-lock.json` (present) | ||
|
|
||
| ## Frameworks | ||
|
|
||
| **Core:** | ||
|
|
||
| - React 19.2.3 - UI framework (`src/ui/`) | ||
| - React Router 7.12.0 - Client-side routing (`src/App.tsx`) | ||
| - TanStack Query 5.90.16 - Server state management (`src/db/hooks/`) | ||
| - Dexie.js 4.2.1 - IndexedDB wrapper (`src/db/schema.ts`) | ||
|
|
||
| **Styling:** | ||
|
|
||
| - Tailwind CSS 4.1.18 - Utility-first CSS (`src/index.css`, all components) | ||
| - PostCSS 8.5.6 - CSS processing with `@tailwindcss/postcss` plugin | ||
|
|
||
| **Testing:** | ||
|
|
||
| - Vitest 4.0.16 - Unit testing (`src/core/__tests__/`) | ||
| - Playwright 1.57.0 - E2E testing (`tests/e2e/`) | ||
| - Testing Library React 16.3.1 - React component testing | ||
| - jsdom 27.4.0 - DOM environment for unit tests | ||
|
|
||
| **Build/Dev:** | ||
|
|
||
| - Vite 7.3.1 - Build tool and dev server (`vite.config.ts`) | ||
| - vite-plugin-pwa 1.2.0 - PWA support with Workbox | ||
| - @vitejs/plugin-react 5.1.2 - React Fast Refresh | ||
|
|
||
| ## Key Dependencies | ||
|
|
||
| **Critical:** | ||
|
|
||
| - `dexie` 4.2.1 - All data persistence via IndexedDB | ||
| - `@tanstack/react-query` 5.90.16 - All async state management | ||
| - `react-router-dom` 7.12.0 - All navigation | ||
|
|
||
| **UI Components:** | ||
|
|
||
| - `recharts` 3.6.0 - Charts and data visualization | ||
| - `lucide-react` 0.562.0 - Icon library | ||
| - `react-body-highlighter` 2.0.5 - Anatomy diagrams | ||
|
|
||
| **Infrastructure:** | ||
|
|
||
| - `@vercel/analytics` 1.6.1 - Production analytics | ||
| - `sharp` 0.34.5 - Image processing (dev dependency for PWA icons) | ||
|
|
||
| **Code Quality:** | ||
|
|
||
| - `eslint` 9.39.2 with typescript-eslint 8.52.0 | ||
| - `prettier` 3.7.4 - Code formatting | ||
| - `typescript` 5.9.3 - Type checking | ||
|
|
||
| ## Configuration | ||
|
|
||
| **TypeScript (`tsconfig.json`):** | ||
|
|
||
| - Target: ES2020 | ||
| - Module: ESNext with bundler resolution | ||
| - Strict mode: enabled | ||
| - Path aliases configured: | ||
| - `@/*` -> `src/*` | ||
| - `@core/*` -> `src/core/*` | ||
| - `@db/*` -> `src/db/*` | ||
| - `@ui/*` -> `src/ui/*` | ||
|
|
||
| **Vite (`vite.config.ts`):** | ||
|
|
||
| - Dev server port: 3000 | ||
| - Path aliases mirrored from tsconfig | ||
| - PWA configuration with Workbox caching | ||
| - Runtime caching for Google Fonts | ||
|
|
||
| **ESLint (`eslint.config.js`):** | ||
|
|
||
| - Strict TypeScript rules | ||
| - No explicit `any` allowed | ||
| - React Hooks rules enforced | ||
| - Prettier integration | ||
|
|
||
| **Prettier (`.prettierrc`):** | ||
|
|
||
| ```json | ||
| { | ||
| "semi": true, | ||
| "singleQuote": true, | ||
| "tabWidth": 2, | ||
| "trailingComma": "es5", | ||
| "printWidth": 100 | ||
| } | ||
| ``` | ||
|
|
||
| **Tailwind (`tailwind.config.js`):** | ||
|
|
||
| - Custom `primary` color palette (purple tones) | ||
| - Custom `accent` colors (orange, blue, cyan) | ||
| - Custom `slideDown` animation | ||
|
|
||
| ## Platform Requirements | ||
|
|
||
| **Development:** | ||
|
|
||
| - Node.js 22.x | ||
| - npm 10.x | ||
| - Modern browser with IndexedDB support | ||
|
|
||
| **Production:** | ||
|
|
||
| - Static hosting (Vercel) | ||
| - PWA-capable browser | ||
| - IndexedDB for data persistence (no backend required) | ||
|
|
||
| **Browser Support:** | ||
|
|
||
| - Desktop Chrome (tested via Playwright) | ||
| - Mobile Safari/Chrome (iPhone 12 tested via Playwright) | ||
| - PWA installable on iOS and Android | ||
|
|
||
| ## Build Commands | ||
|
|
||
| ```bash | ||
| npm run dev # Start dev server at http://localhost:3000 | ||
| npm run build # TypeScript check + Vite production build | ||
| npm run preview # Preview production build | ||
| npm run test # Run Vitest unit tests | ||
| npm run test:e2e # Run Playwright E2E tests | ||
| npm run lint # Check ESLint violations | ||
| npm run lint:fix # Auto-fix ESLint violations | ||
| npm run format # Format with Prettier | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| _Stack analysis: 2026-01-18_ |
Contributor
There was a problem hiding this comment.
Critical inconsistency: File content doesn't match PR objectives.
This is a general technology stack documentation file listing frameworks, dependencies, and build configuration. The PR objectives describe removing focus borders and hover overlays from a bar chart, but this file contains no information about charts, focus states, or the specific styling changes mentioned in the PR.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
108-108: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
141-141: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In @.planning/codebase/STACK.md around lines 1 - 160, The PR accidentally
includes an unrelated STACK.md instead of the chart styling changes noted in the
review; replace or supplement this commit with the actual chart fixes by
updating the BarChart component and styles: modify the BarChart (e.g.,
src/ui/charts/BarChart.tsx or any BarChart/Bar component) to remove or disable
the focus outline/overlay logic (look for identifiers like BarChart, Bar,
handleBarFocus, onBarHover or useBarHover) and remove the corresponding
CSS/Tailwind classes (e.g., bar-focus-overlay, bar-hover-overlay, .bar:focus,
.bar:hover) from the component stylesheet or global tailwind rules
(src/index.css / tailwind.config.js) so bars no longer show focus borders or
hover overlays; then amend the PR to remove the unrelated STACK.md changes or
move STACK.md to a separate docs-only commit so the diff contains only the
BarChart/style updates.
| ``` | ||
|
|
||
| **Location:** | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid bare URL in markdown.
Use a markdown link to satisfy linting and improve readability.
Suggested fix
- url: 'http://localhost:3000',
+ url: '[http://localhost:3000](http://localhost:3000)',🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
224-224: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In @.planning/codebase/TESTING.md at line 224, Replace the bare URL in the
markdown with a proper markdown link by wrapping the link text and URL using the
[text](URL) syntax (e.g., replace plain "https://example.com" with "[Testing
guide](https://example.com)"); ensure the link text is descriptive and update
any surrounding sentence punctuation so linting and readability are satisfied.
Comment on lines
+1
to
+128
| --- | ||
| phase: 01-component-foundation | ||
| plan: 01 | ||
| subsystem: ui | ||
| tags: [react, mobile, hooks, device-detection, pwa] | ||
|
|
||
| # Dependency graph | ||
| requires: [] | ||
| provides: | ||
| - useIsMobileDevice hook for device detection | ||
| - MobileHeatmap component shell with shared data hooks | ||
| - Dashboard conditional rendering for mobile/desktop | ||
| affects: [02-mobile-layout, 03-mobile-visualization, mobile-components] | ||
|
|
||
| # Tech tracking | ||
| tech-stack: | ||
| added: [] | ||
| patterns: | ||
| - User-agent based device detection (no viewport width for stability) | ||
| - Mobile components in src/ui/components/mobile/ (ARCH-01) | ||
| - Shared data hooks from @db/hooks (ARCH-02) | ||
|
|
||
| key-files: | ||
| created: | ||
| - src/ui/hooks/useIsMobileDevice.ts | ||
| - src/ui/components/mobile/MobileHeatmap.tsx | ||
| modified: | ||
| - src/ui/pages/Dashboard.tsx | ||
|
|
||
| key-decisions: | ||
| - 'User-agent detection excludes tablets (iPad, Android tablets get desktop view)' | ||
| - 'Mobile components in dedicated directory for clean separation' | ||
| - 'Removed dead toggle buttons from Dashboard (MuscleHeatmap shows both views simultaneously)' | ||
|
|
||
| patterns-established: | ||
| - 'ARCH-01: Mobile components isolated in src/ui/components/mobile/' | ||
| - 'ARCH-02: Mobile components import shared hooks from @db/hooks, no data duplication' | ||
| - 'Device detection via useMemo for stable value across renders' | ||
|
|
||
| # Metrics | ||
| duration: 4min | ||
| completed: 2026-01-18 | ||
| --- | ||
|
|
||
| # Phase 1 Plan 1: Mobile Component Foundation Summary | ||
|
|
||
| **User-agent device detection hook and mobile heatmap shell component with shared data hooks and conditional Dashboard rendering** | ||
|
|
||
| ## Performance | ||
|
|
||
| - **Duration:** 4 min | ||
| - **Started:** 2026-01-18T21:52:03Z | ||
| - **Completed:** 2026-01-18T21:56:23Z | ||
| - **Tasks:** 3 | ||
| - **Files modified:** 3 | ||
|
|
||
| ## Accomplishments | ||
|
|
||
| - Created useIsMobileDevice hook with user-agent based detection (excludes tablets) | ||
| - Created MobileHeatmap component shell that imports shared data hooks from @db/hooks | ||
| - Added conditional rendering to Dashboard for mobile/desktop components | ||
| - Fixed pre-existing build error (removed invalid view prop from MuscleHeatmap) | ||
|
|
||
| ## Task Commits | ||
|
|
||
| Each task was committed atomically: | ||
|
|
||
| 1. **Task 1: Create device detection hook** - `0b67aea` (feat) | ||
| 2. **Task 2: Create mobile heatmap component shell** - `4f53484` (feat) | ||
| 3. **Task 3: Update Dashboard for conditional rendering** - `d9201b2` (feat) | ||
|
|
||
| ## Files Created/Modified | ||
|
|
||
| - `src/ui/hooks/useIsMobileDevice.ts` - Device detection hook using user-agent patterns | ||
| - `src/ui/components/mobile/MobileHeatmap.tsx` - Mobile heatmap shell with loading/error/success states | ||
| - `src/ui/pages/Dashboard.tsx` - Conditional rendering for mobile vs desktop heatmap | ||
|
|
||
| ## Decisions Made | ||
|
|
||
| - **Device detection approach:** User-agent based (not viewport width) for stability - no layout shift during resize | ||
| - **Tablet handling:** Tablets get desktop view for better screen real estate utilization | ||
| - **Dead code removal:** Removed non-functional toggle buttons (MuscleHeatmap shows both front/back simultaneously) | ||
|
|
||
| ## Deviations from Plan | ||
|
|
||
| ### Auto-fixed Issues | ||
|
|
||
| **1. [Rule 1 - Bug] Removed invalid view prop from MuscleHeatmap usage** | ||
|
|
||
| - **Found during:** Task 3 (Dashboard conditional rendering) | ||
| - **Issue:** Original Dashboard passed `view={bodyView}` to MuscleHeatmap but that prop doesn't exist. Build was failing before this plan started. | ||
| - **Fix:** Removed the invalid prop. MuscleHeatmap shows both front/back views simultaneously. | ||
| - **Files modified:** src/ui/pages/Dashboard.tsx | ||
| - **Verification:** Build succeeds, MuscleHeatmap renders correctly | ||
| - **Committed in:** d9201b2 (Task 3 commit) | ||
|
|
||
| **2. [Rule 1 - Bug] Removed dead toggle buttons** | ||
|
|
||
| - **Found during:** Task 3 (Dashboard conditional rendering) | ||
| - **Issue:** Front/Back toggle buttons were setting bodyView state that was never used (MuscleHeatmap ignores view prop, shows both views) | ||
| - **Fix:** Removed toggle buttons and unused bodyView/setBodyView state | ||
| - **Files modified:** src/ui/pages/Dashboard.tsx | ||
| - **Verification:** Dashboard renders correctly, no dead code | ||
| - **Committed in:** d9201b2 (Task 3 commit) | ||
|
|
||
| --- | ||
|
|
||
| **Total deviations:** 2 auto-fixed (both Rule 1 - Bug fixes) | ||
| **Impact on plan:** Both fixes necessary to make build pass. The original codebase had a pre-existing TypeScript error. | ||
|
|
||
| ## Issues Encountered | ||
|
|
||
| - Pre-existing build failure due to invalid view prop - fixed as part of Task 3 | ||
|
|
||
| ## User Setup Required | ||
|
|
||
| None - no external service configuration required. | ||
|
|
||
| ## Next Phase Readiness | ||
|
|
||
| - Mobile component architecture established (ARCH-01, ARCH-02) | ||
| - Phase 2 can build mobile layout structure | ||
| - Phase 3 can add actual visualization to MobileHeatmap | ||
|
|
||
| --- | ||
|
|
||
| _Phase: 01-component-foundation_ | ||
| _Completed: 2026-01-18_ |
Contributor
There was a problem hiding this comment.
Critical inconsistency: File content doesn't match PR objectives.
This planning document describes a mobile heatmap refactor (device detection hooks, MobileHeatmap component, Dashboard conditional rendering), but the PR objectives state this PR fixes bar chart focus borders and overlays on mobile Safari. There is no mention of bar charts, focus states, or Safari-specific styling issues in this file.
This suggests either:
- These planning files were included in the PR by mistake
- The PR summary/objectives are incorrect
- The AI summary picked up context from a different PR
Please verify that the correct files are included in this PR.
🧰 Tools
🪛 LanguageTool
[grammar] ~59-~59: Use a hyphen to join words.
Context: ...d useIsMobileDevice hook with user-agent based detection (excludes tablets) - Cre...
(QB_NEW_EN_HYPHEN)
[style] ~92-~92: For improved clarity, try using the conjunction “and” instead of a slash.
Context: ...lid prop. MuscleHeatmap shows both front/back views simultaneously. - **Files mod...
(QB_NEW_EN_SLASH_TO_AND)
🪛 markdownlint-cli2 (0.18.1)
47-47: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In @.planning/phases/01-component-foundation/01-01-SUMMARY.md around lines 1 -
128, The planning document
.planning/phases/01-component-foundation/01-01-SUMMARY.md describes a mobile
heatmap refactor while the PR objectives claim fixes to bar chart focus
borders/overlays on mobile Safari; reconcile by either removing this planning
file from the PR if it was added accidentally, or updating the PR
summary/objectives to match this file. Specifically: check the git stage/commit
for this file and either (a) revert/unstage/exclude it from the PR if it’s
unrelated to the bar-chart/Safari fixes, or (b) if the PR is meant to include
the mobile heatmap work, update the PR title/description and any CI/planning
metadata to reference useIsMobileDevice, MobileHeatmap, and Dashboard changes so
the PR objectives and .planning summary align; also verify the actual code
changes for bar-chart focus-border fixes (search for bar chart components and
Safari-specific CSS/JS) and include or remove those commits as appropriate.
Comment on lines
+23
to
+35
| ```typescript | ||
| // Current approach - viewport-based detection | ||
| function useIsMobile(): boolean { | ||
| const [isMobile, setIsMobile] = useState(true); | ||
| useEffect(() => { | ||
| const checkMobile = () => setIsMobile(window.innerWidth < 768); | ||
| checkMobile(); | ||
| window.addEventListener('resize', checkMobile); | ||
| return () => window.removeEventListener('resize', checkMobile); | ||
| }, []); | ||
| return isMobile; | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
markdownlint flags these fences; adding a language improves linting and readability.
Suggested fix
-```
+```typescript
// Current approach - viewport-based detection
function useIsMobile(): boolean {
const [isMobile, setIsMobile] = useState(true);
useEffect(() => {
const checkMobile = () => setIsMobile(window.innerWidth < 768);
checkMobile();
window.addEventListener('resize', checkMobile);
return () => window.removeEventListener('resize', checkMobile);
}, []);
return isMobile;
}
-```
+```
-```
+```text
src/ui/components/
MuscleHeatmap.tsx # 661 lines - monolithic
MuscleVolumeCard.tsx # Small card component
TotalVolumeCard.tsx # Volume summary
WeeklyActivityChart.tsx # Chart component
anatomy/ # Custom SVG components (unused)
BodyHighlighter.tsx
MaleAnatomySVG.tsx
FemaleAnatomySVG.tsx
index.ts # Barrel export
exercise-mapping/ # Exercise mapping UI
-```
+```
-```
+```text
src/ui/components/
mobile/
MobileHeatmap.tsx # New mobile-specific heatmap
index.ts # Barrel export (optional)
MuscleHeatmap.tsx # Becomes desktop-only (or renamed)
-```
+```Also applies to: 148-152, 166-173
🤖 Prompt for AI Agents
In @.planning/phases/01-component-foundation/01-RESEARCH.md around lines 23 -
35, Add language identifiers to the fenced code blocks in
.planning/phases/01-component-foundation/01-RESEARCH.md so markdownlint stops
flagging them; for example, mark the React hook snippet with ```typescript (the
block containing function useIsMobile()) and mark the directory listings with
```text (the blocks showing src/ui/components/ and the mobile/ fragment); update
the other fenced blocks referenced in the comment (the ones around the directory
trees and any additional code fences) to include appropriate languages.
Comment on lines
+220
to
+230
| **Option A: Lightweight navigator check** | ||
|
|
||
| ```typescript | ||
| // No library needed - native API | ||
| function isMobileDevice(): boolean { | ||
| return /Android|webOS|iPhone|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent); | ||
| // Note: Deliberately excludes iPad for tablet -> desktop | ||
| } | ||
| ``` | ||
|
|
||
| **Option B: react-device-detect library** |
Contributor
There was a problem hiding this comment.
Use headings instead of emphasis-only lines.
markdownlint flags emphasis used as headings; convert to ### headings for structure.
Suggested fix
-**Two options (Claude's discretion):**
+### Two options (Claude's discretion)
-**Recommendation:** Use native `navigator.userAgent` check. Avoids adding dependency for a simple check, and gives explicit control over tablet behavior.
+### Recommendation
+Use native `navigator.userAgent` check. Avoids adding dependency for a simple check, and gives explicit control over tablet behavior.🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
220-220: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
230-230: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In @.planning/phases/01-component-foundation/01-RESEARCH.md around lines 220 -
230, Replace emphasis-only lines used as section titles (e.g., "**Option A:
Lightweight navigator check**" and "**Option B: react-device-detect library**")
with proper Markdown headings (e.g., "### Option A: Lightweight navigator check"
and "### Option B: react-device-detect library") so markdownlint stops flagging
them; update any nearby emphasis-only title lines in the same file around the
isMobileDevice() snippet to use the heading syntax and ensure consistent heading
levels for the component options.
Comment on lines
+27
to
+61
| ### Core | ||
| | Library | Version | Purpose | Why Standard | | ||
| |---------|---------|---------|--------------| | ||
| | embla-carousel-react | 8.6.0 | Swipe carousel | Industry standard, 800K weekly downloads, ~7KB gzipped | | ||
| | React | 19 | Component framework | Project standard | | ||
| | Tailwind CSS | 4 | Styling | Project standard | | ||
|
|
||
| ### Supporting | ||
| | Library | Version | Purpose | When to Use | | ||
| |---------|---------|---------|-------------| | ||
| | @db/hooks | local | Volume data | useScientificMuscleVolume for both slides | | ||
| | @core/taxonomy | local | Muscle groupings | Passed to MobileMuscleList | | ||
|
|
||
| ### Alternatives Considered | ||
| | Instead of | Could Use | Tradeoff | | ||
| |------------|-----------|----------| | ||
| | embla-carousel-react | Custom touch handler | More control but high risk - iOS Safari quirks, physics tuning | | ||
| | embla-carousel-react | react-swipeable | Lower-level - still need to implement snap/momentum | | ||
| | embla-carousel-react | swiper.js | Larger bundle (~45KB vs ~7KB), more features than needed | | ||
| | CSS scroll-snap | embla | Native but inconsistent cross-browser, no JS control for indicators | | ||
|
|
||
| **Installation:** | ||
| ```bash | ||
| npm install embla-carousel-react | ||
| ``` | ||
|
|
||
| ## Architecture Patterns | ||
|
|
||
| ### Recommended Project Structure | ||
| ``` | ||
| src/ui/components/mobile/ | ||
| ├── MobileHeatmap.tsx # Existing - slide 1 content | ||
| ├── MobileMuscleList.tsx # Existing - slide 2 content | ||
| └── MobileCarousel.tsx # NEW - carousel container with navigation | ||
| ``` |
Contributor
There was a problem hiding this comment.
Address markdownlint MD058/MD040 around tables and code fences.
Tables should be surrounded by blank lines, and the directory tree fence should specify a language (e.g., text) to satisfy markdownlint.
📝 Example fix for the directory tree fence
-```
+```text
src/ui/components/mobile/
├── MobileHeatmap.tsx # Existing - slide 1 content
├── MobileMuscleList.tsx # Existing - slide 2 content
└── MobileCarousel.tsx # NEW - carousel container with navigation</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
### Core
| Library | Version | Purpose | Why Standard |
|---------|---------|---------|--------------|
| embla-carousel-react | 8.6.0 | Swipe carousel | Industry standard, 800K weekly downloads, ~7KB gzipped |
| React | 19 | Component framework | Project standard |
| Tailwind CSS | 4 | Styling | Project standard |
### Supporting
| Library | Version | Purpose | When to Use |
|---------|---------|---------|-------------|
| `@db/hooks` | local | Volume data | useScientificMuscleVolume for both slides |
| `@core/taxonomy` | local | Muscle groupings | Passed to MobileMuscleList |
### Alternatives Considered
| Instead of | Could Use | Tradeoff |
|------------|-----------|----------|
| embla-carousel-react | Custom touch handler | More control but high risk - iOS Safari quirks, physics tuning |
| embla-carousel-react | react-swipeable | Lower-level - still need to implement snap/momentum |
| embla-carousel-react | swiper.js | Larger bundle (~45KB vs ~7KB), more features than needed |
| CSS scroll-snap | embla | Native but inconsistent cross-browser, no JS control for indicators |
**Installation:**
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
28-28: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
35-35: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
41-41: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @.planning/phases/06-carousel-navigation/06-RESEARCH.md around lines 27 - 61,
Add blank lines before and after each Markdown table to satisfy MD058/MD040 and
change the directory tree code fence to declare a language (e.g., use ```text)
so the block passes linting; update the tables under "### Core" and "###
Supporting" and the "Alternatives Considered" section to have a blank line above
and below each table, and replace the triple-backtick fence around the project
structure with ```text and a blank line before and after that fence (references:
file .planning/phases/06-carousel-navigation/06-RESEARCH.md and symbols
MobileHeatmap.tsx, MobileMuscleList.tsx, MobileCarousel.tsx in the directory
tree).
|
|
||
| **Test:** | ||
| 1. Start dev server: `npm run dev` | ||
| 2. Open http://localhost:3000 in Chrome DevTools mobile emulator (iPhone 12 or similar) |
Contributor
There was a problem hiding this comment.
Avoid bare URL in markdown.
Use a markdown link to satisfy linting and improve readability.
Suggested fix
-**Test:**
-1. Start dev server: `npm run dev`
-2. Open http://localhost:3000 in Chrome DevTools mobile emulator (iPhone 12 or similar)
+**Test:**
+1. Start dev server: `npm run dev`
+2. Open [http://localhost:3000](http://localhost:3000) in Chrome DevTools mobile emulator (iPhone 12 or similar)🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
83-83: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In @.planning/phases/06-carousel-navigation/06-VERIFICATION.md at line 83,
Replace the bare URL in the sentence "Open http://localhost:3000 in Chrome
DevTools mobile emulator (iPhone 12 or similar)" with a markdown link to satisfy
linting and improve readability; for example, change it to use a link like
[http://localhost:3000](http://localhost:3000) or a descriptive label such as
[Localhost:3000](http://localhost:3000) while keeping the rest of the sentence
unchanged.
Comment on lines
+64
to
+101
| return ( | ||
| <div> | ||
| {/* Carousel viewport */} | ||
| <div className="overflow-hidden" ref={emblaRef}> | ||
| <div className="flex touch-pan-y"> | ||
| {/* Slide 1: Body Heatmap (NAV-03: default view) */} | ||
| <div className="flex-[0_0_100%] min-w-0 px-4"> | ||
| <MobileHeatmap profileId={profileId} daysBack={daysBack} /> | ||
| </div> | ||
|
|
||
| {/* Slide 2: Muscle List */} | ||
| <div className="flex-[0_0_100%] min-w-0 px-4"> | ||
| <MobileMuscleList profileId={profileId} daysBack={daysBack} /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Dot indicators (NAV-02) */} | ||
| <div | ||
| className="flex justify-center gap-2 py-4" | ||
| role="tablist" | ||
| aria-label="Carousel navigation" | ||
| > | ||
| {[0, 1].map((index) => ( | ||
| <button | ||
| key={index} | ||
| onClick={() => scrollTo(index)} | ||
| className={` | ||
| h-2 rounded-full transition-all duration-200 | ||
| ${index === selectedIndex | ||
| ? 'w-4 bg-amber-500' | ||
| : 'w-2 bg-primary-600 active:bg-primary-500' | ||
| } | ||
| `} | ||
| role="tab" | ||
| aria-selected={index === selectedIndex} | ||
| aria-label={index === 0 ? 'Body heatmap view' : 'Muscle list view'} | ||
| /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WAI-ARIA tablist tab tabpanel role required attributes aria-controls aria-labelledby
💡 Result:
Required roles / relationships (Tabs pattern)
- Tab container:
role="tablist"must contain/own one or more elements withrole="tab". [3] - Each tab:
role="tab"(owned by thetablist). [3] - Each panel:
role="tabpanel"for the content associated with a tab. [1]
Required labeling / associations (commonly required for an accessible implementation)
-
tablistneeds an accessible name:
Use eitheraria-labelledby="…"(when there’s a visible label) oraria-label="…"(when there isn’t). [1] -
Tab → Panel association:
Eachrole="tab"hasaria-controls="ID_OF_ITS_TABPANEL". [1] -
Panel → Tab association:
Eachrole="tabpanel"hasaria-labelledby="ID_OF_ITS_TAB". [1] -
Selected state:
The active tab hasaria-selected="true"; all other tabs havearia-selected="false". [1]
Common (optional/conditional) attributes
aria-orientation="vertical"on thetablistwhen vertical (default is horizontal). [1]aria-multiselectable="true"on thetablistif multiple panels can be shown/selected. [2]- If multiselectable, use
aria-expandedon tabs to reflect whether their panels are shown. [3] aria-haspopup="menu"(or"true") on a tab that opens a popup menu. [1]
Sources: WAI-ARIA Authoring Practices (Tabs pattern) [1], MDN tablist role reference [2], WAI-ARIA spec (tablist/tabpanel guidance) [3].
Add type="button" and wire tabs to panels with aria-controls/aria-labelledby for ARIA compliance.
The carousel uses tablist/tab roles but is missing required ARIA associations. Buttons need explicit type="button" to prevent form submission behavior. Each tab requires aria-controls pointing to its panel id, and each panel needs role="tabpanel" with aria-labelledby pointing back to its tab id—these are mandatory attributes per WAI-ARIA Authoring Practices.
Suggested fix
- <div className="flex-[0_0_100%] min-w-0 px-4">
+ <div
+ className="flex-[0_0_100%] min-w-0 px-4"
+ role="tabpanel"
+ id="mobile-carousel-panel-0"
+ aria-labelledby="mobile-carousel-tab-0"
+ >
<MobileHeatmap profileId={profileId} daysBack={daysBack} />
</div>
{/* Slide 2: Muscle List */}
- <div className="flex-[0_0_100%] min-w-0 px-4">
+ <div
+ className="flex-[0_0_100%] min-w-0 px-4"
+ role="tabpanel"
+ id="mobile-carousel-panel-1"
+ aria-labelledby="mobile-carousel-tab-1"
+ >
<MobileMuscleList profileId={profileId} daysBack={daysBack} />
</div>
</div>
</div>
@@
- <button
+ <button
key={index}
+ type="button"
+ id={`mobile-carousel-tab-${index}`}
+ aria-controls={`mobile-carousel-panel-${index}`}
onClick={() => scrollTo(index)}
className={`
h-2 rounded-full transition-all duration-200
${index === selectedIndex📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return ( | |
| <div> | |
| {/* Carousel viewport */} | |
| <div className="overflow-hidden" ref={emblaRef}> | |
| <div className="flex touch-pan-y"> | |
| {/* Slide 1: Body Heatmap (NAV-03: default view) */} | |
| <div className="flex-[0_0_100%] min-w-0 px-4"> | |
| <MobileHeatmap profileId={profileId} daysBack={daysBack} /> | |
| </div> | |
| {/* Slide 2: Muscle List */} | |
| <div className="flex-[0_0_100%] min-w-0 px-4"> | |
| <MobileMuscleList profileId={profileId} daysBack={daysBack} /> | |
| </div> | |
| </div> | |
| </div> | |
| {/* Dot indicators (NAV-02) */} | |
| <div | |
| className="flex justify-center gap-2 py-4" | |
| role="tablist" | |
| aria-label="Carousel navigation" | |
| > | |
| {[0, 1].map((index) => ( | |
| <button | |
| key={index} | |
| onClick={() => scrollTo(index)} | |
| className={` | |
| h-2 rounded-full transition-all duration-200 | |
| ${index === selectedIndex | |
| ? 'w-4 bg-amber-500' | |
| : 'w-2 bg-primary-600 active:bg-primary-500' | |
| } | |
| `} | |
| role="tab" | |
| aria-selected={index === selectedIndex} | |
| aria-label={index === 0 ? 'Body heatmap view' : 'Muscle list view'} | |
| /> | |
| return ( | |
| <div> | |
| {/* Carousel viewport */} | |
| <div className="overflow-hidden" ref={emblaRef}> | |
| <div className="flex touch-pan-y"> | |
| {/* Slide 1: Body Heatmap (NAV-03: default view) */} | |
| <div | |
| className="flex-[0_0_100%] min-w-0 px-4" | |
| role="tabpanel" | |
| id="mobile-carousel-panel-0" | |
| aria-labelledby="mobile-carousel-tab-0" | |
| > | |
| <MobileHeatmap profileId={profileId} daysBack={daysBack} /> | |
| </div> | |
| {/* Slide 2: Muscle List */} | |
| <div | |
| className="flex-[0_0_100%] min-w-0 px-4" | |
| role="tabpanel" | |
| id="mobile-carousel-panel-1" | |
| aria-labelledby="mobile-carousel-tab-1" | |
| > | |
| <MobileMuscleList profileId={profileId} daysBack={daysBack} /> | |
| </div> | |
| </div> | |
| </div> | |
| {/* Dot indicators (NAV-02) */} | |
| <div | |
| className="flex justify-center gap-2 py-4" | |
| role="tablist" | |
| aria-label="Carousel navigation" | |
| > | |
| {[0, 1].map((index) => ( | |
| <button | |
| key={index} | |
| type="button" | |
| id={`mobile-carousel-tab-${index}`} | |
| aria-controls={`mobile-carousel-panel-${index}`} | |
| onClick={() => scrollTo(index)} | |
| className={` | |
| h-2 rounded-full transition-all duration-200 | |
| ${index === selectedIndex | |
| ? 'w-4 bg-amber-500' | |
| : 'w-2 bg-primary-600 active:bg-primary-500' | |
| } | |
| `} | |
| role="tab" | |
| aria-selected={index === selectedIndex} | |
| aria-label={index === 0 ? 'Body heatmap view' : 'Muscle list view'} | |
| /> | |
| ))} | |
| </div> | |
| </div> | |
| ); |
🤖 Prompt for AI Agents
In `@src/ui/components/mobile/MobileCarousel.tsx` around lines 64 - 101, Buttons
in the carousel tablist are missing type="button" and ARIA associations; update
the tab buttons (the element that calls scrollTo and reads selectedIndex) to
include type="button", an id (e.g., `carousel-tab-${index}`), and aria-controls
pointing to the corresponding panel id, and update the slide panels that render
MobileHeatmap and MobileMuscleList to include role="tabpanel", an id matching
aria-controls (e.g., `carousel-panel-${index}`) and aria-labelledby pointing
back to the tab id; ensure ids are stable per index and that the button's
aria-selected reflects selectedIndex so tabs and panels are fully wired for
WAI-ARIA compliance.
Comment on lines
+239
to
+243
| {/* Subtle Toggle Button - TOGGLE-02: visually quiet */} | ||
| <button | ||
| onClick={toggleView} | ||
| className="absolute bottom-2 left-1/2 -translate-x-1/2 min-w-[44px] min-h-[44px] px-3 py-1.5 rounded-full text-xs text-primary-400 bg-primary-800/40 backdrop-blur-sm border border-primary-700/20 transition-colors duration-150 active:bg-primary-700/50" | ||
| aria-label={`Show ${view === 'front' ? 'back' : 'front'} view`} |
Contributor
There was a problem hiding this comment.
Add type="button" to the toggle control.
Prevents accidental form submission if this component is ever rendered inside a form.
🔧 Suggested tweak
- <button
+ <button type="button"
onClick={toggleView}
className="absolute bottom-2 left-1/2 -translate-x-1/2 min-w-[44px] min-h-[44px] px-3 py-1.5 rounded-full text-xs text-primary-400 bg-primary-800/40 backdrop-blur-sm border border-primary-700/20 transition-colors duration-150 active:bg-primary-700/50"
aria-label={`Show ${view === 'front' ? 'back' : 'front'} view`}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {/* Subtle Toggle Button - TOGGLE-02: visually quiet */} | |
| <button | |
| onClick={toggleView} | |
| className="absolute bottom-2 left-1/2 -translate-x-1/2 min-w-[44px] min-h-[44px] px-3 py-1.5 rounded-full text-xs text-primary-400 bg-primary-800/40 backdrop-blur-sm border border-primary-700/20 transition-colors duration-150 active:bg-primary-700/50" | |
| aria-label={`Show ${view === 'front' ? 'back' : 'front'} view`} | |
| {/* Subtle Toggle Button - TOGGLE-02: visually quiet */} | |
| <button type="button" | |
| onClick={toggleView} | |
| className="absolute bottom-2 left-1/2 -translate-x-1/2 min-w-[44px] min-h-[44px] px-3 py-1.5 rounded-full text-xs text-primary-400 bg-primary-800/40 backdrop-blur-sm border border-primary-700/20 transition-colors duration-150 active:bg-primary-700/50" | |
| aria-label={`Show ${view === 'front' ? 'back' : 'front'} view`} |
🤖 Prompt for AI Agents
In `@src/ui/components/mobile/MobileHeatmap.tsx` around lines 239 - 243, The
toggle button in MobileHeatmap.tsx (the <button> element with
onClick={toggleView} and aria-label={`Show ${view === 'front' ? 'back' :
'front'} view`}) should include type="button" to avoid acting as a submit button
when rendered inside a form; update that button element to add the type
attribute while leaving the existing onClick, className, and aria-label intact.
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
Selected bars now indicate selection through color change only (cyan-400 vs cyan-500).
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.