Handle Invalid scale and arcDegreeScale values in segments to avoid crash #114

roni-castro-shipt · 2024-12-25T23:35:51Z

Description

Currently, when an invalid number (NaN) is passed as arcDegreeScale, Android crashes, and iOS generates an inconsistent graph.

Solution:

If the arcDegreeScale or scale is a finite number, parse it as usual. However, if either value is invalid (i.e., NaN, Infinity, or -Infinity, or any unexpected numeric value), issue a warning to the user and override the value with a default scale. This default will be calculated based on the remaining value needed to sum to 1 (current solution already). This ensures that neither arcDegreeScale nor scale will ever be NaN, preventing crashes.
Issue a warning every time a SegmentArc is rendered for the first time, under the following conditions:
1. Either arcDegreeScale or scale is not a finite number (i.e., NaN, Infinity, or -Infinity).
2. A segment’s arcDegreeScale or scale value is outside the valid range of 0 (0%) to 1 (100%).
3. The sum of all segment scale or arcDegreeScale values exceeds 1 or is less than 0.

Obs:

Previously, the scale and arcDegreeScale props were being mutated, which is generally discouraged by eslint and considered difficult to debug. With this change, the same behavior will be maintained without mutating the props. Note that this will require some snapshot updates. If this mutation is wanted for some reason, let me know to undo.
- After the PR changes, this will happen in some snapshots, no mutation of the props, and the default values will be set internally in the lib
In the current solution:
- A scale value of zero is considered invalid, so it is overridden by _ensureDefaultSegmentScale. This behavior has been preserved.
- A arcDegreeScale value of zero is considered valid, so it is not overridden by _ensureDefaultArcScale. This logic remains unchanged.
- scale or arcDegreeScale values set to undefined are currently accepted as valid props. The previous functions _ensureDefaultSegmentScale and _ensureDefaultArcScale handle these values by evenly distributing the value across segments, so no warning is generated for these cases, and the same logic is preserved.

Screenshots

Android

Before	After
android_before.mp4	android_after.mp4

iOS

Before	After
ios_before.mp4	ios_after.mp4

No warnings for valid props

Android	iOS

Some possible warnings

arcDegreeScale

scale

jkhusanov

While I'll pull up and run the example app locally, couple of comments.

jkhusanov · 2024-12-31T22:36:29Z

src/utils/segmentedArcWarnings.js

