Fix increment number field drag jump at start#3915
Fix increment number field drag jump at start#3915afrdbaig7 wants to merge 2 commits intoGraphiteEditor:masterfrom
Conversation
…ents to GitHub environments Restore the CI "View deployment" button in PRs after building Consolidate release.yml functionality into build.yml Move build from ci.yml to a delegated run in build.yml
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 an issue where the NumberInput component experienced erratic value jumps when dragging due to unreliable initial 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the number input drag jump issue by implementing a settling window to filter out erratic initial mouse movements. The logic is applied to both web and desktop handlers. My review includes a critical note about a potentially missing isDesktop function, which could break the build. I've also included a couple of medium-severity suggestions: one regarding a possible unintended change in behavior for integer inputs, and another to refactor duplicated code for better maintainability.
| import { PRESS_REPEAT_DELAY_MS, PRESS_REPEAT_INTERVAL_MS } from "@graphite/io-managers/input"; | ||
| import { browserVersion } from "@graphite/utility-functions/platform"; | ||
| import type { NumberInputMode, NumberInputIncrementBehavior, ActionShortcut } from "@graphite/messages"; | ||
| import { browserVersion, isDesktop } from "@graphite/utility-functions/platform"; |
There was a problem hiding this comment.
The function isDesktop is imported from @graphite/utility-functions/platform, but it does not appear to be defined or exported in the provided context for frontend/src/utility-functions/platform.ts. This will likely cause a build or runtime error. Please ensure isDesktop is correctly implemented and exported. It seems to be replacing isPlatformNative, which was previously imported from the wasm package.
| if (initialValueBeforeDragging !== undefined && e instanceof CustomEvent) { | ||
| let delta = (e.detail as { x: number }).x; | ||
| const settling = moveEvents <= SETTLING_EVENTS || performance.now() - dragStart < SETTLING_MS; | ||
| if (settling && Math.abs(delta) > MAX_DELTA) delta = 0; | ||
|
|
||
| pointerLockMoveUpdate(delta, shiftKeyDown, ctrlKeyDown, initialValueBeforeDragging); | ||
| } |
There was a problem hiding this comment.
This block of logic for sanitizing the delta is duplicated from the pointerMove handler above. To improve maintainability and avoid redundancy, you could extract this logic into a single helper function within the beginDrag scope and call it from both places.
For example, you could define a function like this before pointerUp:
const sanitizeDelta = (delta: number): number => {
const settling = moveEvents <= SETTLING_EVENTS || performance.now() - dragStart < SETTLING_MS;
if (settling && Math.abs(delta) > MAX_DELTA) return 0;
return delta;
};Then you could call sanitizeDelta() from both pointerMove and pointerLockMove.
|
|
||
| const combined = initialValue + cumulativeDragDelta; | ||
| const combineSnapped = snapping || isInteger ? Math.round(combined) : combined; | ||
| const combineSnapped = snapping ? Math.round(combined) : combined; |
There was a problem hiding this comment.
Removing || isInteger from this condition changes the dragging behavior for integer inputs. Previously, the value would snap to the nearest integer while dragging. Now, it will only snap when holding Ctrl, and will otherwise show decimal values during the drag, snapping to an integer only when the drag is complete (inside updateValue). Was this change in user experience intentional? The previous behavior seems more intuitive for fields marked as integer-only.
const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;
There was a problem hiding this comment.
1 issue found across 1 file
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="frontend/src/components/widgets/inputs/NumberInput.svelte">
<violation number="1" location="frontend/src/components/widgets/inputs/NumberInput.svelte:489">
P2: Removing `|| isInteger` here changes the dragging behavior for integer-typed inputs: previously the value snapped to the nearest integer during the drag, but now it will show decimals until the drag completes. This seems unintentional given that the PR targets only the pointer-lock jump fix. Restore the original condition to preserve integer-field snapping while dragging.
```suggestion
const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;
```</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| const combined = initialValue + cumulativeDragDelta; | ||
| const combineSnapped = snapping || isInteger ? Math.round(combined) : combined; | ||
| const combineSnapped = snapping ? Math.round(combined) : combined; |
There was a problem hiding this comment.
P2: Removing || isInteger here changes the dragging behavior for integer-typed inputs: previously the value snapped to the nearest integer during the drag, but now it will show decimals until the drag completes. This seems unintentional given that the PR targets only the pointer-lock jump fix. Restore the original condition to preserve integer-field snapping while dragging.
| const combineSnapped = snapping ? Math.round(combined) : combined; | |
| const combineSnapped = snapping || isInteger ? Math.round(combined) : combined; |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/widgets/inputs/NumberInput.svelte, line 489:
<comment>Removing `|| isInteger` here changes the dragging behavior for integer-typed inputs: previously the value snapped to the nearest integer during the drag, but now it will show decimals until the drag completes. This seems unintentional given that the PR targets only the pointer-lock jump fix. Restore the original condition to preserve integer-field snapping while dragging.
```suggestion
const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;
```</comment>
<file context>
@@ -469,7 +486,7 @@
const combined = initialValue + cumulativeDragDelta;
- const combineSnapped = snapping || isInteger ? Math.round(combined) : combined;
+ const combineSnapped = snapping ? Math.round(combined) : combined;
const newValue = updateValue(combineSnapped);
</file context>
| const combineSnapped = snapping ? Math.round(combined) : combined; | |
| const combineSnapped = snapping || isInteger ? Math.round(combined) : combined; |
Fixes #2807
The PointerLock API has been known to report some pretty wild movementX values (often around ±1000px) during the first few events right after you activate the lock, particularly in Chrome. Previously, the code only ignored the very first event, but it turned out that the 2nd or 3rd events could also be off, leading to those crazy jumps. To tackle this issue, I introduced a brief settling window: - We still skip the first event without question. - For the next few events (up to 5 events or 80ms to accommodate high-polling-rate mice), we’ll zero out any delta that exceeds 50px. - This 50px threshold effectively catches those huge ~1000px outliers while still allowing for genuine human drag speeds right at the start of a movement. I applied this same approach to both the web pointerMove and desktop pointerLockMove handlers.