v4: metadata collapsing now preserves operation order for correctness #2115
Conversation
🦋 Changeset detectedLatest commit: d654e74 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis change refactors the logic for collapsing metadata operations within the system. The core function responsible for collapsing operations now processes the operations array sequentially, collapsing only consecutive operations of the same type and key, rather than aggregating all operations by key regardless of their order. Test cases were updated to reflect this new order-preserving collapse behavior, expecting more granular, ordered operation sequences instead of fully merged operations. Additionally, a changeset entry documents this patch, indicating a fix to the correctness of metadata collapsing. No exported or public interfaces were altered. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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/runMetadata/operations.ts (1)
189-196: Consider refactoring to reduce code duplication and improve type safety.The pattern for checking consecutive operations is repeated three times with similar structure. Additionally, the type assertions could be cleaner.
Consider extracting a helper function to check consecutive operations:
+function isConsecutiveSameOperation<T extends RunMetadataChangeOperation>( + operations: RunMetadataChangeOperation[], + index: number, + type: T['type'], + key: string +): operations is T[] { + const op = operations[index]; + return op?.type === type && (op as T).key === key; +}Then simplify the conditions:
- while ( - j < operations.length && - operations[j]?.type === "increment" && - (operations[j] as typeof op)?.key === op.key - ) { - sum += (operations[j] as typeof op).value; + while ( + j < operations.length && + isConsecutiveSameOperation<typeof op>(operations, j, "increment", op.key) + ) { + sum += operations[j].value;This would improve type safety and reduce repetition across all three collapsible operation types.
Also applies to: 206-213, 222-228
.changeset/nasty-cobras-wonder.md (1)
5-5: Consider expanding the changeset description for better clarity.The current description is too brief. Users would benefit from understanding what changed and why it matters.
Consider a more descriptive changeset:
-Fix metadata collapsing correctness +Fix metadata collapsing to preserve operation order for correctness + +Previously, metadata operations were collapsed globally by key, which could produce incorrect results when operations depended on order. Now, only consecutive operations of the same type and key are collapsed, preserving the execution order and ensuring correct final metadata state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/nasty-cobras-wonder.md(1 hunks)packages/core/src/v3/runMetadata/operations.ts(1 hunks)packages/core/test/standardMetadataManager.test.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/src/v3/runMetadata/operations.ts (1)
packages/core/src/v3/schemas/common.ts (2)
RunMetadataChangeOperation(53-60)RunMetadataChangeOperation(62-62)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: units / webapp / 📊 Merge Reports
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
🔇 Additional comments (2)
packages/core/src/v3/runMetadata/operations.ts (1)
176-240: Well-implemented order-preserving collapse logic!The sequential scan approach correctly preserves operation order while collapsing only consecutive operations of the same type and key. This ensures correctness by maintaining the relative order of operations that affect the final metadata state.
packages/core/test/standardMetadataManager.test.ts (1)
398-540: Excellent test coverage for the order-preserving collapse behavior!The updated test expectations correctly reflect the new behavior where operations are only collapsed when consecutive. The comprehensive test suite includes:
- Validation that non-consecutive operations are preserved
- Sanity checks ensuring collapsed operations produce identical results
- Edge cases covering various data types and scenarios
No description provided.