feat(20221): Apply maxzoom for Bivariate and MCDA layers based on Indicators maxZoom property #923
Conversation
…o Bivariate layers
WalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing the handling of zoom levels in MCDA and bivariate layers. Key changes include the addition of Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Bundle size diff
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #923 +/- ##
======================================
Coverage 9.56% 9.57%
======================================
Files 662 668 +6
Lines 28862 29261 +399
Branches 1169 1189 +20
======================================
+ Hits 2761 2802 +41
- Misses 25576 25930 +354
- Partials 525 529 +4
☔ View full report in Codecov by Sentry. |
|
Preview environments for this PR:
These previews are automatically updated with each commit. Note: After a new deployment, it may take a few minutes for the changes to propagate and for caches to update. During this time, you might experience temporary loading issues or see an older version of the app. If the app fails to load, please wait a few minutes and try again. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range comments (3)
src/core/logical_layers/renderers/stylesConfigs/mcda/types.ts (1)
Line range hint
32-38: Track TODOs for future breaking changes.The comments indicate upcoming breaking changes:
- The
axisfield may be removed/renamed- The
transformationFunctionmay become requiredConsider creating tracking issues for these changes to ensure they're not forgotten.
Would you like me to create GitHub issues to track these upcoming changes?
src/core/logical_layers/renderers/stylesConfigs/mcda/calculations/calculations.test.ts (1)
Line range hint
1-1: Review Summary: Implementation IncompleteWhile the PR adds the
maxZoomproperty to theIndicatortype, there are several concerns:
- The implementation lacks test coverage for maxZoom functionality
- Several changes (direction property additions) appear unrelated to the PR objective
- The maxZoom property could benefit from type constraints
Recommendations:
- Add comprehensive test coverage for maxZoom functionality
- Consider moving direction-related changes to a separate PR
- Add type constraints for the maxZoom property
src/utils/bivariate/bivariateColorThemeUtils.ts (1)
Line range hint
1-1: Consider centralizing zoom level constantsCurrently, zoom level fallbacks and calculations are spread across MCDA and Bivariate implementations. Consider creating a centralized zoom level management module to improve maintainability.
This could include:
- Centralized constants for fallback values
- Shared utility functions for zoom level calculations
- Type definitions for zoom level configurations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (11)
src/core/logical_layers/renderers/stylesConfigs/mcda/calculations/calculateLayerPipeline.test.ts(2 hunks)src/core/logical_layers/renderers/stylesConfigs/mcda/calculations/calculations.test.ts(2 hunks)src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.test.ts(4 hunks)src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts(3 hunks)src/core/logical_layers/renderers/stylesConfigs/mcda/types.ts(1 hunks)src/features/bivariate_color_manager/atoms/bivariateColorManagerSamleMap.ts(2 hunks)src/features/mcda/atoms/mcdaLayer.ts(3 hunks)src/utils/bivariate/bivariateColorThemeUtils.ts(4 hunks)src/utils/bivariate/getMaxZoomLevel.test.ts(1 hunks)src/utils/bivariate/getMaxZoomLevel.ts(1 hunks)src/utils/bivariate/types/stat.types.ts(1 hunks)
🔇 Additional comments (15)
src/utils/bivariate/getMaxZoomLevel.ts (1)
4-9: LGTM! Clean implementation of MCDA zoom level calculation.
The function effectively flattens the nested indicators structure and delegates to the generic indicator zoom calculator.
src/core/logical_layers/renderers/stylesConfigs/mcda/types.ts (1)
30-30: LGTM! Type change aligns with zoom level implementation.
The change from MCDAIndicator[] to Indicator[] properly supports the new maxZoom functionality.
src/utils/bivariate/getMaxZoomLevel.test.ts (2)
24-47: LGTM! Well-structured test data.
The test indicators are well-defined with realistic values and proper typing.
1-47: Verify usage of new zoom level functions.
Let's ensure the new functions are used consistently across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response as we have comprehensive information about the zoom level function usage across the codebase.
The zoom level functions are properly integrated and used consistently
The implementation is verified and shows proper usage:
getMaxIndicatorsZoomLevelis correctly used in bivariate color theme utilities and sample map componentsgetMaxMCDAZoomLevelis properly integrated in MCDA layer and style configurations- Both functions are used with appropriate fallback values (FALLBACK_BIVARIATE_MAX_ZOOM)
- The maxZoom property is consistently handled across different layer types (Generic, Bivariate)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential usage of maxZoom in the codebase
echo "Searching for maxZoom usage..."
rg "maxZoom" --type ts
# Search for direct function usage
echo -e "\nSearching for function usage..."
rg "getMaxMCDAZoomLevel|getMaxIndicatorsZoomLevel" --type ts
# Look for potential zoom-related configurations
echo -e "\nSearching for zoom configurations..."
rg "zoom.*config|config.*zoom" --type ts
Length of output: 5283
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.test.ts (1)
Line range hint 1-77: Add test coverage for maxZoom functionality
While the changes to add direction property are present, there appears to be no test coverage for the maxZoom functionality that this PR introduces. Consider adding test cases to verify:
- How maxZoom affects layer behavior
- Edge cases with missing or invalid maxZoom values
Let's check if there are any other test files covering maxZoom:
src/core/logical_layers/renderers/stylesConfigs/mcda/calculations/calculations.test.ts (1)
Line range hint 94-104: Changes appear unrelated to PR objective
The addition of direction properties seems unrelated to the PR's objective of implementing maxZoom functionality. Consider:
- Moving these changes to a separate PR focused on the direction feature
- Adding test coverage for maxZoom in the appropriate test files
Let's verify if these changes are part of a different feature:
src/features/bivariate_color_manager/atoms/bivariateColorManagerSamleMap.ts (3)
9-9: LGTM: Clean import addition
The import of getMaxIndicatorsZoomLevel aligns with the PR's objective to apply maxzoom based on indicators' properties.
69-69: LGTM: Updated function parameter
Correctly passing maxZoom to generateBivariateStyle instead of meta object.
60-63: Verify handling of empty quotients arrays
The maxZoom calculation looks good, but let's ensure it handles edge cases properly.
✅ Verification successful
Empty quotients arrays are properly handled
The implementation of getMaxIndicatorsZoomLevel safely handles empty arrays:
- When an empty array is passed,
Math.max()with spread operator returns -1 - When -1 is detected, it returns the fallbackMaxZoom parameter
- The optional chaining on
indicator.maxZoom ?? -1handles undefined values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if getMaxIndicatorsZoomLevel handles empty quotients gracefully
ast-grep --pattern 'getMaxIndicatorsZoomLevel($args, $_)' | rg -A 5 'getMaxIndicatorsZoomLevel'
Length of output: 95
Script:
#!/bin/bash
# Let's first find the file containing getMaxIndicatorsZoomLevel function
fd getMaxIndicatorsZoomLevel
# Then let's look at its implementation
rg -A 10 "function getMaxIndicatorsZoomLevel"
Length of output: 808
src/features/mcda/atoms/mcdaLayer.ts (3)
20-20: LGTM: Clean import addition
The import of getMaxMCDAZoomLevel is consistent with the PR's objective.
76-76: LGTM: Proper maxZoom assignment
The maxZoom is correctly assigned in the layer source configuration.
56-56: Verify maxZoom calculation with various MCDA configurations
The maxZoom calculation looks good, but let's verify it works with different MCDA configurations.
✅ Verification successful
MaxZoom calculation implementation is robust and well-handled
The getMaxMCDAZoomLevel function correctly handles various MCDA configurations by:
- Extracting zoom levels from all indicators across all layers
- Using the maximum zoom level among all indicators when available
- Falling back to a reasonable default of 14 when no zoom levels are specified
- Properly handling undefined zoom levels with the fallback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check MCDA configurations in the codebase
ast-grep --pattern 'type: "mcda"' | rg -B 5 -A 5 'type: "mcda"'
Length of output: 65
Script:
#!/bin/bash
# Let's check the implementation of getMaxMCDAZoomLevel function
ast-grep --pattern 'function getMaxMCDAZoomLevel' -A 10
# Also check for any MCDA zoom level related configurations
rg -i "mcda.*zoom"
Length of output: 821
Script:
#!/bin/bash
# Let's check the implementation details of getMaxMCDAZoomLevel function
cat src/utils/bivariate/getMaxZoomLevel.ts
# Check the FALLBACK_BIVARIATE_MAX_ZOOM constant
rg "FALLBACK_BIVARIATE_MAX_ZOOM.*=" -A 1
Length of output: 918
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (2)
201-201: LGTM: Source configuration updated
The source configuration now correctly uses the dynamically calculated maxzoom value.
16-16: LGTM: Dynamic maxzoom calculation added
The addition of getMaxMCDAZoomLevel improves layer configuration by dynamically calculating the maximum zoom level based on indicators metadata, with a sensible fallback.
Let's verify the implementation of getMaxMCDAZoomLevel:
Also applies to: 177-178
✅ Verification successful
LGTM: Dynamic maxzoom calculation verified
The implementation of getMaxMCDAZoomLevel is properly implemented and tested:
- It uses
getMaxIndicatorsZoomLevelto find the maximum zoom level from all indicators - Falls back to
FALLBACK_BIVARIATE_MAX_ZOOM(14) when no indicators have maxZoom defined - Test coverage confirms both successful and fallback scenarios
- The function is consistently used across MCDA and bivariate implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify getMaxMCDAZoomLevel implementation
# Expected: Function should exist and handle both valid and fallback cases
ast-grep --pattern 'function getMaxMCDAZoomLevel($_: $_) {
$$$
}'
Length of output: 69
Script:
#!/bin/bash
# Let's try to find the file and implementation using ripgrep
rg -l "getMaxMCDAZoomLevel" --type ts
# Then examine the implementation
rg "getMaxMCDAZoomLevel" -A 10 --type ts
Length of output: 5016
Script:
#!/bin/bash
# Let's check the implementation of getMaxIndicatorsZoomLevel which is used by getMaxMCDAZoomLevel
rg "getMaxIndicatorsZoomLevel" -A 15 --type ts
# Also check for the FALLBACK_BIVARIATE_MAX_ZOOM constant
rg "FALLBACK_BIVARIATE_MAX_ZOOM" -A 2 --type ts
Length of output: 12300
src/utils/bivariate/bivariateColorThemeUtils.ts (1)
148-148: LGTM: Improved parameter typing and usage
The change from meta parameter to explicit maxZoomLevel improves type safety and makes the zoom level configuration more explicit. The source configuration correctly uses this value.
Also applies to: 166-166
...core/logical_layers/renderers/stylesConfigs/mcda/calculations/calculateLayerPipeline.test.ts
Show resolved
Hide resolved
Bundle ReportChanges will increase total bundle size by 507 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/features/bivariate_color_manager/atoms/bivariateColorManagerSamleMap.ts(2 hunks)src/utils/bivariate/bivariateColorThemeUtils.ts(4 hunks)src/utils/bivariate/getMaxZoomLevel.test.ts(1 hunks)src/utils/bivariate/getMaxZoomLevel.ts(1 hunks)
🔇 Additional comments (7)
src/utils/bivariate/getMaxZoomLevel.ts (2)
4-9: LGTM! Clean and focused implementation.
The function has a clear single responsibility of extracting indicators from MCDA config and delegating to getMaxNumeratorZoomLevel.
11-22:
Add validation and consider checking all indicators.
The implementation has two potential issues:
- No input validation for null/empty arrays or negative fallback zoom
- Only checks the first indicator in each array, potentially missing relevant maxZoom values from other indicators
Consider this implementation:
export function getMaxNumeratorZoomLevel(
indicators: Indicator[][],
fallbackMaxZoom: number,
) {
+ if (!indicators?.length) {
+ return fallbackMaxZoom;
+ }
+ if (fallbackMaxZoom < 0) {
+ throw new Error('Fallback zoom level must be non-negative');
+ }
const maxIndicatorsZoom = Math.max(
- ...indicators.map((indicators) => indicators[0]?.maxZoom ?? -1),
+ ...indicators.flatMap((indicatorArray) =>
+ indicatorArray.map((indicator) => indicator?.maxZoom ?? -1)
+ ),
);
if (maxIndicatorsZoom === -1) {
return fallbackMaxZoom;
}
return maxIndicatorsZoom;
}Likely invalid or redundant comment.
src/utils/bivariate/getMaxZoomLevel.test.ts (1)
34-62: LGTM! Well-structured test helper.
The axisWithZoomLevels helper function provides a clean way to create test data with all required properties.
src/features/bivariate_color_manager/atoms/bivariateColorManagerSamleMap.ts (1)
60-69: LGTM! Clean integration of maxZoom calculation.
The maxZoom calculation is properly integrated:
- Correctly extracts quotients from both axes
- Uses meta.max_zoom as fallback
- Passes calculated maxZoom to generateBivariateStyle
src/utils/bivariate/bivariateColorThemeUtils.ts (3)
5-5: LGTM: Import changes are consistent with the new maxzoom implementation
The addition of getMaxNumeratorZoomLevel and removal of the Meta type align well with the refactoring to support dynamic maxzoom calculations.
Also applies to: 9-9
137-137: LGTM: Style generation properly implements dynamic maxzoom
The changes correctly implement the dynamic maxzoom functionality based on indicators metadata.
Let's verify if hardcoding minzoom to 0 is the intended behavior:
Also applies to: 148-148, 166-166
✅ Verification successful
Hardcoded minzoom of 0 aligns with codebase conventions
The verification shows that minzoom:0 is consistently used as a default value across the codebase, particularly in GenericRenderer. For bivariate layers, there's also a FALLBACK_BIVARIATE_MIN_ZOOM constant used in the BivariateRenderer, making the current implementation consistent with existing patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of minzoom configuration in the codebase
rg -B 2 -A 2 'minzoom:' --type ts
Length of output: 3139
128-131:
Fix array structure in maxZoom calculation
The current implementation creates a nested array structure [[...], [...]], which might not match what getMaxNumeratorZoomLevel expects. Additionally, we should handle undefined quotients safely.
Consider this safer implementation:
const maxZoom = getMaxNumeratorZoomLevel(
- [xAxis.quotients ?? [], yAxis.quotients ?? []],
+ [...(xAxis.quotients || []), ...(yAxis.quotients || [])].filter(Boolean),
stats.meta.max_zoom,
);Likely invalid or redundant comment.
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
Plan: Pro
📒 Files selected for processing (4)
src/core/logical_layers/renderers/BivariateRenderer/BivariateRenderer.tsx(2 hunks)src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts(3 hunks)src/utils/bivariate/bivariateColorThemeUtils.ts(4 hunks)src/utils/bivariate/getMaxZoomLevel.ts(1 hunks)
🔇 Additional comments (7)
src/utils/bivariate/getMaxZoomLevel.ts (2)
4-9: LGTM! Function follows single responsibility principle.
The function cleanly extracts indicators from the config layers and delegates to getMaxNumeratorZoomLevel.
11-22: 🛠️ Refactor suggestion
Consider enhancing the zoom level calculation logic.
The current implementation has a few potential issues:
- Only considers the first indicator's maxZoom from each array
- Missing validation for empty arrays and negative zoom values
Consider this enhanced implementation:
export function getMaxNumeratorZoomLevel(
indicators: Indicator[][],
fallbackMaxZoom: number,
) {
+ if (!indicators?.length) {
+ return fallbackMaxZoom;
+ }
+ if (fallbackMaxZoom < 0) {
+ throw new Error('Fallback zoom level must be non-negative');
+ }
const maxIndicatorsZoom = Math.max(
- ...indicators.map((indicators) => indicators[0]?.maxZoom ?? -1),
+ ...indicators.flatMap((arr) =>
+ arr.map((indicator) => indicator?.maxZoom ?? -1)
+ ),
);
if (maxIndicatorsZoom === -1) {
return fallbackMaxZoom;
}
return maxIndicatorsZoom;
}Likely invalid or redundant comment.
src/core/logical_layers/renderers/stylesConfigs/mcda/mcdaStyle.ts (1)
177-178: LGTM! Good use of the new zoom level calculation.
The changes properly integrate the dynamic zoom level calculation while maintaining the fallback behavior.
Also applies to: 201-201
src/utils/bivariate/bivariateColorThemeUtils.ts (2)
149-149: LGTM! Clean parameter update and usage.
The changes to generateBivariateStyle cleanly integrate the maxZoomLevel parameter.
Also applies to: 167-167
129-132: 🧹 Nitpick (assertive)
Consider using more defensive quotients handling.
The current implementation could be made more robust when handling undefined quotients.
Consider this safer implementation:
const maxZoom = getMaxNumeratorZoomLevel(
- [xAxis.quotients ?? [], yAxis.quotients ?? []],
+ [
+ Array.isArray(xAxis.quotients) ? xAxis.quotients : [],
+ Array.isArray(yAxis.quotients) ? yAxis.quotients : []
+ ],
stats.meta.max_zoom || FALLBACK_BIVARIATE_MAX_ZOOM,
);Likely invalid or redundant comment.
src/core/logical_layers/renderers/BivariateRenderer/BivariateRenderer.tsx (2)
21-21: LGTM: Import aligns with PR objective
The addition of getMaxNumeratorZoomLevel import is appropriate for implementing the maxzoom functionality based on indicators.
144-147: Verify maxZoom calculation edge cases
The maxZoom calculation handles undefined cases by defaulting to empty arrays, but this could silently mask configuration issues. Consider adding validation for the legend structure.
Let's check for potential edge cases in the codebase:
src/core/logical_layers/renderers/BivariateRenderer/BivariateRenderer.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/core/logical_layers/renderers/BivariateRenderer/constants.ts(1 hunks)
🔇 Additional comments (1)
src/core/logical_layers/renderers/BivariateRenderer/constants.ts (1)
5-5: Verify the impact of reducing the maximum zoom level.
The maximum zoom level has been significantly reduced from 14 to 9. While this aligns with the PR objective to apply maxzoom based on indicators, we should verify that this fallback value is appropriate for most use cases.
✅ Verification successful
Based on the search results, I can now generate the final response as the findings provide sufficient context about the usage and impact of the FALLBACK_BIVARIATE_MAX_ZOOM constant.
The reduced fallback zoom level (9) appears appropriate and well-integrated
The code search reveals that:
- The fallback is only used when dynamic zoom calculations fail (
stats.meta.max_zoom || FALLBACK_BIVARIATE_MAX_ZOOM) - The system primarily relies on dynamic zoom calculations through
getMaxNumeratorZoomLevelandgetMaxMCDAZoomLevel - The fallback is consistently applied across both Bivariate and MCDA rendering contexts
- No hardcoded maxZoom values in indicator configurations conflict with this fallback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of FALLBACK_BIVARIATE_MAX_ZOOM and typical maxZoom values in indicators
# Find all references to FALLBACK_BIVARIATE_MAX_ZOOM
echo "Searching for FALLBACK_BIVARIATE_MAX_ZOOM usage:"
rg "FALLBACK_BIVARIATE_MAX_ZOOM" -A 3
# Find any hardcoded maxZoom values in indicator configurations
echo -e "\nSearching for maxZoom in indicator configurations:"
rg "maxZoom[\"']?\s*[:=]" -A 1
Length of output: 6210
albaranau
deleted the
20221-fe-apply-maxzoom-for-bivariate-mcda-layers-based-on-indicators-metadata
branch
https://kontur.fibery.io/Tasks/Task/FE-Apply-maxzoom-for-bivariate-MCDA-layers-based-on-indicators-metadata-20221
Summary by CodeRabbit
New Features
directionproperty for better context.Bug Fixes
Tests
Documentation
MCDALayerinterface regarding future changes.