fix: prevent shapes from shrinking to zero via keyboard nudge#3949
fix: prevent shapes from shrinking to zero via keyboard nudge#3949Chetansahney wants to merge 2 commits intoGraphiteEditor:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where keyboard nudging could cause shapes to shrink to a zero-pixel size, making them unselectable and stuck. The changes introduce a more robust calculation for shape enlargement, ensuring that shapes maintain a minimum dimension of 1 pixel. This significantly improves the user experience by preventing unintended shape disappearance and enhancing the reliability of keyboard-based manipulation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 addresses an issue where shapes could shrink to zero size when nudged with the keyboard. The change correctly clamps both the original and new sizes of the shape to a minimum of 1px before calculating the resize factor. This prevents division by zero and ensures shapes don't disappear. The logic is sound, but I have a suggestion to improve a comment for better code clarity.
I am having trouble creating individual review comments. Click here to see my feedback.
editor/src/messages/portfolio/document/document_message_handler.rs (2130)
This comment is a bit vague and doesn't explain the fix. To improve code clarity for future maintainers, consider replacing it with a comment that explains why the new math is correct, specifically mentioning the clamping of both original and new sizes to prevent division by zero and shapes from shrinking to zero size.
// To prevent division by zero or shapes shrinking to nothing, we clamp both the original and new sizes to a minimum of 1px before calculating the enlargement factor.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Confidence score: 3/5
- There is a concrete regression risk in
editor/src/messages/portfolio/document/document_message_handler.rs: clamping the denominator to 1 makes scale factors too small for sub-1px selections, so tiny objects resize much less than expected. - A second medium-severity issue in the same handler can cause translation drift: scale is computed from clamped resize delta while position/pivot still use raw keyboard delta when min-size clamping is hit.
- Given two high-confidence, user-visible resize/transform inconsistencies (both 6/10), this is a moderate merge risk rather than a merge-blocker.
- Pay close attention to
editor/src/messages/portfolio/document/document_message_handler.rs- resize scaling and pivot/position delta handling can diverge under min-size clamps.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/portfolio/document/document_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/document_message_handler.rs:2135">
P2: Clamping the current size to 1 in the denominator makes resize scaling incorrect for sub-1px objects, causing tiny selections to resize very weakly.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/document_message_handler.rs:2137">
P2: Clamped scale uses reduced resize delta, but position/pivot still use raw keyboard delta, causing incorrect translation when min-size clamp is hit.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if resize_opposite_corner != opposite_y { -delta_y } else { delta_y }, | ||
| ); | ||
| let enlargement_factor = (enlargement + size) / size; | ||
| let clamped_size = size.max(DVec2::ONE); |
There was a problem hiding this comment.
P2: Clamping the current size to 1 in the denominator makes resize scaling incorrect for sub-1px objects, causing tiny selections to resize very weakly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/document_message_handler.rs, line 2135:
<comment>Clamping the current size to 1 in the denominator makes resize scaling incorrect for sub-1px objects, causing tiny selections to resize very weakly.</comment>
<file context>
@@ -2127,14 +2127,14 @@ impl DocumentMessageHandler {
if resize_opposite_corner != opposite_y { -delta_y } else { delta_y },
);
- let enlargement_factor = (enlargement + size) / size;
+ let clamped_size = size.max(DVec2::ONE);
+ let clamped_new_size = (size + enlargement).max(DVec2::ONE);
+ let enlargement_factor = clamped_new_size / clamped_size;
</file context>
| let enlargement_factor = (enlargement + size) / size; | ||
| let clamped_size = size.max(DVec2::ONE); | ||
| let clamped_new_size = (size + enlargement).max(DVec2::ONE); | ||
| let enlargement_factor = clamped_new_size / clamped_size; |
There was a problem hiding this comment.
P2: Clamped scale uses reduced resize delta, but position/pivot still use raw keyboard delta, causing incorrect translation when min-size clamp is hit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/document_message_handler.rs, line 2137:
<comment>Clamped scale uses reduced resize delta, but position/pivot still use raw keyboard delta, causing incorrect translation when min-size clamp is hit.</comment>
<file context>
@@ -2127,14 +2127,14 @@ impl DocumentMessageHandler {
- let enlargement_factor = (enlargement + size) / size;
+ let clamped_size = size.max(DVec2::ONE);
+ let clamped_new_size = (size + enlargement).max(DVec2::ONE);
+ let enlargement_factor = clamped_new_size / clamped_size;
let position = DVec2::new(
</file context>
1 participant
Fixes #3947
Added a 1px floor to the keyboard nudge math to stop shapes from shrinking to zero and getting stuck