Skip to content

Bug fix for creating new racks on top of previous real racks #1001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LeviCameron1 merged 1 commit into from
Aug 17, 2026
+75 −61
Merged

Bug fix for creating new racks on top of previous real racks #1001

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 75 additions & 61 deletions CageUI/src/client/context/LayoutEditorContextManager.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,7 +1017,9 @@ export const LayoutEditorContextProvider: FC<LayoutContextProps> = ({children, p
};

const changeRack = async (newType: RackChangeOption): Promise<string | null> => {
let {value: rackChangeValue, label: rackLabel} = newType;
const { value: rackChangeValue } = newType;
const originalSelectedCage = selectedObj as Cage;

@aschmidt34 aschmidt34 Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'selectedObj' is typed 'SelectedObj | null' on line 151

Might want to guard against a null cast here? I see this is how you cast though in other parts of the module so this might be okay, up to you. I doubt 'selectedObj' can ever truly be null for this.

const selectedCagePositionId = originalSelectedCage.positionId;

let prevCages: CageData[] = [];
if (!rackChangeValue.isNew) {
Expand All @@ -1029,72 +1031,84 @@ export const LayoutEditorContextProvider: FC<LayoutContextProps> = ({children, p
]
};
const cageDataRes = await labkeyActionSelectWithPromise(cagesInRackConfig);
prevCages = cageDataRes.rows.map(r => ({
...r,
positionId: r.positionid,
objectId: r.objectid,
}));
if(cageDataRes.rowCount !== 0){
prevCages = cageDataRes.rows.map(r => ({
...r,
positionId: r.positionid,
objectId: r.objectid,
}));
}
}

setLocalRoom(prevRoom => {
const {
rackGroup,
rack,
cage
} = findCageInGroup((selectedObj as Cage).svgId as CageSvgId, prevRoom.rackGroups);
const roomToUpdate: Room = {
...prevRoom,
rackGroups: prevRoom.rackGroups.map(group =>
group.groupId === rackGroup.groupId
? {
...group,
racks: group.racks.map((r) => r.objectId === rack.objectId ? {
...r,
itemId: rackChangeValue.rackId,
objectId: rackChangeValue.rackObjectId,
svgId: `rack_${rackChangeValue.rackObjectId}`,
isNew: rackChangeValue.isNew,
type: {
...r.type,
rowid: rackChangeValue.rackType.rowid,
displayName: rackChangeValue.rackType.displayName,
type: rackChangeValue.rackType.type,
isDefault: rackChangeValue.rackType.isDefault
},
cages: prevCages.length > 0 ? r.cages.map((c) => {
const prevCage = prevCages.find(pc => pc.positionId === c.positionId);
const key = roomItemToString(rackChangeValue.rackType.type);
const newSvgId = `cageSVG_${prevCage.objectId}`;
setUnitLocs((prevState) => ({
...prevState,
[key]: prevState[key].map((loc) => {
if (loc.cageId === c.svgId) {
return {
...loc,
cageId: newSvgId
};
}
return loc;
})
}));
return {
...c,
objectId: prevCage.objectId,
svgId: newSvgId,
positionId: prevCage.positionId,
};
}) : r.cages
} as Rack : r)
}
: group
)
};
setReloadRoom(roomToUpdate);
return roomToUpdate;
const { rackGroup, rack } = findCageInGroup(originalSelectedCage.svgId, localRoom.rackGroups);

@aschmidt34 aschmidt34 Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed the 'prevRoom => ...' from the passed in args here. In your other function calls, 'prevRoom' is always the most current state when the function is called.

With this new version, 'localRoom' is just a variable captured when the render happens, not when the function is called.

SCENARIO:

  1. User triggers setLocalRoom(update A) with moveObjLocation.
  2. Before React re-renders and changeRack gets the new localRoom, the user triggers changeRack (which reads the stale localRoom without 'update A').
  3. changeRack computes roomToUpdate based on the old room and calls setLocalRoom(roomToUpdate).
  4. Because setLocalRoom(roomToUpdate) passes a plain object instead of an updater function, it overwrites state directly with a room that doesn't include 'update A' and 'update A' changes are silently lost.

This is a race condition, and I doubt users will ever make a change before this can take effect, but just wanted you to be aware. changeRack awaits a network call (labkeyActionSelectWithPromise) before reading localRoom so there's a small chance at a race condition error here.

const newUnitLocs = { ...unitLocs };

@aschmidt34 aschmidt34 Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a shallow copy, so the inner array 'newUnitLocs[key]' is referencing the same 'unitLocs[key]'. If you mutate one, it will effect both.

On line 1061, this inner array is updated, meaning the OG value is also updated, which might mutate the currently-rendered unitLocs in-place before setUnitLocs (which references the OG value) commits a new state (since newUnitLocs[key] and the OG unitLocs[key] are the same array reference.)

Not sure how setUnitLocs works or when it runs, this might not be an issue, just wanted you to be aware.

let newSelectedCage: Cage | undefined;

const updatedCages = rack.cages.map(c => {
const key = roomItemToString(rackChangeValue.rackType.type);
let newCageData: { objectId: string, svgId: CageSvgId };

if (rackChangeValue.isNew) {
const newObjId = generateUUID();
newCageData = { objectId: newObjId, svgId: `cageSVG_${newObjId}` as CageSvgId };
} else {
const prevCage = prevCages.find(pc => pc.positionId === c.positionId);

@aschmidt34 aschmidt34 Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'prevCages' is populated by 'cagesInRackConfig' query which is keyed on 'rackChangeValue.rackObjectId', and the map iterates over 'rack.cages' (the old rack's cages) to find matching 'positionId' (in new rack cages).

If old rack has more items than the new rack or position numbering isn't 1:1, prevCages.find() returns undefined and prevCage.objectId throws.

Only marking this because your original code had a guard: 'prevCages.length > 0 ? ... : r.cages'

newCageData = { objectId: prevCage.objectId, svgId: `cageSVG_${prevCage.objectId}` as CageSvgId };
}

const locIndex = newUnitLocs[key]?.findIndex(loc => loc.cageId === c.svgId);
if (locIndex > -1) {
newUnitLocs[key][locIndex] = { ...newUnitLocs[key][locIndex], cageId: newCageData.svgId };
}

const newCage: Cage = { ...c, ...newCageData };
if (c.positionId === selectedCagePositionId) {
newSelectedCage = newCage;
}
return newCage;
});

const roomToUpdate: Room = {
...localRoom,
rackGroups: localRoom.rackGroups.map(group =>
group.groupId === rackGroup.groupId
? {
...group,
racks: group.racks.map(r =>
r.objectId === rack.objectId
? {
...r,
itemId: rackChangeValue.rackId,
objectId: rackChangeValue.rackObjectId,
svgId: `rack_${rackChangeValue.rackObjectId}`,
isNew: rackChangeValue.isNew,
type: {
...r.type,
rowid: rackChangeValue.rackType.rowid,
displayName: rackChangeValue.rackType.displayName,
type: rackChangeValue.rackType.type,
isDefault: rackChangeValue.rackType.isDefault,
},
cages: updatedCages,
}
: r
),
}
: group
),
};

setUnitLocs(newUnitLocs);
setLocalRoom(roomToUpdate);
if (newSelectedCage) {
setSelectedObj(newSelectedCage);
}
setReloadRoom(roomToUpdate);

return `rack_${rackChangeValue.rackObjectId}`;
};


const changeCageNum = (numBefore: number, numAfter: number) => {
const selectedCage = (selectedObj as Cage);

Expand Down