Fix: handling keys with periods when flattening/unflattening #1615
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/core/src/v3/utils/flattenAttributes.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe changes modify the Changes
Assessment against linked issues
Poem
Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/v3/utils/flattenAttributes.ts (1)
6-15: Consider performance optimization inunescapeKey.The
RegExpobject is recreated on every call tounescapeKey. Consider declaring it as a constant outside the function for better performance.const DOT = "__DOT__"; +const DOT_REGEX = new RegExp(DOT, "g"); function escapeKey(key: string) { return key.replace(/\./g, DOT); // Replace . with __DOT__ } function unescapeKey(key: string) { - let re = new RegExp(DOT, "g"); - return key.replace(re, '.'); // Replace __DOT__ back with . (dot) + return key.replace(DOT_REGEX, '.'); // Replace __DOT__ back with . (dot) }packages/core/test/flattenAttributes.test.ts (1)
183-213: Use Math.PI instead of a magic number.Replace the hard-coded PI value with
Math.PIfrom the standard library for better maintainability and accuracy.- const PI = 3.14159265359; + const PI = Math.PI; const result = flattenAttributes(PI); expect(result).toEqual({ "": PI }); expect(unflattenAttributes(result)).toEqual(PI);🧰 Tools
🪛 Biome (1.9.4)
[error] 209-209: Prefer constants from the standard library.
Unsafe fix: Use Math.PI instead.
(lint/suspicious/noApproximativeNumericConstant)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/v3/utils/flattenAttributes.ts(3 hunks)packages/core/test/flattenAttributes.test.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/test/flattenAttributes.test.ts
[error] 209-209: Prefer constants from the standard library.
Unsafe fix: Use Math.PI instead.
(lint/suspicious/noApproximativeNumericConstant)
🔇 Additional comments (3)
packages/core/src/v3/utils/flattenAttributes.ts (2)
67-68: LGTM! Correct implementation of key escaping.The escaping is properly applied only to object keys while preserving array indices, which is the expected behavior.
133-136: Consider adding error handling for malformed keys.The implementation correctly unescapes keys, but it might be good to add validation for malformed keys containing the sentinel string.
function unescapeKey(key: string) { + // Validate that the key doesn't contain malformed sentinel strings + if (key.includes(DOT) && !key.includes("__DOT__")) { + throw new Error(`Malformed key "${key}" contains partial sentinel string`); + } let re = new RegExp(DOT, "g"); return key.replace(re, '.'); }✅ Verification successful
Consider adding error handling for malformed keys.
The implementation uses string replacement without validation, which could lead to ambiguous results if input keys already contain the sentinel string "DOT". The suggested validation in the review comment would prevent such cases.
flattenAttributes.ts: Add validation inunescapeKey()function to detect malformed keys containing partial sentinel strings.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential conflicts with the sentinel string in existing keys rg -l "__DOT__" --type tsLength of output: 118
Script:
#!/bin/bash # Check sentinel string definition and usage echo "=== Implementation ===" rg -A 3 -B 3 "__DOT__" packages/core/src/v3/utils/flattenAttributes.ts echo -e "\n=== Test Cases ===" rg -A 3 -B 3 "__DOT__" packages/core/test/flattenAttributes.test.tsLength of output: 1353
packages/core/test/flattenAttributes.test.ts (1)
296-314: LGTM! Comprehensive test coverage.The test cases thoroughly verify the escaping/unescaping functionality with both simple and nested objects.
Consider adding edge cases:
- Keys with multiple dots
- Keys with the sentinel string as part of the original key
- Empty keys
|
@matt-aitken Please review |
Closes #1510
✅ Checklist
Description
It addresses the issue- Objects with keys with periods in doesn't render properly.
Eg-
Objects like this when unflattening will not become this
Instead it will be unflattening to its original condition.
Tests
Tests have been added for this change and all passed successfully.
Screenshots
💯
Summary by CodeRabbit
New Features
Tests