Support serializing/deserializing empty arrays #1850
Conversation
WalkthroughThe changes introduce a new constant, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Flattener
participant Unflattener
Caller->>Flattener: Send data containing an empty array
Note right of Flattener: Detect empty array and use EMPTY_ARRAY_SENTINEL
Flattener-->>Caller: Return flattened data with sentinel
Caller->>Unflattener: Send flattened data
Note left of Unflattener: Convert EMPTY_ARRAY_SENTINEL back to an empty array
Unflattener-->>Caller: Return original data with empty array
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 1
🧹 Nitpick comments (1)
packages/core/src/v3/utils/flattenAttributes.ts (1)
2-2: Remove unused importThe
debugimport from "node:util" is added but not used anywhere in the code.-import { debug } from "node:util";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webapp/app/utils/taskEvent.ts(2 hunks)apps/webapp/app/v3/eventRepository.server.ts(2 hunks)packages/core/src/v3/index.ts(1 hunks)packages/core/src/v3/utils/flattenAttributes.ts(6 hunks)packages/core/test/flattenAttributes.test.ts(4 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
apps/webapp/app/v3/eventRepository.server.ts (2)
packages/core/src/v3/utils/flattenAttributes.ts (1)
EMPTY_ARRAY_SENTINEL(5-5)packages/core/src/v3/index.ts (1)
EMPTY_ARRAY_SENTINEL(47-47)
apps/webapp/app/utils/taskEvent.ts (2)
packages/core/src/v3/index.ts (1)
EMPTY_ARRAY_SENTINEL(47-47)packages/core/src/v3/utils/flattenAttributes.ts (1)
EMPTY_ARRAY_SENTINEL(5-5)
packages/core/test/flattenAttributes.test.ts (1)
packages/core/src/v3/utils/flattenAttributes.ts (2)
flattenAttributes(8-93)unflattenAttributes(99-179)
packages/core/src/v3/utils/flattenAttributes.ts (1)
packages/core/src/v3/index.ts (2)
NULL_SENTINEL(46-46)EMPTY_ARRAY_SENTINEL(47-47)
🔇 Additional comments (16)
packages/core/src/v3/index.ts (1)
47-47: LGTM: Clean addition of the empty array sentinel exportThe new
EMPTY_ARRAY_SENTINELexport has been properly added to the existing exports from theflattenAttributes.jsmodule, keeping consistent grouping with the relatedNULL_SENTINELexport.apps/webapp/app/v3/eventRepository.server.ts (2)
8-8: LGTM: Properly imported the empty array sentinel constantThe
EMPTY_ARRAY_SENTINELimport has been added in the appropriate location alongside the relatedNULL_SENTINELconstant.
1597-1599: LGTM: Correctly implemented empty array deserializationThe conditional check for
EMPTY_ARRAY_SENTINELhas been properly added to therehydrateJsonfunction, maintaining consistent structure with the existing null value handling. This addition enables the function to correctly deserialize empty arrays.apps/webapp/app/utils/taskEvent.ts (2)
8-8: LGTM: Properly imported the empty array sentinel constantThe
EMPTY_ARRAY_SENTINELimport has been added in the appropriate location alongside the relatedNULL_SENTINELconstant.
450-452: LGTM: Correctly implemented empty array deserializationThe conditional check for
EMPTY_ARRAY_SENTINELhas been properly added to therehydrateJsonfunction, maintaining consistent structure with the existing null value handling. This implementation ensures empty arrays are properly deserialized, matching the implementation in theeventRepository.server.tsfile.packages/core/test/flattenAttributes.test.ts (5)
64-70: LGTM: Good test coverage for empty array serializationThis test case properly verifies that an empty array is correctly flattened to the
EMPTY_ARRAY_SENTINELvalue and then successfully unflattened back to an empty array.
72-78: LGTM: Good test coverage for nested empty array serializationThis test case properly verifies that an object containing an empty array property is correctly flattened and unflattened, maintaining the empty array structure.
176-186: LGTM: Improved formatting in circular reference testThe quotation marks have been removed from object keys in the expected result, which aligns with the standard JavaScript object literal syntax and improves readability.
193-196: LGTM: Improved formatting in nested circular reference testSimilar to the previous test, quotation marks have been removed from object keys in the expected result for better readability and consistency.
266-278: LGTM: Great test coverage for empty array deserialization casesThis test thoroughly validates the unflattening of empty arrays in various contexts:
- A root-level empty array
- An object property that is an empty array
- A nested empty array within another array
All cases are correctly deserialized from their sentinel representations.
packages/core/src/v3/utils/flattenAttributes.ts (6)
5-5: LGTM!Good addition of a constant to represent empty arrays during serialization, consistent with the existing pattern for NULL_SENTINEL.
112-112: LGTM!Updated to use the renamed function consistently.
164-164: LGTM!Updated to use the renamed function consistently.
215-215: LGTM!Good rename to better describe the expanded role of this function in handling both null values and empty arrays.
220-222: LGTM!Properly implements the "rehydration" of empty arrays from their sentinel value, following the same pattern as the null handling.
77-79:❓ Verification inconclusive
LGTM!
This correctly handles empty arrays encountered during iteration. However, due to the issue with the early return for all arrays, this code would never execute for empty arrays.
🏁 Script executed:
#!/bin/bash # Check if there are any tests demonstrating arrays with elements being properly preserved rg -A 15 -B 15 "flattenAttributes.*\[\]" packages/core/testLength of output: 61
Action Required: Confirm Early Return Behavior for Empty Arrays
The block below correctly assigns
EMPTY_ARRAY_SENTINELwhen encountering an empty array:if (!value.length) { result[newPrefix] = EMPTY_ARRAY_SENTINEL; }However, it appears that an earlier return for all arrays prevents this code from ever executing—even for empty arrays. Additionally, a search for tests confirming that non-empty arrays are preserved (and that empty arrays are handled as intended) produced no output, making the actual impact of this logic unclear.
- File & Location:
packages/core/src/v3/utils/flattenAttributes.ts, Lines 77–79- Issue: The empty array handling code is unreachable because of an early return logic for arrays.
- Next Steps: Please manually verify whether the current control flow is intended. If not, consider refactoring the logic or adding tests to validate array behaviors.
|
|
This is great! Can you try and do the same for empty objects in this PR too? |
Closes #1174
✅ Checklist
Testing
Struggling to get re2 to build to properly test this, will try on another setup. I added unit testing for the serializing portion.
Changelog
This allows empty arrays to be serialized and deserialized. This is important for debugging, to properly show the difference between no value and an empty array.
Screenshots
N/a
💯
Follow ups: probably the same behavior for empty objects?
Summary by CodeRabbit
New Features
Tests