Skip to content

Fixes #38899 - Enable react-hooks/exhaustive-deps eslint rule #1016

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
MariaAga merged 4 commits into from
Dec 9, 2025
Merged

Fixes #38899 - Enable react-hooks/exhaustive-deps eslint rule #1016

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
edf00db
Fixes #38899 - Enable react-hooks/exhaustive-deps eslint rule
jeremylenz Nov 11, 2025
19df596
Fixes #38899 - Remove jobTemplate from useEffect dependencies
jeremylenz Nov 11, 2025
d79e3bb
Refs #38899 - Address Copilot review comments and fix category reset bug
jeremylenz Dec 4, 2025
edb3ae9
Refs #38899 - Update test snapshot for ResourceSelect fix
jeremylenz Dec 4, 2025
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
164 changes: 95 additions & 69 deletions webpack/JobInvocationDetail/JobInvocationHostTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ import TableIndexPage from 'foremanReact/components/PF4/TableIndexPage/TableInde
import { getControllerSearchProps } from 'foremanReact/constants';
import { Icon } from 'patternfly-react';
import PropTypes from 'prop-types';
import React, { useEffect, useMemo, useState, useRef } from 'react';
import React, {
useEffect,
useMemo,
useState,
useRef,
useCallback,
} from 'react';
import { FormattedMessage } from 'react-intl';
import { useHistory } from 'react-router-dom';
import { useForemanSettings } from 'foremanReact/Root/Context/ForemanContext';
Expand Down Expand Up @@ -93,19 +99,22 @@ const JobInvocationHostTable = ({
});

// Search filter
const constructFilter = (filter = initialFilter, search = urlSearchQuery) => {
const dropdownFilterClause =
filter && filter !== 'all_statuses'
? `job_invocation.result = ${filter}`
: null;
const parts = [dropdownFilterClause, search];
return parts
.filter(x => x)
.map(fragment => `(${fragment})`)
.join(' AND ');
};
const constructFilter = useCallback(
(filter = initialFilter, search = urlSearchQuery) => {
const dropdownFilterClause =
filter && filter !== 'all_statuses'
? `job_invocation.result = ${filter}`
: null;
const parts = [dropdownFilterClause, search];
return parts
.filter(x => x)
.map(fragment => `(${fragment})`)
.join(' AND ');
},
[initialFilter, urlSearchQuery]
);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Prevents filterApiCall from changing on every render.