+import { SegmentedArcError } from './segmentedArcErrors';
+import WarningManager from './warningManager';
+
+const isInvalidScalePercent = value => {


Since these are local functions can we _underscore these?

jkhusanov · 2024-12-31T23:06:56Z

src/utils/segmentedArcWarnings.js

+
+export const createInvalidScaleValueError = (propertyName, value) => {
+  return new SegmentedArcError(
+    `Each '${propertyName}' value should be a positive number between 0 and 1 inclusive. It was found ${value}`


I think we should paraphrase the ending something like this or better.

Suggested change

`Each '${propertyName}' value should be a positive number between 0 and 1 inclusive. It was found ${value}`

`Each '${propertyName}' value should be a positive number between 0 and 1 inclusive. ${value} was provided instead`

jkhusanov · 2024-12-31T23:28:32Z

src/utils/warningManager.js

+
+  if (!warnings.has(key)) {
+    console.warn(data);
+    warnings.add(key);


Just to clarify, did we opt into having a warning set becase we wanted to avoid showing the same warnings more than once?

This was to avoid depending on the useShowSegmentedArcWarnings useRef to just show the same warning once in case rerender happens, like when an array is passed directly as props without memoization, and avoid spamming multiple warnings per render. But I'll check if that is still needed

<SegmentedArc segments={[ {scale: NaN} ]} />

jkhusanov · 2025-01-02T19:00:25Z

src/utils/scale.js

@@ -0,0 +1,29 @@
+export const ensureDefaultSegmentScale = segments => {
+  const isInvalidScale = value => !Number.isFinite(value) || value === 0;


Can we move this function to outside of ensureDefaultSegmentScale function?

jkhusanov · 2025-01-02T19:15:14Z

src/utils/scale.js

+  const allocatedScale = segments.reduce((sum, { scale }) => sum + (isInvalidScale(scale) ? 0 : scale), 0);
+  const defaultArcScale = invalidScaleCount > 0 ? (1 - allocatedScale) / invalidScaleCount : 0;
+
+  return segments.map(segment => (isInvalidScale(segment.scale) ? { ...segment, scale: defaultArcScale } : segment));


Appreaciate for not manipulating props directly!

jkhusanov · 2025-01-02T19:16:38Z

src/utils/scale.js

+};
+
+export const ensureDefaultSegmentArcDegreeScale = segments => {
+  const isInvalidArcDegreeScale = value => !Number.isFinite(value);


Could we also move this to outside of this function and make it _local?

jkhusanov · 2025-01-02T19:33:02Z

src/__tests__/SegmentedArc.spec.js

+    global.__DEV__ = currentGlobalDev;
+  });
+
+  it('show warnings and renders the component when segments have invalid scale or arcDegreeScale data', () => {


adding s

Suggested change

it('show warnings and renders the component when segments have invalid scale or arcDegreeScale data', () => {

it('shows warnings and renders the component when segments have invalid scale or arcDegreeScale data', () => {

jkhusanov · 2025-01-02T22:33:21Z

src/utils/__tests__/scale.spec.js

+
+    const segmentsResult = ensureDefaultSegmentArcDegreeScale(segments);
+
+    expect(segmentsResult).toEqual([{ arcDegreeScale: 1 / 2 }, { arcDegreeScale: 1 / 2 }]);


did you just use 1/2 to avoid precision issues?

No. it's 1/2 because there're two segments in here const segments = [{ arcDegreeScale: NaN }, { arcDegreeScale: NaN }]; so the default value would be 1 / numberOfSegments. I'll change to 1 / segments.length to make it clear

I wanted to say could we do 0.5 ?

jkhusanov · 2025-01-03T01:57:20Z

src/utils/__tests__/scale.spec.js

+        {
+          arcDegreeScale: 8 / 50
+        },
+        {
+          arcDegreeScale: 42 / 50
+        }
+      ])


nitpick it seems like having exact values like this is more readable

Suggested change

{

arcDegreeScale: 8 / 50

},

{

arcDegreeScale: 42 / 50

}

])

{

arcDegreeScale: 0.16

},

{

arcDegreeScale: 0.84

}

])

jkhusanov · 2025-01-03T01:59:40Z

src/utils/__tests__/scale.spec.js

+describe('ensureDefaultSegmentScaleValues', () => {
+  it('ensures default values for `scale` are set', () => {
+    expect(
+      ensureDefaultSegmentScaleValues([


maybe we could have called this outside of expect function and used a variable instead.

roni-castro-shipt added 8 commits December 24, 2024 17:36

Handle possible invalid scale input

8aa2278

Show warnings about invalid segment props

7866f70

Handle segments with invalid arcDegreeScale

03886ce

Update Segment Arc component to use the new function

d45ce00

Handle the scale sum with default values to avoid NaN or Infinite values

14ea42b

Allow to show the same warning message for different component instance

3d89d7f

Remove duplicated tests

ad70525

Handle potential warning on floating point sum

ea7fb27

roni-castro-shipt marked this pull request as ready for review December 26, 2024 14:27

roni-castro-shipt requested review from jkhusanov, calebmackdavenport, rkyle35242, chaceburnette, joshuakg, rykugur, kathrynnlester and taylorcore as code owners December 26, 2024 14:27

roni-castro-shipt mentioned this pull request Dec 26, 2024

Handle invalid numeric inputs for props to avoid crash #115

Draft

roni-castro-shipt added 2 commits December 30, 2024 12:52

Test ensureDefaultSegmentScaleValues

2171492

Improve test description

75e2476

jkhusanov reviewed Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle Invalid scale and arcDegreeScale values in segments to avoid crash #114

Handle Invalid scale and arcDegreeScale values in segments to avoid crash #114

roni-castro-shipt commented Dec 25, 2024 •

edited

Loading

jkhusanov left a comment

jkhusanov Dec 31, 2024

jkhusanov Dec 31, 2024

jkhusanov Dec 31, 2024

roni-castro-shipt Jan 3, 2025

jkhusanov Jan 2, 2025

jkhusanov Jan 2, 2025

jkhusanov Jan 2, 2025

jkhusanov Jan 2, 2025

jkhusanov Jan 2, 2025

roni-castro-shipt Jan 3, 2025 •

edited

Loading

jkhusanov Jan 3, 2025

jkhusanov Jan 3, 2025

jkhusanov Jan 3, 2025

	`Each '${propertyName}' value should be a positive number between 0 and 1 inclusive. It was found ${value}`
	`Each '${propertyName}' value should be a positive number between 0 and 1 inclusive. ${value} was provided instead`

		@@ -0,0 +1,29 @@
		export const ensureDefaultSegmentScale = segments => {
		const isInvalidScale = value => !Number.isFinite(value) \|\| value === 0;

	it('show warnings and renders the component when segments have invalid scale or arcDegreeScale data', () => {
	it('shows warnings and renders the component when segments have invalid scale or arcDegreeScale data', () => {


		const segmentsResult = ensureDefaultSegmentArcDegreeScale(segments);

		expect(segmentsResult).toEqual([{ arcDegreeScale: 1 / 2 }, { arcDegreeScale: 1 / 2 }]);

Handle Invalid scale and arcDegreeScale values in segments to avoid crash #114

Are you sure you want to change the base?

Handle Invalid scale and arcDegreeScale values in segments to avoid crash #114

Conversation

roni-castro-shipt commented Dec 25, 2024 • edited Loading

Description

Solution:

Screenshots

Android

iOS

No warnings for valid props

Some possible warnings

arcDegreeScale

scale

jkhusanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roni-castro-shipt Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roni-castro-shipt commented Dec 25, 2024 •

edited

Loading

roni-castro-shipt Jan 3, 2025 •

edited

Loading