Skip to content

Commit f1fddd3

Browse files
cbeach47cbeach47
authored
Issue #845: Fix duplicate attr mapping (#849)
resolves #845 moving wires, and also wire selection logic was using only attributeId instead of full path. This was causing errors when updating (moving) or selecting wires due to ambiguity when attributeId was the same even in different entityIdPath(s)
2 parents 962af77 + 5fd4e76 commit f1fddd3

2 files changed

Lines changed: 79 additions & 17 deletions

File tree

‎frontends/mdr-frontend/src/pages/Explore/Mappings/MappingsView.tsx‎

Lines changed: 50 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import {
3535
parseEntityIdPath,
3636
appendAttributeToPath,
3737
extractEntityIds,
38+
extractEntityPath,
3839
buildAttributeLookupKey,
3940
} from '../../../utils/entityIdPath';
4041
import {
@@ -144,6 +145,9 @@ const MappingsView: React.FC = () => {
144145
const [selectedTargetAttrId, setSelectedTargetAttrId] = useState<
145146
number | null
146147
>(null);
148+
const [selectedTargetAttrPath, setSelectedTargetAttrPath] = useState<
149+
string | null
150+
>(null);
147151
const [selectionIndex, setSelectionIndex] = useState(0);
148152
const [selectionAll, setSelectionAll] = useState(false);
149153
const [selectedTransformationIds, setSelectedTransformationIds] =
@@ -158,6 +162,9 @@ const MappingsView: React.FC = () => {
158162
const [reassignHoverTargetId, setReassignHoverTargetId] = useState<
159163
number | null
160164
>(null);
165+
const [reassignHoverTargetPath, setReassignHoverTargetPath] = useState<
166+
string | null
167+
>(null);
161168
// Multi-wire (per-transformation) selection: store source AttributeIds for the currently selected transformation
162169
const [selectedWireSourceAttrIds, setSelectedWireSourceAttrIds] = useState<Set<number>>(new Set());
163170
// Multi-wire detach drag state (supports one or many selected source wires for the single selected transformation)
@@ -173,6 +180,7 @@ const MappingsView: React.FC = () => {
173180
// the latest value without needing reassignHoverTargetId in the dep array
174181
// (which caused effect re-runs + duplicate handleUp registrations).
175182
const reassignHoverTargetIdRef = useRef<number | null>(null);
183+
const reassignHoverTargetPathRef = useRef<string | null>(null);
176184
// Guard against duplicate handleUp invocations caused by the synthetic mouseup
177185
// that handleMove dispatches after the button is released.
178186
const reassignProcessingRef = useRef(false);
@@ -249,8 +257,11 @@ const MappingsView: React.FC = () => {
249257
setReassignTransformations([]);
250258
setReassignPaths([]);
251259
setReassignHoverTargetId(null);
260+
setReassignHoverTargetPath(null);
252261
reassignHoverTargetIdRef.current = null;
262+
reassignHoverTargetPathRef.current = null;
253263
setSelectedTargetAttrId(null);
264+
setSelectedTargetAttrPath(null);
254265
setSelectedTransformationIds(new Set());
255266
setSelectionAll(false);
256267
}, []);
@@ -1552,19 +1563,23 @@ const MappingsView: React.FC = () => {
15521563
) as HTMLElement | null;
15531564
let node: HTMLElement | null = el;
15541565
let targetId: number | null = null;
1566+
let targetEntityPath: string | null = null;
15551567
while (node) {
15561568
if (
15571569
node.classList?.contains('mappings-attr') &&
15581570
node.dataset.attrId
15591571
) {
15601572
if (node.classList.contains('mappings-attr--right'))
15611573
targetId = Number(node.dataset.attrId);
1574+
targetEntityPath = node.dataset.entityPath || null;
15621575
break;
15631576
}
15641577
node = node.parentElement;
15651578
}
15661579
reassignHoverTargetIdRef.current = targetId;
1580+
reassignHoverTargetPathRef.current = targetEntityPath;
15671581
setReassignHoverTargetId(targetId);
1582+
setReassignHoverTargetPath(targetEntityPath);
15681583
};
15691584
const handleUp = async () => {
15701585
// Guard: prevent duplicate invocations caused by the synthetic mouseup
@@ -1574,17 +1589,35 @@ const MappingsView: React.FC = () => {
15741589
// Read from ref so we always get the latest hover target, regardless of
15751590
// which effect-closure version of handleUp fires.
15761591
const reassignHoverTargetId = reassignHoverTargetIdRef.current;
1577-
const droppingOnTarget =
1578-
!!reassignHoverTargetId &&
1579-
reassignHoverTargetId !== selectedTargetAttrId;
1592+
const reassignHoverTargetEntityPath = reassignHoverTargetPathRef.current;
1593+
// Determine if drop target is genuinely different from the current target.
1594+
// Compare both attribute ID and entity path so that the same attribute in a
1595+
// different entity is recognised as a valid drop target.
1596+
const droppingOnTarget = (() => {
1597+
if (!reassignHoverTargetId) return false;
1598+
const currentT = reassignTransformations[0];
1599+
const currentTgtAttrId = currentT?.TargetAttribute?.AttributeId;
1600+
const currentTgtEntityIdPath = (currentT?.TargetAttribute as any)?.EntityIdPath;
1601+
const currentTgtEntityPath = currentTgtEntityIdPath
1602+
? extractEntityPath(currentTgtEntityIdPath)
1603+
: null;
1604+
// Different attribute ID → definitely a different target
1605+
if (reassignHoverTargetId !== currentTgtAttrId) return true;
1606+
// Same attribute ID but different entity path → different target
1607+
if (
1608+
reassignHoverTargetEntityPath != null &&
1609+
currentTgtEntityPath != null &&
1610+
reassignHoverTargetEntityPath !== currentTgtEntityPath
1611+
) return true;
1612+
// One path known and the other not → treat as different
1613+
if (
1614+
(reassignHoverTargetEntityPath != null) !== (currentTgtEntityPath != null)
1615+
) return true;
1616+
return false;
1617+
})();
15801618
try {
15811619
if (droppingOnTarget) {
1582-
// Determine target path from DOM or tree
1583-
const targetAttrEl = rightScrollRef.current?.querySelector(
1584-
`[data-attr-id="${reassignHoverTargetId}"]`
1585-
) as HTMLElement | null;
1586-
const tgtPathFromDom =
1587-
targetAttrEl?.dataset?.entityPath || null;
1620+
const tgtPathFromDom = reassignHoverTargetEntityPath;
15881621
const tgtPath =
15891622
tgtPathFromDom ??
15901623
(() => {
@@ -1923,7 +1956,9 @@ const MappingsView: React.FC = () => {
19231956
setReassignActive(false);
19241957
setReassignPaths([]);
19251958
setReassignHoverTargetId(null);
1959+
setReassignHoverTargetPath(null);
19261960
reassignHoverTargetIdRef.current = null;
1961+
reassignHoverTargetPathRef.current = null;
19271962
pendingReassignRef.current = null;
19281963
reassignProcessingRef.current = false;
19291964
}
@@ -1948,7 +1983,9 @@ const MappingsView: React.FC = () => {
19481983
if (!reassignActive) {
19491984
setReassignPaths([]);
19501985
setReassignHoverTargetId(null);
1986+
setReassignHoverTargetPath(null);
19511987
reassignHoverTargetIdRef.current = null;
1988+
reassignHoverTargetPathRef.current = null;
19521989
}
19531990
}, [reassignActive]);
19541991

@@ -2652,6 +2689,7 @@ const MappingsView: React.FC = () => {
26522689
disableInteractions={groupId < 0}
26532690
selectionContext={{
26542691
selectedTargetAttrId, setSelectedTargetAttrId,
2692+
selectedTargetAttrPath, setSelectedTargetAttrPath,
26552693
selectionIndex, setSelectionIndex,
26562694
selectionAll, setSelectionAll,
26572695
selectedTransformationIds, setSelectedTransformationIds,
@@ -2660,6 +2698,7 @@ const MappingsView: React.FC = () => {
26602698
setReassignTransformations:
26612699
setReassignTransformations as any,
26622700
reassignHoverTargetId,
2701+
reassignHoverTargetPath,
26632702
prepareReassign: (
26642703
e: React.MouseEvent,
26652704
transforms: any[]
@@ -2703,6 +2742,7 @@ const MappingsView: React.FC = () => {
27032742
disableInteractions={groupId < 0}
27042743
selectionContext={{
27052744
selectedTargetAttrId, setSelectedTargetAttrId,
2745+
selectedTargetAttrPath, setSelectedTargetAttrPath,
27062746
selectionIndex, setSelectionIndex,
27072747
selectionAll, setSelectionAll,
27082748
selectedTransformationIds, setSelectedTransformationIds,
@@ -2711,6 +2751,7 @@ const MappingsView: React.FC = () => {
27112751
setReassignTransformations:
27122752
setReassignTransformations as any,
27132753
reassignHoverTargetId,
2754+
reassignHoverTargetPath,
27142755
prepareReassign: (
27152756
e: React.MouseEvent,
27162757
transforms: any[]

‎frontends/mdr-frontend/src/pages/Explore/Mappings/components/ModelColumn.tsx‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,13 @@ export interface DisplayTransformationDataLike {
1616

1717
export interface SelectionContextLike {
1818
selectedTargetAttrId: number | null;
19+
selectedTargetAttrPath?: string | null;
1920
setSelectedTargetAttrId: React.Dispatch<
2021
React.SetStateAction<number | null>
2122
>;
23+
setSelectedTargetAttrPath?: React.Dispatch<
24+
React.SetStateAction<string | null>
25+
>;
2226
selectionIndex: number;
2327
setSelectionIndex: React.Dispatch<React.SetStateAction<number>>;
2428
selectionAll: boolean;
@@ -32,6 +36,7 @@ export interface SelectionContextLike {
3236
React.SetStateAction<DisplayTransformationDataLike[]>
3337
>;
3438
reassignHoverTargetId: number | null;
39+
reassignHoverTargetPath?: string | null;
3540
prepareReassign?: (
3641
e: React.MouseEvent,
3742
transforms: DisplayTransformationDataLike[]
@@ -179,9 +184,17 @@ const ModelColumn: React.FC<ModelColumnProps> = ({
179184
const inboundTs = (
180185
transformations || []
181186
).filter(
182-
(t) =>
183-
t.TargetAttribute?.AttributeId ===
184-
attr.Id
187+
(t) => {
188+
if (t.TargetAttribute?.AttributeId !== attr.Id) return false;
189+
// Also match entity path so same-AttributeId in different entities don't collide
190+
if (currentPath) {
191+
const tgtEntityIdPath = (t.TargetAttribute as any)?.EntityIdPath;
192+
if (tgtEntityIdPath) {
193+
return normalizeForComparison(tgtEntityIdPath) === currentPath;
194+
}
195+
}
196+
return true;
197+
}
185198
);
186199
const wires: Array<{
187200
trans: DisplayTransformationDataLike;
@@ -204,6 +217,7 @@ const ModelColumn: React.FC<ModelColumnProps> = ({
204217
if (!selectionContext) return;
205218
const {
206219
selectedTargetAttrId, setSelectedTargetAttrId,
220+
selectedTargetAttrPath, setSelectedTargetAttrPath,
207221
selectionIndex, setSelectionIndex,
208222
setSelectionAll,
209223
setSelectedTransformationIds,
@@ -212,14 +226,17 @@ const ModelColumn: React.FC<ModelColumnProps> = ({
212226
const SrcWireLength = inboundWires?.length || 0;
213227
if (!SrcWireLength) return;
214228
const inboundTsUnique = Array.from(new Set(inboundWires.map(w => w.trans.Id)));
229+
// Determine if this is the same target (same attr ID AND same entity path)
230+
const isSameTarget = selectedTargetAttrId === attr.Id &&
231+
(selectedTargetAttrPath == null || selectedTargetAttrPath === currentPath);
215232
// Shift = select ALL transformations (all wires)
216233
if (e.shiftKey) {
217234
setSelectedTargetAttrId(attr.Id);
235+
if (setSelectedTargetAttrPath) setSelectedTargetAttrPath(currentPath || null);
218236
setSelectionAll(true);
219237
setSelectionIndex(SrcWireLength);
220238
setSelectedTransformationIds(new Set(inboundTsUnique));
221239
if (setSelectedWireSourceAttrIds) {
222-
// empty set means highlight all, but we want explicit all sources of first trans? choose first trans's sources
223240
const firstTrans = inboundWires[0].trans;
224241
const allSrcIds = new Set<number>((firstTrans.SourceAttributes || []).map((s: any) => s.AttributeId));
225242
setSelectedWireSourceAttrIds(allSrcIds);
@@ -228,8 +245,9 @@ const ModelColumn: React.FC<ModelColumnProps> = ({
228245
}
229246
setSelectionAll(false);
230247
// Initial select or different target: pick first transformation
231-
if (selectedTargetAttrId !== attr.Id) {
248+
if (!isSameTarget) {
232249
setSelectedTargetAttrId(attr.Id);
250+
if (setSelectedTargetAttrPath) setSelectedTargetAttrPath(currentPath || null);
233251
setSelectionIndex(0);
234252
const firstTrans = inboundWires[0].trans;
235253
setSelectedTransformationIds(new Set([firstTrans.Id]));
@@ -245,11 +263,11 @@ const ModelColumn: React.FC<ModelColumnProps> = ({
245263
if (SelectAllWires) {
246264
// Select all wires
247265
setSelectedTargetAttrId(attr.Id);
266+
if (setSelectedTargetAttrPath) setSelectedTargetAttrPath(currentPath || null);
248267
setSelectionAll(true);
249268
setSelectionIndex(nextIndex);
250269
setSelectedTransformationIds(new Set(inboundTsUnique));
251270
if (setSelectedWireSourceAttrIds) {
252-
// empty set means highlight all, but we want explicit all sources of first trans? choose first trans's sources
253271
const firstTrans = inboundWires[0].trans;
254272
const allSrcIds = new Set<number>((firstTrans.SourceAttributes || []).map((s: any) => s.AttributeId));
255273
setSelectedWireSourceAttrIds(allSrcIds);
@@ -349,7 +367,9 @@ const ModelColumn: React.FC<ModelColumnProps> = ({
349367
<span
350368
className={`mappings-attr__name ${
351369
selectionContext?.reassignHoverTargetId ===
352-
attr.Id
370+
attr.Id &&
371+
(selectionContext?.reassignHoverTargetPath == null ||
372+
selectionContext?.reassignHoverTargetPath === node.PathId)
353373
? 'mappings-attr--drop-target'
354374
: ''
355375
}`}
@@ -819,7 +839,8 @@ const ModelColumn: React.FC<ModelColumnProps> = ({
819839
<span
820840
className={`mappings-attr__name ${
821841
selectionContext?.reassignHoverTargetId ===
822-
attr.Id
842+
attr.Id &&
843+
selectionContext?.reassignHoverTargetPath == null
823844
? 'mappings-attr--drop-target'
824845
: ''
825846
}`}

0 commit comments

Comments
 (0)