const handleResponse = (data, key) => {
const handleResponse = useCallback((data, key) => {
if (key === JOB_INVOCATION_HOSTS) {
const ids = data.data.results.map(i => i.id);

Expand All @@ -114,76 +123,94 @@ const JobInvocationHostTable = ({
}
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Stabilizes reference for use in makeApiCall dependency array.


setStatus(STATUS_UPPERCASE.RESOLVED);
};
}, []);

// Call hosts data with params
const makeApiCall = (requestParams, callParams = {}) => {
dispatch(
APIActions.get({
key: callParams.key ?? ALL_JOB_HOSTS,
url: callParams.url ?? `/api/job_invocations/${id}/hosts`,
params: requestParams,
handleSuccess: data => handleResponse(data, callParams.key),
handleError: () => setStatus(STATUS_UPPERCASE.ERROR),
errorToast: ({ response }) =>
response?.data?.error?.full_messages?.[0] || response,
})
);
};
const makeApiCall = useCallback(
(requestParams, callParams = {}) => {
dispatch(
APIActions.get({
key: callParams.key ?? ALL_JOB_HOSTS,
url: callParams.url ?? `/api/job_invocations/${id}/hosts`,
params: requestParams,
handleSuccess: data => handleResponse(data, callParams.key),
handleError: () => setStatus(STATUS_UPPERCASE.ERROR),
errorToast: ({ response }) =>
response?.data?.error?.full_messages?.[0] || response,
})
);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Used as a dependency in filterApiCall and initialization useEffect.

},
[dispatch, id, handleResponse]
);

const filterApiCall = newAPIOptions => {
const newParams = newAPIOptions?.params ?? newAPIOptions ?? {};
const filterApiCall = useCallback(
newAPIOptions => {
const newParams = newAPIOptions?.params ?? newAPIOptions ?? {};

const filterSearch = constructFilter(
initialFilter,
newParams.search ?? urlSearchQuery
);

const finalParams = {
...defaultParams,
...newParams,
};

if (filterSearch === AWAITING_STATUS_FILTER) {
finalParams.awaiting = 'true';
} else if (filterSearch !== '') {
finalParams.search = filterSearch;
}
const filterSearch = constructFilter(
initialFilter,
newParams.search ?? urlSearchQuery
);

makeApiCall(finalParams, { key: JOB_INVOCATION_HOSTS });
const finalParams = {
...defaultParams,
...newParams,
};

const urlSearchParams = new URLSearchParams(window.location.search);
if (filterSearch === AWAITING_STATUS_FILTER) {
finalParams.awaiting = 'true';
} else if (filterSearch !== '') {
finalParams.search = filterSearch;
}

['page', 'per_page', 'order'].forEach(key => {
if (finalParams[key]) urlSearchParams.set(key, finalParams[key]);
});
makeApiCall(finalParams, { key: JOB_INVOCATION_HOSTS });

history.push({ search: urlSearchParams.toString() });
};
const urlSearchParams = new URLSearchParams(window.location.search);

['page', 'per_page', 'order'].forEach(key => {
if (finalParams[key]) urlSearchParams.set(key, finalParams[key]);
});

history.push({ search: urlSearchParams.toString() });
},
[
initialFilter,
urlSearchQuery,
defaultParams,
makeApiCall,
history,
constructFilter,
]
);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Used as a dependency in useEffect below.


// Filter change
const handleFilterChange = newFilter => {
onFilterUpdate(newFilter);
};
const handleFilterChange = useCallback(
newFilter => {
onFilterUpdate(newFilter);
},
[onFilterUpdate]
);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Prevents unnecessary re-renders when passed to child components.


// Effects
// run after mount
const initializedRef = useRef(false);
useEffect(() => {
// Job Invo template load
makeApiCall(
{},
{
url: `/job_invocations/${id}/hosts`,
key: LIST_TEMPLATE_INVOCATIONS,
if (!initializedRef.current) {
// Job Invo template load
makeApiCall(
{},
{
url: `/job_invocations/${id}/hosts`,
key: LIST_TEMPLATE_INVOCATIONS,
}
);

if (initialFilter === '') {
onFilterUpdate('all_statuses');
}
);

if (initialFilter === '') {
onFilterUpdate('all_statuses');
initializedRef.current = true;
}

// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
}, [makeApiCall, id, initialFilter, onFilterUpdate]);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

The initializedRef pattern allows mount-only behavior while satisfying exhaustive-deps.


useEffect(() => {
if (initialFilter !== '') filterApiCall();
Expand All @@ -192,8 +219,7 @@ const JobInvocationHostTable = ({
prevStatusLabel.current = statusLabel;
filterApiCall();
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [initialFilter, statusLabel, id]);
}, [initialFilter, statusLabel, id, filterApiCall]);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Without filterApiCall in deps, the effect could capture a stale closure.


const {
updateSearchQuery: updateSearchQueryBulk,
Expand Down
9 changes: 4 additions & 5 deletions webpack/JobInvocationDetail/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,14 @@ const JobInvocationDetailPage = ({
return () => {
dispatch(stopInterval(JOB_INVOCATION_KEY));
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [dispatch, id, finished, autoRefresh]);
Copy link
Member

@MariaAga MariaAga Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@jeremylenz Here there are also functions that are used and not in the dependency array (getJobInvocation, stopInterval), should they also be added?

Copy link
Contributor Author

@jeremylenz jeremylenz Jan 24, 2026

Choose a reason for hiding this comment

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

No, if they are functions imported from another file or created from outside the component, the rule doesn't care about them.


const taskId = task?.id;
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Complex expressions in dependency arrays can't be statically analyzed by the linter.

useEffect(() => {
if (task?.id !== undefined) {
dispatch(getTask(`${task?.id}`));
if (taskId !== undefined) {
dispatch(getTask(`${taskId}`));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [dispatch, task?.id]);
}, [dispatch, taskId]);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Allows tracking task ID changes without depending on the entire task object.


const pageStatus =
items.id === undefined
Expand Down
16 changes: 10 additions & 6 deletions webpack/JobWizard/JobWizard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,7 @@ export const JobWizard = ({ rerunData }) => {
};
});
},
// eslint-disable-next-line react-hooks/exhaustive-deps
[category.length]
[category, setCategory, setTemplateValues, setAdvancedValues]
);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Used as a dependency in two useEffect hooks below.

useEffect(() => {
if (rerunData) {
Expand All @@ -153,8 +152,7 @@ export const JobWizard = ({ rerunData }) => {
},
});
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [rerunData]);
}, [rerunData, setDefaults]);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Without setDefaults in deps, the effect could use a stale version.

useEffect(() => {
if (jobTemplateID) {
dispatch(
Expand Down Expand Up @@ -199,8 +197,14 @@ export const JobWizard = ({ rerunData }) => {
})
);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [rerunData, jobTemplateID, dispatch]);
}, [
rerunData,
jobTemplateID,
dispatch,
setDefaults,
setTemplateValues,
setAdvancedValues,
]);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

All three functions are called within the effect and need to be tracked to prevent stale closures.


const [isStartsBeforeError, setIsStartsBeforeError] = useState(false);
const [isStartsAtError, setIsStartsAtError] = useState(false);
Expand Down
12 changes: 10 additions & 2 deletions webpack/JobWizard/autofill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,14 @@ export const useAutoFill = ({
});
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [fills]);
}, [
fills,
setFills,
setSelectedTargets,
setHostsSearchQuery,
setJobTemplateID,
setTemplateValues,
setAdvancedValues,
dispatch,
]);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

