fix: map SVG pointer input through rendered transform - #44
Conversation
tannerlinsley
added
the
full-conformance
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change adds a token-usage calendar conformance case, responsive interaction handling, and documentation. It corrects SVG pointer coordinate conversion for transformed viewports and updates conformance, catalog, bundle, comparison, and friction records. ChangesCalendar conformance and interactions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConformanceInput
participant CalendarModel
participant CalendarRenderer
participant CalendarShell
participant Tooltip
ConformanceInput->>CalendarRenderer: provide revision and chart input
CalendarRenderer->>CalendarModel: generate UTC calendar data
CalendarModel-->>CalendarRenderer: return daily usage levels
CalendarRenderer->>CalendarShell: mount responsive calendar
CalendarShell->>Tooltip: expose focused date and tooltip state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/charts-core/src/svg-surface.ts (1)
115-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
try/catcharoundmatrix.inverse().
DOMMatrixReadOnly.inverse()does not throw for a non-invertible matrix; it returns a matrix withNaNcomponents. The existingNumber.isFinite(x) || Number.isFinite(y)guard already returnsnullin that case.♻️ Suggested simplification
- let inverse: DOMMatrix - try { - inverse = matrix.inverse() - } catch { - return null - } - + const inverse = matrix.inverse() const x = inverse.a * clientX + inverse.c * clientY + inverse.e const y = inverse.b * clientX + inverse.d * clientY + inverse.f if (!Number.isFinite(x) || !Number.isFinite(y)) return null🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/charts-core/src/svg-surface.ts` around lines 115 - 124, Remove the try/catch surrounding matrix.inverse() in the coordinate conversion flow, assign the inverse directly, and retain the existing Number.isFinite checks on x and y to return null when the inverse contains NaN values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/conformance/cases/118-token-usage-calendar/shell.ts`:
- Around line 157-179: Update tanstackInteractionState to identify the hovered
day by its dateKey from target.anchor or chart markup rather than matching
tooltip text with formatTokenUsage and days.find. Preserve tooltip visibility
and text extraction, but pass the resolved dateKey to interactionState so
similarly formatted token counts cannot select the wrong day.
- Around line 143-155: Update calendarCells to narrow the Plot fallback query to
the legend swatch rects created by Plot’s ordinal color scale, rather than all
svg rect elements. Select and return the expected legend rect collection when it
matches expectedCount, while preserving the TanStack selector and empty-array
fallback.
In `@benchmarks/conformance/cases/34-pointer-tooltip/tanstack.ts`:
- Around line 101-105: Update resolveTarget’s getScreenCTM handling so a null
matrix uses the existing bounding-rectangle projection to resolve the pointer
target instead of returning null; preserve the current matrix-based projection
when a matrix is available.
---
Nitpick comments:
In `@packages/charts-core/src/svg-surface.ts`:
- Around line 115-124: Remove the try/catch surrounding matrix.inverse() in the
coordinate conversion flow, assign the inverse directly, and retain the existing
Number.isFinite checks on x and y to return null when the inverse contains NaN
values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00acd27d-da1a-4f60-84e1-608f0793d77d
📒 Files selected for processing (27)
.changeset/accurate-svg-pointers.mdAPI-FRICTION.mdbenchmarks/bundle-size/universal-baseline.jsonbenchmarks/comparison/bundle-baseline.jsonbenchmarks/conformance/cases/118-token-usage-calendar/case.jsonbenchmarks/conformance/cases/118-token-usage-calendar/layout.tsbenchmarks/conformance/cases/118-token-usage-calendar/model.test.tsbenchmarks/conformance/cases/118-token-usage-calendar/model.tsbenchmarks/conformance/cases/118-token-usage-calendar/plot.tsbenchmarks/conformance/cases/118-token-usage-calendar/shell.test.tsbenchmarks/conformance/cases/118-token-usage-calendar/shell.tsbenchmarks/conformance/cases/118-token-usage-calendar/tanstack.tsbenchmarks/conformance/cases/25-calendar-heatmap/case.jsonbenchmarks/conformance/cases/34-pointer-tooltip/case.jsonbenchmarks/conformance/cases/34-pointer-tooltip/plot.tsbenchmarks/conformance/cases/34-pointer-tooltip/tanstack.tsbenchmarks/conformance/types.tsdocs/examples/heatmaps-and-densities.mddocs/framework/octane/quick-start.mdpackages/charts-core/docs/examples/heatmaps-and-densities.mdpackages/charts-core/docs/framework/octane/quick-start.mdpackages/charts-core/src/svg-surface.test.tspackages/charts-core/src/svg-surface.tsscripts/catalog-artifact.mjsscripts/catalog-definition-shapes.test.mjsscripts/compare-plot-catalog.mjsscripts/measure-bundles.mjs
2 participants
Supersedes #27 on current main while preserving Kyle Gill's implementation commit.\n\n- map SVG client coordinates through the inverse screen CTM, with the existing bounds fallback when no matrix is available\n- retain the token-usage calendar example and reconcile its catalog identity, counts, docs, and bundle baselines\n- add a real Chromium regression that letterboxes the calendar viewport and asserts the exact Aug 3 cell/tooltip\n\nNegative control: reverting the runtime fix makes the regression resolve Aug 5 or Aug 6 at 320/640px across both revisions.\n\nValidated with pnpm validate, React Native type checks, focused Chromium conformance, unit tests, bundle policy, and comparison benchmark baseline.
Summary by CodeRabbit
Bug Fixes
New Examples
Documentation
Tests