Add angular focus for radial charts - #90
Conversation
|
View your CI Pipeline Execution ↗ for commit 948a884
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughPolar marks now carry focus geometry. The new ChangesPolar focus interaction
Pointer activation stability
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟡 Moderate · up to The PR adds angular focus and radial-arc hit testing, but its stress-validation helper can begin timing too early when the tooltip is already inactive, producing unreliable benchmark or interaction measurements. Merge should wait for that bounded validation issue to be fixed or explicitly accepted; the documented tie-breaker also needs a minor correction. Sequence Diagram(s)sequenceDiagram
participant Pointer
participant focusGroupAngle
participant PolarMarks
participant Tooltip
Pointer->>focusGroupAngle: provide pointer position
focusGroupAngle->>PolarMarks: inspect polar focus geometry
PolarMarks-->>focusGroupAngle: return candidate points
focusGroupAngle-->>Tooltip: return angularly grouped points
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
API-FRICTION.md (1)
7972-7975: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the documented tie-breaker.
focusGroupAngledoes not use radius distance for the primary-point tie-breaker. Inpackages/charts-core/src/polar-focus-internal.ts:49-119,resolvecompares Euclidean pointer-to-point distance after the ray-distance comparison. Update F-271 to document the actual behavior.Proposed wording
- selects the nearest bounded radial ray, uses radius distance as the primary-point - tie-breaker, groups one point per series at the same semantic angle, and + selects the nearest bounded radial ray, uses Euclidean pointer-to-point distance + as the tie-breaker, groups one point per series at the same semantic angle, and🤖 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 `@API-FRICTION.md` around lines 7972 - 7975, Update the F-271 decision text describing focusGroupAngle so its primary-point tie-breaker is documented as Euclidean pointer-to-point distance after ray-distance comparison, replacing the incorrect radius-distance description. Preserve the remaining behavior and export-path wording.
🤖 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 `@scripts/stress-chart-libraries.mjs`:
- Around line 894-900: Update the inactive-frame loop in
__stressPointerWaitInactive so it awaits one animation frame before checking
__stressPointerActive, ensuring two completed consecutive inactive frames are
observed even when initially inactive. Preserve the existing 120-frame timeout
and reset behavior when activity is detected.
---
Outside diff comments:
In `@API-FRICTION.md`:
- Around line 7972-7975: Update the F-271 decision text describing
focusGroupAngle so its primary-point tie-breaker is documented as Euclidean
pointer-to-point distance after ray-distance comparison, replacing the incorrect
radius-distance description. Preserve the remaining behavior and export-path
wording.
🪄 Autofix
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: 373277f5-cdff-4911-832e-fcab096159f4
📒 Files selected for processing (3)
API-FRICTION.mdbenchmarks/comparison/stress/README.mdscripts/stress-chart-libraries.mjs
| let inactiveFrames = 0 | ||
| for (let frame = 0; frame < 120; frame++) { | ||
| if (!globalThis.__stressPointerActive()) return true | ||
| if (globalThis.__stressPointerActive()) { | ||
| inactiveFrames = 0 | ||
| } else if (++inactiveFrames >= 2) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wait for two completed inactive frames.
If __stressPointerWaitInactive starts while the tooltip is already inactive, Lines 894-900 count the entry-time state as frame one and return after only one requestAnimationFrame cycle. This does not enforce the documented two consecutive inactive animation frames and can start activation timing too early. Await the frame before checking the state, while keeping the 120-frame timeout.
Proposed fix
globalThis.__stressPointerWaitInactive = async () => {
let inactiveFrames = 0
for (let frame = 0; frame < 120; frame++) {
+ await nextFrame()
if (globalThis.__stressPointerActive()) {
inactiveFrames = 0
} else if (++inactiveFrames >= 2) {
return true
}
- await nextFrame()
}
throw new Error('Pointer tooltip did not return to an inactive state.')
}📝 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.
| let inactiveFrames = 0 | |
| for (let frame = 0; frame < 120; frame++) { | |
| if (!globalThis.__stressPointerActive()) return true | |
| if (globalThis.__stressPointerActive()) { | |
| inactiveFrames = 0 | |
| } else if (++inactiveFrames >= 2) { | |
| return true | |
| } | |
| globalThis.__stressPointerWaitInactive = async () => { | |
| let inactiveFrames = 0 | |
| for (let frame = 0; frame < 120; frame++) { | |
| await nextFrame() | |
| if (globalThis.__stressPointerActive()) { | |
| inactiveFrames = 0 | |
| } else if (++inactiveFrames >= 2) { | |
| return true | |
| } | |
| } | |
| throw new Error('Pointer tooltip did not return to an inactive state.') | |
| } |
🤖 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 `@scripts/stress-chart-libraries.mjs` around lines 894 - 900, Update the
inactive-frame loop in __stressPointerWaitInactive so it awaits one animation
frame before checking __stressPointerActive, ensuring two completed consecutive
inactive frames are observed even when initially inactive. Preserve the existing
120-frame timeout and reset behavior when activity is detected.
1 participant
Summary
focusGroupAngleas the polar equivalent ofgroup-xradialArcfocus against painted arc geometry, including donut holesValidation
pnpm validateRelease
Minor release for
@tanstack/charts; the fixed package set advances to0.12.0.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests