Fixes #38899 - Enable react-hooks/exhaustive-deps eslint rule #1016
Conversation
Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
Inline comments explaining the rationale for each change
| .join(' AND '); | ||
| }, | ||
| [initialFilter, urlSearchQuery] | ||
| ); |
There was a problem hiding this comment.
Prevents filterApiCall from changing on every render.
|
|
||
| setApiResponse(data.data); | ||
| setAllHostsIds(ids); | ||
| } |
There was a problem hiding this comment.
Stabilizes reference for use in makeApiCall dependency array.
| errorToast: ({ response }) => | ||
| response?.data?.error?.full_messages?.[0] || response, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Used as a dependency in filterApiCall and initialization useEffect.
| history, | ||
| constructFilter, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Used as a dependency in useEffect below.
| onFilterUpdate(newFilter); | ||
| }, | ||
| [onFilterUpdate] | ||
| ); |
There was a problem hiding this comment.
Prevents unnecessary re-renders when passed to child components.
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
| }, [makeApiCall, id, initialFilter, onFilterUpdate]); |
There was a problem hiding this comment.
The initializedRef pattern allows mount-only behavior while satisfying exhaustive-deps.
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [initialFilter, statusLabel, id]); | ||
| }, [initialFilter, statusLabel, id, filterApiCall]); |
There was a problem hiding this comment.
Without filterApiCall in deps, the effect could capture a stale closure.
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
Comments for JobInvocationDetail/index.js
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [dispatch, id, finished, autoRefresh]); | ||
|
|
||
| const taskId = task?.id; |
There was a problem hiding this comment.
Complex expressions in dependency arrays can't be statically analyzed by the linter.
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [dispatch, task?.id]); | ||
| }, [dispatch, taskId]); |
There was a problem hiding this comment.
Allows tracking task ID changes without depending on the entire task object.
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
Comments for JobWizard/JobWizard.js
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [category.length] | ||
| [category, setCategory, setTemplateValues, setAdvancedValues] | ||
| ); |
There was a problem hiding this comment.
Used as a dependency in two useEffect hooks below.
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [rerunData]); | ||
| }, [rerunData, setDefaults]); |
There was a problem hiding this comment.
Without setDefaults in deps, the effect could use a stale version.
| setDefaults, | ||
| setTemplateValues, | ||
| setAdvancedValues, | ||
| ]); |
There was a problem hiding this comment.
All three functions are called within the effect and need to be tracked to prevent stale closures.
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
Comments for autofill.js and CategoryAndTemplate
| setTemplateValues, | ||
| setAdvancedValues, | ||
| dispatch, | ||
| ]); |
There was a problem hiding this comment.
All setters and dispatch are called within the effect and must be tracked to prevent stale closures.
| isCategoryPreselected, | ||
| setCategory, | ||
| setJobTemplate, | ||
| ]); |
There was a problem hiding this comment.
These are used within the effect's callbacks and need to be tracked.
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
Comments for ResourceSelect, SearchSelect, and validation
| ); | ||
| }, | ||
| [dispatch, apiKey, uri] | ||
| ); |
There was a problem hiding this comment.
Prevents the useEffect below from re-running unnecessarily.
| return undefined; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
| }, [onSearch, selected]); |
There was a problem hiding this comment.
The initializedRef pattern allows mount-only behavior while satisfying exhaustive-deps.
| return undefined; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
| }, [onSearch, selected.name]); |
There was a problem hiding this comment.
The initializedRef pattern allows mount-only behavior while satisfying exhaustive-deps.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [advancedValues, templateValues]); | ||
| }, [advancedValues, templateValues, templateInputs, advancedTemplateInputs]); | ||
| return [valid, setValid]; |
There was a problem hiding this comment.
Validation needs to re-run when the input configurations change.
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
Comments for TargetingHosts/index.js
| }), | ||
| 1000 | ||
| ); | ||
| }, [dispatch, intervalExists]); |
There was a problem hiding this comment.
Used in multiple callbacks and needs a stable reference.
| setPagination(defaultPagination); | ||
| }, | ||
| [pagination.per_page, stopApiInterval] | ||
| ); |
There was a problem hiding this comment.
Passed to child components and used in useEffect below.
| setApiUrl(getApiUrl(buildSearchQuery(searchQuery, statusFilter), args)); | ||
| }, | ||
| [searchQuery, statusFilter, stopApiInterval] | ||
| ); |
There was a problem hiding this comment.
Passed to child components and needs to be memoized.
| 1000 | ||
| ), | ||
| [dispatch] | ||
| ); |
There was a problem hiding this comment.
Used in useEffect below and needs a stable reference.
| dispatch(stopInterval(TARGETING_HOSTS)); | ||
| }; | ||
| }, [dispatch, apiUrl, autoRefresh]); | ||
| }, [dispatch, apiUrl, autoRefresh, getData]); |
There was a problem hiding this comment.
Without getData in deps, the effect could use a stale version.
| useEffect(() => { | ||
| handleSearch(searchQuery, statusFilter); | ||
| }, [statusFilter, searchQuery]); | ||
| }, [statusFilter, searchQuery, handleSearch]); |
There was a problem hiding this comment.
Without handleSearch in deps, the effect could use a stale version.
The jobTemplate dependency was causing unnecessary re-renders of the effect when the template changed. The check `if (!jobTemplate)` was redundant because the functional setState `current => current || ...` already handles this case. Removing both fixes the test snapshot failure while maintaining correct behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Updated CategoryAndTemplate/index.jsMade an additional fix to address the test snapshot failure: Change: Removed Why: The check Result: The |
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
Updated comment for latest change
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [category, dispatch]); | ||
| }, [category, dispatch, jobTemplatesSearch, setJobTemplate]); |
There was a problem hiding this comment.
Note: jobTemplate is intentionally NOT in deps. The functional setState handles undefined, and including it would cause unnecessary re-renders when the template changes.
nofaralfasi
left a comment
nofaralfasi
left a comment
There was a problem hiding this comment.
Tested locally and everything works as expected!
- ESLint passes with no
react-hooks/exhaustive-depswarnings - Job Invocations page loads and functions correctly
- Job Wizard workflow completes successfully
- No console errors or regressions observed
LGTM!
There was a problem hiding this comment.
Pull request overview
This PR enables the react-hooks/exhaustive-deps eslint rule across the foreman_remote_execution codebase by removing all disabled eslint comments and refactoring React hooks to include proper dependency arrays using useCallback, useRef, and complete dependency lists.
Key changes:
- Wrapped callback functions in
useCallbackto stabilize references for dependency arrays - Added complete dependency arrays to all
useEffectanduseCallbackhooks - Used
useRefwith initialization flags to prevent effects from running on every render
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack/react_app/components/TargetingHosts/index.js | Wrapped helper functions in useCallback and added complete dependency arrays to effects |
| webpack/JobWizard/validation.js | Added templateInputs and advancedTemplateInputs to effect dependencies |
| webpack/JobWizard/steps/form/SearchSelect.js | Added useRef initialization pattern and included onSearch in effect dependencies |
| webpack/JobWizard/steps/form/ResourceSelect.js | Wrapped onSearch in useCallback with URI object dependency and added initialization pattern |
| webpack/JobWizard/steps/CategoryAndTemplate/index.js | Added complete dependency arrays and removed conditional check in setJobTemplate |
| webpack/JobWizard/autofill.js | Added all setter functions and dispatch to effect dependencies |
| webpack/JobWizard/JobWizard.js | Added complete dependency arrays including setter functions and setDefaults callback |
| webpack/JobInvocationDetail/index.js | Extracted taskId to avoid issues with object property dependencies |
| webpack/JobInvocationDetail/JobInvocationHostTable.js | Wrapped multiple helper functions in useCallback with complete dependency arrays and added initialization pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setJobTemplate( | ||
| current => | ||
| current || | ||
| Number(filterJobTemplates(response?.data?.results)[0]?.id) || | ||
| null | ||
| ); |
There was a problem hiding this comment.
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
);
}
},| setJobTemplate( | |
| current => | |
| current || | |
| Number(filterJobTemplates(response?.data?.results)[0]?.id) || | |
| null | |
| ); | |
| if (!jobTemplate) { | |
| setJobTemplate( | |
| current => | |
| current || | |
| Number(filterJobTemplates(response?.data?.results)[0]?.id) || | |
| null | |
| ); | |
| } |
There was a problem hiding this comment.
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:
- User selects a new category
- setJobTemplate(null) is called in the UI (CategoryAndTemplate.js:52)
- Templates for the new category are fetched
- The old code would set the first template from the new category
- Template details load and trigger setDefaults in JobWizard.js
- setDefaults had code that reset the category to match the template's category
- 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.
This commit addresses three valid Copilot review comments about useEffect optimization and fixes a bug where job category would reset on rerun. **Copilot Review Fixes:** 1. **TargetingHosts/index.js** - Fixed infinite loop risk (HIGH priority) - Used useRef to stabilize pagination.per_page dependency - Prevents handleSearch from being recreated when pagination changes - Eliminates circular dependency that could cause infinite loops 2. **ResourceSelect.js** - Fixed URI object instability - Moved URI creation inside useCallback - Changed dependency from object (uri) to primitive (url) - Prevents unnecessary recreation of onSearch on every render 3. **SelectAPI.js** - Fixed unstable onSearch hook - Added useCallback to memoize onSearch in useNameSearchAPI hook - Moved URI creation inside callback for stable dependencies - Makes hook return stable function references **Bug Fixes:** 4. **JobWizard.js** - Fixed category reset on job rerun - Removed automatic category setting from setDefaults callback - Previous code was forcing category to match template's category - This caused user's category selection to flash and revert back - Also fixed PropTypes: remote_execution_feature_id should be number 5. **CategoryAndTemplate/index.js** - Improved template selection logic - Check if current template is in new category before keeping it - Added explanatory comments documenting the fix for Issue #38899 - Prevents category from resetting when changing categories on rerun **Copilot Comment Disagreement:** - CategoryAndTemplate.js: Copilot suggested restoring the removed `if (!jobTemplate)` condition, but our fix is correct. The old code mixed prop checking with state updates causing race conditions. All changes pass eslint with react-hooks/exhaustive-deps rule enabled. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
pr-processor
bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
|
@MariaAga Updated. The issue with changing the job category on rerun seems to be fixed now, per my testing. 3 of the 4 Copilot comments should also be addressed (the reply to the fourth is above) |
The ResourceSelect component now moves URI creation inside useCallback, which means the search query is no longer included in the URI object at the time of snapshot. This is the correct behavior - the search should be applied when onSearch is called, not when the component renders. Updated snapshot to reflect this optimization.
MariaAga
left a comment
MariaAga
left a comment
There was a problem hiding this comment.
Thanks, the rerun issue is fixed
3 participants
Summary
This PR enables the
react-hooks/exhaustive-depseslint rule across the foreman_remote_execution codebase by removing all disabled eslint comments and properly refactoring React hooks to track dependencies.Changes
react-hooks/exhaustive-depseslint commentsuseCallback,useRef, and complete dependency arraysFiles Modified
/webpack/JobInvocationDetail/JobInvocationHostTable.js- Major refactoring of hooks with proper dependencies/webpack/JobInvocationDetail/index.js- Fixed hook dependencies/webpack/JobWizard/JobWizard.js- Fixed hook dependencies/webpack/JobWizard/autofill.js- Fixed hook dependencies/webpack/JobWizard/steps/CategoryAndTemplate/index.js- Fixed hook dependencies/webpack/JobWizard/steps/form/ResourceSelect.js- Fixed hook dependencies/webpack/JobWizard/steps/form/SearchSelect.js- Fixed hook dependencies/webpack/JobWizard/validation.js- Fixed hook dependencies/webpack/react_app/components/TargetingHosts/index.js- Fixed hook dependenciesTest Plan
Related Issue
Fixes https://projects.theforeman.org/issues/38899
Generated with Claude Code