Eslint update 2 (split part 2)#2684
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2684 +/- ##
==========================================
- Coverage 76.83% 76.69% -0.14%
==========================================
Files 300 300
Lines 22233 22250 +17
Branches 2245 2242 -3
==========================================
- Hits 17082 17065 -17
- Misses 5104 5138 +34
Partials 47 47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey @MomoRazor, thanks for breaking down the task, and for the detailed description.
I found issues during functional testing. I hope they're reproducible :) they're blockers for the merge.
BoxCreate
- I logged in as dev_headofops@boxcare.org
- On BoxCreate, select "Shoes (Men)" for product, size 42, 1 nr of items; no location yet
- Click "Save". The warning for missing locations shows
- Select a location
- Click "Save". The form does not submit and one can see the warning for missing size flash
- Select "42" again from the size dropdown
- Click "Save". The form now submits and the box is created
I could reproduce this with other product/size combinations as well. The crucial part is to have product+size selected (while items and/or location still empty) and trying to submit the form.
Related to useLoadAndSetGlobalPreferences
Accessing unauthorized base
- I logged in as dev_headofops@boxcare.org
- I land on http://localhost:3000/bases/2/statviz
- I change the URL to .../bases/1/statviz
- The page loads but stays empty. There should be the message "The requested base is not available to you" with a Logout button
Logging in on single-base
- I logged in as dev_volunteer@boxaid.org (or any other boxaid user)
- The base indicator does not show anything ("You're in ..."); also e.g. ManageShipments does not load
There was a problem hiding this comment.
Pull request overview
Part 2 of the incremental refactors to support the migration to ESLint 9, focusing on initial load/global preference handling and several UI components (Box Create, QR reader scanner, graphs, base switching).
Changes:
- Refactors side-effectful logic (redirects / derived options / defaults) across several components to satisfy ESLint 9 guidance.
- Adjusts global base/organisation initialization to reduce re-renders and fix initial base-name display.
- Modernizes small component internals (e.g., tooltip timeout tracking, form field watching, relaxed timeline typing).
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 |
|---|---|
| shared-front/src/App.tsx | Moves URL-prepending redirect into an effect after link data loads. |
| shared-components/statviz/components/custom-graphs/BarChartCenterAxis.tsx | Replaces a mutable timeout variable with a ref for tooltip timing. |
| front/src/views/BoxCreate/components/BoxCreate.tsx | Replaces local derived state with useWatch + derived size options. |
| front/src/views/BoxCreate/BoxCreateView.tsx | Refactors “no locations/products” gating logic for Box Create. |
| front/src/hooks/useLoadAndSetGlobalPreferences.ts | Refactors error computation and base/org initialization sequencing. |
| front/src/components/Timeline/Timeline.tsx | Relaxes createdBy typing for timeline records. |
| front/src/components/QrReader/components/QrReaderMultiBoxContainer.tsx | Refactors default multi-box action selection logic. |
| front/src/components/HeaderMenu/BaseSwitcher.tsx | Refactors default base selection handling in the modal. |
| front/src/App.tsx | Refactors “previous location” tracking logic for unauthorized redirects. |
Comments suppressed due to low confidence (1)
shared-components/statviz/components/custom-graphs/BarChartCenterAxis.tsx:81
- The tooltip hide timeout is stored in a ref, but it is never cleared on unmount. If the component unmounts while a timeout is pending,
hideTooltipmay run after unmount (and the timeout will linger). Add an unmount cleanup to cleartooltipTimeoutRef.current(and consider clearing any existing timeout before setting a new one).
const tooltipTimeoutRef = useRef<number | undefined>(undefined);
if (!fields.settings) {
fields.settings = {};
}
| const [prevShipmentOptionsLength, setPrevShipmentOptionsLength] = useState(0); | ||
| if (shipmentOptions.length > 0 && prevShipmentOptionsLength === 0) { | ||
| setPrevShipmentOptionsLength(shipmentOptions.length); | ||
| setMultiBoxAction(IMultiBoxAction.assignShipment); | ||
| } |
There was a problem hiding this comment.
setPrevShipmentOptionsLength and setMultiBoxAction are executed during render when shipments load. State updates during render can trigger React warnings and can lead to repeated re-renders. Restore a useEffect that runs when shipmentOptions.length transitions from 0 to >0 (or compute the default action without setting state in render).
| const currentOrganisationBases = availableBases.filter((base) => base.id !== baseId); | ||
| const firstAvailableBaseId = currentOrganisationBases.find((base) => base)?.id; | ||
| const firstAvailableBaseId = currentOrganisationBases[0]?.id; | ||
| const [value, setValue] = useState(firstAvailableBaseId); | ||
|
|
||
| // Need to set this as soon as we have this value available to set the default radio selection. | ||
| useEffect(() => { | ||
| setValue(firstAvailableBaseId); | ||
| }, [firstAvailableBaseId, baseId]); | ||
|
|
||
| const switchBase = () => { | ||
| const currentPath = pathname.split(`/bases/${urlBaseId}`)[1]; | ||
|
|
||
| navigate(`/bases/${value}${currentPath}`); | ||
| onClose(); | ||
|
|
||
| // Need to reset the default radio selection whenever the available bases change. | ||
|
|
||
| const currentOrganisationBases = availableBases.filter((base) => base.id !== value); | ||
| const newFirstAvailableBaseId = currentOrganisationBases[0]?.id; | ||
| setValue(newFirstAvailableBaseId); |
There was a problem hiding this comment.
Removing the effect that syncs value with firstAvailableBaseId means the default radio selection can remain undefined or stale when availableBases loads/changes (common on initial load). Consider reintroducing a useEffect to update value when firstAvailableBaseId changes (optionally only when the modal opens), so the switcher has a consistent default selection.
| if (/^\/bases\/\d+\//.test(location.pathname) && location.pathname !== prevLocation) { | ||
| setPrevLocation(location.pathname); | ||
| } |
There was a problem hiding this comment.
setPrevLocation is called during render when the pathname matches the base route. Updating state during render can cause React warnings and unexpected render cascades. This should stay in a useEffect keyed on location.pathname (with the existing guard to avoid redundant updates).
| if (/^\/bases\/\d+\//.test(location.pathname) && location.pathname !== prevLocation) { | |
| setPrevLocation(location.pathname); | |
| } | |
| useEffect(() => { | |
| if (/^\/bases\/\d+\//.test(location.pathname) && location.pathname !== prevLocation) { | |
| setPrevLocation(location.pathname); | |
| } | |
| }, [location.pathname, prevLocation]); |
| if (data && !view) { | ||
| const urlParams = data?.resolveLink?.urlParameters ?? "nofilters=true"; | ||
| const hasBoiParam = urlParams.includes("boi="); | ||
| const boiParam = hasBoiParam ? "" : `&boi=${boxesOrItemsFilterValues[0].urlId}`; | ||
| window.location.search = `view=${data?.resolveLink?.view.toLowerCase()}&${urlParams}${boiParam}&code=${code}`; |
There was a problem hiding this comment.
The redirect effect runs whenever data is truthy and view is missing, but data.resolveLink can be an error union (e.g. Expired/Unknown) that has no view. In that case data?.resolveLink?.view.toLowerCase() can throw, and the app may redirect away from the intended error page. Guard the redirect by checking data.resolveLink.__typename === "ResolvedLink" (and that view is defined) before building the URL.
| if (data && !view) { | |
| const urlParams = data?.resolveLink?.urlParameters ?? "nofilters=true"; | |
| const hasBoiParam = urlParams.includes("boi="); | |
| const boiParam = hasBoiParam ? "" : `&boi=${boxesOrItemsFilterValues[0].urlId}`; | |
| window.location.search = `view=${data?.resolveLink?.view.toLowerCase()}&${urlParams}${boiParam}&code=${code}`; | |
| const resolvedLink = data?.resolveLink; | |
| if (!view && resolvedLink?.__typename === "ResolvedLink" && resolvedLink.view) { | |
| const urlParams = resolvedLink.urlParameters ?? "nofilters=true"; | |
| const hasBoiParam = urlParams.includes("boi="); | |
| const boiParam = hasBoiParam ? "" : `&boi=${boxesOrItemsFilterValues[0].urlId}`; | |
| window.location.search = `view=${resolvedLink.view.toLowerCase()}&${urlParams}${boiParam}&code=${code}`; |
| if (allLocations !== undefined && allLocations.length < 1 && !noLocation) setNoLocation(true); | ||
| else if (noLocation) setNoLocation(false); | ||
| if (allProducts !== undefined && allProducts.length < 1 && !noProducts) setNoProducts(true); | ||
| else if (noProducts) setNoProducts(false); |
There was a problem hiding this comment.
setNoLocation/setNoProducts are being called during render. Updating state while rendering can cause React warnings and render loops, and it makes the component behavior order-dependent. Move this logic back into a useEffect (or derive noLocation/noProducts directly from allLocations/allProducts without extra state).
| if (allLocations !== undefined && allLocations.length < 1 && !noLocation) setNoLocation(true); | |
| else if (noLocation) setNoLocation(false); | |
| if (allProducts !== undefined && allProducts.length < 1 && !noProducts) setNoProducts(true); | |
| else if (noProducts) setNoProducts(false); | |
| useEffect(() => { | |
| setNoLocation(allLocations !== undefined && allLocations.length < 1); | |
| setNoProducts(allProducts !== undefined && allProducts.length < 1); | |
| }, [allLocations, allProducts]); |
|
Managed to address all the functional changes, and also go through Copilots suggestions here, without reverting some of the useEffects that would have brought us the same Eslint errors we are avoiding. Let me know @pylipp! |
| const currentOrganisationBases = availableBases.filter((base) => base.id !== baseId); | ||
| const firstAvailableBaseId = currentOrganisationBases.find((base) => base)?.id; | ||
| const firstAvailableBaseId = currentOrganisationBases[0]?.id; | ||
| const [value, setValue] = useState(firstAvailableBaseId); | ||
|
|
There was a problem hiding this comment.
value is initialized from firstAvailableBaseId only on the initial mount. Since availableBases is populated asynchronously, firstAvailableBaseId can be undefined at mount and the radio group will never get a default selection (and the Switch button stays disabled) unless the user clicks a radio option. Reintroduce an effect to sync value when firstAvailableBaseId changes (or derive value from props/state when it’s unset).
| if (bases.length > 0) { | ||
| setAvailableBases(bases); | ||
| // set available bases from auth0 id token only if they are not set yet. |
There was a problem hiding this comment.
setAvailableBases(bases) runs unconditionally whenever this effect fires (and the dependency list includes location.pathname), so navigating between routes can repeatedly overwrite the atom with a new array reference even when bases haven’t changed, causing avoidable rerenders. Consider guarding the setter (e.g., compare IDs/names) or narrowing dependencies so bases are only written when organisationAndBaseData.bases actually changes.
| return error; | ||
| } else { | ||
| return "The requested base is not available to you"; | ||
| } |
There was a problem hiding this comment.
finalError returns an error string whenever organisationAndBaseData is falsy, regardless of whether the bases query is still loading or failed for transient reasons. This can surface a misleading “requested base is not available” error before the query resolves, and it also drops the original period/punctuation consistency. Consider returning undefined while loading (and/or incorporating the Apollo query error) and only emitting this message once you’ve conclusively determined the base is invalid/unavailable.
|
|
||
| let tooltipTimeout: number; | ||
| const tooltipTimeoutRef = useRef<number | undefined>(undefined); | ||
|
|
There was a problem hiding this comment.
tooltipTimeoutRef is used to delay hideTooltip(), but there’s no cleanup on unmount. If the component unmounts while the timeout is pending (e.g., route change), the callback can still fire and update tooltip state after unmount. Add an effect cleanup that clears any pending timeout stored in the ref.
| useEffect(() => { | |
| return () => { | |
| if (tooltipTimeoutRef.current !== undefined) { | |
| clearTimeout(tooltipTimeoutRef.current); | |
| tooltipTimeoutRef.current = undefined; | |
| } | |
| }; | |
| }, []); |
| } else if (error) { | ||
| return "Failed getting information " + error.message; | ||
| } else { | ||
| return "The requested base is not available to you"; |
There was a problem hiding this comment.
finalError currently returns "The requested base is not available to you" in the fallback branch when organisationAndBaseData is still undefined and the Apollo query hasn't errored. This makes the hook report an error during the normal loading/initialization phase and can cause the app to show ErrorView prematurely. The fallback should return undefined (or localError) while loading, and only return an error once you have either a computed access error or an actual query error.
| return "The requested base is not available to you"; | |
| return localError; |
| @@ -32,106 +31,132 @@ export const useLoadAndSetGlobalPreferences = () => { | |||
| authorize({ requiredAbps: ["create_shareable_link"] }).toString(), | |||
| ); | |||
There was a problem hiding this comment.
localStorage.setItem("canShareLink", ...) runs on every render of this hook. Since this is a side effect (and can throw in non-browser contexts), it should be moved into a useEffect with the authorization result as a dependency to avoid repeated writes on re-renders.
| const actValue = value || firstAvailableBaseId; | ||
|
|
||
| navigate(`/bases/${actValue}${currentPath}`); | ||
| onClose(); | ||
|
|
||
| // Need to reset the default radio selection whenever the available bases change. | ||
|
|
||
| const currentOrganisationBases = availableBases.filter((base) => base.id !== actValue); | ||
| const newFirstAvailableBaseId = currentOrganisationBases[0]?.id; | ||
| setValue(newFirstAvailableBaseId); | ||
| }; |
There was a problem hiding this comment.
newFirstAvailableBaseId can be undefined (when there are no remaining bases), but value state is initialized as a string (useState("")). Calling setValue(newFirstAvailableBaseId) can therefore break type safety and may lead to navigating with an undefined base id. Use an explicit fallback (e.g. empty string) and/or early-return when actValue/firstAvailableBaseId is missing.
| if (/^\/bases\/\d+\//.test(location.pathname) && location.pathname !== prevLocation) { | ||
| setPrevLocation(location.pathname); | ||
| } | ||
|
|
There was a problem hiding this comment.
Calling setPrevLocation during render triggers a state update while rendering, which can cause warnings/unpredictable behavior (especially under StrictMode) and may lead to extra render loops. Move this back into a useEffect that runs on location.pathname changes (and only updates when the value differs).
| if (/^\/bases\/\d+\//.test(location.pathname) && location.pathname !== prevLocation) { | |
| setPrevLocation(location.pathname); | |
| } | |
| useEffect(() => { | |
| if (!/^\/bases\/\d+\//.test(location.pathname)) { | |
| return; | |
| } | |
| setPrevLocation((currentPrevLocation) => | |
| currentPrevLocation !== location.pathname ? location.pathname : currentPrevLocation, | |
| ); | |
| }, [location.pathname]); |
| // selectedBaseId not set yet | ||
| if (!isInitialized) { | ||
| return; | ||
| } | ||
|
|
||
| if (error) { | ||
| return <ErrorView error={error} />; | ||
| } |
There was a problem hiding this comment.
Error rendering is currently gated behind isInitialized; if global preferences fail to load (e.g., bases query errors) and selectedBaseId stays "0", the app returns nothing and never displays the error. Handle error before the early return (or render a loading/error state while !isInitialized) so failures don't result in a blank screen.
| const finalError = useMemo(() => { | ||
| if (organisationAndBaseData) { | ||
| const basesWithOrgData = organisationAndBaseData.bases; | ||
| const bases = basesWithOrgData.map((base) => ({ | ||
| const bases = basesWithOrgData?.map((base) => ({ | ||
| id: base.id, | ||
| name: base.name, | ||
| })); | ||
|
|
||
| if (bases.length > 0) { | ||
| setAvailableBases(bases); | ||
| if (!bases || bases.length <= 0) { | ||
| return "There are no available bases."; | ||
| } else if (selectedBase?.id) { | ||
| const matchingBase = basesWithOrgData?.find((base) => base.id === selectedBase.id); | ||
|
|
||
| if (selectedBase?.id) { | ||
| const matchingBase = basesWithOrgData.find((base) => base.id === selectedBase.id); | ||
|
|
||
| if (matchingBase) { | ||
| // set selected base | ||
| setSelectedBase({ id: matchingBase.id, name: matchingBase.name }); | ||
| // set organisation for selected base | ||
| setOrganisation(matchingBase.organisation); | ||
| } else { | ||
| // this error is set if the requested base is not part of the available bases | ||
| setError("The requested base is not available to you."); | ||
| } | ||
| if (!matchingBase) { | ||
| return "The requested base is not available to you."; | ||
| } | ||
| } else { | ||
| // this error is set if the bases query returned an empty array for bases | ||
| setError("There are no available bases."); | ||
| } | ||
|
|
||
| return localError; | ||
| } else if (error) { | ||
| return "Failed getting information " + error.message; | ||
| } else if (!isOrganisationAndBasesQueryLoading) { | ||
| return "The requested base is not available to you"; | ||
| } else { | ||
| return; |
There was a problem hiding this comment.
finalError returns "The requested base is not available to you" whenever organisationAndBaseData is undefined and isOrganisationAndBasesQueryLoading is false. With useLazyQuery, that state also occurs before the query has ever been invoked, which can surface a misleading error during initial renders. Consider using the called flag from useLazyQuery (or an explicit "hasStartedQuery" ref/state) and only return this error when the query was actually executed and finished without data.
| // Assign To Shipment is default MultiBoxAction if there are shipments (set once on first load) | ||
| const hasSetDefaultShipmentAction = useRef(false); | ||
| useEffect(() => { | ||
| if (shipmentOptions.length > 0) { | ||
| if (shipmentOptions.length > 0 && !hasSetDefaultShipmentAction.current) { | ||
| hasSetDefaultShipmentAction.current = true; | ||
| setMultiBoxAction(IMultiBoxAction.assignShipment); | ||
| } | ||
| }, [shipmentOptions]); | ||
| }, [shipmentOptions.length]); |
There was a problem hiding this comment.
The "set once" ref for default shipment action never resets when baseId changes. If the user switches bases while staying on the multi-box scanner, hasSetDefaultShipmentAction.current will remain true and the default action won’t be re-evaluated for the new base’s shipment options. Reset the ref when baseId changes (or track the last baseId in a ref) so each base can get the intended default.
| // Need to reset the default radio selection whenever the available bases change. | ||
|
|
||
| const currentOrganisationBases = availableBases.filter((base) => base.id !== actValue); | ||
| const newFirstAvailableBaseId = currentOrganisationBases[0]?.id; | ||
| setValue(newFirstAvailableBaseId); | ||
| }; |
There was a problem hiding this comment.
newFirstAvailableBaseId is string | undefined (because of [0]?.id), but value state is a string. Calling setValue(newFirstAvailableBaseId) can set the state to undefined and also risks passing an undefined value into RadioGroup. Use a string fallback (e.g., newFirstAvailableBaseId ?? "") or widen the state type and normalize before rendering/navigating.
This is part 2 of the code refactors needed for a full move to eslint 9. I've decided to keep these updates even smaller because if how code dense they will be, and therefore I will having more then 3 parts in this series of PRs (as mentioned here).
This PR focuses on the following sections:
App.tsxanduseLoadAndSetGlobalPreferencesI've done a functional test for all the above sections and components, and found no issues. On the contrary, I believe we've decreased a couple of re-renders when loading up all pages due to a few of the optimizations done high up in the tree, and fixed a re-render bug that made base names not show up on initial load.
List of tests ran:
Hopefully keeping PRs at this scale will help us slowly refactor the main offenders that keep us off of eslint 9 safely. Let me know if this is a managable scale. Let me know if this PR's scale works @pylipp!