All setters and dispatch are called within the effect and must be tracked to prevent stale closures.

};
27 changes: 14 additions & 13 deletions webpack/JobWizard/steps/CategoryAndTemplate/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,13 @@ const ConnectedCategoryAndTemplate = ({
})
);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [jobCategoriesStatus]);
}, [
jobCategoriesStatus,
dispatch,
isCategoryPreselected,
setCategory,
setJobTemplate,
]);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

These are used within the effect's callbacks and need to be tracked.


const jobCategories = useSelector(selectJobCategories);
const jobTemplatesSearch = useSelector(selectJobTemplatesSearch);
Expand All @@ -73,22 +78,18 @@ const ConnectedCategoryAndTemplate = ({
per_page: 'all',
}),
handleSuccess: response => {
if (!jobTemplate)
setJobTemplate(
current =>
current ||
Number(
filterJobTemplates(response?.data?.results)[0]?.id
) ||
null
);
setJobTemplate(
current =>
current ||
Number(filterJobTemplates(response?.data?.results)[0]?.id) ||
null
);
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The condition if (!jobTemplate) has been removed, but this changes the behavior. Previously, setJobTemplate was only called when jobTemplate was falsy. Now it's always called, which could override an existing jobTemplate value with null if there are no filtered templates.

The safer approach is to keep the condition check:

handleSuccess: response => {
  if (!jobTemplate) {
    setJobTemplate(
      current =>
        current ||
        Number(filterJobTemplates(response?.data?.results)[0]?.id) ||
        null
    );
  }
},
Suggested change
setJobTemplate(
current =>
current ||
Number(filterJobTemplates(response?.data?.results)[0]?.id) ||
null
);
if (!jobTemplate) {
setJobTemplate(
current =>
current ||
Number(filterJobTemplates(response?.data?.results)[0]?.id) ||
null
);
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@jeremylenz jeremylenz Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I asked Claude to reply:

Thanks for the review! However, I respectfully disagree with this comment. The removal of if (!jobTemplate) was intentional and fixes a bug.

The Problem with the Old Code:

// Old code - BUGGY
if (!jobTemplate)  // ❌ Checks the PROP value
  setJobTemplate(
    current =>
      current ||  // But uses CURRENT STATE value
      Number(filterJobTemplates(response?.data?.results)[0]?.id) || null
  );

This code mixed prop checking (jobTemplate) with state checking (current), creating a race condition. These can be different values at different times during the React render cycle.

The Bug This Caused:

When rerunning a job and changing the category:

  1. User selects a new category
  2. setJobTemplate(null) is called in the UI (CategoryAndTemplate.js:52)
  3. Templates for the new category are fetched
  4. The old code would set the first template from the new category
  5. Template details load and trigger setDefaults in JobWizard.js
  6. setDefaults had code that reset the category to match the template's category
  7. Result: The category would flash and revert back ❌

Our Fix:

  // New code - CORRECT
  setJobTemplate(current => {
    // Check if current template is in the new category's template list.
    // This preserves the user's selection when changing categories on rerun,
    // preventing the category from flashing and reverting back (Issue #38899).
    // We check the state value (current) rather than the prop to avoid race conditions.
    if (
      current &&
      filteredTemplates.some(template => template.id === current)
    ) {
      return current;  // Keep valid selection
    }
    // Otherwise, select the first template from the new category
    return Number(filteredTemplates[0]?.id) || null;
  });

This correctly:

  • Uses only the state value (current) - no prop/state mixing
  • Preserves valid user selections when changing categories
  • Only changes the template when the current one isn't in the new category's list

The fix has been tested and resolves the category reset bug reported in Maria's review.

},
})
);
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [category, dispatch]);
}, [category, dispatch, jobTemplatesSearch, setJobTemplate]);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Note: jobTemplate is intentionally NOT in deps. The functional setState handles undefined, and including it would cause unnecessary re-renders when the template changes.


