fix(geometry): stop emitting NaN when start and end coincide - #210
Merged
Conversation
Closes #139, closes #171, closes #192. atan(absDy / absDx) is NaN when both deltas are zero, and the NaN then propagates into every coordinate, so the path renders as M 16 16 C 16 16, NaN NaN, NaN NaN and the head and tail transforms go with it. Both deltas are zero whenever start and end sit at the same point, which includes the very common case of elements not yet measured on the first render. That is why #192 sees it with dynamically mounted arrows. Uses atan2(absDy, absDx). Verified identical to atan(absDy / absDx) across all 160k integer pairs from 0 to 400, differing only at (0, 0) where it returns 0 instead of NaN, so a zero-length arrow renders flat. Note that the fix proposed in #139, absDx === 0 ? Math.atan(absDy) : ..., is wrong. Division by zero yields Infinity, not NaN, and atan(Infinity) is already the correct PI/2 for a vertical arrow. Math.atan(absDy) returns 0.785 for absDy = 1 and only approaches PI/2 as absDy grows, so it would have introduced a subtly wrong angle for short vertical arrows. Unskips the waiting test and widens it to sweep curveness, path and head/tail combinations, since the straight-path branch is only one way in. Confirmed measured arrows are untouched: rendered d and transform are byte identical before and after for diagonal, pure horizontal, pure vertical and reverse diagonal geometry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
✅ Deploy Preview for react-xarrows ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change replaces straight-path angle division with ChangesNaN handling and issue reproductions
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
One story per reported bug, reproducing the condition from the report, so the fixes stay verifiable by eye and by assertion rather than only by unit test. Four carry a live verdict that reads PASS or FAIL on screen. Each was checked both ways, by reverting the fix and confirming it flips: #139, #171, #192 FAIL with Math.atan restored, PASS with atan2 #193 FAIL before the animation fix, showing opacity:repeatCount=0 The first attempt at these was not a reproduction at all. The boxes sat at distinct measured positions, so both deltas were never zero and every story passed even with the bug present. Re-reading #139 settled it: the screenshot shows the boxes overlaying one another. Coincident anchors are the trigger, not a zero absDx, which divides to Infinity and gives a correct PI/2. The stories now stack the elements, which reproduces exactly what #192 reported: d = M 16 16 C 16 16, NaN NaN, NaN NaN transform = translate(NaN,NaN) rotate(NaN) scale(24) #105 and #106 are visual only, on purpose. Their symptoms depend on a SMIL endEvent, and a hidden document does not tick SMIL reliably, so an automated verdict would report PASS whether or not the bug was present. #106 in particular still read PASS against the pre-fix code, which is why its verdict was removed rather than kept as false assurance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes #139, closes #171, closes #192.
The bug
Math.atan(absDy / absDx)isNaNwhen both deltas are zero, and that NaN propagates into every downstream coordinate:The head and tail
transformattributes go with it, which is what #192 shows.Both deltas are zero whenever start and end sit at the same point. Critically, that includes elements not yet measured on the first render, which is why #192 hits it with dynamically mounted arrows rather than with genuinely overlapping boxes.
The fix
Math.atan2(absDy, absDx).Verified identical to
Math.atan(absDy / absDx)across all 160k integer pairs from 0 to 400, max difference exactly0, differing only at(0, 0)where it returns0instead ofNaN. A zero-length arrow renders flat rather than disappearing.The fix suggested in #139 is wrong
The reporter proposed:
Dividing by zero yields
Infinity, notNaN, andMath.atan(Infinity)is already the correctPI/2for a vertical arrow, so the branch is not needed. Worse,Math.atan(absDy)is wrong:absDyMath.atan(absDy)PI/2)It only approaches the right answer as
absDygrows, so merging it would have introduced a subtly wrong angle for short vertical arrows. It "seemed to work" in the reporter test because their arrow was long.Tests
Unskips the test already sitting in the suite waiting for this, and widens it into a sweep over curveness x path x head/tail, since the straight-path branch is only one way in and jsdom reports zero-sized rects for everything. 45 combinations, previously 1.
No change to measured arrows
The equivalence above is a proof over the input domain, but confirmed empirically too. With
getBoundingClientRectstubbed to real geometry, the rendereddandtransformare byte identical before and after for diagonal, pure horizontal, pure vertical and reverse diagonal cases. Pure vertical matters most, since it is the case that exercisesatan(dy / 0).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
NaNcoordinates for arrows with coincident endpoints.Tests