fix(mcdaStyle): 18809 don't show hexes with MCDA value === Infinity on the map #777
Conversation
WalkthroughThe recent updates to the Changes
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Bundle size diff
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range comments (2)
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (2)
Line range hint
53-53: Replace non-null assertion with optional chaining.Using non-null assertions can lead to potential runtime errors. It's safer to use optional chaining which includes runtime checks.
- return ['/', calculateLayer(layers.at(0)!), layers.at(0)!.coefficient]; + return ['/', calculateLayer(layers.at(0)?.), layers.at(0)?.coefficient];Tools
Biome
[error] 103-103: Use Number.Infinity instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.Infinity instead.(lint/style/useNumberNamespace)
Line range hint
54-56: Remove unnecessary else clause.The
elseclause is redundant since all previous branches in the conditional logic break early.- else { - return expressionsPaint(props); - } + return expressionsPaint(props);Tools
Biome
[error] 103-103: Use Number.Infinity instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.Infinity instead.(lint/style/useNumberNamespace)
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (1 hunks)
Additional context used
Biome
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts
[error] 53-53: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 53-53: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 54-56: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
[error] 103-103: Use Number.Infinity instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.Infinity instead.(lint/style/useNumberNamespace)
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts
Outdated
Show resolved
Hide resolved
|
It was decided to not filter out Infinity values on FE (because they can be valid in theory), and instead to try and fix this on BE/data side within MVTs. See comment: https://kontur.fibery.io/Tasks/Task/FE-MCDA-hex-shows-Infinity-result-if-the-denominator-is-0-18809/comment=23678 |
… in numerator/denominator
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range comments (2)
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (2)
Line range hint
62-62: Replace non-null assertion with optional chaining.The non-null assertion here is flagged as unsafe. Using optional chaining (
?.) is a safer alternative that also handles cases where the object might beundefined.- return ['/', calculateLayer(layers.at(0)!), layers.at(0)!.coefficient]; + return ['/', calculateLayer(layers.at(0)?.), layers.at(0)?.coefficient];
Line range hint
63-65: Remove unnecessary else clause.Since the previous branches (if conditions) handle all cases that break out of the function early, the else clause is redundant and can be omitted for cleaner code.
- } else { - return ['/', ['+', ...layers.map(calculateLayer)], sumBy(layers, 'coefficient')]; - } + return ['/', ['+', ...layers.map(calculateLayer)], sumBy(layers, 'coefficient')];
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (1 hunks)
Additional context used
Biome
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts
[error] 62-62: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 62-62: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 63-65: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts
Outdated
Show resolved
Hide resolved
…l (not needed atm)
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (1 hunks)
Additional comments not posted (4)
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (4)
Line range hint
53-60: LGTM!The
linearNormalizationfunction is well-implemented and handles the normalization of layer values correctly.
Line range hint
62-91: LGTM!The
sentimentPaintfunction is well-implemented and generates the paint properties correctly based on the MCDA result and color configuration.
Line range hint
93-115: LGTM!The
expressionsPaintfunction is well-implemented and generates the paint properties correctly based on the MCDA result and expression configuration.
Line range hint
117-162: LGTM!The
createMCDAStylefunction is well-implemented and creates the MCDA style configuration correctly based on the provided configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
src/core/logical_layers/renderers/stylesConfigs/mcda/__snapshots__/mcdaStyle.test.ts.snapis excluded by!**/*.snap,!**/*.snap
Files selected for processing (1)
- src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (2 hunks)
Additional comments not posted (2)
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (2)
46-50: Add checks for null values in the denominator.The added condition for checking zero in the denominator is good. However, consider adding checks for null values to make the function more robust.
- // this checks for 0 in denominator (0 in denominator makes the result === Infinity) + // this checks for 0 or null in the denominator (which causes the result to be Infinity or invalid) notEqual(featureProp(axis[1]), 0), + notEqual(featureProp(axis[1]), null),
51-51: UseNumber.POSITIVE_INFINITYfor consistency.To align with modern JavaScript standards, use
Number.POSITIVE_INFINITYinstead ofInfinity.- ['==', ['var', 'mcdaResult'], Infinity], + ['==', ['var', 'mcdaResult'], Number.POSITIVE_INFINITY],
https://kontur.fibery.io/Tasks/Task/FE-MCDA-hex-shows-Infinity-result-if-the-denominator-is-0-18809
UPD: It was decided to not filter out Infinity values on FE (because they can be valid in theory), and instead to try and fix this on BE/data side within MVTs. See comment: https://kontur.fibery.io/Tasks/Task/FE-MCDA-hex-shows-Infinity-result-if-the-denominator-is-0-18809/comment=23678
Summary by CodeRabbit
Infinity) as transparent to avoid display issues.