feat(Datamapper): Implement Field Type Override UI (#2732)#2962
Draft
mmelko wants to merge 6 commits intoKaotoIO:mainfrom
Draft
feat(Datamapper): Implement Field Type Override UI (#2732)#2962mmelko wants to merge 6 commits intoKaotoIO:mainfrom
mmelko wants to merge 6 commits intoKaotoIO:mainfrom
Conversation
mmelko
changed the title
mmelko
force-pushed
the
feature/field-type-override-ui-2732
branch
from
78833ba to
f6f3a0e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2962 +/- ##
==========================================
+ Coverage 89.69% 89.71% +0.02%
==========================================
Files 561 567 +6
Lines 20913 21221 +308
Branches 4889 4936 +47
==========================================
+ Hits 18758 19039 +281
- Misses 2028 2054 +26
- Partials 127 128 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mmelko
force-pushed
the
feature/field-type-override-ui-2732
branch
3 times, most recently
from
1b5b670 to
edc7995
Compare
Contributor
|
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:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds field type-override UI: a TypeOverrideModal, FieldContextMenu, context-menu actions and target/source integrations, wrench indicators on nodes, persistence via FieldTypeOverrideService, and broad test coverage for modal and menu flows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Node as Node (Source / Target)
participant Menu as FieldContextMenu
participant Modal as TypeOverrideModal
participant Service as FieldTypeOverrideService
participant Mapper as Data Mapper (useDataMapper)
User->>Node: Right-click / open context menu
Node->>Menu: Render options (Override / Reset)
User->>Menu: Select "Override Type..."
Menu->>Modal: Open with field data
Modal->>Service: getSafeOverrideCandidates(field, namespaceMap)
Service-->>Modal: Return candidates
User->>Modal: Select type
User->>Modal: Click Save
Modal->>Service: applyFieldTypeOverride(field, selectedType, namespaceMap, variant)
Service->>Mapper: updateDocument(...)
Mapper-->>Node: reloadNodeReferences / re-render
Node->>User: Show wrench indicator (type override)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
mmelko
force-pushed
the
feature/field-type-override-ui-2732
branch
5 times, most recently
from
7cf15c2 to
613b758
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: 2
🧹 Nitpick comments (6)
packages/ui/src/components/Document/actions/FieldContextMenu.tsx (1)
27-48: Use className and create SCSS file for consistent Menu styling.The inline styles bypass PatternFly theming. All other Menu components in the codebase use the className approach with SCSS files (e.g.,
xpath-editor__tab__functions-list,runtime-selector__submenu). Create a corresponding SCSS file and move styles to maintain consistency.Suggested refactor
- <Menu - style={{ - boxShadow: '0 2px 8px rgba(0, 0, 0, 0.15)', - border: '1px solid `#d2d2d2`', - borderRadius: '4px', - backgroundColor: '#fff', - }} - > + <Menu className="field-context-menu">/* FieldContextMenu.scss */ .field-context-menu { box-shadow: var(--pf-v5-global--BoxShadow--md); border: 1px solid var(--pf-v5-global--BorderColor--100); border-radius: var(--pf-v5-global--BorderRadius--sm); background-color: var(--pf-v5-global--BackgroundColor--100); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/FieldContextMenu.tsx` around lines 27 - 48, Remove the inline style object on the Menu component in FieldContextMenu.tsx and replace it with a className (e.g., "field-context-menu"); create a new SCSS file named FieldContextMenu.scss with the provided CSS rules mapping the PatternFly CSS variables (box-shadow, border, border-radius, background-color), import that SCSS at the top of FieldContextMenu.tsx, and ensure the Menu uses className="field-context-menu" so styling matches other components like xpath-editor__tab__functions-list and runtime-selector__submenu.packages/ui/src/components/Document/actions/TypeOverrideModal.tsx (3)
91-99: HandlershandleSaveandhandleRemovecan be simplified.These handlers just forward to
onSave/onRemove. While the current structure is fine, you could simplify by passing the callbacks directly to the buttons if no additional logic is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx` around lines 91 - 99, The two wrappers handleSave and handleRemove just forward to props and can be removed: delete the useCallback definitions for handleSave and handleRemove and update the JSX where they are used (e.g., the Save and Remove Button onClick props) to pass onSave and onRemove directly; ensure you keep the dependency cleanup (remove selectedType from hook deps) and only pass onSave/onRemove where selectedType check is not required or perform that check inline before calling onSave in the Button click handler if needed.
155-179: Empty label on required FormGroup.Line 155 has
label=""withisRequiredwhich doesn't render a visible label. If the field should have a label for accessibility, add one; otherwise, remove theisRequiredprop or add anaria-labelto the Select.♻️ Add proper labeling for accessibility
- <FormGroup label="" fieldId="type-select" isRequired> + <FormGroup label="New Type" fieldId="type-select" isRequired> <Select id="type-select" + aria-label="Select a new type" isOpen={isSelectOpen}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx` around lines 155 - 179, The FormGroup currently has an empty label while marked isRequired which breaks accessibility; update the FormGroup/Select to provide a visible label and accessible name: set a meaningful label string on the FormGroup (e.g., "Type" or "Document type") or, if no visible label is desired, remove isRequired and add an aria-label/aria-labelledby on the Select (used with selectedType, handleTypeSelect, renderToggle) so screen readers can identify the field; ensure the chosen label text is used consistently for FormHelperText and the Select toggle.
183-187: Consider using a CSS class instead of inline style.The inline
style={{ marginRight: 'auto' }}works but could be moved to the stylesheet for consistency with the project's styling approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx` around lines 183 - 187, Replace the inline style on the Remove Override Button with a CSS class for consistency: in the TypeOverrideModal component, remove style={{ marginRight: 'auto' }} from the Button (the one rendered when hasExistingOverride and calling handleRemove) and add a descriptive className (e.g., "removeOverride" or "mr-auto"). Then add the corresponding CSS rule in the component's stylesheet (or module) to set margin-right: auto and any related layout rules so visual behavior is identical.packages/ui/src/components/Document/actions/ConditionMenuAction.tsx (1)
110-155: Consider extracting shared type override handlers.The
handleTypeOverrideSaveandhandleTypeOverrideRemoveimplementations are nearly identical to those inTargetNodeActions.tsx. While not blocking, extracting this logic into a shared hook (e.g.,useTypeOverrideActions) would reduce duplication and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/ConditionMenuAction.tsx` around lines 110 - 155, Extract the duplicated type-override logic from ConditionMenuAction.tsx and TargetNodeActions.tsx into a shared hook named useTypeOverrideActions that exposes at least two handlers (saveTypeOverride and removeTypeOverride) and accepts the dependencies (field, isFieldNode, mappingTree.namespaceMap, updateDocument, onUpdate); inside the hook move the common steps: obtaining document via field.ownerDocument, computing previousRefId with document.getReferenceId(namespaceMap), calling FieldTypeOverrideService.applyFieldTypeOverride or .revertFieldTypeOverride, calling updateDocument(document, document.definition, previousRefId), closing modal state, and invoking onUpdate; replace the local handleTypeOverrideSave and handleTypeOverrideRemove with calls to saveTypeOverride(selectedType) and removeTypeOverride() and ensure the hook’s dependency array mirrors the original callbacks.packages/ui/src/components/Document/actions/TypeOverrideModal.test.tsx (1)
169-175: Weak assertion in type selection test.The assertion
expect(toggleWithSelection === null || screen.getAllByText('string').length > 0).toBeTruthy()will always pass because the second condition (getAllByText('string').length > 0) is true due to the option text. Consider a more specific assertion that verifies the selection state.♻️ Proposed improvement
await waitFor(() => { - // After selection, the toggle should show the selected type name - const toggleWithSelection = screen.queryByText('Select a new type...'); - // Either placeholder is gone (type selected) or still there (not selected yet) - expect(toggleWithSelection === null || screen.getAllByText('string').length > 0).toBeTruthy(); + // After selection, the placeholder should be replaced with the selected type + expect(screen.queryByRole('button', { name: 'Select a new type...' })).not.toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'string' })).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/TypeOverrideModal.test.tsx` around lines 169 - 175, The current test uses a weak OR assertion that can pass spuriously; update the assertion inside the waitFor in TypeOverrideModal.test.tsx to explicitly verify selection state by asserting the placeholder is gone and the selected type is shown: replace the conditional expect involving toggleWithSelection and screen.getAllByText('string') with two explicit checks (e.g., expect(toggleWithSelection).toBeNull() and expect(screen.getByText('string')).toBeInTheDocument()) so the test validates the placeholder is removed and the chosen "string" appears in the toggle.
🤖 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/actions/ConditionMenuAction.test.tsx`:
- Around line 265-311: The tests mutate shared fixture targetDoc by setting
field.typeOverride and field.originalType directly which can leak state; update
each test to create a fresh document/field instance (instead of reusing
module-scoped targetDoc) or reset targetDoc in a beforeEach; specifically
replace direct mutations of targetDoc.fields[0] in the two tests with a newly
constructed field/document used to create TargetFieldNodeData, keep mocks for
useDataMapper (updateDocumentMock) and onUpdateMock the same, and ensure no test
relies on prior test mutations when rendering ConditionMenuAction and asserting
updateDocument/onUpdate calls.
In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx`:
- Around line 48-77: The lookup mixes QName format with candidate keys so the
direct lookup using currentTypeString (built from field.typeQName?.toString())
never matches candidates (keyed by typeString like "xs:string"); update the
lookup to use a key format that matches candidate keys: prefer using field.type
(which matches the candidates' typeString) or normalize field.typeQName into the
candidate typeString using mappingTree.namespaceMap/prefix resolution before
doing candidates[currentKey]; if neither yields a match, keep the existing
fallback search over Object.values(candidates). Adjust code in the useEffect
(references: FieldTypeOverrideService.getSafeOverrideCandidates,
getAllOverrideCandidates, TypeOverrideVariant, setSelectedType, field.typeQName,
field.type, candidates) so the direct candidates lookup uses a consistent key
format rather than the raw QName string.
---
Nitpick comments:
In `@packages/ui/src/components/Document/actions/ConditionMenuAction.tsx`:
- Around line 110-155: Extract the duplicated type-override logic from
ConditionMenuAction.tsx and TargetNodeActions.tsx into a shared hook named
useTypeOverrideActions that exposes at least two handlers (saveTypeOverride and
removeTypeOverride) and accepts the dependencies (field, isFieldNode,
mappingTree.namespaceMap, updateDocument, onUpdate); inside the hook move the
common steps: obtaining document via field.ownerDocument, computing
previousRefId with document.getReferenceId(namespaceMap), calling
FieldTypeOverrideService.applyFieldTypeOverride or .revertFieldTypeOverride,
calling updateDocument(document, document.definition, previousRefId), closing
modal state, and invoking onUpdate; replace the local handleTypeOverrideSave and
handleTypeOverrideRemove with calls to saveTypeOverride(selectedType) and
removeTypeOverride() and ensure the hook’s dependency array mirrors the original
callbacks.
In `@packages/ui/src/components/Document/actions/FieldContextMenu.tsx`:
- Around line 27-48: Remove the inline style object on the Menu component in
FieldContextMenu.tsx and replace it with a className (e.g.,
"field-context-menu"); create a new SCSS file named FieldContextMenu.scss with
the provided CSS rules mapping the PatternFly CSS variables (box-shadow, border,
border-radius, background-color), import that SCSS at the top of
FieldContextMenu.tsx, and ensure the Menu uses className="field-context-menu" so
styling matches other components like xpath-editor__tab__functions-list and
runtime-selector__submenu.
In `@packages/ui/src/components/Document/actions/TypeOverrideModal.test.tsx`:
- Around line 169-175: The current test uses a weak OR assertion that can pass
spuriously; update the assertion inside the waitFor in
TypeOverrideModal.test.tsx to explicitly verify selection state by asserting the
placeholder is gone and the selected type is shown: replace the conditional
expect involving toggleWithSelection and screen.getAllByText('string') with two
explicit checks (e.g., expect(toggleWithSelection).toBeNull() and
expect(screen.getByText('string')).toBeInTheDocument()) so the test validates
the placeholder is removed and the chosen "string" appears in the toggle.
In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx`:
- Around line 91-99: The two wrappers handleSave and handleRemove just forward
to props and can be removed: delete the useCallback definitions for handleSave
and handleRemove and update the JSX where they are used (e.g., the Save and
Remove Button onClick props) to pass onSave and onRemove directly; ensure you
keep the dependency cleanup (remove selectedType from hook deps) and only pass
onSave/onRemove where selectedType check is not required or perform that check
inline before calling onSave in the Button click handler if needed.
- Around line 155-179: The FormGroup currently has an empty label while marked
isRequired which breaks accessibility; update the FormGroup/Select to provide a
visible label and accessible name: set a meaningful label string on the
FormGroup (e.g., "Type" or "Document type") or, if no visible label is desired,
remove isRequired and add an aria-label/aria-labelledby on the Select (used with
selectedType, handleTypeSelect, renderToggle) so screen readers can identify the
field; ensure the chosen label text is used consistently for FormHelperText and
the Select toggle.
- Around line 183-187: Replace the inline style on the Remove Override Button
with a CSS class for consistency: in the TypeOverrideModal component, remove
style={{ marginRight: 'auto' }} from the Button (the one rendered when
hasExistingOverride and calling handleRemove) and add a descriptive className
(e.g., "removeOverride" or "mr-auto"). Then add the corresponding CSS rule in
the component's stylesheet (or module) to set margin-right: auto and any related
layout rules so visual behavior is identical.
Comment on lines
+265
to
+311
| it('should show Reset override option when field has type override', () => { | ||
| const field = targetDoc.fields[0]; | ||
| field.typeOverride = TypeOverrideVariant.SAFE; | ||
| field.originalType = Types.String; | ||
|
|
||
| const nodeData = new TargetFieldNodeData(documentNodeData, field, new FieldItem(mappingTree, field)); | ||
| const onUpdateMock = jest.fn(); | ||
| render(<ConditionMenuAction nodeData={nodeData} onUpdate={onUpdateMock} />); | ||
|
|
||
| const actionToggle = screen.getByTestId('transformation-actions-menu-toggle'); | ||
| act(() => { | ||
| fireEvent.click(actionToggle); | ||
| }); | ||
|
|
||
| expect(screen.getByText('Reset override')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should call updateDocument when Reset override is clicked', () => { | ||
| const field = targetDoc.fields[0]; | ||
| field.typeOverride = TypeOverrideVariant.SAFE; | ||
| field.originalType = Types.String; | ||
|
|
||
| const nodeData = new TargetFieldNodeData(documentNodeData, field, new FieldItem(mappingTree, field)); | ||
| const onUpdateMock = jest.fn(); | ||
| const updateDocumentMock = jest.fn(); | ||
|
|
||
| const { useDataMapper } = jest.requireMock('../../../hooks/useDataMapper'); | ||
| useDataMapper.mockReturnValue({ | ||
| mappingTree, | ||
| updateDocument: updateDocumentMock, | ||
| }); | ||
|
|
||
| render(<ConditionMenuAction nodeData={nodeData} onUpdate={onUpdateMock} />); | ||
|
|
||
| const actionToggle = screen.getByTestId('transformation-actions-menu-toggle'); | ||
| act(() => { | ||
| fireEvent.click(actionToggle); | ||
| }); | ||
|
|
||
| const resetButton = screen.getByText('Reset override'); | ||
| act(() => { | ||
| fireEvent.click(resetButton); | ||
| }); | ||
|
|
||
| expect(updateDocumentMock).toHaveBeenCalled(); | ||
| expect(onUpdateMock).toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Avoid shared mutable fixtures across tests.
These tests mutate field.typeOverride on the module‑scoped targetDoc, which can leak state into later cases. Prefer recreating the document per test (or resetting in beforeEach) to keep isolation and avoid flakiness.
🧪 Example refactor (fresh fixtures per test)
- const targetDoc = TestUtil.createTargetOrderDoc();
- const mappingTree = new MappingTree(DocumentType.TARGET_BODY, BODY_DOCUMENT_ID, DocumentDefinitionType.XML_SCHEMA);
- const documentNodeData = new TargetDocumentNodeData(targetDoc, mappingTree);
+ let targetDoc: ReturnType<typeof TestUtil.createTargetOrderDoc>;
+ let mappingTree: MappingTree;
+ let documentNodeData: TargetDocumentNodeData;
beforeEach(() => {
+ targetDoc = TestUtil.createTargetOrderDoc();
+ mappingTree = new MappingTree(DocumentType.TARGET_BODY, BODY_DOCUMENT_ID, DocumentDefinitionType.XML_SCHEMA);
+ documentNodeData = new TargetDocumentNodeData(targetDoc, mappingTree);
const { useDataMapper } = jest.requireMock('../../../hooks/useDataMapper');
useDataMapper.mockReturnValue({
mappingTree,
updateDocument: jest.fn(),
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/Document/actions/ConditionMenuAction.test.tsx`
around lines 265 - 311, The tests mutate shared fixture targetDoc by setting
field.typeOverride and field.originalType directly which can leak state; update
each test to create a fresh document/field instance (instead of reusing
module-scoped targetDoc) or reset targetDoc in a beforeEach; specifically
replace direct mutations of targetDoc.fields[0] in the two tests with a newly
constructed field/document used to create TargetFieldNodeData, keep mocks for
useDataMapper (updateDocumentMock) and onUpdateMock the same, and ensure no test
relies on prior test mutations when rendering ConditionMenuAction and asserting
updateDocument/onUpdate calls.
d25aa06 to
aea6720
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
packages/ui/src/components/Document/TargetDocumentNode.tsx (1)
93-115: Add a display fallback forfield.originalTypein the indicator title.If
originalTypeis missing, the UI currently risksType overridden: undefined → .... Consider falling back tofield.originalTypeQName?.toString()or'Unknown'(same approach as the modal).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/TargetDocumentNode.tsx` around lines 93 - 115, The type override indicator title can show "undefined" when field.originalType is missing; update the useMemo that defines typeOverrideIndicator to compute a safeOriginal = field.originalType ?? field.originalTypeQName?.toString() ?? 'Unknown' and use that safeOriginal in the title string (`Type overridden: ${safeOriginal} → ${field.type}`) inside the Icon; keep the existing checks (nodeData instanceof TargetFieldNodeData | FieldItemNodeData, hasTypeOverride, !hasMapping) and only change how the original type is rendered.packages/ui/src/components/Document/actions/TypeOverrideModal.tsx (1)
97-108: Modal a11y/UX polish: add a real label for the select, reset open state, and consider sorting candidates.
FormGroup label=""makes the field effectively unlabeled; a short label like “New type” (even if visually subtle) improves accessibility.- Consider forcing
setIsSelectOpen(false)when the modal opens/closes so the select doesn’t reopen in a stale state.- Sorting by
displayNamewould make the list predictable and easier to scan/test.Also applies to: 147-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx` around lines 97 - 108, Add an accessible label to the select FormGroup by setting a short label like "New type" (even if visually hidden) instead of an empty string; ensure the select open state is reset by calling setIsSelectOpen(false) when the modal mounts/unmounts or when the modal open prop changes so it cannot remain stale; and make the candidate list predictable by sorting candidates by their displayName before rendering (affecting the list used by renderToggle / selectedType and the menu items). Locate and update the FormGroup label prop, the setIsSelectOpen usage (including where the modal open/close is handled), and the candidate array feeding renderToggle/menu rendering to add the sort by displayName.packages/ui/src/components/Document/actions/TargetNodeActions.test.tsx (1)
83-119: Test should assert the samenamespaceMapinstance that the component uses.Right now the expectation uses
tree.namespaceMapcreated in the test, while the component uses themappingTreereturned by the mockeduseDataMapper. This is likely{}today, but could diverge later and let a regression slip through.Suggested tweak: capture
mappingTreefrom the mocked hookit('should call FieldTypeOverrideService.applyFieldTypeOverride when saving type override', () => { @@ - const tree = new MappingTree(DocumentType.TARGET_BODY, BODY_DOCUMENT_ID, DocumentDefinitionType.XML_SCHEMA); + const tree = new MappingTree(DocumentType.TARGET_BODY, BODY_DOCUMENT_ID, DocumentDefinitionType.XML_SCHEMA); @@ - expect(FieldTypeOverrideService.applyFieldTypeOverride).toHaveBeenCalledWith( + const { useDataMapper } = jest.requireMock('../../../hooks/useDataMapper'); + const { mappingTree } = useDataMapper.mock.results[0].value; + + expect(FieldTypeOverrideService.applyFieldTypeOverride).toHaveBeenCalledWith( field.ownerDocument, field, mockSelectedType, - tree.namespaceMap, + mappingTree.namespaceMap, TypeOverrideVariant.SAFE, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/TargetNodeActions.test.tsx` around lines 83 - 119, The test currently asserts FieldTypeOverrideService.applyFieldTypeOverride was called with tree.namespaceMap (the local MappingTree instance) but the component actually uses the mappingTree provided by the mocked useDataMapper hook; update the test to capture the mappingTree returned by the mocked useDataMapper (or the mock's mappingTree variable) and assert that applyFieldTypeOverride was called with that mappingTree.namespaceMap instead of tree.namespaceMap, keeping the rest of the expectations (TargetNodeActions render, invoking the mocked TypeOverrideModal onSave, and checking mockUpdateDocument/mockOnUpdate) unchanged.packages/ui/src/components/Document/actions/ConditionMenuAction.tsx (1)
44-48: MakehasTypeOverrideexplicitly depend onfieldbeing present.Today it’s only used under the
isFieldNoderendering guard, but making itconst hasTypeOverride = !!field && field.typeOverride !== TypeOverrideVariant.NONE;avoids accidental truthiness if this gets refactored later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/ConditionMenuAction.tsx` around lines 44 - 48, The current hasTypeOverride uses optional chaining and could evaluate truthily if nodeData changes; update its definition so it explicitly checks field exists: when computing hasTypeOverride (used alongside isFieldNode which checks nodeData instanceof TargetFieldNodeData or FieldItemNodeData), make it depend on !!field and then compare field.typeOverride !== TypeOverrideVariant.NONE (i.e., compute hasTypeOverride as a boolean that first verifies field is present before checking TypeOverrideVariant.NONE).packages/ui/src/components/Document/actions/TargetNodeActions.tsx (1)
87-102: Indicator tooltip should use the same “type display” formatting as the modal/popover (avoidundefined/ enum-y text).
title={Type overridden: ${originalType} → ${field?.type}...}can degrade toundefined → integeror show internal enum values. Consider using the same fallback logic asTypeOverrideModal(originalTypeQName?.toString() ?? originalType ?? 'Unknown') and a human-friendly representation for the overridden type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/TargetNodeActions.tsx` around lines 87 - 102, The tooltip title for the type-override indicator uses raw values (originalType and field?.type) which can be undefined or internal enum names; update the title in the hasTypeOverride ActionListItem (where handleOpenTypeOverrideModal is used) to use the same display logic as TypeOverrideModal — e.g. use originalTypeQName?.toString() ?? originalType ?? 'Unknown' for the original and a human-friendly representation/fallback for the overridden type instead of field?.type so the tooltip shows the same formatted, readable type strings as the modal/popover.packages/ui/src/components/Document/SourceDocumentNode.test.tsx (1)
1098-1137: Indicator assertion bytitlesubstring is fragile; consider a stable hook.If feasible, expose an
aria-labelordata-testidfor the override indicator so tests don’t depend on tooltip text formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/SourceDocumentNode.test.tsx` around lines 1098 - 1137, The tests rely on a fragile title substring to find the override indicator; update the SourceDocumentNode component to render the indicator with a stable attribute (e.g., data-testid="type-override-indicator" or aria-label="type-override-indicator") and then change the two tests in SourceDocumentNode.test.tsx to query for that attribute (use getByTestId/queryByTestId or getByLabelText/queryByLabelText) instead of container.querySelector(`[title*="Type overridden"]`), keeping the existing assertions (toBeInTheDocument / not.toBeInTheDocument`) and the same tree/node setup logic.packages/ui/src/components/Document/actions/ConditionMenuAction.test.tsx (1)
241-263: Prefer clicking the element with the test id / underlying button for dropdown items.A few tests click
getByText(...)results directly; with PatternFly dropdown internals this can become brittle. Since the production items already have stabledata-testids (transformation-actions-type-override,transformation-actions-reset-override), it’s safer to click those.Also applies to: 299-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/ConditionMenuAction.test.tsx` around lines 241 - 263, The test should click the stable test-id buttons instead of text nodes to avoid PatternFly dropdown brittleness: in the ConditionMenuAction test, replace clicks on screen.getByText('Override field type') and similar getByText usages (also update the other occurrence around the 299-311 block) with screen.getByTestId('transformation-actions-type-override') for the "Override" action (and use 'transformation-actions-reset-override' where appropriate), then fireEvent.click that element; keep the same assertions for modal content (references: ConditionMenuAction component and the test ids transformation-actions-type-override / transformation-actions-reset-override).
🤖 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/actions/TypeOverrideModal.test.tsx`:
- Around line 119-175: The test "should update selected type when a type is
selected from dropdown" uses a weak assertion; replace the final waitFor
assertion with a definitive check that the selection actually updated the UI by
asserting the TypeOverrideModal toggle now displays "string" (the selected
displayName) or that the Save control becomes enabled after selection. Locate
the test's toggle variable (the button found with screen.getByRole('button', {
name: 'Select a new type...' })) and, after firing the click on the string
option, waitFor and assert that the toggle text equals "string" (or assert that
the Save button in TypeOverrideModal is enabled) to prove the selection took
effect.
- Around line 24-40: Replace jest.clearAllMocks() in the afterEach block with
jest.restoreAllMocks() (or enable restoreMocks: true in your Jest config) so
spies on FieldTypeOverrideService are restored between tests; also remove the
tautological assertion using toggleWithSelection and instead assert the selected
value directly by checking the toggleWithSelection display text equals the
expected type name or that the "Save" button is enabled to verify selection
state.
In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx`:
- Around line 48-69: Preselection currently matches candidates by the enum
field.type which can map multiple XML types to the same enum; update the
useEffect in TypeOverrideModal (after calling
FieldTypeOverrideService.getSafeOverrideCandidates / getAllOverrideCandidates
and setting typeCandidates) to prefer matching by field.typeQName: when
field.typeOverride is SAFE or FORCE, look up the candidate by the qualified name
(e.g., candidates[field.typeQName?.toString()] or
Object.values(candidates).find(c => c.typeString ===
field.typeQName?.toString())) and call setSelectedType with that candidate
(falling back to null), instead of matching by candidate.type.
In `@packages/ui/src/components/Document/SourceDocumentNode.test.tsx`:
- Around line 983-1097: The test leaves a persistent spy on
FieldTypeOverrideService.getSafeOverrideCandidates which can leak into other
tests; restore it after the assertion. Locate the test that sets
jest.spyOn(FieldTypeOverrideService,
'getSafeOverrideCandidates').mockReturnValue(mockCandidates) (inside the "should
call FieldTypeOverrideService.applyFieldTypeOverride when saving type override"
test) and call mockRestore() on that spy (e.g., store the spy in a variable or
call FieldTypeOverrideService.getSafeOverrideCandidates.mockRestore()) after the
expect(...) assertions and alongside applySpy.mockRestore() to mirror the
existing cleanup pattern.
In `@packages/ui/src/components/Document/SourceDocumentNode.tsx`:
- Around line 124-136: The onSave callback from TypeOverrideModal must be
extended to accept a TypeOverrideVariant and that variant must be forwarded to
callers instead of hardcoding TypeOverrideVariant.SAFE; update TypeOverrideModal
(handleSave/getAllOverrideCandidates) to call onSave(selectedType, variant),
change the signature of the onSave prop accordingly, and update each
caller—SourceDocumentNode.handleTypeOverrideSave, TargetNodeActions, and
ConditionMenuAction—to accept the variant parameter and pass it into
FieldTypeOverrideService.applyFieldTypeOverride (replace the hardcoded
TypeOverrideVariant.SAFE in that call with the passed-in variant) so FORCE is
used when getAllOverrideCandidates fallback occurs.
- Around line 214-253: The inner dialog's onKeyDown currently stops all keyboard
events (onKeyDown={(e) => e.stopPropagation()}), which prevents the backdrop
Escape handler from closing the menu; update the onKeyDown on the dialog
container (the element using contextMenuPosition and rendering FieldContextMenu)
to explicitly handle the Escape key by calling setShowContextMenu(false) (and
stopping propagation for that event if desired), and only stopPropagation for
other keys—i.e., if e.key === 'Escape' then close via setShowContextMenu(false),
else e.stopPropagation().
---
Duplicate comments:
In `@packages/ui/src/components/Document/actions/ConditionMenuAction.test.tsx`:
- Around line 23-33: The tests share module-scoped fixtures (targetDoc,
mappingTree, documentNodeData) causing state leakage when tests mutate fields
like field.typeOverride; move creation of TestUtil.createTargetOrderDoc(), new
MappingTree(DocumentType.TARGET_BODY,...), and new TargetDocumentNodeData(...)
into the beforeEach so each test gets fresh instances, and update the
useDataMapper.mockReturnValue call to return the newly created mappingTree and a
fresh updateDocument mock; ensure any test that previously referenced the
top-level targetDoc/documentNodeData uses the per-test instances instead.
---
Nitpick comments:
In `@packages/ui/src/components/Document/actions/ConditionMenuAction.test.tsx`:
- Around line 241-263: The test should click the stable test-id buttons instead
of text nodes to avoid PatternFly dropdown brittleness: in the
ConditionMenuAction test, replace clicks on screen.getByText('Override field
type') and similar getByText usages (also update the other occurrence around the
299-311 block) with screen.getByTestId('transformation-actions-type-override')
for the "Override" action (and use 'transformation-actions-reset-override' where
appropriate), then fireEvent.click that element; keep the same assertions for
modal content (references: ConditionMenuAction component and the test ids
transformation-actions-type-override / transformation-actions-reset-override).
In `@packages/ui/src/components/Document/actions/ConditionMenuAction.tsx`:
- Around line 44-48: The current hasTypeOverride uses optional chaining and
could evaluate truthily if nodeData changes; update its definition so it
explicitly checks field exists: when computing hasTypeOverride (used alongside
isFieldNode which checks nodeData instanceof TargetFieldNodeData or
FieldItemNodeData), make it depend on !!field and then compare
field.typeOverride !== TypeOverrideVariant.NONE (i.e., compute hasTypeOverride
as a boolean that first verifies field is present before checking
TypeOverrideVariant.NONE).
In `@packages/ui/src/components/Document/actions/TargetNodeActions.test.tsx`:
- Around line 83-119: The test currently asserts
FieldTypeOverrideService.applyFieldTypeOverride was called with
tree.namespaceMap (the local MappingTree instance) but the component actually
uses the mappingTree provided by the mocked useDataMapper hook; update the test
to capture the mappingTree returned by the mocked useDataMapper (or the mock's
mappingTree variable) and assert that applyFieldTypeOverride was called with
that mappingTree.namespaceMap instead of tree.namespaceMap, keeping the rest of
the expectations (TargetNodeActions render, invoking the mocked
TypeOverrideModal onSave, and checking mockUpdateDocument/mockOnUpdate)
unchanged.
In `@packages/ui/src/components/Document/actions/TargetNodeActions.tsx`:
- Around line 87-102: The tooltip title for the type-override indicator uses raw
values (originalType and field?.type) which can be undefined or internal enum
names; update the title in the hasTypeOverride ActionListItem (where
handleOpenTypeOverrideModal is used) to use the same display logic as
TypeOverrideModal — e.g. use originalTypeQName?.toString() ?? originalType ??
'Unknown' for the original and a human-friendly representation/fallback for the
overridden type instead of field?.type so the tooltip shows the same formatted,
readable type strings as the modal/popover.
In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx`:
- Around line 97-108: Add an accessible label to the select FormGroup by setting
a short label like "New type" (even if visually hidden) instead of an empty
string; ensure the select open state is reset by calling setIsSelectOpen(false)
when the modal mounts/unmounts or when the modal open prop changes so it cannot
remain stale; and make the candidate list predictable by sorting candidates by
their displayName before rendering (affecting the list used by renderToggle /
selectedType and the menu items). Locate and update the FormGroup label prop,
the setIsSelectOpen usage (including where the modal open/close is handled), and
the candidate array feeding renderToggle/menu rendering to add the sort by
displayName.
In `@packages/ui/src/components/Document/SourceDocumentNode.test.tsx`:
- Around line 1098-1137: The tests rely on a fragile title substring to find the
override indicator; update the SourceDocumentNode component to render the
indicator with a stable attribute (e.g., data-testid="type-override-indicator"
or aria-label="type-override-indicator") and then change the two tests in
SourceDocumentNode.test.tsx to query for that attribute (use
getByTestId/queryByTestId or getByLabelText/queryByLabelText) instead of
container.querySelector(`[title*="Type overridden"]`), keeping the existing
assertions (toBeInTheDocument / not.toBeInTheDocument`) and the same tree/node
setup logic.
In `@packages/ui/src/components/Document/TargetDocumentNode.tsx`:
- Around line 93-115: The type override indicator title can show "undefined"
when field.originalType is missing; update the useMemo that defines
typeOverrideIndicator to compute a safeOriginal = field.originalType ??
field.originalTypeQName?.toString() ?? 'Unknown' and use that safeOriginal in
the title string (`Type overridden: ${safeOriginal} → ${field.type}`) inside the
Icon; keep the existing checks (nodeData instanceof TargetFieldNodeData |
FieldItemNodeData, hasTypeOverride, !hasMapping) and only change how the
original type is rendered.
packages/ui/src/components/Document/actions/FieldTypeOverride/TypeOverrideModal.test.tsx
Show resolved
Hide resolved
packages/ui/src/components/Document/actions/FieldTypeOverride/TypeOverrideModal.test.tsx
Show resolved
Hide resolved
packages/ui/src/components/Document/actions/TypeOverrideModal.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+124
to
+136
| const handleTypeOverrideSave = useCallback( | ||
| (selectedType: IFieldTypeInfo) => { | ||
| if (field) { | ||
| const document = field.ownerDocument; | ||
| const namespaceMap = mappingTree.namespaceMap; | ||
| const previousRefId = document.getReferenceId(namespaceMap); | ||
| FieldTypeOverrideService.applyFieldTypeOverride( | ||
| document, | ||
| field, | ||
| selectedType, | ||
| namespaceMap, | ||
| TypeOverrideVariant.SAFE, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and examine TypeOverrideModal
echo "=== Checking TypeOverrideModal ==="
rg -n "TypeOverrideModal" packages/ui/src/components/Document/ --type ts --type tsx
echo -e "\n=== Looking for onSave in TypeOverrideModal ==="
if [ -f "packages/ui/src/components/Document/actions/TypeOverrideModal.tsx" ]; then
wc -l packages/ui/src/components/Document/actions/TypeOverrideModal.tsx
fi
echo -e "\n=== Searching for IFieldTypeInfo definition ==="
rg -n "interface IFieldTypeInfo|type IFieldTypeInfo" packages/ui/src -g '*.ts*' -A 10
echo -e "\n=== Searching for applyFieldTypeOverride usage ==="
rg -n "applyFieldTypeOverride" packages/ui/src -g '*.ts*'Repository: KaotoIO/kaoto
Length of output: 4444
🏁 Script executed:
#!/bin/bash
# Examine TypeOverrideModal to see how it's structured and if it determines variant
echo "=== TypeOverrideModal.tsx content ==="
cat -n packages/ui/src/components/Document/actions/TypeOverrideModal.tsx
echo -e "\n=== Looking for TypeOverrideVariant enum definition ==="
rg -n "enum TypeOverrideVariant|type TypeOverrideVariant" packages/ui/src -g '*.ts' -A 5Repository: KaotoIO/kaoto
Length of output: 7918
🏁 Script executed:
#!/bin/bash
# Now let's look at SourceDocumentNode.tsx context around the modal invocation
echo "=== SourceDocumentNode.tsx lines 100-160 ==="
sed -n '100,160p' packages/ui/src/components/Document/SourceDocumentNode.tsx
echo -e "\n=== Check TargetNodeActions.tsx for comparison ==="
sed -n '40,80p' packages/ui/src/components/Document/actions/TargetNodeActions.tsx
echo -e "\n=== Check ConditionMenuAction.tsx for comparison ==="
sed -n '100,140p' packages/ui/src/components/Document/actions/ConditionMenuAction.tsxRepository: KaotoIO/kaoto
Length of output: 4903
🏁 Script executed:
#!/bin/bash
# Let's verify the getSafeOverrideCandidates vs getAllOverrideCandidates logic
echo "=== Check FieldTypeOverrideService methods ==="
rg -n "getSafeOverrideCandidates|getAllOverrideCandidates" packages/ui/src/services/field-type-override.service.ts -A 2Repository: KaotoIO/kaoto
Length of output: 1763
Thread the override variant from TypeOverrideModal to all callers, not just this component.
When TypeOverrideModal falls back to getAllOverrideCandidates() (lines 52–55), safe candidates don't exist, but all three callers (SourceDocumentNode, TargetNodeActions, ConditionMenuAction) hardcode TypeOverrideVariant.SAFE. This persists incorrect metadata—incompatible overrides should use TypeOverrideVariant.FORCE.
The modal determines which candidate set was used but doesn't communicate it. Update the onSave callback signature to include the variant:
🛠️ Suggested changes
TypeOverrideModal.tsx:
export type TypeOverrideModalProps = {
isOpen: boolean;
onClose: () => void;
- onSave: (selectedType: IFieldTypeInfo) => void;
+ onSave: (selectedType: IFieldTypeInfo, variant: TypeOverrideVariant) => void;
onRemove: () => void;
field: IField;
};In handleSave (line 83):
const handleSave = useCallback(() => {
if (selectedType) {
- onSave(selectedType);
+ const variant = Object.keys(typeCandidates).length > 0
+ ? TypeOverrideVariant.SAFE
+ : TypeOverrideVariant.FORCE;
+ onSave(selectedType, variant);
}
}, [selectedType, onSave, typeCandidates]);SourceDocumentNode.tsx + TargetNodeActions.tsx + ConditionMenuAction.tsx:
- const handleTypeOverrideSave = useCallback(
- (selectedType: IFieldTypeInfo) => {
+ const handleTypeOverrideSave = useCallback(
+ (selectedType: IFieldTypeInfo, variant: TypeOverrideVariant) => {
// ...
FieldTypeOverrideService.applyFieldTypeOverride(
document,
field,
selectedType,
namespaceMap,
- TypeOverrideVariant.SAFE,
+ variant,
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/Document/SourceDocumentNode.tsx` around lines 124
- 136, The onSave callback from TypeOverrideModal must be extended to accept a
TypeOverrideVariant and that variant must be forwarded to callers instead of
hardcoding TypeOverrideVariant.SAFE; update TypeOverrideModal
(handleSave/getAllOverrideCandidates) to call onSave(selectedType, variant),
change the signature of the onSave prop accordingly, and update each
caller—SourceDocumentNode.handleTypeOverrideSave, TargetNodeActions, and
ConditionMenuAction—to accept the variant parameter and pass it into
FieldTypeOverrideService.applyFieldTypeOverride (replace the hardcoded
TypeOverrideVariant.SAFE in that call with the passed-in variant) so FORCE is
used when getAllOverrideCandidates fallback occurs.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/components/Document/actions/FieldContextMenu.tsx (1)
27-34: Prefer theme‑aware styles over hard‑coded inline colors.Inline boxShadow/border/background can drift from design tokens and theming; consider moving this to a class (SCSS) and using themed variables.
Based on learnings: "Follow PatternFly design system conventions when implementing UI components".🎨 Suggested tweak
- <Menu - style={{ - boxShadow: '0 2px 8px rgba(0, 0, 0, 0.15)', - border: '1px solid `#d2d2d2`', - borderRadius: '4px', - backgroundColor: '#fff', - }} - > + <Menu className="field-context-menu">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/FieldContextMenu.tsx` around lines 27 - 34, The Menu in FieldContextMenu currently uses hard‑coded inline styles (boxShadow, border, borderRadius, backgroundColor); extract these into a themed CSS/SCSS class (e.g., .field-context-menu) that uses PatternFly/design tokens or your app's theme variables, replace the inline style prop on the Menu with className="field-context-menu", add the new SCSS rule referencing the design tokens for shadow, border and background, and import the stylesheet into the FieldContextMenu component so the menu follows the theme instead of fixed colors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ui/src/components/Document/actions/ConditionMenuAction.test.tsx`:
- Around line 23-33: The test mutates a module-scoped targetDoc (created via
TestUtil.createTargetOrderDoc) which leaks state between tests; move creation of
targetDoc, mappingTree and documentNodeData into the beforeEach (or
recreate/reset them in beforeEach) and update the useDataMapper mock there (the
jest.requireMock('../../../hooks/useDataMapper') call) to return a fresh
mappingTree and updateDocument mock each test so tests do not share mutated
state from targetDoc, mappingTree or documentNodeData.
In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx`:
- Around line 58-65: The preselection logic in TypeOverrideModal (when checking
field.typeOverride === TypeOverrideVariant.SAFE || TypeOverrideVariant.FORCE)
currently matches candidates by the enum value (candidate.type === field.type),
which can choose the wrong XML type; change the match to use the qualified
name/type string instead (e.g., find the candidate where candidate.typeString
=== field.typeString or candidate.qname === field.qname) and call
setSelectedType(currentTypeInfo || null) with that result so the actual
overridden XML type is preselected.
- Around line 28-35: The modal currently hard-codes the override variant as
SAFE; change TypeOverrideModalProps and the onSave callback signature to accept
a variant (e.g., 'SAFE' | 'FORCE'), determine which candidate set was used
inside TypeOverrideModal (if using safeCandidates => variant = 'SAFE', if
falling back to allTypes => variant = 'FORCE') and pass that variant along with
the selected type to onSave; then update the call sites (SourceDocumentNode,
TargetNodeActions, ConditionMenuAction) to accept the new onSave parameter and
handle the variant accordingly. Ensure the prop type IFieldTypeInfo payload
remains unchanged except for the added variant argument and adjust any handlers
to preserve previous behavior for SAFE but apply FORCE where indicated.
In `@packages/ui/src/components/Document/SourceDocumentNode.test.tsx`:
- Around line 1025-1095: The test creates a spy via
jest.spyOn(FieldTypeOverrideService,
'getSafeOverrideCandidates').mockReturnValue(...) but never restores it, causing
cross-test leakage; capture that spy (e.g. const getSafeCandidatesSpy =
jest.spyOn(FieldTypeOverrideService,
'getSafeOverrideCandidates').mockReturnValue(...)) and call
getSafeCandidatesSpy.mockRestore() after the assertions (or add it to a common
afterEach cleanup), analogous to the existing applySpy.mockRestore() for
applyFieldTypeOverride in SourceDocumentNode.test.tsx.
In `@packages/ui/src/components/Document/SourceDocumentNode.tsx`:
- Around line 214-243: The dialog currently stops all keydown propagation (in
the element using role="dialog") which prevents the backdrop Escape handler from
running when focus is inside the menu; update the dialog's onKeyDown handler
(the one on the element that uses contextMenuPosition and role="dialog") to
check for Escape and call setShowContextMenu(false) when e.key === 'Escape', and
only call e.stopPropagation() for other keys so non-Escape keys still don't
bubble.
---
Nitpick comments:
In `@packages/ui/src/components/Document/actions/FieldContextMenu.tsx`:
- Around line 27-34: The Menu in FieldContextMenu currently uses hard‑coded
inline styles (boxShadow, border, borderRadius, backgroundColor); extract these
into a themed CSS/SCSS class (e.g., .field-context-menu) that uses
PatternFly/design tokens or your app's theme variables, replace the inline style
prop on the Menu with className="field-context-menu", add the new SCSS rule
referencing the design tokens for shadow, border and background, and import the
stylesheet into the FieldContextMenu component so the menu follows the theme
instead of fixed colors.
aea6720 to
0d86239
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/components/Document/actions/TypeOverrideModal.tsx (1)
97-99: Consider functional update pattern.Using functional update avoids the dependency on
isSelectOpenand prevents potential stale closure issues:Suggested change
const handleToggleSelect = useCallback(() => { - setIsSelectOpen(!isSelectOpen); - }, [isSelectOpen]); + setIsSelectOpen((prev) => !prev); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx` around lines 97 - 99, The handleToggleSelect callback currently closes over isSelectOpen causing a dependency; change it to use the functional state updater with setIsSelectOpen(prev => !prev) and remove isSelectOpen from the dependency array so useCallback can use an empty array (or only stable deps) — update the implementation of handleToggleSelect to call setIsSelectOpen with the functional toggle to avoid stale closures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ui/src/components/Document/actions/ConditionMenuAction.test.tsx`:
- Around line 23-33: The tests share mutable fixtures (targetDoc, mappingTree,
documentNodeData) at module scope and later tests mutate them (e.g.,
field.typeOverride), causing state leakage; move creation of targetDoc,
mappingTree and documentNodeData into the beforeEach so each test gets fresh
instances and update the mocked hook usage in beforeEach (the
useDataMapper.mockReturnValue should reference the newly created mappingTree and
a new jest.fn() updateDocument) to ensure isolation; locate the module-scoped
symbols targetDoc, mappingTree, documentNodeData and the beforeEach that
configures useDataMapper and recreate those fixtures inside that beforeEach.
In `@packages/ui/src/components/Document/actions/TargetNodeActions.tsx`:
- Around line 49-67: The handler handleTypeOverrideSave currently hardcodes
TypeOverrideVariant.SAFE when calling
FieldTypeOverrideService.applyFieldTypeOverride; change the onSave signature
from the TypeOverrideModal to include the chosen TypeOverrideVariant and thread
that variant into handleTypeOverrideSave so you pass the actual variant to
applyFieldTypeOverride instead of TypeOverrideVariant.SAFE; also update the
other callers (ConditionMenuAction and SourceDocumentNode) and any
TypeOverrideModal call sites to accept and forward the variant so FORCE
overrides (or others) are persisted correctly.
In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx`:
- Around line 58-67: The pre-select logic in TypeOverrideModal.tsx incorrectly
matches candidates by the enum value (candidate.type === field.type) which can
conflate distinct QNames; update the lookup to match the candidate's qualified
name against the existing override by using candidate.typeQName ===
field.typeQName (when field.typeOverride is SAFE or FORCE) and then call
setSelectedType with that found candidate (or null) so the exact overridden XML
Schema type is pre-selected.
---
Nitpick comments:
In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx`:
- Around line 97-99: The handleToggleSelect callback currently closes over
isSelectOpen causing a dependency; change it to use the functional state updater
with setIsSelectOpen(prev => !prev) and remove isSelectOpen from the dependency
array so useCallback can use an empty array (or only stable deps) — update the
implementation of handleToggleSelect to call setIsSelectOpen with the functional
toggle to avoid stale closures.
557862c to
e19c9fc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ui/src/components/Document/actions/ConditionMenuAction.tsx (1)
47-47: InconsistenthasTypeOverridelogic compared to SourceDocumentNode.When
fieldis undefined,field?.typeOverridereturnsundefined, andundefined !== TypeOverrideVariant.NONEevaluates totrue. This makeshasTypeOverrideincorrectlytruewhen there's no field.While the UI is protected by
isFieldNodeguards at lines 185 and 199, this differs from the safer pattern in SourceDocumentNode (line 96):field && field.typeOverride !== TypeOverrideVariant.NONE.♻️ Proposed fix for consistency
- const hasTypeOverride = field?.typeOverride !== TypeOverrideVariant.NONE; + const hasTypeOverride = field && field.typeOverride !== TypeOverrideVariant.NONE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/ConditionMenuAction.tsx` at line 47, hasTypeOverride currently uses optional chaining which makes it true when field is undefined; change the check to ensure field exists before comparing typeOverride (e.g., use the pattern used in SourceDocumentNode: ensure field is truthy and then compare field.typeOverride !== TypeOverrideVariant.NONE) so hasTypeOverride is false when field is undefined; update the declaration using the symbols hasTypeOverride, field, and TypeOverrideVariant accordingly.
🤖 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/actions/TargetNodeActions.tsx`:
- Around line 49-67: handleTypeOverrideSave always uses TypeOverrideVariant.SAFE
but the modal can return types from getAllOverrideCandidates that should be
FORCE; change the flow so the variant is correct: update the
TargetNodeActions.handleTypeOverrideSave signature to accept a variant (e.g.,
handleTypeOverrideSave(selectedType: IFieldTypeInfo, variant:
TypeOverrideVariant)) or derive the variant by checking whether the selectedType
came from getAllOverrideCandidates vs the safe candidate set, then pass that
variant into FieldTypeOverrideService.applyFieldTypeOverride instead of
hardcoding SAFE; also update the caller (TypeOverrideModal usage) to return or
pass the chosen variant and adjust any affected dependencies
(mappingTree.namespaceMap, updateDocument, onUpdate) accordingly.
---
Duplicate comments:
In `@packages/ui/src/components/Document/actions/ConditionMenuAction.test.tsx`:
- Around line 265-311: The tests mutate the module-scoped targetDoc (and its
fields) which can leak state between tests; change the tests to use a fresh
fixture per test by creating a new target document/field instance inside each it
or in a beforeEach (e.g., call a helper like createFreshTargetDoc() or
deep-clone the module fixture with
structuredClone/JSON.parse(JSON.stringify(...)) to produce a fresh field) and
then construct the TargetFieldNodeData and FieldItem from that fresh copy;
ensure updateDocument/useDataMapper mocks remain per-test so no shared state
persists.
In `@packages/ui/src/components/Document/actions/ConditionMenuAction.tsx`:
- Around line 110-136: handleTypeOverrideSave currently hardcodes
TypeOverrideVariant.SAFE when calling
FieldTypeOverrideService.applyFieldTypeOverride; change it so the variant is
provided by the TypeOverrideModal via its onSave callback (the modal should
determine SAFE vs FORCE based on whether it used getAllOverrideCandidates) and
passed into handleTypeOverrideSave. Update the handleTypeOverrideSave signature
to accept a variant parameter (e.g., variant: TypeOverrideVariant) and forward
that variant into FieldTypeOverrideService.applyFieldTypeOverride (and keep
updateDocument, setIsTypeOverrideModalOpen, onUpdate behavior unchanged). Ensure
TypeOverrideModal’s onSave callsite sends the chosen variant so fields without
safe candidates use TypeOverrideVariant.FORCE.
In `@packages/ui/src/components/Document/actions/TypeOverrideModal.test.tsx`:
- Around line 169-174: The assertion inside the waitFor in
TypeOverrideModal.test.tsx is too weak (it allows the test to pass if the toggle
never updates); update the check so it asserts the actual visible toggle label
or that the "Save" action becomes enabled after selection: locate the same
waitFor block and replace the current toggleWithSelection/expect logic with a
stronger assertion such as expecting the toggle button text to equal the
selected type (e.g., use the same query for the toggle button instead of
queryByText('Select a new type...') and assert its visible label is 'string') or
assert the Save button is enabled (getByRole('button', { name: /Save/ }) and
expect it not.toBeDisabled()). Ensure you reference the existing
toggleWithSelection variable or target the toggle button and Save button
elements so the test fails if selection did not actually occur.
In `@packages/ui/src/components/Document/actions/TypeOverrideModal.tsx`:
- Around line 58-64: The current preselection logic in TypeOverrideModal.tsx
matches candidates by the enum value (using field.type) which can select the
wrong candidate when different XML/QName types share the same enum; update the
lookup used when field.typeOverride is TypeOverrideVariant.SAFE or
TypeOverrideVariant.FORCE to prefer matching by the candidate's qualified
identifier (e.g., candidate.typeString or candidate.typeQName) against
field.typeString or field.typeQName first, falling back to the enum match
(candidate.type === field.type) only if no exact qualified-name match is found,
and then call setSelectedType with the resolved candidate or null.
In `@packages/ui/src/components/Document/SourceDocumentNode.tsx`:
- Around line 124-143: handleTypeOverrideSave currently always passes
TypeOverrideVariant.SAFE to FieldTypeOverrideService.applyFieldTypeOverride;
change the onSave callback signature used by TypeOverrideModal to accept the
chosen variant and forward it to applyFieldTypeOverride instead of hardcoding
SAFE. Update the handleTypeOverrideSave function to accept (selectedType:
IFieldTypeInfo, variant: TypeOverrideVariant), call
FieldTypeOverrideService.applyFieldTypeOverride(document, field, selectedType,
namespaceMap, variant), and ensure any places that open the modal pass the
correct variant (FORCE when getAllOverrideCandidates() is used). Also update the
useCallback dependency list to include any newly referenced symbols (e.g.,
TypeOverrideVariant) and adjust callers of handleTypeOverrideSave to match the
new signature.
---
Nitpick comments:
In `@packages/ui/src/components/Document/actions/ConditionMenuAction.tsx`:
- Line 47: hasTypeOverride currently uses optional chaining which makes it true
when field is undefined; change the check to ensure field exists before
comparing typeOverride (e.g., use the pattern used in SourceDocumentNode: ensure
field is truthy and then compare field.typeOverride !==
TypeOverrideVariant.NONE) so hasTypeOverride is false when field is undefined;
update the declaration using the symbols hasTypeOverride, field, and
TypeOverrideVariant accordingly.
packages/ui/src/components/Document/actions/TargetNodeActions.tsx
Outdated
Show resolved
Hide resolved
Contributor
Author
|
converted to draft again, to update with schema upload feature. |
mmelko
force-pushed
the
feature/field-type-override-ui-2732
branch
5 times, most recently
from
c3cfd0a to
862a2b5
Compare
Contributor
Author
|
Screenshot and video updated |
igarashitm
reviewed
Mar 2, 2026
Member
There was a problem hiding this comment.
Looking good, almost there - a couple of touches needed:
Add stop propagation in type override modal
so that datamapper main canvas doesn't react to the mouse action (DnD is triggered) here is the example
And here is what could happen if it doesn't stop propagation, notice that a target field is being dragged even though the modal is open
Namespace format bug in backend logic persisting IFieldTypeOverride into .kaoto
both type and originalType has to be namespace prefixed and the namespace prefix should be registered in namespaceMap
"fieldTypeOverrides": [
{
"schemaPath": "/Root/Person",
"type": "EmployeeType", // missing namespace prefix
"originalType": "{http://www.example.com/LAZYTEST}PersonType", // use prefix instead of full namespace URI
"variant": "SAFE"
}
]
...
"namespaceMap": {
"xs": "http://www.w3.org/2001/XMLSchema",
// There should be an entry for the LAZYTEST namespace
"fn": "http://www.w3.org/2005/xpath-functions"
}
Showing field type in hover
When we show "type" and "originalType" for Field Type Override specifically, we'd want to show IField.typeQName and IField.originalTypeQName in the format of nsprefix:LocalName instead of showing DataMapper field types such as Container.
Disable context menu when ReadOnly
When isReadOnly is true on SourceDocumentNode, we should disable Field Type Override context menu. This is used in XPath Editor. Inside the XPath editor, any modification on source documents are disabled.
igarashitm
reviewed
Mar 2, 2026
| if (!newSchemas || Object.keys(newSchemas).length === 0) return; | ||
|
|
||
| try { | ||
| if (field.ownerDocument instanceof XmlSchemaDocument) { |
Member
There was a problem hiding this comment.
FieldTypeOverrideService.addSchemaFilesForTypeOverride() is supposed to be used instead of doing if (XML) here, doesn't it help?
Member
|
@mmelko I'm moving this to draft to save some chromatic tokens |
Contributor
Author
|
@igarashitm thanks for your review! I'm on it. |
mmelko
force-pushed
the
feature/field-type-override-ui-2732
branch
from
dc0088d to
bfd8f19
Compare
igarashitm
reviewed
Mar 4, 2026
Member
There was a problem hiding this comment.
2-button action for attaching additional schema files
I personally prefer the previous UX, just one button and reflect types immediately. If we want to allow user to remove schema files additionally uploaded in this modal, we would need to
- Keep the list of additional files to distinguish from others (i.e. uploaded outside this modal) even after types are loaded
- Immediately load the types from additional files
- Allow removal only for those additional files, which triggers document update
But I think this could be left as a separate issue. For now I think it's fine to just immediately reflect and don't allow removal.
Schema file list overflows
SchemaFileList component would need height limit and its own scroll
Type dropdown overflows
Same for the dropdown, it overflows when there're many candidates, it would also need height limit and scroll
Hover on override wrench icon
I feel it's little bit harder to get this hover, effective area is a bit too narrow?
Hover on field name
This still shows Container for both Original Type and Overridden Type, would be better to show ns0:EmployeeType for example
User might wonder what namespace is mapped to ns0 here, but it's basically what is used in the output XSLT, and also in the future user can manage those prefixes by themselves - #1818
FYI, the field hover on the field without override doesn't show Original Type and Overridden Type, it's field override specific display
igarashitm
reviewed
Mar 4, 2026
| // For Container types without namespace prefix, resolve namespace from schema collection | ||
| // to ensure the QName matches the fragment key format ({namespaceURI}localPart) | ||
| if (type === Types.Container && !namespaceURI) { | ||
| namespaceURI = XmlSchemaTypesService.resolveNamespaceForUnprefixedType(localPart, field); |
Member
There was a problem hiding this comment.
The need of this looks fishy... generally speaking, if the type is not prefixed, it means that the type is in blank namespace. There could be a bug before coming to this if the typeString comes in without ns prefix, but is actually in a namespace.
| * Format a QName as `prefix:localPart` using the namespace map. | ||
| * Falls back to the provided fallback string if the QName is null. | ||
| */ | ||
| static formatQNameWithPrefix( |
Member
There was a problem hiding this comment.
nitpicking, this one looks like a more common routine than being in field-type-override.service - how about create a qname-util.ts in services/?
| import { TrashIcon } from '@patternfly/react-icons'; | ||
| import { FunctionComponent, memo } from 'react'; | ||
|
|
||
| import { getFileName } from './AttachSchema/utils'; |
Member
There was a problem hiding this comment.
Good to have a 2nd user of util.ts here - WDYT to move this utils.ts to one level upper?
Also, we might want to create FieldOverride sub directory here and put these Field Override components&tests into there, just like we have AttachSchema sub directory.
Member
There was a problem hiding this comment.
Claude also suggests to extract more common routines as following, I'd leave it to you if it's worth doing it or not
in the upload orchestration inside TypeOverrideModal.handleSchemaUpload vs AttachSchemaModal.onFileUpload. Both repeat the same sequence:
const paths = await DataMapperMetadataService.selectDocumentSchema(api, pattern);
const newPaths = Array.isArray(paths) ? paths : [paths];
const firstExt = getFileExtension(newPaths[0]);
const extensionError = validateFileExtension(firstExt, documentType);
const mixedTypeError = validateNoMixedTypes(firstExt, existingPaths);The selectDocumentSchema + normalize-to-array + validate sequence could be extracted to utils.ts as something like pickAndValidateSchemaFiles(api, pattern, documentType, existingPaths): Promise<{paths: string[], error: string | null}>. That's the one real consolidation candidate, though the two callers handle errors differently (one sets a createDocumentResult error object, the other sets an uploadError: string), so it would need a shared return shape.
igarashitm
reviewed
Mar 4, 2026
packages/ui/src/components/Document/actions/TypeOverrideModal.tsx
Outdated
Show resolved
Hide resolved
Member
Member
Another alternative is to add a confirmation modal when user is uploading additional schema files, with showing the file name to be uploaded. In this way, user will stay focusing on the files they are uploading. |
mmelko
force-pushed
the
feature/field-type-override-ui-2732
branch
4 times, most recently
from
a57bd7b to
16ea354
Compare
igarashitm
reviewed
Mar 7, 2026
Add type override modal, field type override service, and context menu for source/target document nodes. Includes type candidate discovery, supplementary schema upload with immediate schema loading into document collection, override indicator icons, and keyboard accessibility. - Fix complex named type fragments to use Types.Container instead of AnyType, ensuring overridden fields retain their container structure - Schema uploads are permanently attached to the document on upload - Add extension type override tests with LazyLoadingTestExtensions.xsd
…finition Previously, addSchemaFilesForTypeOverride returned a new DocumentDefinition, forcing callers to manually assign it back to document.definition before calling updateDocument(). This was inconsistent with other mutating service methods (applyFieldTypeOverride, revertFieldTypeOverride) that operate in place and return void. Making the method mutate in place eliminates the awkward two-step pattern at the call site and aligns the API surface across all FieldTypeOverrideService mutation methods. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d type names - Refactor field type override components into FieldTypeOverride/ subdirectory - Display qualified type names with namespace prefixes (e.g., xs:string, ns0:EmployeeType) in hover tooltips - Add namespaceMap prop to NodeTitle component for proper type name formatting - Improve wrench icon hover area with larger padding and min-width/height (24px) - Increase wrench icon size from sm to md for better visibility - Update TypeOverrideModal tests to match immediate schema attachment behavior - Fix import paths after refactoring to subdirectory - Extract formatQNameWithPrefix to qname-util.ts for better code organization - Extract pickAndValidateSchemaFiles to utils.ts to consolidate duplicate schema upload logic Addresses PR review feedback: - Qualified type names now show namespace prefixes on hover - Better wrench icon hover UX with larger clickable area - All tests passing with immediate schema attachment flow - QName formatting utility moved to dedicated file for reusability - Schema file selection and validation logic consolidated in one place Related to KaotoIO#2732
…chema upload Document-level namespaces (e.g., targetNamespace from XSD schemas) were not added to mappingTree.namespaceMap during initial schema upload. This caused type override typeStrings to be generated without namespace prefixes, leading to failed QName lookups in namedTypeFragments and missing children in the document tree after a type override. - Extract targetNamespace from XmlSchemaCollection during createXmlSchemaDocument and populate definition.namespaceMap - Sync definition.namespaceMap into mappingTree.namespaceMap in updateDocument - Remove resolveNamespaceForUnprefixedType workaround, no longer needed - Refactor createXmlSchemaDocument to reduce cognitive complexity by extracting helper methods Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… in type overrides Per-document namespace extraction generated prefixes independently, so two schemas could both claim ns0 — the last Object.assign win silently corrupted the global namespace map. Replace with a single authoritative mappingTree.namespaceMap passed directly into document creation and type override application; lazily register new namespaces on demand instead of eagerly extracting them per document.
mmelko
force-pushed
the
feature/field-type-override-ui-2732
branch
from
33645a4 to
465a3d8
Compare
igarashitm
previously approved these changes
Mar 9, 2026
Member
igarashitm
left a comment
igarashitm
left a comment
There was a problem hiding this comment.
Just one cleanup left, otherwise LGTM 👍
| const typePrefix = candidate.namespaceURI | ||
| ? (Object.entries(namespaceMap).find(([, uri]) => uri === candidate.namespaceURI)?.[0] ?? '') | ||
| : ''; | ||
| const typeString = typePrefix ? `${typePrefix}:${localPart}` : localPart; |
Member
There was a problem hiding this comment.
This could use qname-util.ts function
Extract reusable formatWithPrefix utility for namespace prefix lookup and use it in both formatQNameWithPrefix and createFieldTypeOverride. Remove unused deprecated formatQNameWithPrefix re-export from FieldTypeOverrideService.
mmelko
force-pushed
the
feature/field-type-override-ui-2732
branch
from
465a3d8 to
0a5e2a3
Compare
|
igarashitm
reviewed
Mar 11, 2026
| const originalTypeString = field.originalTypeQName?.toString() ?? field.originalType; | ||
| const originalTypeString = formatQNameWithPrefix(field.originalTypeQName, namespaceMap, field.originalType); | ||
| const localPart = candidate.typeString.includes(':') ? candidate.typeString.split(':')[1] : candidate.typeString; | ||
| const typeString = formatWithPrefix(candidate.namespaceURI, localPart, namespaceMap); |
Member
There was a problem hiding this comment.
wait, isn't this just candidate.typeString? if we do need to split and then concatenate yet again, that smells like a bug in somewhere else...
Member
There was a problem hiding this comment.
I think the root cause is that IFieldTypeInfo should not hold prefixed typeString as it doesn't have clear correlation with any namespaceMap, fix is here - mmelko#6
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: #2732
This PR implements the UI for field type override functionality in the DataMapper, allowing users to change field types through a visual interface:
Screen.Recording.2026-03-02.at.14.33.57.mov
Summary by CodeRabbit
New Features
Tests