feat(DataMapper): Add XSLT file recovery and rename functionality#3214
feat(DataMapper): Add XSLT file recovery and rename functionality#3214pvillant wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR implements XSLT file renaming from the DataMapper step configuration form. It adds service methods to update metadata and step references, ensures XSLT files exist at initialization, and provides a complete rename workflow in the launcher UI with validation and file management. ChangesXSLT file rename and edit workflow
Sequence DiagramsequenceDiagram
participant User
participant LauncherUI
participant MetadataAPI
participant StepService
participant ResourceAPI
participant Router
User->>LauncherUI: enter new XSLT filename
LauncherUI->>LauncherUI: validate filename (\.xsl pattern)
User->>LauncherUI: click Configure button
LauncherUI->>MetadataAPI: load metadata entry
LauncherUI->>ResourceAPI: fetch old XSLT content
LauncherUI->>ResourceAPI: save content to new resource name
LauncherUI->>MetadataAPI: updateXsltPath (new path)
LauncherUI->>StepService: updateXsltFileName (new filename)
LauncherUI->>ResourceAPI: delete old resource
LauncherUI->>Router: navigate to DataMapper route
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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@@ Coverage Diff @@
## main #3214 +/- ##
==========================================
+ Coverage 92.25% 92.27% +0.01%
==========================================
Files 636 636
Lines 24614 24673 +59
Branches 5639 5652 +13
==========================================
+ Hits 22707 22766 +59
Misses 1905 1905
Partials 2 2 ☔ 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/DataMapper/DataMapper.tsx`:
- Around line 61-63: Change the recovery condition so an existing empty XSLT
file isn't overwritten: in DataMapper.tsx check specifically for xsltContent ===
undefined (not a falsy check) before calling
ctx.saveResourceContent(meta.xsltPath, EMPTY_XSL), so only truly missing
resources are initialized; keep references to xsltContent,
ctx.saveResourceContent, meta.xsltPath and EMPTY_XSL to locate the code.
In `@packages/ui/src/components/DataMapper/DataMapperLauncher.test.tsx`:
- Around line 111-116: The test's jest.spyOn(console, 'error') mock
implementation calls console.error inside itself causing infinite recursion; fix
by capturing the original console.error before mocking (e.g., const
originalConsoleError = console.error), then replace the mockImplementation on
the jest.spyOn in DataMapperLauncher.test.tsx to check the 'not wrapped in act'
substring and return for those, otherwise call originalConsoleError with the
original arguments; ensure any teardown restores the mock if needed.
In `@packages/ui/src/components/DataMapper/DataMapperLauncher.tsx`:
- Around line 125-129: The rename flow unconditionally writes to newPath which
can silently overwrite an existing mapping file; before calling
metadata.saveResourceContent(newPath, content) check whether the target exists
(e.g., via metadata.hasResource/newResourceExists/getResource or equivalent) and
if it does, abort the operation (throw an error or return a user-facing
validation failure) instead of writing; only proceed to call
metadata.saveResourceContent(newPath, content),
DataMapperMetadataService.updateXsltPath(metadata, metadataId, meta, newPath),
DataMapperStepService.updateXsltFileName(vizNode, newPath, entitiesContext), and
metadata.deleteResource(oldPath) when the path is confirmed absent (or after
safely generating a unique non-conflicting newPath).
In `@packages/ui/src/services/datamapper-step.service.ts`:
- Around line 129-131: The code assumes xsltStep.to is an object and directly
sets xsltStep.to.uri which can throw if to is a string; mirror the defensive
pattern used in setUseJsonBody by checking that xsltStep.to exists and is an
object (e.g., typeof xsltStep.to === 'object' && xsltStep.to !== null) before
assigning xsltStep.to.uri = `${XSLT_COMPONENT_NAME}:${newFileName}`, then call
vizNode.updateModel(model) as before; ensure you do not mutate non-object to
values and handle the absent/invalid case appropriately.
🪄 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: 54292916-243d-4bad-bc18-f7d4ffeceb44
📒 Files selected for processing (8)
packages/ui/src/components/DataMapper/DataMapper.test.tsxpackages/ui/src/components/DataMapper/DataMapper.tsxpackages/ui/src/components/DataMapper/DataMapperLauncher.test.tsxpackages/ui/src/components/DataMapper/DataMapperLauncher.tsxpackages/ui/src/services/datamapper-metadata.service.test.tspackages/ui/src/services/datamapper-metadata.service.tspackages/ui/src/services/datamapper-step.service.test.tspackages/ui/src/services/datamapper-step.service.ts
| if (!xsltContent) { | ||
| await ctx.saveResourceContent(meta.xsltPath, EMPTY_XSL); | ||
| } |
There was a problem hiding this comment.
Only recover when the XSLT resource is actually missing
Line 61 uses a falsy check, so an existing empty file ('') is treated as missing and gets overwritten. Recovery should trigger only when content is undefined.
Proposed fix
- if (!xsltContent) {
+ if (xsltContent === undefined) {
await ctx.saveResourceContent(meta.xsltPath, EMPTY_XSL);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!xsltContent) { | |
| await ctx.saveResourceContent(meta.xsltPath, EMPTY_XSL); | |
| } | |
| if (xsltContent === undefined) { | |
| await ctx.saveResourceContent(meta.xsltPath, EMPTY_XSL); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/DataMapper/DataMapper.tsx` around lines 61 - 63,
Change the recovery condition so an existing empty XSLT file isn't overwritten:
in DataMapper.tsx check specifically for xsltContent === undefined (not a falsy
check) before calling ctx.saveResourceContent(meta.xsltPath, EMPTY_XSL), so only
truly missing resources are initialized; keep references to xsltContent,
ctx.saveResourceContent, meta.xsltPath and EMPTY_XSL to locate the code.
There was a problem hiding this comment.
IMO empty XSLT file is anyway invalid and we would want to initialize with the template
There was a problem hiding this comment.
@igarashitm, that's a fair point. An empty XSLT file ('') is invalid XML and would break the DataMapper just as much as a missing file, so overwriting it with EMPTY_XSL (the baseline valid template) is the right recovery behaviour. The existing falsy check correctly handles both cases. I'll withdraw the suggestion.
✏️ Learnings added
Learnt from: igarashitm
Repo: KaotoIO/kaoto PR: 3214
File: packages/ui/src/components/DataMapper/DataMapper.tsx:61-63
Timestamp: 2026-05-14T13:59:40.259Z
Learning: In KaotoIO/kaoto DataMapper (`packages/ui/src/components/DataMapper/DataMapper.tsx`), the XSLT recovery check intentionally uses a falsy guard (`!xsltContent`) rather than `=== undefined`. An empty XSLT string is considered equally invalid as a missing file, so both cases should be recovered by writing `EMPTY_XSL` (the baseline valid template) via `ctx.saveResourceContent`. Do not flag this as a bug or suggest changing to a strict `undefined` check.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: PVinaches
Repo: KaotoIO/kaoto PR: 3025
File: packages/ui/src/components/ResizableSplitPanels/SplitPanel.tsx:3-25
Timestamp: 2026-03-10T14:42:13.009Z
Learning: In KaotoIO/kaoto, the UI package is migrating from PatternFly to Carbon (carbon/react, carbon/icons-react). During this migration, both design systems may coexist in packages/ui. Do not flag or block Carbon imports as violations within packages/ui/src/components. Mixing Carbon and PatternFly is expected until the migration is complete. When reviewing files in packages/ui, allow Carbon imports in TSX components; continue to flag true PatternFly-only conventions in other areas or outside this migration scope.
There was a problem hiding this comment.
Still need to adjust the code here due to Sonar issue. Something like:
if (xsltContent) {
return;
}
await ctx.saveResourceContent(meta.xsltPath, EMPTY_XSL);There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| jest.spyOn(console, 'error').mockImplementation((message) => { | ||
| if (typeof message === 'string' && message.includes('not wrapped in act')) { | ||
| return; | ||
| } | ||
| console.error(message); | ||
| }); |
There was a problem hiding this comment.
Avoid recursive console.error mock implementation
The mocked console.error calls console.error again on Line 115, which re-enters the same mock and can recurse indefinitely for non-act() messages.
Proposed fix
beforeEach(() => {
jest.clearAllMocks();
+ const originalConsoleError = console.error;
// Suppress act() warnings for async useEffect in component
jest.spyOn(console, 'error').mockImplementation((message) => {
if (typeof message === 'string' && message.includes('not wrapped in act')) {
return;
}
- console.error(message);
+ originalConsoleError(message);
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest.spyOn(console, 'error').mockImplementation((message) => { | |
| if (typeof message === 'string' && message.includes('not wrapped in act')) { | |
| return; | |
| } | |
| console.error(message); | |
| }); | |
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| const originalConsoleError = console.error; | |
| // Suppress act() warnings for async useEffect in component | |
| jest.spyOn(console, 'error').mockImplementation((message) => { | |
| if (typeof message === 'string' && message.includes('not wrapped in act')) { | |
| return; | |
| } | |
| originalConsoleError(message); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/DataMapper/DataMapperLauncher.test.tsx` around
lines 111 - 116, The test's jest.spyOn(console, 'error') mock implementation
calls console.error inside itself causing infinite recursion; fix by capturing
the original console.error before mocking (e.g., const originalConsoleError =
console.error), then replace the mockImplementation on the jest.spyOn in
DataMapperLauncher.test.tsx to check the 'not wrapped in act' substring and
return for those, otherwise call originalConsoleError with the original
arguments; ensure any teardown restores the mock if needed.
| await metadata.saveResourceContent(newPath, content); | ||
| await DataMapperMetadataService.updateXsltPath(metadata, metadataId, meta, newPath); | ||
| DataMapperStepService.updateXsltFileName(vizNode, newPath, entitiesContext); | ||
| await metadata.deleteResource(oldPath); | ||
| } |
There was a problem hiding this comment.
Prevent overwrite when the target XSLT filename already exists
The rename flow writes to newPath unconditionally on Line 125. If that file already exists, this can overwrite another mapping file silently.
Proposed fix
if (meta) {
const content = await metadata.getResourceContent(oldPath);
if (content !== undefined) {
+ const targetContent = await metadata.getResourceContent(newPath);
+ if (targetContent !== undefined) {
+ // TODO: surface a user-facing error/notification
+ return;
+ }
await metadata.saveResourceContent(newPath, content);
await DataMapperMetadataService.updateXsltPath(metadata, metadataId, meta, newPath);
DataMapperStepService.updateXsltFileName(vizNode, newPath, entitiesContext);
await metadata.deleteResource(oldPath);
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/DataMapper/DataMapperLauncher.tsx` around lines
125 - 129, The rename flow unconditionally writes to newPath which can silently
overwrite an existing mapping file; before calling
metadata.saveResourceContent(newPath, content) check whether the target exists
(e.g., via metadata.hasResource/newResourceExists/getResource or equivalent) and
if it does, abort the operation (throw an error or return a user-facing
validation failure) instead of writing; only proceed to call
metadata.saveResourceContent(newPath, content),
DataMapperMetadataService.updateXsltPath(metadata, metadataId, meta, newPath),
DataMapperStepService.updateXsltFileName(vizNode, newPath, entitiesContext), and
metadata.deleteResource(oldPath) when the path is confirmed absent (or after
safely generating a unique non-conflicting newPath).
There was a problem hiding this comment.
@igarashitm WDYT, is this a valid point?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yes this is a valid point, I wonder if we can use something like "inline edit" here https://www.patternfly.org/components/inline-edit#single-inline-edit-active
And disable the "save" button when the file already exists with showing an error message - if api.getResourceContent() isn't fast enough for that validation, we could think about adding api.isResourceExist()
| if (xsltStep?.to) { | ||
| xsltStep.to.uri = `${XSLT_COMPONENT_NAME}:${newFileName}`; | ||
| vizNode.updateModel(model); |
There was a problem hiding this comment.
Guard xsltStep.to shape before writing uri
Line 130 can throw when to is present but not an object (e.g., string form). Please mirror the defensive check used in setUseJsonBody.
Proposed fix
const model = vizNode.getNodeDefinition();
const xsltStep = (model.steps as ProcessorDefinition[]).find(isXSLTComponent);
- if (xsltStep?.to) {
+ if (xsltStep?.to && typeof xsltStep.to === 'object') {
xsltStep.to.uri = `${XSLT_COMPONENT_NAME}:${newFileName}`;
vizNode.updateModel(model);
entitiesContext.updateSourceCodeFromEntities();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (xsltStep?.to) { | |
| xsltStep.to.uri = `${XSLT_COMPONENT_NAME}:${newFileName}`; | |
| vizNode.updateModel(model); | |
| const model = vizNode.getNodeDefinition(); | |
| const xsltStep = (model.steps as ProcessorDefinition[]).find(isXSLTComponent); | |
| if (xsltStep?.to && typeof xsltStep.to === 'object') { | |
| xsltStep.to.uri = `${XSLT_COMPONENT_NAME}:${newFileName}`; | |
| vizNode.updateModel(model); | |
| entitiesContext.updateSourceCodeFromEntities(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/services/datamapper-step.service.ts` around lines 129 - 131,
The code assumes xsltStep.to is an object and directly sets xsltStep.to.uri
which can throw if to is a string; mirror the defensive pattern used in
setUseJsonBody by checking that xsltStep.to exists and is an object (e.g.,
typeof xsltStep.to === 'object' && xsltStep.to !== null) before assigning
xsltStep.to.uri = `${XSLT_COMPONENT_NAME}:${newFileName}`, then call
vizNode.updateModel(model) as before; ensure you do not mutate non-object to
values and handle the absent/invalid case appropriately.
| checkFile(); | ||
| }, [xsltDocumentName, metadata]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This useEffect() could be removed by [localXsltDocumentName, setLocalXsltDocumentName] = useState<string>(xsltDocumentName), no?
| * - disallow invalid filename characters | ||
| */ | ||
| // eslint-disable-next-line no-control-regex | ||
| const xsltFileNameRegex = /^(?!\.)([^<>:"/\\|?*\x00-\x1F]+)\.(xsl)$/i; |
There was a problem hiding this comment.
This could be moved up and make it module level const to avoid regex generation on every function call
2 participants
Resolves: #1847
Summary
Enhanced DataMapper to automatically recover missing XSLT files and added the ability to rename XSLT documents directly from the launcher interface.
Changes
1. XSLT File Recovery (
DataMapper.tsx)2. XSLT Document Renaming (
DataMapperLauncher.tsx).xsland be a valid filename)3. Service Layer Enhancements
DataMapperMetadataService: AddedupdateXsltPath()method to update XSLT path in metadataDataMapperStepService: AddedupdateXsltFileName()method to update XSLT filename in step URI4. Test Coverage
Benefits
Summary by CodeRabbit
New Features
Bug Fixes
Tests