QOL: Arrow bending and termination#3723
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refines the CSE Machine's layout and arrow routing logic, including updates to configuration constants and improved path generation for arrows to prevent corner collapse. It also introduces a parentFrame reference for text components to ensure arrows extend beyond frame boundaries before bending and mocks directory fetch requests in the test setup. Feedback identifies an accidentally committed temporary file and trailing whitespace that should be removed.
There was a problem hiding this comment.
Code Review
This pull request enhances the CSE Machine's visual layout and arrow routing logic. Key changes include updating layout constants in CseMachineConfig, refactoring ArrowFromText to handle frame boundaries and Manhattan segments, and improving corner radius calculations in GenericArrow. Additionally, it fixes a typo in ArrayValue, updates test snapshots, and mocks fetch in the test setup. Review feedback suggests removing a mistakenly included temporary file and refining the routing logic comments in ArrowFromText to be more accurate.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| this._x = frameX + newReference.frame.width() + Config.FrameMarginX; | ||
|
|
||
| this._y = newReference.y(); | ||
| this._y = newReference.y() + Config.ArrayVerticalOffset; |
There was a problem hiding this comment.
Bug: Adding ArrayVerticalOffset to ArrayValue without updating the parent Binding's height calculation causes layout issues, as the binding's height is now too small to contain the array.
Severity: MEDIUM
Suggested Fix
Update the height calculation for bindings in Binding.tsx. When the value is an ArrayValue, the height calculation must also include the ArrayVerticalOffset (and any other y-offsets) to ensure the binding is tall enough to fully contain the positioned array. The new height should be approximately max(key.height(), keyYOffset + ArrayVerticalOffset + value.totalHeight).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/features/cseMachine/components/values/ArrayValue.tsx#L53
Potential issue: A new vertical offset, `Config.ArrayVerticalOffset`, is applied to
`ArrayValue` components at `ArrayValue.tsx:53`. However, the height calculation for the
parent `Binding` was not updated to account for this new offset. The binding's height is
calculated based on the array's `totalHeight`, but it fails to include the added
vertical offset. This results in the binding being shorter than its contents, causing
subsequent bindings in the same frame to be drawn too high and overlap with the array.
This visual bug affects all layouts containing array bindings, making frames appear too
short to contain their content.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
@RogerZhang888 please check if it's valid. I'll merge the PR first so we can test. Feel free to make a new PR to fix this if it turns out to be valid.
…nto pr/RogerZhang888/3723
4 participants
Description
[This is an updated PR that only includes changes to the arrows' apperance. Arrow filtering is in a separate PR].
Arrows now have
• standardised radius of curvature (except when there are space constraints)
• Terminating segment length (so there is no abrupt "bent" arrow-head)
The arrows which pointed to arrays from the left were initially straight arrows. Now, they also follow Manhattan routing like the rest of the arrows.
Type of change
How to test
Run any Source program and step through the CSE Machine to see the arrows.
Checklist