fix(web): style code condition values need to be wrapped #1295
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.ts (1)
205-235: Refactor to eliminate code duplication in wrapping functionsThe functions
wrapConditionApplyValueandunwrapConditionAppliedValue, as well aswrapExpressionandunwrapExpression, contain similar switch statements based onfield. Consider refactoring to reduce code duplication, which can improve maintainability and readability.You might create a generic utility function that handles the wrapping and unwrapping logic based on
fieldand an action (wrap or unwrap). This will centralize the logic and make future updates easier.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.test.tsx(4 hunks)web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.ts(6 hunks)
🔇 Additional comments (3)
web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.test.tsx (2)
Line range hint 183-195: LGTM! Test cases properly validate the updated parseStyleValue signature
The test cases effectively cover both simple value and expression scenarios with the new type parameters, ensuring proper handling of style values.
232-235: Enhance test coverage for URL conditions
While the current test validates number conditions, consider adding a test case using URL conditions. Also, verify that the mock data in mockStyleNodes includes appropriate test cases for URL handling.
const conditions = generateConditions(
"number",
mockStyleNodes?.marker[0].conditions
);
it("should generate conditions'", () => {
expect(conditions).toEqual([
["${marker-size} === 'small'", "8"],
["${marker-size} === 'medium'", "12"]
]);
});
+
+it("should generate conditions with URL values'", () => {
+ const urlConditions = generateConditions(
+ "text",
+ [{
+ variable: "${type}",
+ operator: "===",
+ value: "'custom'",
+ applyValue: "https://example.com/model.glb"
+ }]
+ );
+ expect(urlConditions).toEqual([
+ ["${type} === 'custom'", "'https://example.com/model.glb'"]
+ ]);
+});web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.ts (1)
273-281: Verify the correctness of wrapColor function
Ensure that the regular expression used in wrapColor correctly matches all valid color formats and that the wrapping logic handles edge cases.
Run the following script to check if wrapColor correctly wraps valid hex color codes:
This script will help verify that wrapColor behaves as expected with different inputs.
✅ Verification successful
The previous attempt to test the function directly failed because we couldn't execute the code. Instead, I'm trying to gather more context about how this function is used in the codebase, any existing tests, and related color utilities that might help us understand the expected behavior and verify the implementation.
The wrapColor function implementation is correct and verified
Based on the codebase analysis, the wrapColor function is correctly implemented and well-tested:
-
The regular expression
/^#(?:[0-9A-F]{3}|[0-9A-F]{4}|[0-9A-F]{6}|[0-9A-F]{8})$/icorrectly matches:- 3-digit hex colors (#FFF)
- 4-digit hex colors (#FFFF)
- 6-digit hex colors (#FFFFFF)
- 8-digit hex colors (#FFFFFFFF)
-
The function is thoroughly tested in
convert.test.tsx:- Tests verify handling of 3-digit to 8-digit hex colors
- Tests confirm non-hex colors are returned unchanged
- Tests validate the correct wrapping format
color('#FFFFFF')
-
The function is used consistently throughout the codebase for color handling in the map layer styling system, working in conjunction with
unwrapColorfor bidirectional conversion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test wrapColor with various color inputs
# Define an array of test colors
colors=('#FFF' '#FFFFFF' '#123456' '#ABCDEF' '#123' '#ABC' 'invalidColor' '')
# Loop through the colors and test wrapColor function
for color in "${colors[@]}"; do
echo "Testing color: $color"
node -e "
const { wrapColor } = require('./convert');
console.log(wrapColor('$color'));
"
done
Length of output: 7790
Script:
#!/bin/bash
# Let's analyze the usage of wrapColor function in the codebase
# First, let's find where this function is used
rg "wrapColor" -B 2 -A 2
# Then, let's check if there are any tests for this function
fd "convert.test" -t f
# Finally, let's check if there are any color-related utilities or constants that might give us context
rg "color\(" -B 2 -A 2
Length of output: 24744
web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.test.tsx
Show resolved
Hide resolved
web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.ts (1)
205-270: Consider extracting common wrapping logic to reduce duplication.The wrapping/unwrapping functions follow a similar pattern for handling different field types. Consider extracting the common logic into a higher-order function to improve maintainability.
type WrapperFn = (value: string) => string; const createFieldWrapper = ( colorFn: WrapperFn, stringFn: WrapperFn, defaultFn: WrapperFn = (v) => v ) => (field: AppearanceField, value: string) => { switch (field) { case "color": return colorFn(value); case "image": case "text": case "model": return stringFn(value); default: return defaultFn(value); } };web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.test.tsx (1)
223-236: Add test cases for URL edge casesConsider adding test cases for:
- URLs with multiple colons (e.g.,
https://user:pass@example.com)- URLs with query parameters
- Invalid URL formats
This will ensure robust handling of various URL patterns.
Example addition:
it("should handle complex URLs correctly", () => { const conditions = parseConditions("model", [ ["${type}==='2'", "'https://user:pass@example.com/path?key=value'"] ]); expect(conditions).toEqual([ { variable: "${type}", operator: "===", value: "'2'", applyValue: "'https://user:pass@example.com/path?key=value'" } ]); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.test.tsx(5 hunks)web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.ts(6 hunks)
🔇 Additional comments (5)
web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.ts (3)
105-122: LGTM! Field-aware value parsing is well implemented.
The updated parseStyleValue function correctly handles field-specific unwrapping for both expressions and conditions while maintaining the original logic structure.
273-275: LGTM! Improved color validation regex.
The updated regex pattern correctly handles all valid hex color formats (3, 4, 6, and 8 digits) while maintaining case insensitivity.
150-157:
Add null checks for node.field to prevent runtime errors.
The function assumes node.field is defined when passing it to wrapExpression and generateConditions, but it could be undefined according to the type definition.
if (node.valueType === "expression") {
+ if (!node.field) return undefined;
return { expression: wrapExpression(node.field, node.expression ?? "") };
}
if (node.valueType === "conditions") {
+ if (!node.field) return undefined;
return {
expression: {
conditions: generateConditions(node.field, node.conditions)
}
};
}web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.test.tsx (2)
Line range hint 183-195: LGTM: Type parameter addition improves type safety
The updated test cases correctly validate the new function signature with the type parameter, improving type safety while maintaining the existing functionality.
247-250: LGTM: Type parameter correctly added
The test case has been properly updated to include the type parameter while maintaining the existing functionality.
web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.ts
Show resolved
Hide resolved
web/src/beta/features/Editor/Map/LayerStylePanel/Editor/StyleInterface/convert.test.tsx
Show resolved
Hide resolved
Overview
As a special requirement from style code conditions, some values need to be wrapped with
''or"", espcially for urls since there's:within the url string.What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Bug Fixes
Documentation