diff --git a/webpack/JobInvocationDetail/JobInvocationHostTable.js b/webpack/JobInvocationDetail/JobInvocationHostTable.js index 33cde9700..5b17a0d3e 100644 --- a/webpack/JobInvocationDetail/JobInvocationHostTable.js +++ b/webpack/JobInvocationDetail/JobInvocationHostTable.js @@ -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'; @@ -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] + ); - const handleResponse = (data, key) => { + const handleResponse = useCallback((data, key) => { if (key === JOB_INVOCATION_HOSTS) { const ids = data.data.results.map(i => i.id); @@ -114,76 +123,94 @@ const JobInvocationHostTable = ({ } 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, + }) + ); + }, + [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, + ] + ); // Filter change - const handleFilterChange = newFilter => { - onFilterUpdate(newFilter); - }; + const handleFilterChange = useCallback( + newFilter => { + onFilterUpdate(newFilter); + }, + [onFilterUpdate] + ); // 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]); useEffect(() => { if (initialFilter !== '') filterApiCall(); @@ -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]); const { updateSearchQuery: updateSearchQueryBulk, diff --git a/webpack/JobInvocationDetail/index.js b/webpack/JobInvocationDetail/index.js index 5d4dcd735..6b266241b 100644 --- a/webpack/JobInvocationDetail/index.js +++ b/webpack/JobInvocationDetail/index.js @@ -79,15 +79,14 @@ const JobInvocationDetailPage = ({ return () => { dispatch(stopInterval(JOB_INVOCATION_KEY)); }; - // eslint-disable-next-line react-hooks/exhaustive-deps }, [dispatch, id, finished, autoRefresh]); + const taskId = task?.id; 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]); const pageStatus = items.id === undefined diff --git a/webpack/JobWizard/JobWizard.js b/webpack/JobWizard/JobWizard.js index 7a22b8389..47677c84c 100644 --- a/webpack/JobWizard/JobWizard.js +++ b/webpack/JobWizard/JobWizard.js @@ -89,9 +89,6 @@ export const JobWizard = ({ rerunData }) => { concurrency_control = {}, }, }) => { - if (category !== job_category) { - setCategory(job_category); - } const advancedTemplateValues = {}; const defaultTemplateValues = {}; const inputs = template_inputs; @@ -131,8 +128,7 @@ export const JobWizard = ({ rerunData }) => { }; }); }, - // eslint-disable-next-line react-hooks/exhaustive-deps - [category.length] + [setTemplateValues, setAdvancedValues] ); useEffect(() => { if (rerunData) { @@ -153,8 +149,7 @@ export const JobWizard = ({ rerunData }) => { }, }); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [rerunData]); + }, [rerunData, setDefaults]); useEffect(() => { if (jobTemplateID) { dispatch( @@ -199,8 +194,14 @@ export const JobWizard = ({ rerunData }) => { }) ); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [rerunData, jobTemplateID, dispatch]); + }, [ + rerunData, + jobTemplateID, + dispatch, + setDefaults, + setTemplateValues, + setAdvancedValues, + ]); const [isStartsBeforeError, setIsStartsBeforeError] = useState(false); const [isStartsAtError, setIsStartsAtError] = useState(false); @@ -514,7 +515,7 @@ JobWizard.propTypes = { }), execution_timeout_interval: PropTypes.number, time_to_pickup: PropTypes.number, - remote_execution_feature_id: PropTypes.string, + remote_execution_feature_id: PropTypes.number, template_invocations: PropTypes.arrayOf( PropTypes.shape({ template_id: PropTypes.number, diff --git a/webpack/JobWizard/autofill.js b/webpack/JobWizard/autofill.js index 6d8feffc8..2cd48b5e5 100644 --- a/webpack/JobWizard/autofill.js +++ b/webpack/JobWizard/autofill.js @@ -95,6 +95,14 @@ export const useAutoFill = ({ }); } } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [fills]); + }, [ + fills, + setFills, + setSelectedTargets, + setHostsSearchQuery, + setJobTemplateID, + setTemplateValues, + setAdvancedValues, + dispatch, + ]); }; diff --git a/webpack/JobWizard/steps/AdvancedFields/__tests__/__snapshots__/AdvancedFields.test.js.snap b/webpack/JobWizard/steps/AdvancedFields/__tests__/__snapshots__/AdvancedFields.test.js.snap index 09105ba41..76eaed427 100644 --- a/webpack/JobWizard/steps/AdvancedFields/__tests__/__snapshots__/AdvancedFields.test.js.snap +++ b/webpack/JobWizard/steps/AdvancedFields/__tests__/__snapshots__/AdvancedFields.test.js.snap @@ -57,7 +57,7 @@ Array [ "port": null, "preventInvalidHostname": false, "protocol": null, - "query": "resource=ForemanTasks%3A%3ATask&name=some+search", + "query": "resource=ForemanTasks%3A%3ATask", "urn": null, "username": null, }, diff --git a/webpack/JobWizard/steps/CategoryAndTemplate/index.js b/webpack/JobWizard/steps/CategoryAndTemplate/index.js index 680289142..da8ea3d0f 100644 --- a/webpack/JobWizard/steps/CategoryAndTemplate/index.js +++ b/webpack/JobWizard/steps/CategoryAndTemplate/index.js @@ -55,8 +55,13 @@ const ConnectedCategoryAndTemplate = ({ }) ); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [jobCategoriesStatus]); + }, [ + jobCategoriesStatus, + dispatch, + isCategoryPreselected, + setCategory, + setJobTemplate, + ]); const jobCategories = useSelector(selectJobCategories); const jobTemplatesSearch = useSelector(selectJobTemplatesSearch); @@ -73,22 +78,29 @@ const ConnectedCategoryAndTemplate = ({ per_page: 'all', }), handleSuccess: response => { - if (!jobTemplate) - setJobTemplate( - current => - current || - Number( - filterJobTemplates(response?.data?.results)[0]?.id - ) || - null - ); + const filteredTemplates = filterJobTemplates( + response?.data?.results + ); + 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; + } + // Otherwise, select the first template from the new category + return Number(filteredTemplates[0]?.id) || null; + }); }, }) ); } } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [category, dispatch]); + }, [category, dispatch, jobTemplatesSearch, setJobTemplate]); const jobTemplates = useSelector(selectJobTemplates); diff --git a/webpack/JobWizard/steps/HostsAndInputs/SelectAPI.js b/webpack/JobWizard/steps/HostsAndInputs/SelectAPI.js index 203e3f2ea..72daf3006 100644 --- a/webpack/JobWizard/steps/HostsAndInputs/SelectAPI.js +++ b/webpack/JobWizard/steps/HostsAndInputs/SelectAPI.js @@ -1,4 +1,4 @@ -import React from 'react'; +import React, { useCallback } from 'react'; import { useSelector, useDispatch } from 'react-redux'; import URI from 'urijs'; import { SelectVariant } from '@patternfly/react-core/deprecated'; @@ -8,16 +8,21 @@ import { SearchSelect } from '../form/SearchSelect'; export const useNameSearchAPI = (apiKey, url) => { const dispatch = useDispatch(); - const uri = new URI(url); - const onSearch = search => - dispatch( - get({ - key: apiKey, - url: uri.addSearch({ - search: `name~"${search}"`, - }), - }) - ); + + const onSearch = useCallback( + search => { + const uri = new URI(url); + dispatch( + get({ + key: apiKey, + url: uri.addSearch({ + search: `name~"${search}"`, + }), + }) + ); + }, + [dispatch, apiKey, url] + ); const response = useSelector(state => selectResponse(state, apiKey)); const isLoading = useSelector(state => selectIsLoading(state, apiKey)); diff --git a/webpack/JobWizard/steps/form/ResourceSelect.js b/webpack/JobWizard/steps/form/ResourceSelect.js index 16389bea6..7796690c7 100644 --- a/webpack/JobWizard/steps/form/ResourceSelect.js +++ b/webpack/JobWizard/steps/form/ResourceSelect.js @@ -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, @@ -25,28 +25,30 @@ export const ResourceSelect = ({ const { perPage } = useForemanSettings(); 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 => { + const uri = new URI(url); + dispatch( + get({ + key: apiKey, + url: uri.addSearch(search), + }) + ); + }, + [dispatch, apiKey, url] + ); 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]); let selectOptions = []; if (response.subtotal > maxResults) { selectOptions = [ diff --git a/webpack/JobWizard/steps/form/SearchSelect.js b/webpack/JobWizard/steps/form/SearchSelect.js index 526acde2f..b920b2489 100644 --- a/webpack/JobWizard/steps/form/SearchSelect.js +++ b/webpack/JobWizard/steps/form/SearchSelect.js @@ -1,4 +1,4 @@ -import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useRef } from 'react'; import PropTypes from 'prop-types'; import { Select, @@ -25,14 +25,13 @@ export const SearchSelect = ({ const [onSearch, response, isLoading] = useNameSearch(apiKey, url); const [isOpen, setIsOpen] = useState(false); const [typingTimeout, setTypingTimeout] = useState(null); + const initializedRef = useRef(false); useEffect(() => { - onSearch(selected.name || ''); - if (typingTimeout) { - return () => clearTimeout(typingTimeout); + if (!initializedRef.current) { + onSearch(selected.name || ''); + initializedRef.current = true; } - return undefined; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [onSearch, selected.name]); let selectOptions = []; if (response.subtotal > maxResults) { selectOptions = [ diff --git a/webpack/JobWizard/validation.js b/webpack/JobWizard/validation.js index da2da4664..f5ad9f395 100644 --- a/webpack/JobWizard/validation.js +++ b/webpack/JobWizard/validation.js @@ -46,8 +46,6 @@ export const useValidation = ({ advancedValues, templateValues }) => { setValid(currValid => ({ ...currValid, advanced: false })); } }); - - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [advancedValues, templateValues]); + }, [advancedValues, templateValues, templateInputs, advancedTemplateInputs]); return [valid, setValid]; }; diff --git a/webpack/react_app/components/TargetingHosts/index.js b/webpack/react_app/components/TargetingHosts/index.js index 159efe051..0c6fd64ea 100644 --- a/webpack/react_app/components/TargetingHosts/index.js +++ b/webpack/react_app/components/TargetingHosts/index.js @@ -1,4 +1,4 @@ -import React, { useEffect, useState } from 'react'; +import React, { useEffect, useState, useCallback, useRef } from 'react'; import { useSelector, useDispatch } from 'react-redux'; import { get } from 'foremanReact/redux/API'; @@ -50,38 +50,55 @@ const WrappedTargetingHosts = () => { const [apiUrl, setApiUrl] = useState(getApiUrl(searchQuery, pagination)); const intervalExists = useSelector(selectIntervalExists); - const handleSearch = (query, status) => { - const defaultPagination = { page: 1, per_page: pagination.per_page }; - stopApiInterval(); - - setApiUrl(getApiUrl(buildSearchQuery(query, status), defaultPagination)); - setSearchQuery(query); - setPagination(defaultPagination); - }; - - const handlePagination = args => { - stopApiInterval(); - setPagination(args); - setApiUrl(getApiUrl(buildSearchQuery(searchQuery, statusFilter), args)); - }; - - const stopApiInterval = () => { + const stopApiInterval = useCallback(() => { if (intervalExists) { dispatch(stopInterval(TARGETING_HOSTS)); } - }; - - const getData = url => - withInterval( - get({ - key: TARGETING_HOSTS, - url, - handleError: () => { - dispatch(stopInterval(TARGETING_HOSTS)); - }, - }), - 1000 - ); + }, [dispatch, intervalExists]); + + // Use ref to avoid infinite loop from handleSearch depending on pagination.per_page + const perPageRef = useRef(pagination.per_page); + + const handleSearch = useCallback( + (query, status) => { + const defaultPagination = { page: 1, per_page: perPageRef.current }; + stopApiInterval(); + + setApiUrl(getApiUrl(buildSearchQuery(query, status), defaultPagination)); + setSearchQuery(query); + setPagination(defaultPagination); + }, + [stopApiInterval] + ); + + // Keep ref in sync with pagination + useEffect(() => { + perPageRef.current = pagination.per_page; + }, [pagination.per_page]); + + const handlePagination = useCallback( + args => { + stopApiInterval(); + setPagination(args); + setApiUrl(getApiUrl(buildSearchQuery(searchQuery, statusFilter), args)); + }, + [searchQuery, statusFilter, stopApiInterval] + ); + + const getData = useCallback( + url => + withInterval( + get({ + key: TARGETING_HOSTS, + url, + handleError: () => { + dispatch(stopInterval(TARGETING_HOSTS)); + }, + }), + 1000 + ), + [dispatch] + ); useEffect(() => { dispatch(getData(apiUrl)); @@ -93,11 +110,11 @@ const WrappedTargetingHosts = () => { return () => { dispatch(stopInterval(TARGETING_HOSTS)); }; - }, [dispatch, apiUrl, autoRefresh]); + }, [dispatch, apiUrl, autoRefresh, getData]); useEffect(() => { handleSearch(searchQuery, statusFilter); - }, [statusFilter, searchQuery]); + }, [statusFilter, searchQuery, handleSearch]); return (