fix(number): address number field precision edge cases#1474
fix(number): address number field precision edge cases#1474
Conversation
Skaiir
requested review from
vsgoulart
and removed request for
barmac,
Copilot and
pablocabrera85
bpmn-io-tasks
Bot
added
in progress
bpmn-io-tasks
Bot
added
needs review
There was a problem hiding this comment.
Pull request overview
Addresses numeric precision edge cases in the viewer’s Number field by detecting unsafe numeric inputs, warning users when typed values exceed JS Number precision, and preventing stepper interactions that would “stick” due to rounding.
Changes:
- Added
isNumberInputSafe()utility (with tests) to detect loss of precision throughNumber()conversion. - Updated Number field to show a non-error warning when typed values exceed supported precision.
- Disabled stepper arrow UI when stepping would result in precision loss, plus added a reusable
<Warning />component + styling.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/form-js-viewer/test/spec/render/components/form-fields/util/numberFieldUtil.spec.js | Adds unit tests for isNumberInputSafe() safe/unsafe cases. |
| packages/form-js-viewer/test/spec/render/components/form-fields/Number.spec.js | Adds integration tests for arrow disabling and precision warning behavior (incl. serializeToString). |
| packages/form-js-viewer/src/render/components/util/numberFieldUtil.js | Introduces isNumberInputSafe() utility used by Number field logic. |
| packages/form-js-viewer/src/render/components/form-fields/Number.js | Implements arrow disabling + precision warnings and renders the new <Warning />. |
| packages/form-js-viewer/src/render/components/Warning.js | New warning UI component to render non-error messages. |
| packages/form-js-viewer/assets/form-js-base.css | Adds caution color token and warning styles; avoids error styling on disabled arrow container. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| decrementedValue = base.minus(offset).toFixed(); | ||
| } | ||
|
|
||
| return !isNumberInputSafe(incrementedValue) && !isNumberInputSafe(decrementedValue); |
There was a problem hiding this comment.
arrowsDisabled only becomes true when both the incremented and decremented values are unsafe (&&). This leaves cases where only one direction loses precision (e.g. base = 9007199254740992 with step = 1: increment is unsafe, decrement is safe) with both arrows enabled, so clicking increment can result in no underlying value change while the display updates to an unrepresentable string. Consider computing incrementDisabled/decrementDisabled separately (and disabling each button accordingly), or at least disabling arrows when either direction is unsafe.
| return !isNumberInputSafe(incrementedValue) && !isNumberInputSafe(decrementedValue); | |
| return !isNumberInputSafe(incrementedValue) || !isNumberInputSafe(decrementedValue); |
| --color-red-360-100-95: hsl(360, 100%, 95%); | ||
| --color-red-360-100-97: hsl(360, 100%, 97%); | ||
|
|
||
| --color-orange-33-100-40: hsl(33, 100%, 40%); |
There was a problem hiding this comment.
@Skaiir We need to check the Carbonization of this on Tasklist
| }); | ||
| }); | ||
|
|
||
| describe('high decimal digits without string serialization', function () { |
There was a problem hiding this comment.
I know it's inconsistent on the codebase but I don't like that we're using class names for tests. We should use more accessible selectors which won't be coupled to implementation of the class names
bpmn-io-tasks
Bot
added
in progress
Closes #1021
Adds: