[WIP] fix(DataMapper): Add DnD drop validation with visual feedback and er…#3026
Draft
igarashitm wants to merge 1 commit intoKaotoIO:mainfrom
Draft
[WIP] fix(DataMapper): Add DnD drop validation with visual feedback and er…#3026igarashitm wants to merge 1 commit intoKaotoIO:mainfrom
igarashitm wants to merge 1 commit intoKaotoIO:mainfrom
Conversation
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:
📝 WalkthroughWalkthroughThis pull request implements extensible drag-and-drop validation infrastructure for the DataMapper. It introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant DragSource as Drag Source
participant DropTarget as Drop Target
participant MappingValidation as MappingValidationService
participant DnDHandler as DnD Handler
participant Provider as DataMapperDndProvider
participant UI as Alert/Toast UI
User->>DragSource: Initiates drag
DragSource->>Provider: activeNode set in context
User->>DropTarget: Hover over target
DropTarget->>MappingValidation: validateMappingPair(activeNode, targetNode)
alt Valid mapping
MappingValidation-->>DropTarget: isValid: true
DropTarget->>DropTarget: Apply droppable-container class
else Invalid mapping
MappingValidation-->>DropTarget: isValid: false, errorMessage
DropTarget->>DropTarget: Apply droppable-invalid class
DropTarget->>DropTarget: Show not-allowed cursor
end
User->>DragSource: Drop on target
DragSource->>DnDHandler: handleDragEnd(event)
DnDHandler->>MappingValidation: validateMappingPair(sourceField, targetField)
alt Validation passes
MappingValidation-->>DnDHandler: isValid: true
DnDHandler->>DnDHandler: engageMapping()
DnDHandler-->>Provider: DnDResult { success: true }
Provider->>Provider: Call onUpdate()
else Validation fails
MappingValidation-->>DnDHandler: isValid: false, errorMessage
DnDHandler-->>Provider: DnDResult { success: false, errorMessage }
Provider->>UI: sendAlert(variant: danger, message)
UI->>User: Display error toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
igarashitm
force-pushed
the
2358-2898
branch
3 times, most recently
from
e668911 to
49f2557
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3026 +/- ##
==========================================
+ Coverage 89.73% 89.88% +0.14%
==========================================
Files 564 565 +1
Lines 21030 21082 +52
Branches 4766 4942 +176
==========================================
+ Hits 18872 18950 +78
+ Misses 2156 2130 -26
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
🧹 Nitpick comments (1)
packages/ui/src/components/Document/NodeContainer.test.tsx (1)
48-57: Consider using a typed context value for robustness.The empty object
{}for context value works but doesn't reflect the expected shape. IfDataMapperDndContexttype changes, this test won't catch the mismatch.♻️ Suggested improvement
it('should call useDroppable with disabled: false when there is no active node', () => { render( - <DataMapperDndContext.Provider value={{}}> + <DataMapperDndContext.Provider value={{ activeNode: undefined }}> <DroppableContainer id="test" nodeData={mockNodeData}> content </DroppableContainer> </DataMapperDndContext.Provider>, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/NodeContainer.test.tsx` around lines 48 - 57, The test passes an untyped empty object to DataMapperDndContext which hides shape mismatches; update the test to provide a properly typed context value matching the DataMapperDndContext shape (instead of `{}`) when rendering DroppableContainer so the mock reflects expected properties used by DroppableContainer/useDroppable (use the context type or the expected fields from DataMapperDndContext), e.g., construct a value based on the DataMapperDndContext type and include relevant keys used by DroppableContainer and mockNodeData to ensure useDroppable is exercised with disabled: false.
🤖 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/providers/datamapper-dnd.provider.tsx`:
- Around line 168-179: The provider currently only clears drag state inside
handleDragEnd, leaving stale activeData/activeDragSideRef when a drag is
cancelled; extract the cleanup (setActiveData(null) and
activeDragSideRef.current = null) into a shared function (e.g., clearActiveDrag)
and call it from handleDragEnd after the handler logic and also wire it into the
DndContext.onDragCancel callback so cancellation runs the same cleanup;
reference handleDragEnd, setActiveData, activeDragSideRef, and
DndContext.onDragCancel when making the change.
In `@packages/ui/src/services/mapping-validation.service.ts`:
- Around line 63-65: The current branch in mapping-validation.service.ts treats
any non-TargetFieldNodeData/FieldItemNodeData as valid, causing false-success
drops; change this to a closed allowlist: consult
VisualizationService.engageMapping() to identify which concrete target node
classes it supports, then only return { isValid: true, sourceNode, targetNode }
when targetNode is an instance of one of those supported classes (e.g., the same
concrete classes handled in engageMapping); for all other target node types
return { isValid: false, sourceNode, targetNode } so SourceTargetDnDHandler does
not report success for unsupported targets. Ensure you reference the same class
constructors (not string names) as used in engageMapping and update the
instanceof checks in mapping-validation.service.ts accordingly.
- Around line 74-77: validateChoiceRules currently ignores the source field and
runs after validateContainerRules, so add source-side choice checks inside
validateChoiceRules to detect when the source is a choice or a choice member and
produce the actionable "select a member first" validation result for cases like
dropping a scalar onto an unselected choice; then change the validationRules
array to put MappingValidationService.validateChoiceRules before
MappingValidationService.validateContainerRules (and apply the same
change/update to the other occurrence around the 113-120 block) so
choice-specific validation runs with higher precedence than container/terminal
checks.
---
Nitpick comments:
In `@packages/ui/src/components/Document/NodeContainer.test.tsx`:
- Around line 48-57: The test passes an untyped empty object to
DataMapperDndContext which hides shape mismatches; update the test to provide a
properly typed context value matching the DataMapperDndContext shape (instead of
`{}`) when rendering DroppableContainer so the mock reflects expected properties
used by DroppableContainer/useDroppable (use the context type or the expected
fields from DataMapperDndContext), e.g., construct a value based on the
DataMapperDndContext type and include relevant keys used by DroppableContainer
and mockNodeData to ensure useDroppable is exercised with disabled: false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6cc20d0-77b1-459b-8824-c1cb09681d09
📒 Files selected for processing (12)
packages/ui/src/components/Document/NodeContainer.scsspackages/ui/src/components/Document/NodeContainer.test.tsxpackages/ui/src/components/Document/NodeContainer.tsxpackages/ui/src/providers/datamapper-dnd.provider.test.tsxpackages/ui/src/providers/datamapper-dnd.provider.tsxpackages/ui/src/providers/dnd/DnDHandler.tspackages/ui/src/providers/dnd/ExpressionEditorDnDHandler.test.tspackages/ui/src/providers/dnd/ExpressionEditorDnDHandler.tspackages/ui/src/providers/dnd/SourceTargetDnDHandler.test.tspackages/ui/src/providers/dnd/SourceTargetDnDHandler.tspackages/ui/src/services/mapping-validation.service.test.tspackages/ui/src/services/mapping-validation.service.ts
igarashitm
force-pushed
the
2358-2898
branch
2 times, most recently
from
149c761 to
8968aff
Compare
…or toasts Fixes: KaotoIO#2898 Introduces mapping validation when user performs drag and drop to create a data mapping. Introduces `MappingValidationService` to centralise mapping pair validation rules, replacing `VisualizationService testNodePair()`. `DnDHandler.handleDragEnd()` now returns a `DnDResult` to propagate error message if there is. - Invalid cross-side drops (unselected xs:choice, container/terminal mismatch) show an error toast - Hovering over an invalid target shows a disabled border and field name style - Hovering over a same-side droppable (source→source, target→target) is disabled at DNDKit level, i.e. no visual feedback on hover, silently ignored if dropped
|
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
1 participant
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.
…ror toasts
#2954 has to go in before this one, and this one needs adjustment. Keep this as draft until then.
Fixes: #2898
Introduces mapping validation when user performs drag and drop to create a data mapping.
Introduces
MappingValidationServiceto centralise mapping pair validation rules, replacingVisualizationService testNodePair().DnDHandler.handleDragEnd()now returns aDnDResultto propagate error message if there is.2026-03-10.09-17-02.mp4
Summary by CodeRabbit
New Features
Tests