chore(DataMapper): Add ForEachGroupItem and GroupingStrategy#3154
chore(DataMapper): Add ForEachGroupItem and GroupingStrategy#3154lordrip merged 1 commit intoKaotoIO:mainfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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/ui/src/models/datamapper/mapping.ts (1)
263-274: Optional: extract the sharedsortItemsclone logic.
doClone()here and inForEachItem(lines 217-226) duplicate identicalsortItems.map(...)logic. A small helper (e.g.,cloneSortItems(sortItems: SortItem[])) would remove the duplication and reduce drift risk ifSortItemgrows fields.♻️ Proposed refactor
+const cloneSortItems = (sortItems: SortItem[]): SortItem[] => + sortItems.map((sort) => ({ expression: sort.expression, order: sort.order }) as SortItem); + export class ForEachItem extends InstructionItem implements IExpressionHolder { ... doClone() { const cloned = new ForEachItem(this.parent); - cloned.sortItems = this.sortItems.map((sort) => { - return { - expression: sort.expression, - order: sort.order, - } as SortItem; - }); + cloned.sortItems = cloneSortItems(this.sortItems); return cloned; } ... export class ForEachGroupItem extends InstructionItem implements IExpressionHolder { ... doClone() { const cloned = new ForEachGroupItem(this.parent); - cloned.sortItems = this.sortItems.map((sort) => { - return { - expression: sort.expression, - order: sort.order, - } as SortItem; - }); + cloned.sortItems = cloneSortItems(this.sortItems); return cloned; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/datamapper/mapping.ts` around lines 263 - 274, The two doClone implementations in ForEachGroupItem and ForEachItem duplicate the same sortItems.map(...) cloning logic; extract that into a small helper function (e.g., cloneSortItems(sortItems: SortItem[]): SortItem[]) that returns a deep copy of each SortItem, then replace the inline map in ForEachGroupItem.doClone and ForEachItem.doClone to call cloneSortItems(this.sortItems) to remove duplication and centralize future changes to SortItem cloning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/models/datamapper/mapping.test.ts`:
- Around line 199-211: The current test for ForEachGroupItem.contextPath only
compares firstCall.contextPath to secondCall.contextPath which can be the same
MappingTree reference and thus masks mutations; update the test to assert on the
specific PathExpression fields that extractContextPath populates (e.g.,
pathSegments, isRelative, documentReferenceName) for both firstCall and
secondCall to ensure the returned PathExpression objects are independent and
stable across calls, and apply the same replacement to the analogous ForEachItem
test that mirrors this block; locate the contextPath getter usage on the
ForEachGroupItem and ForEachItem tests and replace the weak toEqual check with
explicit assertions on those PathExpression fields.
---
Nitpick comments:
In `@packages/ui/src/models/datamapper/mapping.ts`:
- Around line 263-274: The two doClone implementations in ForEachGroupItem and
ForEachItem duplicate the same sortItems.map(...) cloning logic; extract that
into a small helper function (e.g., cloneSortItems(sortItems: SortItem[]):
SortItem[]) that returns a deep copy of each SortItem, then replace the inline
map in ForEachGroupItem.doClone and ForEachItem.doClone to call
cloneSortItems(this.sortItems) to remove duplication and centralize future
changes to SortItem cloning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4f421f4-d1b6-4f05-b66e-9777c25407df
📒 Files selected for processing (2)
packages/ui/src/models/datamapper/mapping.test.tspackages/ui/src/models/datamapper/mapping.ts
|
2 participants
Fixes: #2861
Summary by CodeRabbit
New Features
Tests