-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Closed
Labels
Description
Required confirmations before submitting
- I can reproduce this issue on the latest released versions of both CIPP and CIPP-API.
- I have searched existing issues (both open and closed) to avoid duplicates.
- I am not requesting general support; this is an actual bug report.
Issue Description
Summary
Several edit components in CIPP have useEffect hooks doing too much work inline —
data transformation, state updates, and form resets all in a single block.
This makes them hard to read and introduces subtle bugs (missing validation
triggers, no stale-closure guards).
contacts/edit.jsx already implements the correct pattern and should be
treated as the reference implementation for this refactor.
Affected Files
src/pages/identity/administration/groups/edit.jsx — HIGH PRIORITY
Issues:
- Single
useEffectdoes 4 things: builds membership table data,
classifies group type (inline IIFE), sets initial values for change
detection, and resets the form. This should be split. - Inline IIFE for
groupTypeclassification runs synchronously inside
the effect — should be auseMemo. setCombinedData()andsetInitialValues()called in the same effect
asformControl.reset()— causes unnecessary batched re-renders.- No
formControl.trigger()afterreset()— form appears valid on load
even if loaded data violates field constraints.
Suggested fix:
// Extract to useMemo outside the effect
const groupType = useMemo(() => {
const group = groupInfo.data?.groupInfo;
if (!group) return null;
if (group.groupTypes?.includes("Unified")) return "Microsoft 365";
if (!group.mailEnabled && group.securityEnabled) return "Security";
if (group.mailEnabled && group.securityEnabled) return "Mail-Enabled Security";
if (!group.groupTypes?.length && group.mailEnabled && !group.securityEnabled)
return "Distribution List";
return null;
}, [groupInfo.data]);
// Split effects by concern:
// Effect 1: table data
// Effect 2: form reset + trigger
useEffect(() => {
if (!groupInfo.isSuccess || !groupInfo.data?.groupInfo) return;
let cancelled = false;
formControl.reset(formValues);
formControl.trigger().then(() => {
if (cancelled) return;
});
return () => { cancelled = true; };
}, [groupInfo.isSuccess, groupInfo.isFetching]);src/pages/email/resources/management/list-rooms/edit.jsx — MEDIUM PRIORITY
Issues:
- Large
formControl.reset()payload built inline insideuseEffect—
the data transform (timezone lookup, country lookup, tags mapping)
should be extracted touseMemo. void formControl.trigger()present (good) but no stale-closure guard
— if the component unmounts while trigger is in-flight, the resolved
promise touches dead state.
Suggested fix:
// Extract reset payload to useMemo
const roomFormValues = useMemo(() => {
if (!roomInfo.isSuccess || !roomInfo.data?.[0]) return null;
const room = roomInfo.data[0];
return {
// ... all field mappings here
};
}, [roomInfo.isSuccess, roomInfo.data]);
// Lean effect with cancellation guard
useEffect(() => {
if (!roomFormValues) return;
let cancelled = false;
formControl.reset(roomFormValues);
formControl.trigger().then(() => {
if (cancelled) return;
});
return () => { cancelled = true; };
}, [roomFormValues]);Reference Implementation
src/pages/email/administration/contacts/edit.jsx already does this correctly:
- Data transform extracted to
useMemo(processedContactData) - Reset logic extracted to
useCallback(resetForm) useEffectbody is a single line:resetForm()- Module-level
Mapfor O(1) country lookup
This file should be the template for the above refactors.
Impact
- Improves readability and separation of concerns
- Fixes silent validation-pass-on-load bug in groups and rooms editors
- Eliminates stale-closure risk on unmount for async trigger calls
- Reduces unnecessary re-renders from batched state + form updates in
the same effect
Environment Type
Sponsored (paying) user
Front End Version
latest
Back End Version
latest
Relevant Logs / Stack Trace
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
Projects
Status
Done