fix(DataMapper): Preserve Unknown mapping item#3042
Merged
igarashitm merged 1 commit intoKaotoIO:mainfrom Mar 18, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces support for handling unsupported XSLT elements in the DataMapper. It creates new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
igarashitm
force-pushed
the
2716-unknown
branch
from
b82b3d5 to
6724c78
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
packages/ui/src/services/mapping-serializer.service.ts (1)
146-152:⚠️ Potential issue | 🟠 MajorUnknown items may be reordered during serialization, changing XSLT behavior.
After Line 147 introduces unknown mappings into serialization, they still pass through global sort (Line 74 / Line 151). That can move unknown instructions relative to siblings, violating “preserve and write back as-is.”
🛠️ Suggested safeguard in sort comparator
private static sortMappingItem(left: MappingItem, right: MappingItem) { + if (left instanceof UnknownMappingItem || right instanceof UnknownMappingItem) { + // keep insertion order for unknown fragments + return 0; + } const leftFields = left instanceof FieldItem ? [left.field] : MappingService.getConditionalFields(left as ConditionItem);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/services/mapping-serializer.service.ts` around lines 146 - 152, The global sort (MappingSerializerService.sortMappingItem) is reordering UnknownMappingItem instances introduced by MappingSerializerService.populateUnknownMappingItem, violating the "preserve and write back as-is" requirement; update the sort comparator (sortMappingItem) to detect when either operand is an UnknownMappingItem (or both) and return 0 to preserve original sibling order for unknown items (i.e., do not change relative order of UnknownMappingItem vs siblings), then continue calling MappingSerializerService.populateMapping as before for mapping.children so unknown items are written back unchanged.
🧹 Nitpick comments (1)
packages/ui/src/services/mapping-serializer.service.test.ts (1)
405-420: Round-trip test should assert unknown subtree preservation, not only@select.Line 419 currently checks one attribute, but the objective is preserving the full unknown fragment.
✅ Suggested test enhancement
it('should round-trip UnknownMappingItem verbatim', () => { @@ const applyTemplates = xsltDocument @@ .iterateNext(); expect(applyTemplates?.nodeValue).toEqual('/ns0:ShipOrder/Item'); + + const sort = xsltDocument + .evaluate( + '/xsl:stylesheet/xsl:template/ShipOrder/xsl:apply-templates/xsl:sort/@select', + xsltDocument, + null, + XPathResult.ORDERED_NODE_ITERATOR_TYPE, + ) + .iterateNext(); + expect(sort?.nodeValue).toEqual('Title'); + + const withParam = xsltDocument + .evaluate( + '/xsl:stylesheet/xsl:template/ShipOrder/xsl:apply-templates/xsl:with-param[`@name`="prefix"]/@select', + xsltDocument, + null, + XPathResult.ORDERED_NODE_ITERATOR_TYPE, + ) + .iterateNext(); + expect(withParam?.nodeValue).toEqual("'Item:'"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/services/mapping-serializer.service.test.ts` around lines 405 - 420, The test 'should round-trip UnknownMappingItem verbatim' currently only asserts the `@select` attribute value; instead update the test to assert that the entire unknown subtree/fragment is preserved after deserialize+serialize: parse the serialized XSLT (as done) and locate the whole xsl:apply-templates element (or its parent unknown fragment) returned by MappingSerializerService.serialize, then compare its serialized XML text (or inner XML) against the original unknown fragment from getUnknownApplyTemplateXslt(); ensure you still use getUnknownApplyTemplateXslt(), MappingSerializerService.deserialize and MappingSerializerService.serialize and compare the full node content (not just applyTemplates?.nodeValue) to confirm subtree preservation.
🤖 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/services/visualization.service.ts`:
- Around line 179-183: The generatePrimitiveDocumentChildren routine creates
child nodes using new MappingNodeData directly, which bypasses the
unknown-mapping handling added in createNodeDataFromMappingItem; update
generatePrimitiveDocumentChildren to call createNodeDataFromMappingItem(parent,
mapping) for each mapping so FieldItem, UnknownMappingItem and other MappingItem
types produce the correct FieldItemNodeData, UnknownMappingNodeData or
MappingNodeData instances (ensuring it uses the existing
createNodeDataFromMappingItem helper instead of constructing MappingNodeData
directly).
---
Outside diff comments:
In `@packages/ui/src/services/mapping-serializer.service.ts`:
- Around line 146-152: The global sort
(MappingSerializerService.sortMappingItem) is reordering UnknownMappingItem
instances introduced by MappingSerializerService.populateUnknownMappingItem,
violating the "preserve and write back as-is" requirement; update the sort
comparator (sortMappingItem) to detect when either operand is an
UnknownMappingItem (or both) and return 0 to preserve original sibling order for
unknown items (i.e., do not change relative order of UnknownMappingItem vs
siblings), then continue calling MappingSerializerService.populateMapping as
before for mapping.children so unknown items are written back unchanged.
---
Nitpick comments:
In `@packages/ui/src/services/mapping-serializer.service.test.ts`:
- Around line 405-420: The test 'should round-trip UnknownMappingItem verbatim'
currently only asserts the `@select` attribute value; instead update the test to
assert that the entire unknown subtree/fragment is preserved after
deserialize+serialize: parse the serialized XSLT (as done) and locate the whole
xsl:apply-templates element (or its parent unknown fragment) returned by
MappingSerializerService.serialize, then compare its serialized XML text (or
inner XML) against the original unknown fragment from
getUnknownApplyTemplateXslt(); ensure you still use
getUnknownApplyTemplateXslt(), MappingSerializerService.deserialize and
MappingSerializerService.serialize and compare the full node content (not just
applyTemplates?.nodeValue) to confirm subtree preservation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17bd3e7c-9340-44ea-b83d-9a6a369c9bb4
📒 Files selected for processing (11)
packages/ui/src/components/Document/NodeTitle.test.tsxpackages/ui/src/components/Document/NodeTitle.tsxpackages/ui/src/components/Document/TargetDocumentNode.tsxpackages/ui/src/models/datamapper/mapping.tspackages/ui/src/models/datamapper/visualization.tspackages/ui/src/services/mapping-serializer.service.test.tspackages/ui/src/services/mapping-serializer.service.tspackages/ui/src/services/visualization.service.test.tspackages/ui/src/services/visualization.service.tspackages/ui/src/stubs/datamapper/data-mapper.tspackages/ui/src/stubs/datamapper/xml/UnknownApplyTemplate.xsl
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3042 +/- ##
==========================================
+ Coverage 90.01% 90.04% +0.03%
==========================================
Files 564 565 +1
Lines 21062 21105 +43
Branches 4772 4782 +10
==========================================
+ Hits 18958 19004 +46
+ Misses 2102 2099 -3
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
igarashitm
force-pushed
the
2716-unknown
branch
from
6724c78 to
5009064
Compare
PVinaches
reviewed
Mar 18, 2026
igarashitm
force-pushed
the
2716-unknown
branch
from
5009064 to
8ec8023
Compare
Member
Author
|
Also added a header in the popover, thanks for the idea @PVinaches 👍 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/ui/src/services/mapping-serializer.service.test.ts (1)
410-452: Assert the full unknown subtree, not just the root attribute.The preservation contract here is the DOM fragment. These assertions still pass if nested unsupported children like
xsl:sortorxsl:with-paramare dropped, and the “mixed children” case only covers the already-sorted field-then-unknown order. Please compare the serializedxsl:apply-templatessubtree against the original fragment and add one reversed-order case.Based on learnings
SortItemis declared as a model object only and is not yet supported. Full support is planned for a future subtask.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/services/mapping-serializer.service.test.ts` around lines 410 - 452, Update the two tests so they assert the entire unknown xsl:apply-templates subtree is preserved (not just the `@select` attribute) and add a reversed-order mixed-children case where apply-templates appears before xsl:attribute to ensure order handling both ways; specifically, in the "should round-trip UnknownMappingItem verbatim" test (which uses getUnknownApplyTemplateXslt and MappingSerializerService.deserialize/serialize) compare the serialized xsl:apply-templates element's full XML/text content against the original fragment, and in the "should sort UnknownMappingItem after FieldItems when serializing mixed children" test (which builds mixedXslt and calls MappingSerializerService.deserialize/serialize) add a new case with the children reversed and assert that both the entire apply-templates subtree and the final children ordering match expectations.
🤖 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/components/Document/NodeTitle/UnknownMappingLabel.tsx`:
- Around line 15-29: The Label inside UnknownMappingLabel.tsx is not
keyboard-accessible so the Popover can't be opened by keyboard; make the Label
interactive by adding isClickable and an onClick that toggles/opens the Popover
(or wrap the Label in a semantic <button>), ensuring the Popover
(triggerAction="click") receives keyboard activation via Enter/Space; update the
Label props (the Label element that currently uses color="yellow"
icon={<WarningTriangleIcon />} style={{ cursor: 'pointer' }}) to include
isClickable and a handler that opens the preview so Tab + Enter/Space works for
keyboard users.
In `@packages/ui/src/services/visualization.service.ts`:
- Around line 103-112: The current predicate assigned to
filterPriorityMappingItem replaces UnknownMappingItem with ValueSelector when
parent.isPrimitive, causing unknown mappings to be dropped; update the predicate
logic in the block that builds mappings (the filter using
filterPriorityMappingItem) so that UnknownMappingItem instances are always
included and ValueSelector is included when parent.isPrimitive (e.g., predicate
returns m instanceof UnknownMappingItem || (parent.isPrimitive && m instanceof
ValueSelector)), then continue to call
VisualizationService.createNodeDataFromMappingItem(parent as TargetNodeData, m)
for each filtered mapping.
---
Nitpick comments:
In `@packages/ui/src/services/mapping-serializer.service.test.ts`:
- Around line 410-452: Update the two tests so they assert the entire unknown
xsl:apply-templates subtree is preserved (not just the `@select` attribute) and
add a reversed-order mixed-children case where apply-templates appears before
xsl:attribute to ensure order handling both ways; specifically, in the "should
round-trip UnknownMappingItem verbatim" test (which uses
getUnknownApplyTemplateXslt and MappingSerializerService.deserialize/serialize)
compare the serialized xsl:apply-templates element's full XML/text content
against the original fragment, and in the "should sort UnknownMappingItem after
FieldItems when serializing mixed children" test (which builds mixedXslt and
calls MappingSerializerService.deserialize/serialize) add a new case with the
children reversed and assert that both the entire apply-templates subtree and
the final children ordering match expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68e2c808-460d-479f-a0cb-d8b4fe3a81ee
📒 Files selected for processing (15)
packages/ui/src/components/Document/AddMappingNode.tsxpackages/ui/src/components/Document/NodeTitle/NodeTitle.scsspackages/ui/src/components/Document/NodeTitle/NodeTitle.test.tsxpackages/ui/src/components/Document/NodeTitle/NodeTitle.tsxpackages/ui/src/components/Document/NodeTitle/UnknownMappingLabel.tsxpackages/ui/src/components/Document/SourceDocumentNode.tsxpackages/ui/src/components/Document/TargetDocumentNode.tsxpackages/ui/src/models/datamapper/mapping.tspackages/ui/src/models/datamapper/visualization.tspackages/ui/src/services/mapping-serializer.service.test.tspackages/ui/src/services/mapping-serializer.service.tspackages/ui/src/services/visualization.service.test.tspackages/ui/src/services/visualization.service.tspackages/ui/src/stubs/datamapper/data-mapper.tspackages/ui/src/stubs/datamapper/xml/UnknownApplyTemplate.xsl
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ui/src/models/datamapper/visualization.ts
- packages/ui/src/components/Document/TargetDocumentNode.tsx
- packages/ui/src/stubs/datamapper/xml/UnknownApplyTemplate.xsl
lordrip
previously approved these changes
Mar 18, 2026
Member
lordrip
left a comment
lordrip
left a comment
There was a problem hiding this comment.
LGTM, @igarashitm, please confirm if the second coderabbit's issue is valid, just in case
igarashitm
force-pushed
the
2716-unknown
branch
from
8ec8023 to
8bd0310
Compare
Fixes: KaotoIO#2716 When deserializing XSLT, preserve XSLT control elements that is not recognized by DataMapper as an unknown item, keep the DOM fragment as it is so that it could be written back when DataMapper generate the XSLT on serialization
igarashitm
force-pushed
the
2716-unknown
branch
from
8bd0310 to
d57eed1
Compare
|
Member
Author
|
Thank you for the review @lordrip , fixed the coderabbitai issues |
PVinaches
approved these changes
Mar 18, 2026
This was referenced Mar 31, 2026
This was referenced Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #2716
When deserializing XSLT, preserve XSLT control elements that is not recognized by DataMapper as an unknown item, keep the DOM fragment as it is so that it could be written back when DataMapper generate the XSLT on serialization
It is shown as
unknownitem with yellow labelWhen the
unknownlabel is clicked, the unknown part of the XML snippet is shown in tooltipSummary by CodeRabbit
Release Notes