starred/CIPP

Fork 0

mirror of https://github.com/KelvinTegelaar/CIPP.git synced 2026-04-25 00:06:06 +03:00

[GH-ISSUE #5454] useEffect bloat refactor: extract data transforms and add validation guards #2628

New issue

Open

opened 2026-03-02 13:53:52 +03:00 by kerem · 1 comment

kerem commented

2026-03-02 13:53:52 +03:00

Owner

Originally created by @redanthrax on GitHub (Feb 24, 2026).
Original GitHub issue: https://github.com/KelvinTegelaar/CIPP/issues/5454

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 useEffect does 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 groupType classification runs synchronously inside
the effect — should be a useMemo.
setCombinedData() and setInitialValues() called in the same effect
as formControl.reset() — causes unnecessary batched re-renders.
No formControl.trigger() after reset() — 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 inside useEffect —
the data transform (timezone lookup, country lookup, tags mapping)
should be extracted to useMemo.
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)
useEffect body is a single line: resetForm()
Module-level Map for 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

Front End Version

latest

Back End Version

latest

Relevant Logs / Stack Trace

Originally created by @redanthrax on GitHub (Feb 24, 2026). Original GitHub issue: https://github.com/KelvinTegelaar/CIPP/issues/5454 ### Required confirmations before submitting - [x] **I can reproduce this issue on the latest released versions** of both CIPP and CIPP-API. - [x] **I have searched existing issues** (both open and closed) to avoid duplicates. - [x] 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:** 1. Single `useEffect` does 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. 2. Inline IIFE for `groupType` classification runs synchronously inside the effect — should be a `useMemo`. 3. `setCombinedData()` and `setInitialValues()` called in the same effect as `formControl.reset()` — causes unnecessary batched re-renders. 4. No `formControl.trigger()` after `reset()` — form appears valid on load even if loaded data violates field constraints. **Suggested fix:** ```js // 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:** 1. Large `formControl.reset()` payload built inline inside `useEffect` — the data transform (timezone lookup, country lookup, tags mapping) should be extracted to `useMemo`. 2. `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:** ```js // 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`) - `useEffect` body is a single line: `resetForm()` - Module-level `Map` for 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 ```plaintext ```

kerem added the

bug

unconfirmed-by-user

labels

2026-03-02 13:53:52 +03:00

kerem commented

2026-03-02 13:53:52 +03:00

Author

Owner

@github-actions[bot] commented on GitHub (Feb 24, 2026):

Thank you for reporting a potential bug. If you would like to work on this bug, please comment:

I would like to work on this please!

Thank you for helping us maintain the project!

@github-actions[bot] commented on GitHub (Feb 24, 2026): Thank you for reporting a potential bug. If you would like to work on this bug, please comment: > I would like to work on this please! Thank you for helping us maintain the project!

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

starred/CIPP#2628

No description provided.

Rows
Columns

[GH-ISSUE #5454] useEffect bloat refactor: extract data transforms and add validation guards #2628

Required confirmations before submitting

Issue Description

Summary

Affected Files

src/pages/identity/administration/groups/edit.jsx — HIGH PRIORITY

src/pages/email/resources/management/list-rooms/edit.jsx — MEDIUM PRIORITY

Reference Implementation

Impact

Environment Type

Front End Version

Back End Version

Relevant Logs / Stack Trace

`src/pages/identity/administration/groups/edit.jsx` — HIGH PRIORITY

`src/pages/email/resources/management/list-rooms/edit.jsx` — MEDIUM PRIORITY