fix: off-by-one error in array optional cleanup #26
Conversation
WalkthroughThis PR clears entries in Changes
Sequence Diagram(s)(omitted — changes are internal cleanup and tests; no new multi-component control flow warranting a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
472-484: Correct fix for the off-by-one error.The index now correctly references
optionalsInArray[i]for the current array nesting level. This matches the pattern inhandleRecordand resolves the same off-by-one bug.Note: The static analysis tool suggests wrapping the
const optionalsdeclaration in a block to scope it to this switch clause. While valid, this is a minor refinement separate from the critical bug fix.🔎 Optional: wrap declaration in block per static analysis
case 'array': + { // ... existing code ... const optionals = instruction.optionalsInArray[i] if (optionals) { // optional index for (let oi = 0; oi < optionals.length; oi++) { // since pointer is checked in object case with ternary as undefined, this is not need // const pointer = `ar${i}p.${optionals[oi]}` const target = `ar${i}v[i]${optionals[oi]}` // we can add semi-colon here because it delimit recursive mirror v += `;if(${target}===undefined)delete ${target}` } } v += `}` if (!isRoot) v += `return ar${i}v})(${property})` break + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.tstest/array-nested-optionals.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/array-nested-optionals.test.ts (1)
src/index.ts (1)
createMirror(537-594)
🪛 Biome (2.1.2)
src/index.ts
[error] 472-473: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (2)
test/array-nested-optionals.test.ts (2)
7-63: Excellent regression test coverage.This test comprehensively validates the fix for the off-by-one bug with a realistic scenario that includes:
- Nested arrays with optional properties
- Union types (including null)
- Transform operations (Date handling)
- Extra property removal at nested levels
The assertions confirm both data preservation and proper cleanup, directly addressing the issue described in #1623.
65-104: Strong validation of multi-level nesting.This test ensures the fix works correctly across multiple nesting levels (outer → middle → inner arrays), confirming that
optionalsInArray[i]correctly tracks and removes extra properties at each level without affecting valid data.
src/index.ts
Outdated
| `ar${i}v[ar${i}s[i]]=${mirror(child, `ar${i}p`, instruction)}` | ||
|
|
||
| const optionals = instruction.optionalsInArray[i + 1] | ||
| const optionals = instruction.optionalsInArray[i] |
There was a problem hiding this comment.
Correct fix for the off-by-one error.
The index now correctly references optionalsInArray[i] for the current array nesting level, matching the captured index before incrementing instruction.array. This resolves the bug where i + 1 would incorrectly access optionals from the next nested level.
🤖 Prompt for AI Agents
In src/index.ts around line 122, the code currently references the wrong nesting
level when reading optionals (it used optionalsInArray[i + 1]); change that
reference to optionalsInArray[i] so it uses the captured index for the current
array nesting level (i) rather than the next level, ensuring the index is used
before any increment to instruction.array.
The issue occurred when processing sibling arrays at the same nesting level (e.g., 'pours' and 'tags' both inside data items). After processing one array's items, its optionals remained in optionalsInArray and were incorrectly applied to the next sibling array, causing cleanup code to try deleting non-existent properties. Solution: Clear optionalsInArray[i + 1] after using it to generate cleanup code, ensuring each array only processes its own optionals.
MarcelOlsen
force-pushed
the
fix/array-optional-cleanup
branch
from
7581081 to
ae57409
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/array-nested-optionals.test.ts (2)
7-63: Consider verifying the Transform behavior.The test validates array preservation and property removal correctly. However, the
Type.Transformat lines 29-33 defines bothDecodeandEncodefunctions, but the test assertions don't verify that the transformation actually occurred on thecreatedAtfield. Consider adding an assertion to confirm the date handling works as expected.🔎 Optional assertion to verify transform behavior
Add after line 59:
expect(result.data[0].tags[0].name).toBe('test') +expect(result.data[0].createdAt).toBeInstanceOf(Date) +expect(result.data[0].createdAt.toISOString()).toBe('2025-01-01T00:00:00.000Z')
65-104: Test suite name may not fully align with test content.The test correctly validates nested array handling and property removal across multiple nesting levels. However, the describe block is titled "Nested Array with Optional Properties," while this specific test doesn't exercise optional properties (no
Type.Optional,Type.Union([Type.Null(), ...]), etc.). The first test does include optional-like patterns, but this one focuses purely on nested arrays without optionals.Consider either adding optional properties to this test or clarifying that the suite covers both optional and non-optional nested array scenarios.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.tstest/array-nested-optionals.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/array-nested-optionals.test.ts (1)
src/index.ts (1)
createMirror(545-602)
🔇 Additional comments (1)
test/array-nested-optionals.test.ts (1)
1-5: LGTM!The imports are correct and properly structured for testing the mirror cleaning logic with TypeBox schemas.
1 participant
potential fix for the nested array validation issue reported in elysiajs/elysia#1631
the problem was that
optionalsInArraycleanup code was bleeding into sibling structures. when processing sibling properties (like an Object and a Record at the same level), the cleanup state from one would pollute the other, causing it to try accessing object properties on Record values.fixed by clearing the
optionalsInArraystate after use in line 130:this prevents pollution across sibling arrays/records. tested with the minimal reproduction from the issue and it works now - returns 200 with data instead of the validation error
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.