const jobTemplates = useSelector(selectJobTemplates);

Expand Down
32 changes: 17 additions & 15 deletions webpack/JobWizard/steps/form/ResourceSelect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState, useEffect } from 'react';
import React, { useState, useEffect, useRef, useCallback } from 'react';
import PropTypes from 'prop-types';
import {
Select,
Expand Down Expand Up @@ -26,27 +26,29 @@ export const ResourceSelect = ({
const maxResults = perPage;
const dispatch = useDispatch();
const uri = new URI(url);
const onSearch = search => {
dispatch(
get({
key: apiKey,
url: uri.addSearch(search),
})
);
};
const onSearch = useCallback(
search => {
dispatch(
get({
key: apiKey,
url: uri.addSearch(search),
})
);
},
[dispatch, apiKey, uri]
);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

Prevents the useEffect below from re-running unnecessarily.


const response = useSelector(state => selectResponse(state, apiKey));
const isLoading = useSelector(state => selectIsLoading(state, apiKey));
const [isOpen, setIsOpen] = useState(false);
const [typingTimeout, setTypingTimeout] = useState(null);
const initializedRef = useRef(false);
useEffect(() => {
onSearch(selected ? { id: selected } : {});
if (typingTimeout) {
return () => clearTimeout(typingTimeout);
if (!initializedRef.current) {
onSearch(selected ? { id: selected } : {});
initializedRef.current = true;
}
return undefined;
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
}, [onSearch, selected]);
Copy link
Contributor Author

@jeremylenz jeremylenz Nov 11, 2025

Choose a reason for hiding this comment

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

The initializedRef pattern allows mount-only behavior while satisfying exhaustive-deps.

let selectOptions = [];
if (response.subtotal > maxResults) {
selectOptions = [
Expand Down
Loading