diff --git a/src/components/GPSTripStateChecker/useUpdateGpsTripOnReconnect.ts b/src/components/GPSTripStateChecker/useUpdateGpsTripOnReconnect.ts index 386db785d03b..c04e3991d882 100644 --- a/src/components/GPSTripStateChecker/useUpdateGpsTripOnReconnect.ts +++ b/src/components/GPSTripStateChecker/useUpdateGpsTripOnReconnect.ts @@ -7,9 +7,11 @@ import {addressFromGpsPoint, getGpsPoints} from '@libs/GPSDraftDetailsUtils'; import ONYXKEYS from '@src/ONYXKEYS'; import type {GPSPoint, TrimmedGPSPoint} from '@src/types/onyx/GpsDraftDetails'; +import type {ReadonlyDeep} from 'type-fest'; + import {useEffect, useRef} from 'react'; -function useUpdateGpsTripOnReconnect({gpsPoints}: {gpsPoints: GPSPoint[][]}) { +function useUpdateGpsTripOnReconnect({gpsPoints}: {gpsPoints: ReadonlyDeep}) { const [gpsDraftDetails] = useOnyx(ONYXKEYS.GPS_DRAFT_DETAILS); // Mirror the latest gpsDraftDetails into a ref so the async onReconnect handler can read the newest value // after awaiting reverse geocoding, instead of the stale value captured when the callback started. diff --git a/src/hooks/useSwitchToDelegator.ts b/src/hooks/useSwitchToDelegator.ts index 1674c22a5698..3acf01d2279e 100644 --- a/src/hooks/useSwitchToDelegator.ts +++ b/src/hooks/useSwitchToDelegator.ts @@ -4,16 +4,14 @@ import {ModalActions} from '@components/Modal/Global/ModalContext'; import {connect, disconnect} from '@libs/actions/Delegate'; import {close as modalClose} from '@libs/actions/Modal'; import {getGpsPoints, stopGpsTrip} from '@libs/GPSDraftDetailsUtils'; +import Onyx from '@libs/OnyxReadonly'; -import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import {isTrackingSelector} from '@src/selectors/GPSDraftDetails'; -import type {Account} from '@src/types/onyx'; import useConfirmModal from './useConfirmModal'; import useLocalize from './useLocalize'; import useNetwork from './useNetwork'; -import useOnyx from './useOnyx'; /** * Encapsulates the safety checks needed before switching to a delegator account: @@ -22,6 +20,9 @@ import useOnyx from './useOnyx'; * to the original user, shows the "not so fast" modal. * 3. GPS tracking check – if a GPS trip is in progress, asks the user to confirm * stopping the trip before switching. + * + * Every value this needs is read when the switch is pressed rather than subscribed to, so the hook holds no + * Onyx subscription and the screens mounting it do not re-render on account, session or GPS changes. */ function useSwitchToDelegator() { const {translate} = useLocalize(); @@ -30,15 +31,6 @@ function useSwitchToDelegator() { const {isActingAsDelegate} = useDelegateNoAccessState(); const {showDelegateNoAccessModal} = useDelegateNoAccessActions(); - const [delegatedAccess] = useOnyx(ONYXKEYS.ACCOUNT, {selector: (account: Account | undefined) => account?.delegatedAccess}); - const [credentials] = useOnyx(ONYXKEYS.CREDENTIALS); - const [stashedCredentials = CONST.EMPTY_OBJECT] = useOnyx(ONYXKEYS.STASHED_CREDENTIALS); - const [session] = useOnyx(ONYXKEYS.SESSION); - const [stashedSession] = useOnyx(ONYXKEYS.STASHED_SESSION); - const [activePolicyID] = useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID); - const [gpsDraftDetails] = useOnyx(ONYXKEYS.GPS_DRAFT_DETAILS); - const [isTrackingGPS = false] = useOnyx(ONYXKEYS.GPS_DRAFT_DETAILS, {selector: isTrackingSelector}); - const showOfflineModal = () => { showConfirmModal({ title: translate('common.youAppearToBeOffline'), @@ -60,15 +52,31 @@ function useSwitchToDelegator() { return; } + // Points keep arriving while the modal is open, so the trip is read again here rather than reusing + // the value the tracking check was made on. + const gpsDraftDetails = await Onyx.get(ONYXKEYS.GPS_DRAFT_DETAILS); + await stopGpsTrip(false, getGpsPoints(gpsDraftDetails), true); switchAccount(); }; - const switchToDelegator = (email: string) => { + const switchToDelegator = async (email: string) => { if (isOffline) { modalClose(() => showOfflineModal()); return; } + + // Read in one go so every value comes from the same cache state, and before anything writes in this tick. + const [account, credentials, stashedCredentials, session, stashedSession, activePolicyID, gpsDraftDetails] = await Promise.all([ + Onyx.get(ONYXKEYS.ACCOUNT), + Onyx.get(ONYXKEYS.CREDENTIALS), + Onyx.get(ONYXKEYS.STASHED_CREDENTIALS), + Onyx.get(ONYXKEYS.SESSION), + Onyx.get(ONYXKEYS.STASHED_SESSION), + Onyx.get(ONYXKEYS.NVP_ACTIVE_POLICY_ID), + Onyx.get(ONYXKEYS.GPS_DRAFT_DETAILS), + ]); + const isReturningToOriginalUser = isActingAsDelegate && email === stashedSession?.email; // Chained delegation isn't supported by the backend — if we're already acting as a delegate, // the only legal switch is back to the original user. Anything else triggers the "Not so fast" modal. @@ -76,13 +84,16 @@ function useSwitchToDelegator() { modalClose(() => showDelegateNoAccessModal()); return; } + const switchAction = isReturningToOriginalUser ? () => disconnect({stashedCredentials, stashedSession}) - : () => connect({email, delegatedAccess, credentials, session, activePolicyID}); - if (isTrackingGPS) { + : () => connect({email, delegatedAccess: account?.delegatedAccess, credentials, session, activePolicyID}); + + if (isTrackingSelector(gpsDraftDetails)) { modalClose(() => showGpsInProgressModal(switchAction)); return; } + switchAction(); }; diff --git a/src/libs/GPSDraftDetailsUtils.ts b/src/libs/GPSDraftDetailsUtils.ts index f6244ce4c271..e5852db202bb 100644 --- a/src/libs/GPSDraftDetailsUtils.ts +++ b/src/libs/GPSDraftDetailsUtils.ts @@ -8,7 +8,7 @@ import type {GPSPoint, TrimmedGPSPoint} from '@src/types/onyx/GpsDraftDetails'; import type {Routes, Waypoint} from '@src/types/onyx/Transaction'; import geodesicDistance from '@src/utils/geodesicDistance'; -import type {SetRequired} from 'type-fest'; +import type {ReadonlyDeep, SetRequired} from 'type-fest'; import {hasStartedLocationUpdatesAsync, reverseGeocodeAsync, stopLocationUpdatesAsync} from 'expo-location'; @@ -26,15 +26,15 @@ function getGPSWaypoint(gpsPoint: GPSPoint, waypointIndex: number): GPSWaypointC }; } -function getEffectiveDistance(gpsDraftDetails: GpsDraftDetails | undefined): number { +function getEffectiveDistance(gpsDraftDetails: ReadonlyDeep | undefined): number { return gpsDraftDetails?.modifiedDistance ?? gpsDraftDetails?.distanceInMeters ?? 0; } -function getEffectiveEndPoint(gpsDraftDetails: GpsDraftDetails | undefined): GPSPoint | undefined { +function getEffectiveEndPoint(gpsDraftDetails: ReadonlyDeep | undefined): GPSPoint | undefined { return gpsDraftDetails?.trimmedEndPoint ?? gpsDraftDetails?.gpsPoints?.at(-1)?.at(-1); } -function getGPSWaypoints(gpsDraftDetails: GpsDraftDetails | undefined, trimmedEndPoint?: TrimmedGPSPoint): GPSWaypointCollection { +function getGPSWaypoints(gpsDraftDetails: ReadonlyDeep | undefined, trimmedEndPoint?: TrimmedGPSPoint): GPSWaypointCollection { const gpsTrip = getTrimmedGpsTrip(gpsDraftDetails, trimmedEndPoint); const waypointCollection: GPSWaypointCollection = {}; @@ -65,7 +65,7 @@ function getGPSWaypoints(gpsDraftDetails: GpsDraftDetails | undefined, trimmedEn return waypointCollection; } -function getGPSRoutes(gpsDraftDetails: GpsDraftDetails | undefined): Routes { +function getGPSRoutes(gpsDraftDetails: ReadonlyDeep | undefined): Routes { const distanceInMeters = roundToTwoDecimalPlaces(gpsDraftDetails?.modifiedDistance ?? gpsDraftDetails?.distanceInMeters ?? 0); const gpsCoordinates = getTrimmedGpsTrip(gpsDraftDetails); const coordinates: Array> = gpsCoordinates.map((points) => points.map(({lat, long}) => [long, lat])); @@ -87,7 +87,7 @@ function getGPSRoutes(gpsDraftDetails: GpsDraftDetails | undefined): Routes { * If the trimmed end point is set, it is added to the recorded GPS coordinates according to the data * saved in trimmedEndPoint - this is a backend requirement to make the receipt generation easier. */ -function getStringifiedGPSCoordinates(gpsDraftDetails: GpsDraftDetails | undefined): string | undefined { +function getStringifiedGPSCoordinates(gpsDraftDetails: ReadonlyDeep | undefined): string | undefined { const trimmedEndPoint = gpsDraftDetails?.trimmedEndPoint; const gpsPoints = gpsDraftDetails?.gpsPoints; @@ -136,7 +136,7 @@ function coordinatesToString(gpsPoint: {lat: number; long: number}): string { return `${gpsPoint.lat},${gpsPoint.long}`; } -function isLastSegmentEmptyOrHasOnlyOnePoint(lastSegment: GPSPoint[]): boolean { +function isLastSegmentEmptyOrHasOnlyOnePoint(lastSegment: ReadonlyDeep): boolean { if (lastSegment.length <= 1) { return true; } @@ -144,7 +144,7 @@ function isLastSegmentEmptyOrHasOnlyOnePoint(lastSegment: GPSPoint[]): boolean { return false; } -async function stopGpsTrip(isOffline: boolean, gpsPoints: GPSPoint[][], skipLastPointAddressFetching = false) { +async function stopGpsTrip(isOffline: boolean, gpsPoints: ReadonlyDeep, skipLastPointAddressFetching = false) { const isBackgroundTaskRunning = await hasStartedLocationUpdatesAsync(BACKGROUND_LOCATION_TRACKING_TASK_NAME); if (isBackgroundTaskRunning) { @@ -196,27 +196,27 @@ async function stopGpsTrip(isOffline: boolean, gpsPoints: GPSPoint[][], skipLast setEndWaypointAddress({value: formattedCoordinates, type: 'coordinates'}, gpsPoints); } -function getTotalGpsTripPoints(gpsDraftDetails: GpsDraftDetails | undefined): number { +function getTotalGpsTripPoints(gpsDraftDetails: ReadonlyDeep | undefined): number { return gpsDraftDetails?.gpsPoints?.flat().length ?? 0; } -function getTotalGpsTripPointsInLastSegment(gpsPoints: GPSPoint[][]): number { +function getTotalGpsTripPointsInLastSegment(gpsPoints: ReadonlyDeep): number { return gpsPoints.at(-1)?.length ?? 0; } -function isTripStopped(gpsDraftDetails: GpsDraftDetails | undefined): boolean { +function isTripStopped(gpsDraftDetails: ReadonlyDeep | undefined): boolean { return !gpsDraftDetails?.isTracking && getTotalGpsTripPoints(gpsDraftDetails) > 0; } -function getGpsPoints(gpsDraftDetails: GpsDraftDetails | undefined): GPSPoint[][] { +function getGpsPoints(gpsDraftDetails: ReadonlyDeep | undefined): ReadonlyDeep { return gpsDraftDetails?.gpsPoints ?? [[]]; } -function getFirstGpsPoint(gpsDraftDetails: GpsDraftDetails | undefined): GPSPoint | undefined { +function getFirstGpsPoint(gpsDraftDetails: ReadonlyDeep | undefined): GPSPoint | undefined { return gpsDraftDetails?.gpsPoints?.at(0)?.at(0); } -function calculateTrimmedEndPoint(gpsPoints: GPSPoint[][], targetDistanceMeters: number): TrimmedGPSPoint | null { +function calculateTrimmedEndPoint(gpsPoints: ReadonlyDeep, targetDistanceMeters: number): TrimmedGPSPoint | null { let distanceTraveled = 0; for (let segmentIndex = 0; segmentIndex < gpsPoints.length; segmentIndex++) { @@ -252,11 +252,19 @@ function calculateTrimmedEndPoint(gpsPoints: GPSPoint[][], targetDistanceMeters: return null; } -function getTrimmedGpsTrip(gpsDraftDetails: GpsDraftDetails | undefined, trimmedEndPoint?: TrimmedGPSPoint): GPSPoint[][]; -function getTrimmedGpsTrip(gpsPoints: GPSPoint[][], trimmedEndPoint: TrimmedGPSPoint | undefined): GPSPoint[][]; -function getTrimmedGpsTrip(gpsData: GPSPoint[][] | GpsDraftDetails | undefined, trimmedEndPointProp?: TrimmedGPSPoint | undefined): GPSPoint[][] { - const gpsPoints = Array.isArray(gpsData) ? gpsData : getGpsPoints(gpsData); - const trimmedEndPoint = trimmedEndPointProp ?? (Array.isArray(gpsData) ? undefined : gpsData?.trimmedEndPoint); +/** + * `Array.isArray()` is typed `(value: any) => value is any[]`, so it does not discriminate a union whose + * array member is read-only: the array stays in the type of the false branch. This guard does. + */ +function isGpsPointSegments(gpsData: ReadonlyDeep | ReadonlyDeep | undefined): gpsData is ReadonlyDeep { + return Array.isArray(gpsData); +} + +function getTrimmedGpsTrip(gpsDraftDetails: ReadonlyDeep | undefined, trimmedEndPoint?: TrimmedGPSPoint): ReadonlyDeep; +function getTrimmedGpsTrip(gpsPoints: ReadonlyDeep, trimmedEndPoint: TrimmedGPSPoint | undefined): ReadonlyDeep; +function getTrimmedGpsTrip(gpsData: ReadonlyDeep | ReadonlyDeep | undefined, trimmedEndPointProp?: TrimmedGPSPoint | undefined): ReadonlyDeep { + const gpsPoints = isGpsPointSegments(gpsData) ? gpsData : getGpsPoints(gpsData); + const trimmedEndPoint = trimmedEndPointProp ?? (isGpsPointSegments(gpsData) ? undefined : gpsData?.trimmedEndPoint); if (!trimmedEndPoint) { return gpsPoints; @@ -276,7 +284,7 @@ function getTrimmedGpsTrip(gpsData: GPSPoint[][] | GpsDraftDetails | undefined, return gpsPoints.slice(0, trimmedEndPointSegment).concat([updatedSegment]); } -function gpsPointsToMapboxCoordinates(coordinates: GPSPoint[][]): Coordinate[][] { +function gpsPointsToMapboxCoordinates(coordinates: ReadonlyDeep): Coordinate[][] { return coordinates.map((segment): Coordinate[] => segment.map(({lat, long}) => [long, lat])); } diff --git a/src/libs/OnyxReadonly.ts b/src/libs/OnyxReadonly.ts new file mode 100644 index 000000000000..6a0a19d961e5 --- /dev/null +++ b/src/libs/OnyxReadonly.ts @@ -0,0 +1,41 @@ +import type {OnyxKey, OnyxValue} from 'react-native-onyx'; +import type {ReadonlyDeep} from 'type-fest'; + +import OnyxLib from 'react-native-onyx'; + +/** + * The value `Onyx.get()` really hands back: the cached object itself, shared with every other reader of + * that key. Typing it deeply read-only is what turns the "the result of `Onyx.get()` MUST NOT be mutated" + * convention into a rule the compiler enforces instead of a comment reviewers have to remember. + */ +type OnyxReadonlyValue = ReadonlyDeep>; + +/** + * Local experiment for the Onyx immutability audit (https://github.com/Expensify/App/issues/71206). + * + * `Onyx.get()` re-typed so a one-shot read can only produce read-only data. Runtime is untouched: this is + * the same `Onyx` object with a narrower type on one method, so nothing about behaviour, cache identity or + * bundle size changes. It lives in the app rather than in the library so the cost can be measured on real + * call sites before it is proposed as the library's own signature. + * + * `get()` is deliberately the only method touched. It is the read whose result reaches the fewest places, and + * the one whose contract already says the value must not be mutated. The rest of the API is left alone: + * + * - `useOnyx()` hands back the same shared object, but its result flows through render and hundreds of props. + * Typing it read-only costs 303 errors in 165 files shallow, or 2421 in 731 deep, the latter including + * ~110 that no signature change can fix (`ReadonlyDeep` breaks the shape of `ReportAction['message']`). + * - The writers keep their mutable input types. Widening them to accept read-only data would cost 23 errors + * in 6 files and would let a value read with `get()` be written straight back, but it is a separate change + * and does not have to land first. Until it does, a caller that reads with `get()` and writes the same + * value back rebuilds the mutable parts itself. + */ +// The cast is the whole point of the file: the same object, with a narrower type on one method. There is no +// expression TypeScript can check here, because `Promise>` and +// `Promise>>` are not comparable while `TKey` is still generic. +// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion +const OnyxReadonly = OnyxLib as Omit & { + get: (key: TKey) => Promise>; +}; + +export default OnyxReadonly; +export type {OnyxReadonlyValue}; diff --git a/src/libs/actions/Delegate.ts b/src/libs/actions/Delegate.ts index 2c8926bb4bf5..45792e372237 100644 --- a/src/libs/actions/Delegate.ts +++ b/src/libs/actions/Delegate.ts @@ -22,6 +22,7 @@ import type Credentials from '@src/types/onyx/Credentials'; import type Session from '@src/types/onyx/Session'; import type {OnyxEntry, OnyxUpdate} from 'react-native-onyx'; +import type {ReadonlyDeep} from 'type-fest'; import HybridAppModule from '@expensify/react-native-hybrid-app'; import Onyx from 'react-native-onyx'; @@ -76,7 +77,7 @@ function clearOnyxForDelegateTransition(): Promise { type WithDelegatedAccess = { // Optional keeps call sites clean, but still encourages passing `account?.delegatedAccess`. - delegatedAccess: DelegatedAccess | undefined; + delegatedAccess: ReadonlyDeep | undefined; }; type WithCredentials = { diff --git a/src/libs/actions/GPSDraftDetails.ts b/src/libs/actions/GPSDraftDetails.ts index 43591d4ee888..ac63581c2118 100644 --- a/src/libs/actions/GPSDraftDetails.ts +++ b/src/libs/actions/GPSDraftDetails.ts @@ -10,6 +10,7 @@ import type {Unit} from '@src/types/onyx/Policy'; import geodesicDistance from '@src/utils/geodesicDistance'; import type {OnyxEntry} from 'react-native-onyx'; +import type {ReadonlyDeep} from 'type-fest'; import Onyx from 'react-native-onyx'; @@ -19,11 +20,20 @@ function resetGPSDraftDetails() { Onyx.merge(ONYXKEYS.GPS_DRAFT_DETAILS, null); } -function getGpsPoints(gpsDraftDetails: GpsDraftDetails | undefined): GPSPoint[][] { +function getGpsPoints(gpsDraftDetails: ReadonlyDeep | undefined): ReadonlyDeep { return gpsDraftDetails?.gpsPoints ?? [[]]; } -function setStartWaypointAddress(startAddress: GPSPointAddress, tripSegmentIndex: number, gpsPoints: GPSPoint[][]) { +/** + * Onyx's write inputs are typed mutable while a trip read with `Onyx.get()` is read-only, so the segment + * arrays are rebuilt on the way in. Only the arrays are new: the points themselves are shared, which is what + * the store ends up holding either way. + */ +function toWritableGpsPoints(gpsPoints: ReadonlyDeep): GPSPoint[][] { + return gpsPoints.map((segment) => [...segment]); +} + +function setStartWaypointAddress(startAddress: GPSPointAddress, tripSegmentIndex: number, gpsPoints: ReadonlyDeep) { const tripSegment = gpsPoints.at(tripSegmentIndex); const segmentFirstPoint = tripSegment?.at(0); @@ -32,7 +42,7 @@ function setStartWaypointAddress(startAddress: GPSPointAddress, tripSegmentIndex } const newSegment = [{...segmentFirstPoint, address: startAddress}, ...tripSegment.slice(1)]; - const newGpsPoints = [...gpsPoints]; + const newGpsPoints = toWritableGpsPoints(gpsPoints); newGpsPoints.splice(tripSegmentIndex, 1, newSegment); Onyx.merge(ONYXKEYS.GPS_DRAFT_DETAILS, { @@ -40,7 +50,7 @@ function setStartWaypointAddress(startAddress: GPSPointAddress, tripSegmentIndex }); } -function setEndWaypointAddress(endAddress: GPSPointAddress, gpsPoints: GPSPoint[][], tripSegmentIndex = -1) { +function setEndWaypointAddress(endAddress: GPSPointAddress, gpsPoints: ReadonlyDeep, tripSegmentIndex = -1) { const tripSegment = gpsPoints.at(tripSegmentIndex); const segmentLastPoint = tripSegment?.at(-1); @@ -49,7 +59,7 @@ function setEndWaypointAddress(endAddress: GPSPointAddress, gpsPoints: GPSPoint[ } const newSegment = [...tripSegment.slice(0, -1), {...segmentLastPoint, address: endAddress}]; - const newGpsPoints = [...gpsPoints]; + const newGpsPoints = toWritableGpsPoints(gpsPoints); newGpsPoints.splice(tripSegmentIndex, 1, newSegment); Onyx.merge(ONYXKEYS.GPS_DRAFT_DETAILS, { @@ -57,13 +67,13 @@ function setEndWaypointAddress(endAddress: GPSPointAddress, gpsPoints: GPSPoint[ }); } -function updateGpsPoints(gpsPoints: GPSPoint[][]) { +function updateGpsPoints(gpsPoints: ReadonlyDeep) { Onyx.merge(ONYXKEYS.GPS_DRAFT_DETAILS, { - gpsPoints, + gpsPoints: toWritableGpsPoints(gpsPoints), }); } -function removeLastSegment(gpsPoints: GPSPoint[][]) { +function removeLastSegment(gpsPoints: ReadonlyDeep) { // Clear the last segment instead of removing it if there is only one segment if (gpsPoints.length === 1) { Onyx.merge(ONYXKEYS.GPS_DRAFT_DETAILS, { @@ -72,7 +82,7 @@ function removeLastSegment(gpsPoints: GPSPoint[][]) { return; } - const newGpsPoints = [...gpsPoints]; + const newGpsPoints = toWritableGpsPoints(gpsPoints); newGpsPoints.pop(); Onyx.merge(ONYXKEYS.GPS_DRAFT_DETAILS, { @@ -91,13 +101,13 @@ function initGpsDraft(reportID: string, unit: Unit, accountID?: number) { }); } -function resumeGpsTrip(gpsDraftDetails: OnyxEntry) { +function resumeGpsTrip(gpsDraftDetails: ReadonlyDeep>) { if (!gpsDraftDetails) { return; } const lastTripSegment = gpsDraftDetails.gpsPoints.at(-1); - const newGpsPoints = [...gpsDraftDetails.gpsPoints]; + const newGpsPoints = toWritableGpsPoints(gpsDraftDetails.gpsPoints); if (lastTripSegment && lastTripSegment.length !== 0) { newGpsPoints.push([]); @@ -120,7 +130,7 @@ function setIsTracking(isTracking: boolean) { /** * Adds new GPS points to the captured points and updates the start address if the last segment is empty */ -function addGpsPoints(gpsDraftDetails: OnyxEntry, newGpsPoints: GPSPoint[]): GPSPoint[][] { +function addGpsPoints(gpsDraftDetails: ReadonlyDeep>, newGpsPoints: GPSPoint[]): ReadonlyDeep { const capturedPoints = getGpsPoints(gpsDraftDetails); const lastTripSegment = capturedPoints.at(-1); @@ -152,7 +162,7 @@ function addGpsPoints(gpsDraftDetails: OnyxEntry, newGpsPoints: const updatedDistance = capturedDistance + distanceToAdd; - const newCapturedPoints = [...capturedPoints]; + const newCapturedPoints = toWritableGpsPoints(capturedPoints); newCapturedPoints.splice(newCapturedPoints.length - 1, 1, [...lastTripSegment, ...gpsPointsToAdd]); const latestPoint = newCapturedPoints.at(-1)?.at(-1); diff --git a/src/libs/actions/User.ts b/src/libs/actions/User.ts index 548329ea67ac..2d88edba517e 100644 --- a/src/libs/actions/User.ts +++ b/src/libs/actions/User.ts @@ -1801,7 +1801,12 @@ function clearExpenseRuleErrors(expenseRules: ExpenseRule[], selectedRuleKey: st Onyx.set(ONYXKEYS.NVP_EXPENSE_RULES, updatedExpenseRules); } -function saveExpenseRule(expenseRules: ExpenseRule[], newRule: ExpenseRule, existingRuleKey: string | undefined, getKeyForRule: (rule: ExpenseRule) => string) { +/** + * `expenseRules` is read-only because the caller reads it with `Onyx.get()`, which hands back the cached + * array itself. Nothing here needs it mutable: every rule list below is built with `map` or a spread, both + * of which produce a fresh array. + */ +function saveExpenseRule(expenseRules: readonly ExpenseRule[], newRule: ExpenseRule, existingRuleKey: string | undefined, getKeyForRule: (rule: ExpenseRule) => string) { const isEditing = !!existingRuleKey; const pendingAction = isEditing ? CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE : CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD; @@ -1848,7 +1853,8 @@ function saveExpenseRule(expenseRules: ExpenseRule[], newRule: ExpenseRule, exis } else { optimisticRules = [...expenseRules, ruleWithPendingAction]; successRules = [...expenseRules, ruleForSuccess]; - failureRules = expenseRules; + // Copied like the two lists above, because the read-only array cannot be handed to `Onyx.set` as-is. + failureRules = [...expenseRules]; } const rulesForAPI = successRules diff --git a/src/pages/iou/request/step/IOURequestStepDistanceGPS/GPSButtons/index.tsx b/src/pages/iou/request/step/IOURequestStepDistanceGPS/GPSButtons/index.tsx index 00fa0335aa1a..f94bf7b3b19c 100644 --- a/src/pages/iou/request/step/IOURequestStepDistanceGPS/GPSButtons/index.tsx +++ b/src/pages/iou/request/step/IOURequestStepDistanceGPS/GPSButtons/index.tsx @@ -21,6 +21,8 @@ import ONYXKEYS from '@src/ONYXKEYS'; import type {GPSPoint} from '@src/types/onyx/GpsDraftDetails'; import type {Unit} from '@src/types/onyx/Policy'; +import type {ReadonlyDeep} from 'type-fest'; + import {hasServicesEnabledAsync, startLocationUpdatesAsync} from 'expo-location'; import React, {useState} from 'react'; import {Linking, View} from 'react-native'; @@ -45,7 +47,7 @@ type ButtonsProps = { unit: Unit; /** Captured GPS points */ - gpsPoints: GPSPoint[][]; + gpsPoints: ReadonlyDeep; }; function GPSButtons({navigateToNextStep, setShouldShowStartError, setShouldShowPermissionsError, reportID, unit, gpsPoints}: ButtonsProps) { diff --git a/src/pages/settings/Rules/RulePageBase.tsx b/src/pages/settings/Rules/RulePageBase.tsx index 7c0b79def208..43f66ab3f28e 100644 --- a/src/pages/settings/Rules/RulePageBase.tsx +++ b/src/pages/settings/Rules/RulePageBase.tsx @@ -16,6 +16,7 @@ import {clearDraftRule, saveExpenseRule, updateDraftRule} from '@libs/actions/Us import {getAvailableNonPersonalPolicyCategories, getDecodedCategoryName} from '@libs/CategoryUtils'; import {extractRuleFromForm, getKeyForRule} from '@libs/ExpenseRuleUtils'; import Navigation from '@libs/Navigation/Navigation'; +import Onyx from '@libs/OnyxReadonly'; import {hasEnabledOptions} from '@libs/OptionsListUtils'; import Parser from '@libs/Parser'; import {getAllTaxRatesNamesAndValues, getCleanedTagName, getTagLists} from '@libs/PolicyUtils'; @@ -29,7 +30,7 @@ import ONYXKEYS from '@src/ONYXKEYS'; import ROUTES from '@src/ROUTES'; import type {ExpenseRuleForm, ExpenseRuleFormFieldID} from '@src/types/form/ExpenseRuleForm'; import EXPENSE_RULE_INPUT_IDS from '@src/types/form/ExpenseRuleForm'; -import type {ExpenseRule, PolicyCategories, PolicyTagLists} from '@src/types/onyx'; +import type {PolicyCategories, PolicyTagLists} from '@src/types/onyx'; import getEmptyArray from '@src/types/utils/getEmptyArray'; import type {OnyxCollection} from 'react-native-onyx'; @@ -96,7 +97,6 @@ const getErrorMessage = (translate: LocalizedTranslate, form?: ExpenseRuleForm) function RulePageBase({titleKey, testID, hash}: RulePageBaseProps) { const {translate} = useLocalize(); - const [expenseRules = getEmptyArray()] = useOnyx(ONYXKEYS.NVP_EXPENSE_RULES); const [form] = useOnyx(ONYXKEYS.FORMS.EXPENSE_RULE_FORM); // Cannot use useRef because react compiler fails const [isSaving, setIsSaving] = useState(false); @@ -136,7 +136,7 @@ function RulePageBase({titleKey, testID, hash}: RulePageBaseProps) { const errorMessage = getErrorMessage(translate, form); - const handleSubmit = () => { + const handleSubmit = async () => { if (errorMessage) { setShouldShowError(true); return; @@ -145,6 +145,10 @@ function RulePageBase({titleKey, testID, hash}: RulePageBaseProps) { return; } + // The rule list is only ever handed to saveExpenseRule(), so it is read on submit rather than + // subscribed to. Read before the spinner starts, so the work after the paint stays synchronous. + const expenseRules = (await Onyx.get(ONYXKEYS.NVP_EXPENSE_RULES)) ?? []; + startWithLoading(() => { setIsSaving(true); @@ -286,7 +290,9 @@ function RulePageBase({titleKey, testID, hash}: RulePageBaseProps) { containerStyles={[styles.m4, styles.mb5]} isAlertVisible={shouldShowError && !!errorMessage} message={errorMessage} - onSubmit={handleSubmit} + onSubmit={() => { + handleSubmit(); + }} isLoading={isLoading} shouldShowLoadingImmediatelyOnPress={false} enabledWhenOffline diff --git a/src/selectors/GPSDraftDetails.ts b/src/selectors/GPSDraftDetails.ts index 0bf40b94d116..16f3e8d3f776 100644 --- a/src/selectors/GPSDraftDetails.ts +++ b/src/selectors/GPSDraftDetails.ts @@ -1,8 +1,9 @@ import type {GpsDraftDetails} from '@src/types/onyx'; import type {OnyxEntry} from 'react-native-onyx'; +import type {ReadonlyDeep} from 'type-fest'; -const isTrackingSelector = (gpsDraftDetails?: OnyxEntry) => !!gpsDraftDetails?.isTracking; +const isTrackingSelector = (gpsDraftDetails?: ReadonlyDeep>) => !!gpsDraftDetails?.isTracking; // eslint-disable-next-line import/prefer-default-export export {isTrackingSelector}; diff --git a/src/setup/backgroundLocationTrackingTask/index.native.ts b/src/setup/backgroundLocationTrackingTask/index.native.ts index 1113ffc08e91..0e80330c7072 100644 --- a/src/setup/backgroundLocationTrackingTask/index.native.ts +++ b/src/setup/backgroundLocationTrackingTask/index.native.ts @@ -9,6 +9,7 @@ import type {GPSPoint} from '@src/types/onyx/GpsDraftDetails'; import type {LocationObject} from 'expo-location'; import type {OnyxEntry} from 'react-native-onyx'; +import type {ReadonlyDeep} from 'type-fest'; import NetInfo from '@react-native-community/netinfo'; import {defineTask} from 'expo-task-manager'; @@ -19,11 +20,11 @@ type BackgroundLocationTrackingTaskData = {locations: LocationObject[]}; // This is a headless background task (registered via defineTask) that runs outside React, so useOnyx() // isn't available. GPS_DRAFT_DETAILS is read twice (at task start and again after reverse geocoding), so // use a short-lived connectWithoutView one-shot read that disconnects as soon as it has the latest value. -function getGpsDraftDetails(): Promise> { +function getGpsDraftDetails(): Promise>> { return new Promise((resolve) => { const connection = Onyx.connectWithoutView({ key: ONYXKEYS.GPS_DRAFT_DETAILS, - callback: (gpsDraftDetails: OnyxEntry) => { + callback: (gpsDraftDetails: ReadonlyDeep>) => { Onyx.disconnect(connection); resolve(gpsDraftDetails); }, @@ -58,11 +59,11 @@ defineTask(BACKGROUND_LOCATION_TRACKING_TASK }); // Checks if latest update added first point to the current segment -function shouldUpdateStartAddress(gpsPoints: GPSPoint[][], updatedGpsPoints: GPSPoint[][]) { +function shouldUpdateStartAddress(gpsPoints: ReadonlyDeep, updatedGpsPoints: ReadonlyDeep) { return getTotalGpsTripPointsInLastSegment(gpsPoints) === 0 && getTotalGpsTripPointsInLastSegment(updatedGpsPoints) !== 0; } -async function updateStartAddress(gpsPoints: GPSPoint[][], isOffline: boolean) { +async function updateStartAddress(gpsPoints: ReadonlyDeep, isOffline: boolean) { const startPoint = gpsPoints.at(-1)?.at(0); if (!startPoint) { return;