refactor(dashboard): true visual graduation markers on decay curve#61
refactor(dashboard): true visual graduation markers on decay curve#61
Conversation
…n fallback - Move data-graduation-marker hook onto an SVG <g> rendered via the ReferenceLine label prop. Markers now live on the actual chart, not a sibling hidden div. - Add role='img' + aria-label + SVG <title> for native hover tooltip showing the graduated rule's description (no extra JS needed). - Add invisible hit-target <rect> so hovering near the dashed line triggers the SVG title tooltip. - Drop the separate <div aria-hidden><span data-graduation-marker> fallback block now that the hook lives on the real marker. - Mock Recharts ResponsiveContainer in tests/setup.ts so jsdom actually renders chart SVG (fixed-800x400). Required for in-chart test hooks going forward. - Add test: marker renders at correct x position for a lesson graduated 5 days ago in the 7d range (left third of plot area). - Add test: marker carries aria-label + SVG <title> with rule description. - Fix dashboard-page.test.tsx false collision on 'Never use em dashes' (now also appears in the marker <title>, so use getAllByText). All 137 dashboard tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oliver Le <oliver@gradata.ai>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gradata has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughSummary
WalkthroughThis PR refactors the graduation marker rendering in the CorrectionDecayCurve component by replacing hidden span elements with accessible SVG labels attached to ReferenceLine elements. The new labels include native tooltips, ARIA attributes, hit targets, and test data attributes. Related tests and test infrastructure are updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Deploying gradata-dashboard with
|
| Latest commit: |
a4ab152
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://97c0e6a3.gradata-dashboard.pages.dev |
| Branch Preview URL: | https://refactor-decay-curve-clean-m.gradata-dashboard.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cloud/dashboard/tests/setup.ts`:
- Around line 13-21: The ResponsiveContainer mock currently only accepts
children and drops any other props; update the mock signature for
ResponsiveContainer to accept and forward all props (e.g., ({ children, ...props
})) and spread those props into both the outer div and the call to
actual.ResponsiveContainer (React.createElement(actual.ResponsiveContainer, {
width: 800, height: 400, ...props }, children)) so additional props are
preserved while still keeping the fixed size used in the test.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af7aba85-a9dc-4fb6-9235-11100320f498
📒 Files selected for processing (4)
cloud/dashboard/src/components/brain/CorrectionDecayCurve.tsxcloud/dashboard/tests/CorrectionDecayCurve.test.tsxcloud/dashboard/tests/dashboard-page.test.tsxcloud/dashboard/tests/setup.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
cloud/dashboard/src/components/brain/CorrectionDecayCurve.tsx (1)
108-150: SVG marker label implementation looks solid.Attaching the hook and accessibility metadata directly to the rendered
ReferenceLinelabel is a good improvement, and the native<title>+ hit-target approach matches the intended behavior.cloud/dashboard/tests/CorrectionDecayCurve.test.tsx (1)
45-74: Great coverage additions for marker geometry and accessibility text.These tests validate the new marker contract (
data-x,aria-label, and SVG<title>) and directly protect the refactor.cloud/dashboard/tests/dashboard-page.test.tsx (1)
81-84: Good matcher adjustment for multi-surface text rendering.Switching to
getAllByTexthere correctly handles duplicated lesson text from chart SVG tooltip/title content.
| ResponsiveContainer: ({ children }: { children: React.ReactNode }) => | ||
| React.createElement( | ||
| 'div', | ||
| { style: { width: 800, height: 400 } }, | ||
| React.createElement( | ||
| actual.ResponsiveContainer, | ||
| { width: 800, height: 400 }, | ||
| children, | ||
| ), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any ResponsiveContainer call sites rely on props beyond width/height.
rg -nP --type=tsx -C2 '<ResponsiveContainer\b'Repository: Gradata/gradata
Length of output: 86
🏁 Script executed:
# First, verify the file exists and check its structure
ls -la cloud/dashboard/tests/setup.tsRepository: Gradata/gradata
Length of output: 137
🏁 Script executed:
# Search for ResponsiveContainer usages without the tsx type flag
rg -nP -C2 '<ResponsiveContainer\b' --max-count=20Repository: Gradata/gradata
Length of output: 785
🏁 Script executed:
# Also examine the actual code at the specified lines in setup.ts
head -30 cloud/dashboard/tests/setup.ts | cat -nRepository: Gradata/gradata
Length of output: 1100
🏁 Script executed:
# Search more broadly for all ResponsiveContainer usages to see what props are passed
rg 'ResponsiveContainer' --max-count=30 -A3Repository: Gradata/gradata
Length of output: 2613
🏁 Script executed:
# Check the recharts type definitions or documentation to understand what props ResponsiveContainer accepts
# First, check if there's a node_modules or package.json
ls cloud/dashboard/package.json 2>/dev/null || ls package.json 2>/dev/nullRepository: Gradata/gradata
Length of output: 87
🏁 Script executed:
# Look for recharts imports to understand the library version
rg 'from ["\']recharts' --max-count=10Repository: Gradata/gradata
Length of output: 127
🏁 Script executed:
# Check recharts version in package.json
cat cloud/dashboard/package.json | grep -A2 -B2 rechartsRepository: Gradata/gradata
Length of output: 192
🏁 Script executed:
# Search for any other ResponsiveContainer prop patterns or attributes
rg 'ResponsiveContainer' -B1 -A5 cloud/dashboard/srcRepository: Gradata/gradata
Length of output: 2473
Consider forwarding extra props through the ResponsiveContainer mock for better completeness.
The mock currently accepts only children and drops all other props passed to ResponsiveContainer. While the single visible usage only passes width and height (which the mock overrides anyway), forwarding additional props makes the mock more robust and future-proof in case other props are needed.
The proposed fix is sound:
♻️ Proposed fix
- ResponsiveContainer: ({ children }: { children: React.ReactNode }) =>
+ ResponsiveContainer: (
+ { children, ...props }: React.ComponentProps<typeof actual.ResponsiveContainer>,
+ ) =>
React.createElement(
'div',
{ style: { width: 800, height: 400 } },
React.createElement(
actual.ResponsiveContainer,
- { width: 800, height: 400 },
+ { ...props, width: 800, height: 400 },
children,
),
),📝 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.
| ResponsiveContainer: ({ children }: { children: React.ReactNode }) => | |
| React.createElement( | |
| 'div', | |
| { style: { width: 800, height: 400 } }, | |
| React.createElement( | |
| actual.ResponsiveContainer, | |
| { width: 800, height: 400 }, | |
| children, | |
| ), | |
| ResponsiveContainer: ( | |
| { children, ...props }: React.ComponentProps<typeof actual.ResponsiveContainer>, | |
| ) => | |
| React.createElement( | |
| 'div', | |
| { style: { width: 800, height: 400 } }, | |
| React.createElement( | |
| actual.ResponsiveContainer, | |
| { ...props, width: 800, height: 400 }, | |
| children, | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cloud/dashboard/tests/setup.ts` around lines 13 - 21, The ResponsiveContainer
mock currently only accepts children and drops any other props; update the mock
signature for ResponsiveContainer to accept and forward all props (e.g., ({
children, ...props })) and spread those props into both the outer div and the
call to actual.ResponsiveContainer
(React.createElement(actual.ResponsiveContainer, { width: 800, height: 400,
...props }, children)) so additional props are preserved while still keeping the
fixed size used in the test.
1 participant
Summary
data-graduation-markertest hook onto the actual<ReferenceLine>via a custom SVGlabelrenderer (<g role='img' aria-label=... ><title>...</title></g>), and drop the redundant hidden-span fallback.<title>tooltip on hover showing the graduated rule's description (no extra JS/tooltip library).ResponsiveContainermock intests/setup.tsso jsdom actually renders Recharts SVG (required for on-chart test hooks).Changes
cloud/dashboard/src/components/brain/CorrectionDecayCurve.tsx— custom label onReferenceLine, invisible hit-target<rect>for hover, hidden-span block removed.buildDecayCurveuntouched. 12-marker cap + "+N more" caption preserved (kept — still useful when markers cluster).cloud/dashboard/tests/CorrectionDecayCurve.test.tsx— 2 new tests: x-position correctness for a 5-day-old graduation in a 7d range, and aria-label/SVG<title>accessibility.cloud/dashboard/tests/setup.ts— mockResponsiveContainerwith fixed 800x400.cloud/dashboard/tests/dashboard-page.test.tsx— switch one matcher togetAllByText(marker<title>now legitimately surfaces the same description).Test plan
npm run test:run— 137/137 pass