Skip to content

Commit 67efbb4

Browse files
cbeach47cbeach47
authored
Issue #798: second pass (#836)
follow up to address additional comments from #831 related to #798
2 parents d359e09 + 24c4fee commit 67efbb4

5 files changed

Lines changed: 118 additions & 68 deletions

File tree

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

Lines changed: 88 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import {
3535
parseEntityIdPath,
3636
appendAttributeToPath,
3737
extractEntityIds,
38+
buildAttributeLookupKey,
3839
} from '../../../utils/entityIdPath';
3940
import {
4041
DataModelWithDetailsDTO,
@@ -119,7 +120,7 @@ const MappingsView: React.FC = () => {
119120
const leftScrollRef = useRef<HTMLDivElement | null>(null);
120121
const rightScrollRef = useRef<HTMLDivElement | null>(null);
121122
const wiresSlotRef = useRef<HTMLDivElement | null>(null);
122-
// Path-aware attribute element registries: keys are `${EntityIdPath}|${AttributeId}` or just `${AttributeId}` as fallback
123+
// Path-aware attribute element registries: keys are `${PathId}|${AttributeId}` or just `${AttributeId}` as fallback
123124
const attrElementsLeft = useRef<Map<string, HTMLElement>>(new Map());
124125
const attrElementsRight = useRef<Map<string, HTMLElement>>(new Map());
125126

@@ -168,6 +169,13 @@ const MappingsView: React.FC = () => {
168169
startY: number;
169170
transforms: DisplayTransformationData[];
170171
} | null>(null);
172+
// Ref mirror of reassignHoverTargetId so the reassign effect's handleUp reads
173+
// the latest value without needing reassignHoverTargetId in the dep array
174+
// (which caused effect re-runs + duplicate handleUp registrations).
175+
const reassignHoverTargetIdRef = useRef<number | null>(null);
176+
// Guard against duplicate handleUp invocations caused by the synthetic mouseup
177+
// that handleMove dispatches after the button is released.
178+
const reassignProcessingRef = useRef(false);
171179

172180
// Build a JSONata-compatible expression path like EntityA.EntityB.Attribute from an EntityIdPath.
173181
// Uses entity/attribute NAMES (not IDs) because JSONata navigates JSON documents by property names.
@@ -241,6 +249,7 @@ const MappingsView: React.FC = () => {
241249
setReassignTransformations([]);
242250
setReassignPaths([]);
243251
setReassignHoverTargetId(null);
252+
reassignHoverTargetIdRef.current = null;
244253
setSelectedTargetAttrId(null);
245254
setSelectedTransformationIds(new Set());
246255
setSelectionAll(false);
@@ -1103,14 +1112,34 @@ const MappingsView: React.FC = () => {
11031112
return undefined;
11041113
})();
11051114

1115+
// Build the full source EntityIdPath (includes attribute as negative suffix)
1116+
const fullSrcEntityIdPath = appendAttributeToPath(srcPath, dragSourceAttr.Id);
1117+
// Build the full target EntityIdPath for comparison (includes attribute as negative suffix)
1118+
const fullTgtEntityIdPath = appendAttributeToPath(tgtPath, dragTargetAttrId);
1119+
1120+
// Check if this exact source->target pair already exists in ANY transformation
1121+
const duplicateExists = (group.Transformations || []).some((t) => {
1122+
const tpath = (t.TargetAttribute as any)?.EntityIdPath || '';
1123+
const targetMatches =
1124+
t.TargetAttribute?.AttributeId === dragTargetAttrId &&
1125+
String(tpath) === String(fullTgtEntityIdPath || '');
1126+
if (!targetMatches) return false;
1127+
// Check if this specific source attribute is already mapped
1128+
const srcs: any[] = Array.isArray((t as any).SourceAttributes) ? (t as any).SourceAttributes : [];
1129+
return srcs.some((s: any) => String(s.EntityIdPath || '') === String(fullSrcEntityIdPath || ''));
1130+
});
1131+
if (duplicateExists) {
1132+
// Source->target pair already exists; silently cancel
1133+
return;
1134+
}
1135+
11061136
// Enforce 1 transformation per target (EntityIdPath+AttributeId)
11071137
const existing = (group.Transformations || []).find((t) => {
1108-
const tid = t.TargetAttribute?.AttributeId;
11091138
const tpath =
1110-
(t.TargetAttribute as any)?.EntityIdPath || null;
1139+
(t.TargetAttribute as any)?.EntityIdPath || '';
11111140
return (
1112-
tid === dragTargetAttrId &&
1113-
String(tpath || '') === String(tgtPath || '')
1141+
t.TargetAttribute?.AttributeId === dragTargetAttrId &&
1142+
String(tpath) === String(fullTgtEntityIdPath || '')
11141143
);
11151144
});
11161145

@@ -1146,8 +1175,7 @@ const MappingsView: React.FC = () => {
11461175
},
11471176
]
11481177
: [];
1149-
const key = (sa: any) =>
1150-
`${sa.EntityIdPath || ''}|${sa.AttributeId}`;
1178+
const key = (sa: any) => String(sa.EntityIdPath || '');
11511179
const nextSrcs = [...currentSrcs];
11521180
const deriveEntityIdForUpdate = (
11531181
path: string | null | undefined,
@@ -1175,9 +1203,12 @@ const MappingsView: React.FC = () => {
11751203
EntityIdPath: appendAttributeToPath(srcPath, dragSourceAttr.Id),
11761204
EntityId: deriveEntityIdForUpdate(srcPath, dragSourceAttr.Id),
11771205
} as any;
1178-
if (!nextSrcs.some((s) => key(s) === key(newSrc))) {
1179-
nextSrcs.push(newSrc);
1206+
const sourceAlreadyExists = nextSrcs.some((s) => key(s) === key(newSrc));
1207+
if (sourceAlreadyExists) {
1208+
// Source->target pair already exists; cancel without making any changes
1209+
return;
11801210
}
1211+
nextSrcs.push(newSrc);
11811212
const updated = await updateTransformationAttributes(
11821213
existing.Id,
11831214
{
@@ -1361,7 +1392,7 @@ const MappingsView: React.FC = () => {
13611392

13621393
const computeStart = (srcAttrId: number) => {
13631394
const srcEntry = (transformation as any).SourceAttributes?.find((s: any) => s.AttributeId === srcAttrId);
1364-
const srcKey = srcEntry?.EntityIdPath ? `${srcEntry.EntityIdPath}|${srcAttrId}` : String(srcAttrId);
1395+
const srcKey = buildAttributeLookupKey(srcEntry?.EntityIdPath, srcAttrId);
13651396
const leftEl = attrElementsLeft.current.get(srcKey) || attrElementsLeft.current.get(String(srcAttrId));
13661397
const leftDot = leftEl?.querySelector<HTMLElement>('.mappings-column__dot--end') || leftEl || null;
13671398
const lb = leftDot?.getBoundingClientRect();
@@ -1496,10 +1527,8 @@ const MappingsView: React.FC = () => {
14961527
(t as any).SourceAttributes?.[0];
14971528
const srcId = srcAttr?.AttributeId;
14981529
if (!srcId) return;
1499-
const srcKey = srcAttr?.EntityIdPath
1500-
? `${srcAttr.EntityIdPath}|${srcId}`
1501-
: String(srcId);
1502-
const leftEl = attrElementsLeft.current.get(srcKey);
1530+
const srcKey = buildAttributeLookupKey(srcAttr?.EntityIdPath, srcId);
1531+
const leftEl = attrElementsLeft.current.get(srcKey) || attrElementsLeft.current.get(String(srcId));
15031532
if (!leftEl) return;
15041533
const leftDot =
15051534
leftEl.querySelector<HTMLElement>(
@@ -1534,12 +1563,21 @@ const MappingsView: React.FC = () => {
15341563
}
15351564
node = node.parentElement;
15361565
}
1566+
reassignHoverTargetIdRef.current = targetId;
15371567
setReassignHoverTargetId(targetId);
15381568
};
15391569
const handleUp = async () => {
1570+
// Guard: prevent duplicate invocations caused by the synthetic mouseup
1571+
// that handleMove dispatches when e.buttons indicates the button is already released.
1572+
if (reassignProcessingRef.current) return;
1573+
reassignProcessingRef.current = true;
1574+
// Read from ref so we always get the latest hover target, regardless of
1575+
// which effect-closure version of handleUp fires.
1576+
const reassignHoverTargetId = reassignHoverTargetIdRef.current;
15401577
const droppingOnTarget =
15411578
!!reassignHoverTargetId &&
15421579
reassignHoverTargetId !== selectedTargetAttrId;
1580+
try {
15431581
if (droppingOnTarget) {
15441582
// Determine target path from DOM or tree
15451583
const targetAttrEl = rightScrollRef.current?.querySelector(
@@ -1565,20 +1603,38 @@ const MappingsView: React.FC = () => {
15651603
})();
15661604

15671605
const results: Array<TransformationData | null> = [];
1606+
// Build full target EntityIdPath (includes attribute as negative suffix) for proper comparison
1607+
const fullReassignTgtPath = appendAttributeToPath(tgtPath, reassignHoverTargetId!);
15681608
for (const t of reassignTransformations) {
15691609
try {
15701610
const firstSrc: any = (t as any).SourceAttributes?.[0];
1611+
const srcEntityIdPath = String((firstSrc as any)?.EntityIdPath || '');
1612+
1613+
// Check if this exact source->target pair already exists; if so, skip entirely (revert the move)
1614+
const duplicateExists = (group?.Transformations || []).some((et) => {
1615+
if (et.Id === t.Id) return false; // don't match the transformation being moved
1616+
const tpath = (et.TargetAttribute as any)?.EntityIdPath || '';
1617+
const targetMatches =
1618+
et.TargetAttribute?.AttributeId === reassignHoverTargetId &&
1619+
String(tpath) === String(fullReassignTgtPath || '');
1620+
if (!targetMatches) return false;
1621+
const srcs: any[] = Array.isArray((et as any).SourceAttributes) ? (et as any).SourceAttributes : [];
1622+
return srcs.some((s: any) => String(s.EntityIdPath || '') === srcEntityIdPath);
1623+
});
1624+
if (duplicateExists) {
1625+
// Source->target pair already exists; skip this reassignment (revert)
1626+
results.push(null);
1627+
continue;
1628+
}
1629+
15711630
// Check if a transformation already exists for this target (id+path)
15721631
const existing = (group?.Transformations || []).find(
15731632
(et) => {
1574-
const tid = et.TargetAttribute?.AttributeId;
15751633
const tpath =
1576-
(et.TargetAttribute as any)?.EntityIdPath ||
1577-
null;
1634+
(et.TargetAttribute as any)?.EntityIdPath || '';
15781635
return (
1579-
tid === reassignHoverTargetId &&
1580-
String(tpath || '') ===
1581-
String(tgtPath || '')
1636+
et.TargetAttribute?.AttributeId === reassignHoverTargetId &&
1637+
String(tpath) === String(fullReassignTgtPath || '')
15821638
);
15831639
}
15841640
);
@@ -1603,8 +1659,7 @@ const MappingsView: React.FC = () => {
16031659
},
16041660
]
16051661
: [];
1606-
const key = (sa: any) =>
1607-
`${sa.EntityIdPath || ''}|${sa.AttributeId}`;
1662+
const key = (sa: any) => String(sa.EntityIdPath || '');
16081663
const merged = [...currentSrcs];
16091664
const newSrc = {
16101665
AttributeId: firstSrc?.AttributeId || firstSrc?.Id,
@@ -1631,12 +1686,13 @@ const MappingsView: React.FC = () => {
16311686
return undefined;
16321687
})(),
16331688
} as any;
1634-
if (
1635-
newSrc.AttributeId &&
1636-
!merged.some((s) => key(s) === key(newSrc))
1637-
) {
1638-
merged.push(newSrc);
1689+
const sourceAlreadyExists = newSrc.AttributeId && merged.some((s) => key(s) === key(newSrc));
1690+
if (sourceAlreadyExists) {
1691+
// Source->target pair already exists in target; skip this reassignment (don't merge or delete)
1692+
results.push(null);
1693+
continue;
16391694
}
1695+
merged.push(newSrc);
16401696
const updated =
16411697
await updateTransformationAttributes(
16421698
existing.Id,
@@ -1881,11 +1937,15 @@ const MappingsView: React.FC = () => {
18811937
cleanupSelection();
18821938
}
18831939
}
1940+
} finally {
18841941
// Always exit reassign mode and clear dashed overlays
18851942
setReassignActive(false);
18861943
setReassignPaths([]);
18871944
setReassignHoverTargetId(null);
1945+
reassignHoverTargetIdRef.current = null;
18881946
pendingReassignRef.current = null;
1947+
reassignProcessingRef.current = false;
1948+
}
18891949
};
18901950
window.addEventListener('mousemove', handleMove);
18911951
window.addEventListener('mouseup', handleUp, { once: true });
@@ -1896,7 +1956,6 @@ const MappingsView: React.FC = () => {
18961956
}, [
18971957
reassignActive,
18981958
reassignTransformations,
1899-
reassignHoverTargetId,
19001959
selectedTargetAttrId,
19011960
cleanupSelection,
19021961
wireDetachDragging,
@@ -1908,6 +1967,7 @@ const MappingsView: React.FC = () => {
19081967
if (!reassignActive) {
19091968
setReassignPaths([]);
19101969
setReassignHoverTargetId(null);
1970+
reassignHoverTargetIdRef.current = null;
19111971
}
19121972
}, [reassignActive]);
19131973

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import type {
55
AttributeDTO,
66
EntityTreeNode,
77
} from '../../../../types';
8-
import { apiPathToDotFormat, isNewCommaFormat } from '../../../../utils/entityIdPath';
8+
import { extractEntityPath, isNewCommaFormat } from '../../../../utils/entityIdPath';
99

1010
// Types re-declared locally to avoid tight coupling; import actual interfaces from caller when wiring up
1111
export interface DisplayTransformationDataLike {
@@ -140,8 +140,8 @@ const ModelColumn: React.FC<ModelColumnProps> = ({
140140
const normalizeForComparison = (apiPath: string | null | undefined): string => {
141141
if (!apiPath) return '';
142142
if (isNewCommaFormat(apiPath)) {
143-
// Both internal PathId and API format use comma-separated format now
144-
return apiPathToDotFormat(apiPath).dotPath;
143+
// Extract entity-only path from EntityIdPath (strips negative attribute suffix)
144+
return extractEntityPath(apiPath);
145145
}
146146
// For legacy name-based paths, we can't match against numeric PathId
147147
// Return the apiPath as-is (it won't match, which is correct behavior)

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export type WirePath = {
1111
tgtPathName?: string;
1212
srcEntityIdPath?: string;
1313
tgtEntityIdPath?: string;
14-
srcKey?: string; // EntityIdPath|AttrId composite
14+
srcKey?: string; // PathId|AttrId composite for element registry matching
1515
tgtKey?: string;
1616
};
1717

‎frontends/mdr-frontend/src/pages/Explore/Mappings/hooks/useMappingWires.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { MutableRefObject, useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react';
2-
import { buildAttributeLookupKey, apiPathToDotFormat } from '../../../../utils/entityIdPath';
2+
import { buildAttributeLookupKey } from '../../../../utils/entityIdPath';
33

44
// Track warned paths to avoid spamming console on every scroll
55
const warnedPaths = new Set<string>();

0 commit comments

Comments
 (0)