From 1d42b701016c779e2d58184c8b32b223145405aa Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Thu, 24 Sep 2026 09:34:20 +0000 Subject: [PATCH 01/16] Restore: Show up to three report fields per row and edit them inline Reverts the revert in PR 101854, restoring the changes originally merged in PR 100735 so the work is available on a branch for regression triage. Co-authored-by: daledah --- config/eslint/eslint.seatbelt.tsv | 3 +- src/CONST/index.ts | 6 + src/components/DatePicker/index.tsx | 9 +- src/components/DatePicker/types.ts | 9 + .../MoneyRequestReportActionsList.tsx | 3 +- .../MoneyRequestReportEmptyStateView.tsx | 3 +- .../MoneyRequestViewReportFields.tsx | 111 ++++-- .../ReportFieldInlineInput.tsx | 246 ++++++++++++ .../ReportActionItem/MoneyReportView.tsx | 83 +--- src/hooks/useSaveReportField.ts | 79 ++++ src/libs/ReportFieldOptionsListUtils.ts | 14 +- src/libs/actions/Report/index.ts | 2 +- src/pages/DynamicEditReportFieldPage.tsx | 43 +- src/pages/EditReportFieldDropdown.tsx | 48 ++- src/styles/index.ts | 8 + tests/ui/EditReportFieldDropdownTest.tsx | 109 +++++ tests/ui/MoneyReportViewTest.tsx | 43 +- tests/ui/MoneyRequestViewReportFieldsTest.tsx | 374 ++++++++++++++++++ tests/unit/ReportFieldOptionsListUtilsTest.ts | 31 ++ 19 files changed, 1070 insertions(+), 154 deletions(-) create mode 100644 src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx create mode 100644 src/hooks/useSaveReportField.ts create mode 100644 tests/ui/EditReportFieldDropdownTest.tsx create mode 100644 tests/ui/MoneyRequestViewReportFieldsTest.tsx diff --git a/config/eslint/eslint.seatbelt.tsv b/config/eslint/eslint.seatbelt.tsv index 986284975e8e..64d25be38bd7 100644 --- a/config/eslint/eslint.seatbelt.tsv +++ b/config/eslint/eslint.seatbelt.tsv @@ -268,7 +268,7 @@ "../../src/components/ReceiptHoverZoom/useReceiptHoverZoom.ts" "@typescript-eslint/no-unsafe-type-assertion" 1 "../../src/components/ReportActionAvatars/useReportPreviewSenderID.ts" "@typescript-eslint/no-unsafe-type-assertion" 2 "../../src/components/ReportActionItem/IssueCardMessage.tsx" "@typescript-eslint/no-unsafe-type-assertion" 2 -"../../src/components/ReportActionItem/MoneyReportView.tsx" "@typescript-eslint/no-unsafe-type-assertion" 2 +"../../src/components/ReportActionItem/MoneyReportView.tsx" "@typescript-eslint/no-unsafe-type-assertion" 1 "../../src/components/ReportActionItem/MoneyRequestReceiptView.tsx" "@typescript-eslint/no-unsafe-type-assertion" 2 "../../src/components/ReportActionItem/MoneyRequestReceiptView.tsx" "react-hooks/set-state-in-effect" 1 "../../src/components/ReportActionItem/MoneyRequestReceiptView.tsx" "rulesdir/no-direct-personal-details-list" 3 @@ -929,7 +929,6 @@ "../../src/pages/domain/Members/MoveUserBetweenGroupsPage.tsx" "rulesdir/no-direct-personal-details-list" 1 "../../src/pages/domain/Members/TwoFactorAuth/DomainMemberForceTwoFactorAuthPage.tsx" "rulesdir/no-direct-personal-details-list" 1 "../../src/pages/domain/Members/TwoFactorAuth/DomainMemberResetTwoFactorAuthPage.tsx" "rulesdir/no-direct-personal-details-list" 1 -"../../src/pages/DynamicEditReportFieldPage.tsx" "@typescript-eslint/no-unsafe-type-assertion" 1 "../../src/pages/DynamicReportChangeWorkspacePage.tsx" "rulesdir/no-direct-personal-details-list" 3 "../../src/pages/DynamicReportDetailsPage.tsx" "@typescript-eslint/no-unsafe-type-assertion" 3 "../../src/pages/DynamicReportDetailsPage.tsx" "rulesdir/no-direct-personal-details-list" 1 diff --git a/src/CONST/index.ts b/src/CONST/index.ts index d816013a9837..f7eff7788782 100644 --- a/src/CONST/index.ts +++ b/src/CONST/index.ts @@ -7132,6 +7132,12 @@ const CONST = { REPORT_FIELD_TITLE_FIELD_ID: 'text_title', + /** How many report fields are shown side by side in the report view on a wide layout */ + REPORT_FIELDS_PER_ROW: 3, + + /** Below this many options, a report field list is short enough to scan without a search input */ + REPORT_FIELD_LIST_SEARCH_THRESHOLD: 8, + MOBILE_PAGINATION_SIZE: 15, WEB_PAGINATION_SIZE: 30, diff --git a/src/components/DatePicker/index.tsx b/src/components/DatePicker/index.tsx index c228d91bd00f..34279f0ec9a7 100644 --- a/src/components/DatePicker/index.tsx +++ b/src/components/DatePicker/index.tsx @@ -49,6 +49,8 @@ function DatePicker({ forwardedFSClass, shouldDeferShowUntilPositioned = false, shouldDismissKeyboardBeforeShow = false, + wrapperStyle, + onBlur, rightHandSideComponent, onPickerVisibilityChange, shouldHideCalendarIcon = false, @@ -228,13 +230,15 @@ function DatePicker({ <> showDatePickerModal()} + onBlur={onBlur} onSubmitEditing={() => showDatePickerModal()} onKeyPress={handleInputKeyPress} textInputContainerStyles={isModalVisible ? styles.borderColorFocus : {}} diff --git a/src/components/DatePicker/types.ts b/src/components/DatePicker/types.ts index 8f54da70932f..b501e0740afa 100644 --- a/src/components/DatePicker/types.ts +++ b/src/components/DatePicker/types.ts @@ -5,6 +5,8 @@ import type {ForwardedFSClassProps} from '@libs/Fullstory/types'; import type {OnyxFormValuesMapping} from '@src/ONYXKEYS'; +import type {StyleProp, ViewStyle} from 'react-native'; + type DatePickerBaseProps = ForwardedFSClassProps & { /** * The datepicker supports any value that `new Date()` can parse. @@ -70,6 +72,13 @@ type DateInputWithPickerProps = DatePickerBaseProps & */ shouldDismissKeyboardBeforeShow?: boolean; + /** + * Style for the view that wraps the input and anchors the calendar. It carries a vertical margin by default, + * which a caller laying the field out itself can override, for example to line the input up with a plain + * `TextInput` beside it. + */ + wrapperStyle?: StyleProp; + /** * Reports whether the calendar is open. Opening the picker blurs the input, so this is the signal for "the * user is on this field" rather than `onFocus`, and it is what drives the input's focused border. diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx index 9043769883ad..dec904a18b0c 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx @@ -383,7 +383,8 @@ function MoneyRequestReportActionsListContent({reportIDFromRoute, onLayout}: Mon onViewableItemsChanged={onViewableItemsChanged} onEndReached={onEndReached} onStartReached={onStartReached} - contentContainerStyle={shouldUseNarrowLayout ? styles.pt4 : styles.pt3} + // 20px between the report header and the first row of content, which is the report field inputs when the report has fields. + contentContainerStyle={styles.pt5} isLoadingInitialActions={isInitialReportLoadPending} /* This list is not inverted, so the footer is the bottom of the message feed — the same position the indicator occupies in the inverted ReportActionsList. */ diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportEmptyStateView.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportEmptyStateView.tsx index 729a87232a79..9f79693af7ed 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportEmptyStateView.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportEmptyStateView.tsx @@ -27,7 +27,8 @@ function MoneyRequestReportEmptyStateView({report, policy, onLayout}: MoneyReque const styles = useThemeStyles(); return ( - + // `pt5` matches the 20px the unified list puts between the report header and its first row. + ; @@ -44,6 +44,16 @@ type MoneyRequestViewReportFieldsProps = { /** Indicates whether we have any pending actions from parent component */ pendingAction?: PendingAction; + + /** Extra styles for the block. Each call site sits in a container with its own padding, so it sets the spacing it needs. */ + style?: StyleProp; + + /** + * Stacks the fields one per row at any width, instead of filling rows of `CONST.REPORT_FIELDS_PER_ROW`. + * A one-expense report reads as a single expense rather than a table, so its fields stay vertical there. + * @default false + */ + shouldUseSingleColumn?: boolean; }; type EnrichedPolicyReportField = { @@ -54,39 +64,43 @@ type EnrichedPolicyReportField = { violationTranslation: string; } & PolicyReportField; -function ReportFieldView(reportField: EnrichedPolicyReportField, report: OnyxEntry, styles: ThemeStyles, pendingAction?: PendingAction) { +function ReportFieldView( + reportField: EnrichedPolicyReportField, + report: OnyxEntry, + policy: OnyxEntry, + styles: ThemeStyles, + onSaveValue: (reportField: PolicyReportField, value: string) => void, + pendingAction?: PendingAction, +) { return ( - clearReportFieldKeyErrors(report?.reportID, reportField.fieldKey)} + - { - if (!report?.policyID) { - return; - } - - Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.EDIT_REPORT_FIELD.getRoute(report.policyID, reportField.fieldID))); - }} - shouldShowRightIcon={!reportField.isFieldDisabled} - wrapperStyle={[styles.pv2, styles.taskDescriptionMenuItem]} - shouldGreyOutWhenDisabled={false} - numberOfLinesTitle={0} - interactive={!reportField.isFieldDisabled} - titleWithTooltips={[]} - brickRoadIndicator={reportField.violation ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined} - errorText={reportField.violationTranslation} - /> - + clearReportFieldKeyErrors(report?.reportID, reportField.fieldKey)} + > + onSaveValue(reportField, value)} + /> + + ); } -function MoneyRequestViewReportFields({report, policy, pendingAction}: MoneyRequestViewReportFieldsProps) { +function MoneyRequestViewReportFields({report, policy, pendingAction, style, shouldUseSingleColumn = false}: MoneyRequestViewReportFieldsProps) { const styles = useThemeStyles(); + // The report view is a RightModalNavigator screen shown as a wide RHP, where `useResponsiveLayout` reports a narrow layout at any pane width. + const {shouldUseNarrowLayout} = useResponsiveLayoutOnWideRHP(); + const saveReportField = useSaveReportField(report, policy); const {accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${getNonEmptyStringOnyxID(report?.reportID)}`); const {getCurrencyDecimals} = useCurrencyListActions(); @@ -128,10 +142,35 @@ function MoneyRequestViewReportFields({report, policy, pendingAction}: MoneyRequ return null; } + const columnCount = shouldUseNarrowLayout || shouldUseSingleColumn ? 1 : CONST.REPORT_FIELDS_PER_ROW; + const fieldRows: EnrichedPolicyReportField[][] = []; + for (let index = 0; index < sortedPolicyReportFields.length; index += columnCount) { + fieldRows.push(sortedPolicyReportFields.slice(index, index + columnCount)); + } + return ( - - {sortedPolicyReportFields.map((reportField) => { - return ReportFieldView(reportField, report, styles, pendingAction); + + {fieldRows.map((fieldRow) => { + const rowKey = `reportFieldRow-${fieldRow.at(0)?.fieldKey}`; + + return ( + + {fieldRow.map((reportField) => ReportFieldView(reportField, report, policy, styles, saveReportField, pendingAction))} + {/* A partly filled last row is padded out so its fields stay the same width as the rows above it. */} + {Array.from({length: columnCount - fieldRow.length}, (_unused, index) => ( + + ))} + + ); })} ); diff --git a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx new file mode 100644 index 000000000000..2449795d36cb --- /dev/null +++ b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx @@ -0,0 +1,246 @@ +/** + * Renders a single report field in the report view as an editable input instead of a row that navigates to the + * report field editor page. Text and formula fields are typed into directly, dates open the calendar in place, and + * list fields open the option list in a dropdown anchored under the input, so changing a value never takes the user + * off the report. + */ + +import DatePicker from '@components/DatePicker'; +import FilterPopupButton from '@components/Search/FilterDropdowns/FilterPopupButton'; +import type {FilterPopupButtonProps} from '@components/Search/FilterDropdowns/FilterPopupButton'; +import TextInput from '@components/TextInput'; + +import useIsInLandscapeMode from '@hooks/useIsInLandscapeMode'; +import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; +import useLocalize from '@hooks/useLocalize'; +import useThemeStyles from '@hooks/useThemeStyles'; +import useWindowDimensions from '@hooks/useWindowDimensions'; + +import {hasCircularReferences} from '@libs/Formula'; +import type {FieldList} from '@libs/Formula'; +import StringUtils from '@libs/StringUtils'; + +import EditReportFieldDropdown from '@pages/EditReportFieldDropdown'; + +import CONST from '@src/CONST'; +import type {PolicyReportField} from '@src/types/onyx'; + +import {Str} from 'expensify-common'; +import React, {useState} from 'react'; +import {View} from 'react-native'; + +type ReportFieldInlineInputProps = { + reportField: PolicyReportField; + + /** Key the field is stored under, used as the input ID */ + fieldKey: string; + + /** Value currently shown for the field */ + value: string; + + /** Whether the current user is allowed to change the value */ + isDisabled: boolean; + + /** Message shown under the input, for example a field violation */ + errorText?: string; + + /** Policy field list, used to reject values that make a formula refer to itself */ + fieldList?: FieldList; + + onSaveValue: (value: string) => void; +}; + +function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, errorText, fieldList, onSaveValue}: ReportFieldInlineInputProps) { + const styles = useThemeStyles(); + const {translate} = useLocalize(); + const {windowHeight} = useWindowDimensions(); + const isInLandscapeMode = useIsInLandscapeMode(); + const icons = useMemoizedLazyExpensifyIcons(['DownArrow']); + + const [draftValue, setDraftValue] = useState(value); + const [previousValue, setPreviousValue] = useState(value); + const [validationError, setValidationError] = useState(''); + + // A field violation such as "Field is required" is derived from the value being empty, so it is already true when + // the report opens. Holding it back until the user has left the field keeps a report full of empty fields from + // opening as a wall of red. + const [hasBeenBlurred, setHasBeenBlurred] = useState(false); + + // Tracks the value that was last sent to the server so a second save attempt for the same value, for example when + // the input is blurred right after it was submitted, is skipped while the update is still in flight. + const [lastSavedValue, setLastSavedValue] = useState(value); + + // The saved value changes while this input is mounted whenever the field is updated elsewhere, for example from + // another device or by an optimistic update, so the draft follows it. + if (value !== previousValue) { + setPreviousValue(value); + setDraftValue(value); + setValidationError(''); + setLastSavedValue(value); + setHasBeenBlurred(false); + } + + const label = Str.UCFirst(reportField.name); + const isRequired = !reportField.deletable; + const isReadOnly = isDisabled || reportField.type === CONST.REPORT_FIELD_TYPES.FORMULA; + const violationError = hasBeenBlurred ? errorText : undefined; + + const saveDraftValue = () => { + const valueToSave = StringUtils.lineBreaksToSpaces(draftValue); + const trimmedValue = valueToSave.trim(); + + if (trimmedValue === lastSavedValue.trim()) { + setValidationError(''); + return; + } + + if (trimmedValue === '') { + if (isRequired) { + setValidationError(translate('common.error.fieldRequired')); + return; + } + // Clearing an optional field is a real change, so the empty value is saved. Any whitespace the user + // left behind is normalized away first so the stored value is empty rather than blank. + setValidationError(''); + setDraftValue(''); + setLastSavedValue(''); + onSaveValue(''); + return; + } + + if (hasCircularReferences(trimmedValue, reportField.name, fieldList)) { + setValidationError(translate('workspace.reportFields.circularReferenceError')); + return; + } + + setValidationError(''); + setLastSavedValue(valueToSave); + onSaveValue(valueToSave); + }; + + const saveSelectedOption = (selectedValue: string) => { + if (selectedValue === lastSavedValue) { + return; + } + setLastSavedValue(selectedValue); + onSaveValue(selectedValue); + }; + + if (reportField.type === CONST.REPORT_FIELD_TYPES.DATE && !isReadOnly) { + return ( + saveSelectedOption(selectedDate)} + onBlur={() => setHasBeenBlurred(true)} + // The grid puts its own gap between the fields, so the default vertical margin would push a date + // input below the plain text inputs sharing its row. + wrapperStyle={styles.mv0} + shouldDeferShowUntilPositioned + shouldHideClearButton + /> + ); + } + + if (reportField.type === CONST.REPORT_FIELD_TYPES.LIST && !isReadOnly) { + const enabledOptions = reportField.values.filter((_option: string, index: number) => !reportField.disabledOptions.at(index)); + + // A short list is quicker to scan than to search, so the search input is dropped and its height is not reserved. + const shouldShowSearchInput = enabledOptions.length >= CONST.REPORT_FIELD_LIST_SEARCH_THRESHOLD; + + // A short list is easier to read in a stable alphabetical order than one that moves the current value to the + // top, so only a long list is worth reordering to save scrolling. + const shouldPinSelectedOption = enabledOptions.length >= CONST.STANDARD_LIST_ITEM_LIMIT; + + // FilterPopupButton calls this as a plain function during its own render, so it must not use hooks. Everything + // it needs is read from this component's scope. + const renderOptionsPopup: FilterPopupButtonProps['PopoverComponent'] = ({closeOverlay}) => ( + + { + closeOverlay(); + saveSelectedOption(form[fieldKey] ?? ''); + }} + /> + + ); + + return ( + // No `popoverWidth`, so the dropdown falls back to `CONST.POPOVER_DROPDOWN_WIDTH` — the same width every + // Spend filter dropdown uses. Matching the field's own width instead made narrow fields open odd, cramped + // popovers and wide ones open oversized. + ( + + setHasBeenBlurred(true)} + onSubmitEditing={onPress} + disableKeyboard + /> + + )} + /> + ); + } + + return ( + { + setHasBeenBlurred(true); + saveDraftValue(); + }} + onSubmitEditing={saveDraftValue} + /> + ); +} + +export default ReportFieldInlineInput; diff --git a/src/components/ReportActionItem/MoneyReportView.tsx b/src/components/ReportActionItem/MoneyReportView.tsx index 07d679851ffa..ffcb014416c3 100644 --- a/src/components/ReportActionItem/MoneyReportView.tsx +++ b/src/components/ReportActionItem/MoneyReportView.tsx @@ -1,13 +1,11 @@ import ActivityIndicator from '@components/ActivityIndicator'; import Icon from '@components/Icon'; -import MenuItemWithTopDescription from '@components/MenuItemWithTopDescription'; -import OfflineWithFeedback from '@components/OfflineWithFeedback'; +import MoneyRequestViewReportFields from '@components/MoneyRequestReportView/MoneyRequestViewReportFields'; import SpacerView from '@components/SpacerView'; import Text from '@components/Text'; import UnreadActionIndicator from '@components/UnreadActionIndicator'; import {useCurrencyListActions} from '@hooks/useCurrencyList'; -import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; import useLocalize from '@hooks/useLocalize'; import useNetwork from '@hooks/useNetwork'; @@ -17,22 +15,15 @@ import useStyleUtils from '@hooks/useStyleUtils'; import useTheme from '@hooks/useTheme'; import useThemeStyles from '@hooks/useThemeStyles'; -import {resolveReportFieldValue} from '@libs/Formula'; import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; import {isSingleTransactionReport} from '@libs/MoneyRequestReportUtils'; -import createDynamicRoute from '@libs/Navigation/helpers/dynamicRoutesUtils/createDynamicRoute'; -import Navigation from '@libs/Navigation/Navigation'; import {isPolicyTaxEnabled} from '@libs/PolicyUtils'; import { getBillableAndTaxTotal, - getFieldViolation, - getFieldViolationTranslation, getMoneyRequestSpendBreakdown, - getReportFieldKey, getReportFieldMaps, hasUpdatedTotal, isClosedExpenseReportWithNoExpenses as isClosedExpenseReportWithNoExpensesReportUtils, - isReportFieldDisabledForUser, isReportFieldTargetMatchingReport, isSettled as isSettledReportUtils, shouldDisplayReportFields as shouldDisplayReportFieldsUtils, @@ -45,16 +36,13 @@ import AnimatedEmptyStateBackground from '@pages/inbox/report/AnimatedEmptyState import {fontScale} from '@styles/typography'; import type {TranslationPaths} from '@src/languages/types'; -import {clearReportFieldKeyErrors} from '@src/libs/actions/Report'; import ONYXKEYS from '@src/ONYXKEYS'; -import {DYNAMIC_ROUTES} from '@src/ROUTES'; import type {Policy, Report} from '@src/types/onyx'; import type {PendingAction} from '@src/types/onyx/OnyxCommon'; import type {StyleProp, TextStyle} from 'react-native'; import type {OnyxEntry} from 'react-native-onyx'; -import {Str} from 'expensify-common'; import React, {useMemo} from 'react'; import {View} from 'react-native'; @@ -95,13 +83,11 @@ function MoneyReportView({ }: MoneyReportViewProps) { const theme = useTheme(); const styles = useThemeStyles(); - const {accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${getNonEmptyStringOnyxID(report?.reportID)}`); const StyleUtils = useStyleUtils(); const {translate} = useLocalize(); - const {convertToDisplayString, getCurrencyDecimals} = useCurrencyListActions(); + const {convertToDisplayString} = useCurrencyListActions(); const {isOffline} = useNetwork(); - const [rules] = useOnyx(ONYXKEYS.COLLECTION.RULE); const isSettled = isSettledReportUtils(report?.reportID); const isTotalUpdated = hasUpdatedTotal(report, policy) && !isTotalPending; @@ -136,12 +122,13 @@ function MoneyReportView({ StyleUtils.getColorStyle(theme.textSupporting), ]; - const {sortedPolicyReportFields, fieldValues, fieldsByName} = useMemo(() => { - const {fieldValues: values, fieldsByName: byName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs); - const sorted = Object.values(byName) + // Only used to decide whether the report field block is worth rendering — `MoneyRequestViewReportFields` builds and + // resolves the fields it displays itself. + const sortedPolicyReportFields = useMemo(() => { + const {fieldsByName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs); + return Object.values(fieldsByName) .filter((field) => isReportFieldTargetMatchingReport(report, field)) .sort(({orderWeight: a}, {orderWeight: b}) => a - b); - return {sortedPolicyReportFields: sorted, fieldValues: values, fieldsByName: byName}; }, [policy?.fieldList, report, reportNameValuePairs]); const isOnlyTitleFieldEnabled = sortedPolicyReportFields.every(shouldHideSingleReportField); @@ -175,50 +162,18 @@ function MoneyReportView({ {!isClosedExpenseReportWithNoExpenses && ( <> {shouldDisplayReportFields && - (!isCombinedReport || !isOnlyTitleFieldEnabled) && - sortedPolicyReportFields.map((reportField) => { - if (shouldHideSingleReportField(reportField)) { - return null; - } - - const fieldValue = resolveReportFieldValue(reportField, report, policy, fieldValues, fieldsByName, getCurrencyDecimals); - const isFieldDisabled = isReportFieldDisabledForUser(report, reportField, policy, currentUserAccountID, rules); - const fieldKey = getReportFieldKey(reportField.fieldID); - - const violation = isFieldDisabled ? undefined : getFieldViolation(reportField); - const violationTranslation = getFieldViolationTranslation(reportField, violation); - - return ( - clearReportFieldKeyErrors(report?.reportID, fieldKey)} - > - { - if (!report?.policyID) { - return; - } - - Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.EDIT_REPORT_FIELD.getRoute(report.policyID, reportField.fieldID))); - }} - shouldShowRightIcon={!isFieldDisabled} - wrapperStyle={[styles.pv2, styles.taskDescriptionMenuItem]} - shouldGreyOutWhenDisabled={false} - numberOfLinesTitle={0} - interactive={!isFieldDisabled} - titleWithTooltips={[]} - brickRoadIndicator={violation ? 'error' : undefined} - errorText={violationTranslation} - /> - - ); - })} + (!isCombinedReport || !isOnlyTitleFieldEnabled) && ( + // One-expense reports edit their fields in place like the report view does, instead of + // opening the report field editor page. They stay one field per row though: this screen + // reads as a single expense rather than a table, so the fields stack vertically here. + + )} {shouldShowTotalRow && ( diff --git a/src/hooks/useSaveReportField.ts b/src/hooks/useSaveReportField.ts new file mode 100644 index 000000000000..dc76ffa6a71d --- /dev/null +++ b/src/hooks/useSaveReportField.ts @@ -0,0 +1,79 @@ +/** + * Builds the callback that persists a report field value. + * + * Both the report field editor page and the inline report field inputs in the report view save through this, so the + * violation, beta and recently-used handling only has to exist in one place. + */ + +import {useSession} from '@components/OnyxListItemProvider'; + +import {updateReportField, updateReportName} from '@libs/actions/Report'; +import {getReportFieldKey, hasViolations as hasViolationsReportUtils, isReportFieldDisabled, isReportFieldOfTypeTitle} from '@libs/ReportUtils'; + +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; +import type {Policy, PolicyReportField, Report} from '@src/types/onyx'; + +import type {OnyxEntry} from 'react-native-onyx'; + +import {isTrackIntentUserSelector} from '@selectors/Onboarding'; + +import useOnyx from './useOnyx'; +import usePermissions from './usePermissions'; +import useReportTransactions from './useReportTransactions'; +import useReportTransactionViolations from './useReportTransactionViolations'; + +function useSaveReportField(report: OnyxEntry, policy: OnyxEntry) { + const session = useSession(); + const {isBetaEnabled} = usePermissions(); + const [recentlyUsedReportFields] = useOnyx(ONYXKEYS.RECENTLY_USED_REPORT_FIELDS); + // Both of these are scoped to this report so a transaction or violation change in an unrelated report does not re-render every consumer. + const reportTransactions = useReportTransactions(report?.reportID); + const [transactionViolations] = useReportTransactionViolations(reportTransactions); + const [isTrackIntentUser] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED, {selector: isTrackIntentUserSelector}); + const [rules] = useOnyx(ONYXKEYS.COLLECTION.RULE); + + return (reportField: PolicyReportField, value: string) => { + if (!report) { + return; + } + + if (isReportFieldOfTypeTitle(reportField)) { + // A report always has a name, so clearing the title is discarded rather than saved. + if (value === '') { + return; + } + updateReportName(report.reportID, value, report.reportName ?? ''); + return; + } + + const fieldKey = getReportFieldKey(reportField.fieldID); + const hasViolations = hasViolationsReportUtils( + report.reportID, + transactionViolations, + session?.accountID ?? CONST.DEFAULT_NUMBER_ID, + session?.email ?? '', + undefined, + reportTransactions, + ); + const isFieldDisabled = isReportFieldDisabled(report, reportField, policy, rules); + const hasOtherViolations = Object.entries(report.fieldList ?? {}).some(([key, field]) => key !== fieldKey && field.value === '' && !isFieldDisabled); + + updateReportField({ + report: {...report, reportID: report.reportID}, + reportField: {...reportField, value}, + previousReportField: reportField, + policy, + isASAPSubmitBetaEnabled: isBetaEnabled(CONST.BETAS.ASAP_SUBMIT), + accountID: session?.accountID ?? CONST.DEFAULT_NUMBER_ID, + email: session?.email ?? '', + hasViolationsParam: hasViolations, + recentlyUsedReportFields, + shouldFixViolations: hasOtherViolations, + isTrackIntentUser, + rules, + }); + }; +} + +export default useSaveReportField; diff --git a/src/libs/ReportFieldOptionsListUtils.ts b/src/libs/ReportFieldOptionsListUtils.ts index 610c1bdfe700..912280000169 100644 --- a/src/libs/ReportFieldOptionsListUtils.ts +++ b/src/libs/ReportFieldOptionsListUtils.ts @@ -30,6 +30,8 @@ function getReportFieldOptionsSection({ initiallySelectedValue, searchValue, translate, + shouldPinSelectedOption = true, + shouldShowSectionTitles = true, }: { options: string[]; recentlyUsedOptions: string[]; @@ -37,12 +39,18 @@ function getReportFieldOptionsSection({ initiallySelectedValue: string; searchValue: string; translate: LocalizedTranslate; + + /** Whether the selected value is lifted into its own section at the top of the list */ + shouldPinSelectedOption?: boolean; + + /** Whether the "Recent" and "All" section titles are rendered */ + shouldShowSectionTitles?: boolean; }) { const reportFieldOptionsSections = []; // The live selection drives the checkmark (isSelected)... const selectedKeySet = new Set(selectedOptions.map(({text, keyForList, name}) => text ?? keyForList ?? name ?? '').filter((o) => !!o)); // ...while the frozen initial selection drives the pinned "Selected" section, so selecting a value marks it without reordering the list until reopen. - const pinnedOptionKeys = initiallySelectedValue ? [initiallySelectedValue] : []; + const pinnedOptionKeys = shouldPinSelectedOption && initiallySelectedValue ? [initiallySelectedValue] : []; const pinnedKeySet = new Set(pinnedOptionKeys); if (searchValue) { @@ -81,7 +89,7 @@ function getReportFieldOptionsSection({ if (filteredRecentlyUsedOptions.length > 0) { reportFieldOptionsSections.push({ // "Recent" section - title: translate('common.recent'), + title: shouldShowSectionTitles ? translate('common.recent') : '', sectionIndex: 2, data: getReportFieldOptions(filteredRecentlyUsedOptions, selectedKeySet), }); @@ -89,7 +97,7 @@ function getReportFieldOptionsSection({ reportFieldOptionsSections.push({ // "All" section when items amount more than the threshold - title: translate('common.all'), + title: shouldShowSectionTitles ? translate('common.all') : '', sectionIndex: 3, data: getReportFieldOptions(filteredOptions, selectedKeySet), }); diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index 11fb1b0ceedf..9bfcdc7510cb 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -4110,7 +4110,7 @@ function updateReportField({ report: Report; reportField: PolicyReportField; previousReportField: PolicyReportField; - policy: Policy; + policy: OnyxEntry; isASAPSubmitBetaEnabled: boolean; accountID: number; email: string; diff --git a/src/pages/DynamicEditReportFieldPage.tsx b/src/pages/DynamicEditReportFieldPage.tsx index 6a63a50e7a10..f24e212352c5 100644 --- a/src/pages/DynamicEditReportFieldPage.tsx +++ b/src/pages/DynamicEditReportFieldPage.tsx @@ -2,7 +2,6 @@ import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView import type {FormOnyxValues} from '@components/Form/types'; import HeaderWithBackButton from '@components/HeaderWithBackButton'; import {ModalActions} from '@components/Modal/Global/ModalContext'; -import {useSession} from '@components/OnyxListItemProvider'; import type {PopoverMenuItem} from '@components/PopoverMenu'; import ScreenWrapper from '@components/ScreenWrapper'; @@ -12,10 +11,10 @@ import useDynamicBackPath from '@hooks/useDynamicBackPath'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; import useLocalize from '@hooks/useLocalize'; import useOnyx from '@hooks/useOnyx'; -import usePermissions from '@hooks/usePermissions'; import {useDerivedReportNameByReportID} from '@hooks/useReportAttributes'; +import useSaveReportField from '@hooks/useSaveReportField'; -import {deleteReportField, updateReportField, updateReportName} from '@libs/actions/Report'; +import {deleteReportField} from '@libs/actions/Report'; import Navigation from '@libs/Navigation/Navigation'; import type {PlatformStackScreenProps} from '@libs/Navigation/PlatformStackNavigation/types'; import type {EditRequestNavigatorParamList} from '@libs/Navigation/types'; @@ -25,10 +24,8 @@ import { getReportFieldFromReportNameValuePairs, getReportFieldKey, getTitleFieldWithFallback, - hasViolations as hasViolationsReportUtils, isGroupPolicyExpenseReport, isInvoiceReport, - isReportFieldDisabled, isReportFieldDisabledForUser, isReportFieldOfTypeTitle, isReportFieldTargetMatchingReport, @@ -38,9 +35,7 @@ import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import {DYNAMIC_ROUTES} from '@src/ROUTES'; import type SCREENS from '@src/SCREENS'; -import type {Policy} from '@src/types/onyx'; -import {isTrackIntentUserSelector} from '@selectors/Onboarding'; import {Str} from 'expensify-common'; import React from 'react'; @@ -58,7 +53,7 @@ function DynamicEditReportFieldPage({route}: DynamicEditReportFieldPageProps) { const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`); const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`); const derivedReportName = useDerivedReportNameByReportID(reportID); - const [recentlyUsedReportFields] = useOnyx(ONYXKEYS.RECENTLY_USED_REPORT_FIELDS); + const saveReportField = useSaveReportField(report, policy); const isTitleField = route.params.fieldID === CONST.REPORT_FIELD_TITLE_FIELD_ID; const reportFieldFromNVP = getReportFieldFromReportNameValuePairs(reportNameValuePairs, fieldKey) ?? getReportFieldFromReportNameValuePairs(reportNameValuePairs, route.params.fieldID); @@ -76,20 +71,12 @@ function DynamicEditReportFieldPage({route}: DynamicEditReportFieldPageProps) { const {accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); const [rules] = useOnyx(ONYXKEYS.COLLECTION.RULE); const isDisabled = isReportFieldDisabledForUser(report, reportField, policy, currentUserAccountID, rules) && reportField?.type !== CONST.REPORT_FIELD_TYPES.FORMULA; - const {isBetaEnabled} = usePermissions(); - const isASAPSubmitBetaEnabled = isBetaEnabled(CONST.BETAS.ASAP_SUBMIT); - const session = useSession(); - const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS); - const [isTrackIntentUser] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED, {selector: isTrackIntentUserSelector}); - const hasViolations = hasViolationsReportUtils(report?.reportID, transactionViolations, session?.accountID ?? CONST.DEFAULT_NUMBER_ID, session?.email ?? ''); const {translate} = useLocalize(); const {showConfirmModal} = useConfirmModal(); const icons = useMemoizedLazyExpensifyIcons(['Trashcan']); const isReportFieldTitle = isReportFieldOfTypeTitle(reportField); const isReportFieldsFeatureEnabled = report?.type === CONST.REPORT.TYPE.INVOICE ? policy?.areInvoiceFieldsEnabled : policy?.areReportFieldsEnabled; const reportFieldsEnabled = ((isGroupPolicyExpenseReport(report, policy?.type) || isInvoiceReport(report)) && !!isReportFieldsFeatureEnabled) || isReportFieldTitle; - const hasOtherViolations = - report?.fieldList && Object.entries(report.fieldList).some(([key, field]) => key !== fieldKey && field.value === '' && !isReportFieldDisabled(report, reportField, policy, rules)); if (!reportFieldsEnabled || !reportField || !policyField || !report || !isReportFieldTargetMatchingReport(report, reportField) || isDisabled) { return ( @@ -140,28 +127,8 @@ function DynamicEditReportFieldPage({route}: DynamicEditReportFieldPageProps) { return; } - if (isReportFieldTitle) { - updateReportName(report.reportID, value, report.reportName ?? ''); - goBack(); - } else { - if (value !== '') { - updateReportField({ - report: {...report, reportID: report.reportID}, - reportField: {...reportField, value}, - previousReportField: reportField, - policy: policy as unknown as Policy, - isASAPSubmitBetaEnabled, - accountID: session?.accountID ?? CONST.DEFAULT_NUMBER_ID, - email: session?.email ?? '', - hasViolationsParam: hasViolations, - recentlyUsedReportFields, - shouldFixViolations: hasOtherViolations ?? false, - isTrackIntentUser, - rules, - }); - } - goBack(); - } + saveReportField(reportField, value); + goBack(); }; const menuItems: PopoverMenuItem[] = []; diff --git a/src/pages/EditReportFieldDropdown.tsx b/src/pages/EditReportFieldDropdown.tsx index 64af2b99cf86..3601e03ae27d 100644 --- a/src/pages/EditReportFieldDropdown.tsx +++ b/src/pages/EditReportFieldDropdown.tsx @@ -5,10 +5,13 @@ import useDebouncedState from '@hooks/useDebouncedState'; import useInitialSelection from '@hooks/useInitialSelection'; import useLocalize from '@hooks/useLocalize'; import useOnyx from '@hooks/useOnyx'; +import useThemeStyles from '@hooks/useThemeStyles'; import {getHeaderMessageForNonUserList} from '@libs/OptionsListUtils'; import {getReportFieldOptionsSection} from '@libs/ReportFieldOptionsListUtils'; +import variables from '@styles/variables'; + import ONYXKEYS from '@src/ONYXKEYS'; import React from 'react'; @@ -25,13 +28,39 @@ type EditReportFieldDropdownPageProps = { /** Callback to fire when the Save button is pressed */ onSubmit: (form: Record) => void; + + /** Whether the search input is shown. Callers with a short list hide it, since there is little to search through. */ + shouldShowTextInput?: boolean; + + /** Whether the "Recent" section is shown above the full option list */ + shouldShowRecentlyUsedOptions?: boolean; + + /** Whether the selected value is lifted to the top of the list. Callers with a short list leave it in place, so the order doesn't change as the value does. */ + shouldPinSelectedOption?: boolean; + + /** Whether the "Recent" and "All" section titles are rendered */ + shouldShowSectionTitles?: boolean; + + /** Whether options use the shorter 52px row the Spend dropdowns use, instead of the default 64px page row */ + shouldUseCompactRows?: boolean; }; -function EditReportFieldDropdown({onSubmit, fieldKey, fieldValue, fieldOptions}: EditReportFieldDropdownPageProps) { +function EditReportFieldDropdown({ + onSubmit, + fieldKey, + fieldValue, + fieldOptions, + shouldShowTextInput = true, + shouldShowRecentlyUsedOptions = true, + shouldPinSelectedOption = true, + shouldShowSectionTitles = true, + shouldUseCompactRows = false, +}: EditReportFieldDropdownPageProps) { + const styles = useThemeStyles(); const [recentlyUsedReportFields] = useOnyx(ONYXKEYS.RECENTLY_USED_REPORT_FIELDS); const [searchValue, debouncedSearchValue, setSearchValue] = useDebouncedState(''); const {translate, localeCompare} = useLocalize(); - const recentlyUsedOptions = recentlyUsedReportFields?.[fieldKey]?.sort(localeCompare) ?? []; + const recentlyUsedOptions = shouldShowRecentlyUsedOptions ? (recentlyUsedReportFields?.[fieldKey]?.sort(localeCompare) ?? []) : []; const validFieldOptions = fieldOptions?.filter((option) => !!option)?.sort(localeCompare); @@ -54,6 +83,8 @@ function EditReportFieldDropdown({onSubmit, fieldKey, fieldValue, fieldOptions}: options: validFieldOptions, recentlyUsedOptions, translate, + shouldPinSelectedOption, + shouldShowSectionTitles, }); const policyReportFieldData = sections.at(0)?.data ?? []; @@ -63,15 +94,24 @@ function EditReportFieldDropdown({onSubmit, fieldKey, fieldValue, fieldOptions}: label: translate('common.search'), onChangeText: setSearchValue, headerMessage: getHeaderMessageForNonUserList(policyReportFieldData.length > 0, debouncedSearchValue), + // Nothing is rendered above the search input here, so it needs the top padding the surrounding page or popover doesn't provide. + style: {containerStyle: styles.pt3}, }; return ( onSubmit({[fieldKey]: !option?.text || fieldValue === option.text ? '' : option.text})} + // Re-selecting the value the field already holds submits that same value rather than an empty string. + // Both call sites compare the submitted value against the stored one and skip the save themselves, so + // signalling "no change" this way is no longer needed and leaves '' free to mean "cleared". + onSelectRow={(option) => onSubmit({[fieldKey]: option?.text ?? ''})} initiallyFocusedItemKey={initialFieldValue} shouldUpdateFocusedIndex /> diff --git a/src/styles/index.ts b/src/styles/index.ts index e9ee9db2178d..20662cdcff89 100644 --- a/src/styles/index.ts +++ b/src/styles/index.ts @@ -3307,6 +3307,14 @@ const staticStyles = (theme: ThemeColors) => transform: [{rotate: '180deg'}], }, + // Use this instead of `flipUpsideDown` when the element being flipped has asymmetric horizontal padding. + // `rotate(180deg)` turns the whole box about its centre, so it mirrors that padding too and the content + // visibly slides sideways. Mirroring on the vertical axis leaves the box where it is, and looks identical + // for content that is already symmetric left-to-right (a caret, a chevron, a tooltip pointer). + flipUpsideDownInPlace: { + transform: [{scaleY: -1}], + }, + navigationScreenCardStyle: { height: '100%', }, diff --git a/tests/ui/EditReportFieldDropdownTest.tsx b/tests/ui/EditReportFieldDropdownTest.tsx new file mode 100644 index 000000000000..0f74164fdb6e --- /dev/null +++ b/tests/ui/EditReportFieldDropdownTest.tsx @@ -0,0 +1,109 @@ +import {act, fireEvent, render, screen} from '@testing-library/react-native'; + +import ComposeProviders from '@components/ComposeProviders'; +import OnyxListItemProvider from '@components/OnyxListItemProvider'; + +import EditReportFieldDropdown from '@pages/EditReportFieldDropdown'; + +import ONYXKEYS from '@src/ONYXKEYS'; + +import type * as NativeNavigation from '@react-navigation/native'; + +import React from 'react'; +import Onyx from 'react-native-onyx'; + +import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; + +jest.mock('@react-navigation/native', () => ({ + ...((): typeof NativeNavigation => jest.requireActual('@react-navigation/native'))(), + useNavigation: jest.fn(() => ({navigate: jest.fn(), addListener: jest.fn(() => jest.fn())})), + useIsFocused: jest.fn(() => true), + useRoute: jest.fn(() => ({key: '', name: ''})), + // The dropdown is rendered outside a navigator here, so the focus effect that resets the pinned selection is a noop. + useFocusEffect: jest.fn(), +})); + +const fieldKey = 'expensify_field_dropdown'; +const fieldOptions = ['Alpha', 'Bravo', 'Charlie']; + +const renderDropdown = async (props: Partial> = {}) => { + const rendered = render( + + + , + ); + await waitForBatchedUpdatesWithAct(); + return rendered; +}; + +describe('EditReportFieldDropdown', () => { + beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); + }); + + beforeEach(async () => { + await act(async () => { + // "Bravo" was used recently, so the "Recent" section has something to render when it is enabled. + await Onyx.merge(ONYXKEYS.RECENTLY_USED_REPORT_FIELDS, {[fieldKey]: ['Bravo']}); + }); + await waitForBatchedUpdatesWithAct(); + }); + + afterEach(async () => { + await act(async () => { + await Onyx.clear(); + }); + }); + + it('shows the search input by default', async () => { + await renderDropdown(); + + expect(screen.getByTestId('selection-list-text-input')).toBeOnTheScreen(); + }); + + it('hides the search input when the caller opts out', async () => { + await renderDropdown({shouldShowTextInput: false}); + + expect(screen.queryByTestId('selection-list-text-input')).not.toBeOnTheScreen(); + }); + + it('lists a recently used option twice by default, once in the "Recent" section and once in the full list', async () => { + await renderDropdown(); + + expect(screen.getAllByTestId('base-list-item-Bravo')).toHaveLength(2); + }); + + it('lists a recently used option only once when the caller opts out of the "Recent" section', async () => { + await renderDropdown({shouldShowRecentlyUsedOptions: false}); + + expect(screen.getAllByTestId('base-list-item-Bravo')).toHaveLength(1); + }); + + it('submits the option that was picked', async () => { + const onSubmit = jest.fn(); + await renderDropdown({onSubmit}); + + fireEvent.press(screen.getByTestId('base-list-item-Charlie')); + await waitForBatchedUpdatesWithAct(); + + expect(onSubmit).toHaveBeenCalledWith({[fieldKey]: 'Charlie'}); + }); + + it('submits the current value rather than an empty string when the option already selected is picked again', async () => { + const onSubmit = jest.fn(); + await renderDropdown({onSubmit}); + + fireEvent.press(screen.getByTestId('base-list-item-Alpha')); + await waitForBatchedUpdatesWithAct(); + + // Callers skip a save that would not change anything by comparing this against the stored value, so an empty + // string here would be read as "the user cleared the field" instead. + expect(onSubmit).toHaveBeenCalledWith({[fieldKey]: 'Alpha'}); + }); +}); diff --git a/tests/ui/MoneyReportViewTest.tsx b/tests/ui/MoneyReportViewTest.tsx index 5d525c985783..eb7b0a874efd 100644 --- a/tests/ui/MoneyReportViewTest.tsx +++ b/tests/ui/MoneyReportViewTest.tsx @@ -19,6 +19,20 @@ import * as LHNTestUtils from '../utils/LHNTestUtils'; import * as TestHelper from '../utils/TestHelper'; import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; +// Pinned to a wide layout so the column-count assertion below exercises the `shouldUseSingleColumn` prop rather than +// the narrow-layout fallback, which would collapse the fields to one per row on its own. +jest.mock('@hooks/useResponsiveLayoutOnWideRHP', () => ({ + __esModule: true, + default: () => ({ + shouldUseNarrowLayout: false, + shouldUseNarrowLayoutIgnoringWideRHP: false, + isSmallScreenWidth: false, + isInNarrowPaneModal: false, + isWideRHPDisplayedOnWideLayout: true, + isSuperWideRHPDisplayedOnWideLayout: false, + }), +})); + jest.mock('@hooks/useLocalize', () => jest.fn(() => ({ translate: jest.fn((key: string) => key), @@ -357,9 +371,34 @@ describe('MoneyReportView report fields visibility', () => { renderMoneyReportView(approvedReport, policy, true); await waitForBatchedUpdatesWithAct(); - // The custom field (rendered read-only after approval) must still show for the submitter. + // The custom field (rendered read-only after approval) must still show for the submitter. It renders as an + // inline input now rather than a row that opens the report field editor, so it's found by its label. + await waitFor(() => { + expect(screen.getByLabelText('Test')).toBeOnTheScreen(); + }); + }); + + it('stacks the report fields one per row on a wide layout, because this is the one-expense report view', async () => { + const customFields = Array.from({length: 4}, (_unused, index) => ({ + ...buildCustomTextField(), + fieldID: `field_test${index}`, + name: `Test${index}`, + orderWeight: index + 2, + })); + const fieldList: Record = {[CONST.REPORT_FIELD_TITLE_FIELD_ID]: buildTitleField()}; + for (const field of customFields) { + fieldList[`expensify_${field.fieldID}`] = field; + } + const policy = buildReportFieldsPolicy(fieldList); + const report = buildExpenseReport({fieldList}); + await seedReportFieldsPolicy(policy, report); + + renderMoneyReportView(report, policy); + await waitForBatchedUpdatesWithAct(); + + // Four fields would fill two rows of three if this view used the grid, so four rows proves one per row. await waitFor(() => { - expect(screen.getByText('Test')).toBeOnTheScreen(); + expect(screen.getAllByTestId('reportFieldsRow')).toHaveLength(4); }); }); diff --git a/tests/ui/MoneyRequestViewReportFieldsTest.tsx b/tests/ui/MoneyRequestViewReportFieldsTest.tsx new file mode 100644 index 000000000000..7a70deb1d2e1 --- /dev/null +++ b/tests/ui/MoneyRequestViewReportFieldsTest.tsx @@ -0,0 +1,374 @@ +import {act, fireEvent, render, screen} from '@testing-library/react-native'; + +import ComposeProviders from '@components/ComposeProviders'; +import MoneyRequestViewReportFields from '@components/MoneyRequestReportView/MoneyRequestViewReportFields'; +import OnyxListItemProvider from '@components/OnyxListItemProvider'; +import PopoverWithMeasuredContent from '@components/PopoverWithMeasuredContent'; + +import {updateReportField} from '@libs/actions/Report'; + +import initOnyxDerivedValues from '@userActions/OnyxDerived'; + +import CONST from '@src/CONST'; +import IntlStore from '@src/languages/IntlStore'; +import ONYXKEYS from '@src/ONYXKEYS'; +import type * as OnyxTypes from '@src/types/onyx'; + +import type * as NativeNavigation from '@react-navigation/native'; + +import React from 'react'; +import {StyleSheet} from 'react-native'; +import Onyx from 'react-native-onyx'; + +import * as LHNTestUtils from '../utils/LHNTestUtils'; +import * as TestHelper from '../utils/TestHelper'; +import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; + +let mockShouldUseNarrowLayout = false; + +jest.mock('@hooks/useResponsiveLayoutOnWideRHP', () => ({ + __esModule: true, + default: () => ({ + shouldUseNarrowLayout: mockShouldUseNarrowLayout, + shouldUseNarrowLayoutIgnoringWideRHP: mockShouldUseNarrowLayout, + isSmallScreenWidth: mockShouldUseNarrowLayout, + isInNarrowPaneModal: false, + isWideRHPDisplayedOnWideLayout: !mockShouldUseNarrowLayout, + isSuperWideRHPDisplayedOnWideLayout: false, + }), +})); + +jest.mock('@hooks/useLocalize', () => + jest.fn(() => ({ + translate: jest.fn((key: string) => key), + numberFormat: jest.fn((num: number) => num.toString()), + toLocaleDigit: jest.fn((digit: string) => digit), + localeCompare: jest.fn((first: string, second: string) => first.localeCompare(second)), + })), +); + +jest.mock('@react-navigation/native', () => ({ + ...((): typeof NativeNavigation => jest.requireActual('@react-navigation/native'))(), + useNavigation: jest.fn(() => ({navigate: jest.fn(), addListener: jest.fn(() => jest.fn())})), + useIsFocused: jest.fn(() => true), + useRoute: jest.fn(() => ({key: '', name: '', params: {reportID: '1'}})), + // The option list is rendered outside a navigator here, so there is no focus cycle to refresh its snapshot on. + useFocusEffect: jest.fn(), +})); + +// The list field's dropdown is built on FilterPopupButton, which reads focus straight from @react-navigation/core. +jest.mock('@react-navigation/core', () => ({ + ...jest.requireActual>('@react-navigation/core'), + useIsFocused: jest.fn(() => true), +})); + +jest.mock('@libs/actions/Report', () => ({ + ...jest.requireActual>('@libs/actions/Report'), + updateReportField: jest.fn(), +})); + +TestHelper.setupGlobalFetchMock(); + +const policyID = 'policy_report_fields_grid'; +const reportID = 'report_report_fields_grid'; +const accountID = 1; + +const buildTextField = (index: number): OnyxTypes.PolicyReportField => ({ + name: `Field${index}`, + fieldID: `field${index}`, + defaultValue: '', + value: `Value${index}`, + orderWeight: index, + type: CONST.REPORT_FIELD_TYPES.TEXT, + deletable: true, + values: [], + keys: [], + externalIDs: [], + disabledOptions: [], + target: CONST.REPORT_FIELD_TARGETS.EXPENSE, + isTax: false, +}); + +// An empty value is what `getFieldViolation` turns into a " is required" violation. +const buildEmptyField = (): OnyxTypes.PolicyReportField => ({ + ...buildTextField(1), + name: 'EmptyField', + fieldID: 'emptyField', + value: '', +}); + +// A field that cannot be deleted is the one field that must always hold a value, so clearing it is rejected. +const buildRequiredField = (): OnyxTypes.PolicyReportField => ({ + ...buildTextField(1), + name: 'RequiredField', + fieldID: 'requiredField', + deletable: false, +}); + +const buildListField = (): OnyxTypes.PolicyReportField => ({ + ...buildTextField(1), + name: 'ListField', + fieldID: 'listField', + type: CONST.REPORT_FIELD_TYPES.LIST, + value: 'Option1', + values: ['Option1', 'Option2'], + disabledOptions: [false, false], +}); + +/** Every flattened style currently on screen, so a test can assert that some node does (or does not) carry one. */ +const getRenderedStyles = (): unknown[] => { + const renderedStyles: unknown[] = []; + for (const node of screen.UNSAFE_root.findAll(() => true)) { + renderedStyles.push(StyleSheet.flatten(node.props.style)); + } + return renderedStyles; +}; + +// `getTextInputIconContainerStyles` gives every icon container a full-height centred box, and `pr0` then strips the +// right half of its padding. That one-sided padding identifies the caret container. +const caretContainer = {height: '100%', paddingHorizontal: 11, paddingRight: 0}; + +const buildFieldList = (fieldCount: number): Record => { + const fieldList: Record = {}; + for (let index = 1; index <= fieldCount; index++) { + const field = buildTextField(index); + fieldList[`expensify_${field.fieldID}`] = field; + } + return fieldList; +}; + +const buildPolicy = (fieldCount: number, extraFields: OnyxTypes.PolicyReportField[] = []): OnyxTypes.Policy => { + const fieldList = buildFieldList(fieldCount); + for (const field of extraFields) { + fieldList[`expensify_${field.fieldID}`] = field; + } + + return { + ...LHNTestUtils.getFakePolicy(policyID, 'Policy'), + type: CONST.POLICY.TYPE.TEAM, + role: CONST.POLICY.ROLE.ADMIN, + outputCurrency: CONST.CURRENCY.USD, + areReportFieldsEnabled: true, + fieldList, + }; +}; + +const buildReport = (): OnyxTypes.Report => ({ + ...LHNTestUtils.getFakeReport([accountID, 2]), + reportID, + type: CONST.REPORT.TYPE.EXPENSE, + policyID, + ownerAccountID: accountID, + currency: CONST.CURRENCY.USD, + stateNum: CONST.REPORT.STATE_NUM.OPEN, + statusNum: CONST.REPORT.STATUS_NUM.OPEN, + total: 0, +}); + +const renderReportFields = async (fieldCount: number, extraFields: OnyxTypes.PolicyReportField[] = [], shouldUseSingleColumn = false) => { + const policy = buildPolicy(fieldCount, extraFields); + const report = buildReport(); + + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, policy); + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, report); + }); + await waitForBatchedUpdatesWithAct(); + + const rendered = render( + + + , + ); + await waitForBatchedUpdatesWithAct(); + + return rendered; +}; + +describe('MoneyRequestViewReportFields', () => { + beforeAll(async () => { + Onyx.init({keys: ONYXKEYS, evictableKeys: [ONYXKEYS.COLLECTION.REPORT_ACTIONS]}); + initOnyxDerivedValues(); + // Field violation copy comes from `translateLocal`, not the mocked `useLocalize`, so the locale has to be real. + IntlStore.load('en'); + await waitForBatchedUpdatesWithAct(); + }); + + beforeEach(async () => { + mockShouldUseNarrowLayout = false; + jest.clearAllMocks(); + await act(async () => { + await TestHelper.signInWithTestUser(accountID); + }); + await waitForBatchedUpdatesWithAct(); + }); + + afterEach(async () => { + await act(async () => { + await Onyx.clear(); + }); + }); + + it('shows up to three fields per row on a wide layout', async () => { + await renderReportFields(7); + + // 7 fields fill two rows of three and one row holding the remaining field + expect(screen.getAllByTestId('reportFieldsRow')).toHaveLength(3); + }); + + it('shows one field per row on a narrow layout', async () => { + mockShouldUseNarrowLayout = true; + await renderReportFields(4); + + expect(screen.getAllByTestId('reportFieldsRow')).toHaveLength(4); + }); + + it('shows one field per row on a wide layout when a single column is requested', async () => { + // A one-expense report passes `shouldUseSingleColumn`, so its fields stack even though the layout is wide + await renderReportFields(4, [], true); + + expect(screen.getAllByTestId('reportFieldsRow')).toHaveLength(4); + }); + + it('renders every field as an input holding its value', async () => { + await renderReportFields(2); + + expect(screen.getByLabelText('Field1')).toHaveProp('value', 'Value1'); + expect(screen.getByLabelText('Field2')).toHaveProp('value', 'Value2'); + }); + + it('saves a value typed into a field without leaving the report', async () => { + await renderReportFields(2); + + const input = screen.getByLabelText('Field1'); + fireEvent.changeText(input, 'Updated value'); + fireEvent(input, 'blur'); + await waitForBatchedUpdatesWithAct(); + + expect(updateReportField).toHaveBeenCalledTimes(1); + expect(jest.mocked(updateReportField).mock.calls.at(0)?.at(0)).toMatchObject({ + reportField: {fieldID: 'field1', value: 'Updated value'}, + previousReportField: {fieldID: 'field1', value: 'Value1'}, + }); + }); + + it('saves the empty value when an optional field is cleared', async () => { + await renderReportFields(2); + + const input = screen.getByLabelText('Field1'); + fireEvent.changeText(input, ''); + fireEvent(input, 'blur'); + await waitForBatchedUpdatesWithAct(); + + expect(updateReportField).toHaveBeenCalledTimes(1); + expect(jest.mocked(updateReportField).mock.calls.at(0)?.at(0)).toMatchObject({ + reportField: {fieldID: 'field1', value: ''}, + previousReportField: {fieldID: 'field1', value: 'Value1'}, + }); + }); + + it('rejects clearing a required field instead of saving it', async () => { + await renderReportFields(1, [buildRequiredField()]); + + const input = screen.getByLabelText('RequiredField'); + fireEvent.changeText(input, ''); + fireEvent(input, 'blur'); + await waitForBatchedUpdatesWithAct(); + + expect(updateReportField).not.toHaveBeenCalled(); + expect(screen.getByText('common.error.fieldRequired')).toBeOnTheScreen(); + }); + + it('does not save when the option a list field already holds is picked again', async () => { + await renderReportFields(1, [buildListField()]); + + fireEvent.press(screen.getByLabelText('ListField')); + await waitForBatchedUpdatesWithAct(); + + fireEvent.press(screen.getByTestId('base-list-item-Option1')); + await waitForBatchedUpdatesWithAct(); + + expect(updateReportField).not.toHaveBeenCalled(); + }); + + it('does not save when the value is left unchanged', async () => { + await renderReportFields(2); + + const input = screen.getByLabelText('Field1'); + fireEvent(input, 'blur'); + await waitForBatchedUpdatesWithAct(); + + expect(updateReportField).not.toHaveBeenCalled(); + }); + + it('keeps the fields of a row aligned at the top so an error grows the row downwards', async () => { + await renderReportFields(3); + + expect(screen.getAllByTestId('reportFieldsRow').at(0)).toHaveStyle({alignItems: 'flex-start'}); + }); + + it('holds back a field violation until the field has been left', async () => { + await renderReportFields(1, [buildEmptyField()]); + + expect(screen.queryByText('EmptyField is required')).toBeNull(); + + fireEvent(screen.getByLabelText('EmptyField'), 'blur'); + await waitForBatchedUpdatesWithAct(); + + expect(screen.getByText('EmptyField is required')).toBeOnTheScreen(); + }); + + it('renders a list field as a collapsed combobox rather than a row that opens a page', async () => { + await renderReportFields(1, [buildListField()]); + + const listInput = screen.getByLabelText('ListField'); + expect(listInput).toHaveProp('role', CONST.ROLE.COMBOBOX); + expect(listInput).toHaveProp('accessibilityState', {expanded: false}); + expect(listInput).toHaveProp('value', 'Option1'); + + // The dropdown content is deferred until the field is first opened, so no option row is mounted up front. + expect(screen.queryByText('Option2')).toBeNull(); + }); + + it('opens the option list in place when a list field is pressed', async () => { + await renderReportFields(1, [buildListField()]); + + fireEvent.press(screen.getByLabelText('ListField')); + await waitForBatchedUpdatesWithAct(); + + expect(screen.getByLabelText('ListField')).toHaveProp('accessibilityState', {expanded: true}); + expect(screen.getByText('Option2')).toBeOnTheScreen(); + }); + + it('keeps the caret in the same place when the option list opens', async () => { + await renderReportFields(1, [buildListField()]); + + expect(getRenderedStyles()).toEqual(expect.arrayContaining([expect.objectContaining(caretContainer)])); + + fireEvent.press(screen.getByLabelText('ListField')); + await waitForBatchedUpdatesWithAct(); + + // The caret container is padded on one side only, so `rotate(180deg)` would mirror that padding along with + // the caret and slide it 11px left the moment the list opens. Mirroring on the vertical axis turns the caret + // over while leaving its box — and so the caret's distance from the field's right edge — untouched. + const expandedStyles = getRenderedStyles(); + expect(expandedStyles).toEqual(expect.arrayContaining([expect.objectContaining({...caretContainer, transform: [{scaleY: -1}]})])); + expect(expandedStyles).not.toEqual(expect.arrayContaining([expect.objectContaining({transform: [{rotate: '180deg'}]})])); + }); + + it('sizes the option popover like the Spend dropdowns instead of matching the field width', async () => { + await renderReportFields(1, [buildListField()]); + + fireEvent.press(screen.getByLabelText('ListField')); + await waitForBatchedUpdatesWithAct(); + + // A popover as narrow as the field it is anchored to looked cramped, so it uses the width every Spend + // filter dropdown uses. + const popover = screen.UNSAFE_getByType(PopoverWithMeasuredContent); + expect(popover.props.popoverDimensions).toEqual(expect.objectContaining({width: CONST.POPOVER_DROPDOWN_WIDTH})); + }); +}); diff --git a/tests/unit/ReportFieldOptionsListUtilsTest.ts b/tests/unit/ReportFieldOptionsListUtilsTest.ts index 04ad47cc42cf..ad2a4b28d8d9 100644 --- a/tests/unit/ReportFieldOptionsListUtilsTest.ts +++ b/tests/unit/ReportFieldOptionsListUtilsTest.ts @@ -73,6 +73,37 @@ describe('ReportFieldOptionsListUtils', () => { expect(searchData).toHaveLength(3); }); + it('leaves the selected value in place when pinning is turned off', () => { + const sections = getReportFieldOptionsSection({ + options: OPTIONS, + recentlyUsedOptions: [], + selectedOptions: [{text: 'Charlie'}], + initiallySelectedValue: 'Charlie', + searchValue: '', + translate: translateLocal, + shouldPinSelectedOption: false, + }); + + // No "Selected" section, and the value keeps its natural position in the full list while staying checked. + expect(sectionByIndex(sections, 1)).toBeUndefined(); + expect(sectionByIndex(sections, 3)?.data.map((option) => option.keyForList)).toEqual(OPTIONS); + expect(sectionByIndex(sections, 3)?.data.find((option) => option.keyForList === 'Charlie')?.isSelected).toBe(true); + }); + + it('drops the section titles when they are turned off', () => { + const sections = getReportFieldOptionsSection({ + options: OPTIONS, + recentlyUsedOptions: ['Delta'], + selectedOptions: [], + initiallySelectedValue: '', + searchValue: '', + translate: translateLocal, + shouldShowSectionTitles: false, + }); + + expect(sections.map((section) => section.title)).toEqual(['', '']); + }); + it('does not force-show the pinned value when it does not match the search', () => { const sections = getReportFieldOptionsSection({ options: ['Rate A', 'Rate B', 'Zulu'], From bb69d06e6e1482561e711fe6f912363f0a9a2817 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Thu, 24 Sep 2026 11:33:04 +0000 Subject: [PATCH 02/16] Fix: hide the caret on the list report field inline input The list field is rendered as one of our regular inputs so it matches the text and date fields sharing its row, but its value is picked from a dropdown rather than typed. disableKeyboard only suppresses the software keyboard, so on web BaseTextInput's own onPress still focused the underlying input and flashed a caret before the popover opened. caretHidden keeps the field focusable and keyboard reachable while hiding the caret, matching how SearchAutocompleteInput and the waypoint page already use it. Co-authored-by: daledah --- .../MoneyRequestReportView/ReportFieldInlineInput.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx index 2449795d36cb..46d35fbe1c49 100644 --- a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx +++ b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx @@ -217,6 +217,10 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error onBlur={() => setHasBeenBlurred(true)} onSubmitEditing={onPress} disableKeyboard + // The field is focused so it stays keyboard reachable, but it holds a value picked from a + // list rather than typed text, so the caret would be a caret you cannot type into. + // `disableKeyboard` only suppresses the software keyboard, it does not hide the caret. + caretHidden /> )} From 32eee41abf0e257d0dd11dc1e34a8b5f8cdd95c6 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Thu, 24 Sep 2026 11:36:57 +0000 Subject: [PATCH 03/16] Add a test locking in the hidden caret on the list report field Co-authored-by: daledah --- tests/ui/MoneyRequestViewReportFieldsTest.tsx | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/ui/MoneyRequestViewReportFieldsTest.tsx b/tests/ui/MoneyRequestViewReportFieldsTest.tsx index 7a70deb1d2e1..52d2c7b897c1 100644 --- a/tests/ui/MoneyRequestViewReportFieldsTest.tsx +++ b/tests/ui/MoneyRequestViewReportFieldsTest.tsx @@ -334,6 +334,20 @@ describe('MoneyRequestViewReportFields', () => { expect(screen.queryByText('Option2')).toBeNull(); }); + it('hides the text cursor on a list field but keeps it on a text field', async () => { + // Given a report holding both a text field and a list field + await renderReportFields(1, [buildListField()]); + + // When the two inputs are compared + // Then the list field hides the cursor, because its value is picked from the dropdown rather than typed, and + // `disableKeyboard` alone only suppresses the software keyboard and would still leave a cursor blinking in a + // field the user cannot type into. + expect(screen.getByLabelText('ListField')).toHaveProp('caretHidden', true); + + // Then the text field is untouched and still shows its cursor. + expect(screen.getByLabelText('Field1')).not.toHaveProp('caretHidden', true); + }); + it('opens the option list in place when a list field is pressed', async () => { await renderReportFields(1, [buildListField()]); From c6d94882f6248f537b2ef1b5171e7c367b3751d7 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Thu, 24 Sep 2026 11:51:42 +0000 Subject: [PATCH 04/16] Reset the inline field display when a whitespace-only edit is skipped Document every added test case with Given/When/Then per tests/README.md. Co-authored-by: daledah --- .../ReportFieldInlineInput.tsx | 4 + tests/ui/EditReportFieldDropdownTest.tsx | 26 +++++- tests/ui/MoneyReportViewTest.tsx | 14 ++- tests/ui/MoneyRequestViewReportFieldsTest.tsx | 93 +++++++++++++++++-- tests/unit/ReportFieldOptionsListUtilsTest.ts | 44 +++++++-- 5 files changed, 161 insertions(+), 20 deletions(-) diff --git a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx index 46d35fbe1c49..88d7079c50ac 100644 --- a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx +++ b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx @@ -91,6 +91,10 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error if (trimmedValue === lastSavedValue.trim()) { setValidationError(''); + // Nothing is sent, so the saved value never changes and the resync above that follows it never runs. + // Without this the field would keep displaying the whitespace the user left behind until it remounted, + // at which point the text would visibly snap back to the value that was really stored. + setDraftValue(lastSavedValue); return; } diff --git a/tests/ui/EditReportFieldDropdownTest.tsx b/tests/ui/EditReportFieldDropdownTest.tsx index 0f74164fdb6e..9bc104cc7955 100644 --- a/tests/ui/EditReportFieldDropdownTest.tsx +++ b/tests/ui/EditReportFieldDropdownTest.tsx @@ -62,48 +62,70 @@ describe('EditReportFieldDropdown', () => { }); it('shows the search input by default', async () => { + // Given a dropdown rendered without any opt-out await renderDropdown(); + // When the list is displayed + // Then the search input is there, because the editor page has always been searchable and the new flags must + // not quietly change what an existing caller gets expect(screen.getByTestId('selection-list-text-input')).toBeOnTheScreen(); }); it('hides the search input when the caller opts out', async () => { + // Given a caller that opts out of the search input, which the inline field does for a short option list await renderDropdown({shouldShowTextInput: false}); + // When the list is displayed + // Then the search input is gone, so the popover does not reserve height for a box that is quicker to scan + // past than to type into expect(screen.queryByTestId('selection-list-text-input')).not.toBeOnTheScreen(); }); it('lists a recently used option twice by default, once in the "Recent" section and once in the full list', async () => { + // Given "Bravo" recorded as recently used and a dropdown rendered without any opt-out await renderDropdown(); + // When the list is displayed + // Then "Bravo" appears in both the "Recent" section and the full list, which is the editor page's existing + // behavior and must survive the flags being added expect(screen.getAllByTestId('base-list-item-Bravo')).toHaveLength(2); }); it('lists a recently used option only once when the caller opts out of the "Recent" section', async () => { + // Given "Bravo" recorded as recently used and a caller that opts out of the "Recent" section await renderDropdown({shouldShowRecentlyUsedOptions: false}); + // When the list is displayed + // Then "Bravo" appears once. A short list shown in a small popover is worse for having the same few options + // repeated at the top than it is for making the user read all of them expect(screen.getAllByTestId('base-list-item-Bravo')).toHaveLength(1); }); it('submits the option that was picked', async () => { + // Given a dropdown whose field currently holds "Alpha" const onSubmit = jest.fn(); await renderDropdown({onSubmit}); + // When a different option is pressed fireEvent.press(screen.getByTestId('base-list-item-Charlie')); await waitForBatchedUpdatesWithAct(); + // Then it is submitted under the field's own key, which is what both the editor page and the inline field + // rely on to know which field the value belongs to expect(onSubmit).toHaveBeenCalledWith({[fieldKey]: 'Charlie'}); }); it('submits the current value rather than an empty string when the option already selected is picked again', async () => { + // Given a dropdown whose field currently holds "Alpha" const onSubmit = jest.fn(); await renderDropdown({onSubmit}); + // When that same option is pressed again fireEvent.press(screen.getByTestId('base-list-item-Alpha')); await waitForBatchedUpdatesWithAct(); - // Callers skip a save that would not change anything by comparing this against the stored value, so an empty - // string here would be read as "the user cleared the field" instead. + // Then the current value is submitted, not an empty string. Callers skip a no-op save by comparing this + // against the stored value, so an empty string would be read as the user having cleared the field expect(onSubmit).toHaveBeenCalledWith({[fieldKey]: 'Alpha'}); }); }); diff --git a/tests/ui/MoneyReportViewTest.tsx b/tests/ui/MoneyReportViewTest.tsx index eb7b0a874efd..a083c77c75fa 100644 --- a/tests/ui/MoneyReportViewTest.tsx +++ b/tests/ui/MoneyReportViewTest.tsx @@ -356,6 +356,8 @@ describe('MoneyReportView report fields visibility', () => { }); it('keeps a custom report field visible for a non-admin submitter after the report is approved (single-expense combined view)', async () => { + // Given an approved report with a custom field, viewed by its non-admin submitter, who cannot edit a field + // once the report has been approved const fieldList = { [CONST.REPORT_FIELD_TITLE_FIELD_ID]: buildTitleField(), [customFieldKey]: buildCustomTextField(), @@ -368,17 +370,20 @@ describe('MoneyReportView report fields visibility', () => { }); await seedReportFieldsPolicy(policy, approvedReport); + // When the combined view is rendered renderMoneyReportView(approvedReport, policy, true); await waitForBatchedUpdatesWithAct(); - // The custom field (rendered read-only after approval) must still show for the submitter. It renders as an - // inline input now rather than a row that opens the report field editor, so it's found by its label. + // Then the field is still on the screen, because a submitter who can no longer change a value still needs to + // read it. It is looked up by its label since it is a read-only inline input now rather than a row that opens + // the report field editor await waitFor(() => { expect(screen.getByLabelText('Test')).toBeOnTheScreen(); }); }); it('stacks the report fields one per row on a wide layout, because this is the one-expense report view', async () => { + // Given a one-expense report with 4 custom fields on a wide layout const customFields = Array.from({length: 4}, (_unused, index) => ({ ...buildCustomTextField(), fieldID: `field_test${index}`, @@ -393,10 +398,13 @@ describe('MoneyReportView report fields visibility', () => { const report = buildExpenseReport({fieldList}); await seedReportFieldsPolicy(policy, report); + // When this view renders them renderMoneyReportView(report, policy); await waitForBatchedUpdatesWithAct(); - // Four fields would fill two rows of three if this view used the grid, so four rows proves one per row. + // Then each field gets its own row. Four fields would fill two rows of three if the grid were in use, so four + // rows proves this view asks for a single column: it renders inside a container too narrow for three fields + // side by side even though the window itself is wide await waitFor(() => { expect(screen.getAllByTestId('reportFieldsRow')).toHaveLength(4); }); diff --git a/tests/ui/MoneyRequestViewReportFieldsTest.tsx b/tests/ui/MoneyRequestViewReportFieldsTest.tsx index 52d2c7b897c1..5cb124a41d9f 100644 --- a/tests/ui/MoneyRequestViewReportFieldsTest.tsx +++ b/tests/ui/MoneyRequestViewReportFieldsTest.tsx @@ -214,41 +214,61 @@ describe('MoneyRequestViewReportFields', () => { }); it('shows up to three fields per row on a wide layout', async () => { + // Given a report with 7 fields, a count that does not divide evenly into rows of three, because a partly + // filled last row is the case that would regress if the chunking were off by one await renderReportFields(7); - // 7 fields fill two rows of three and one row holding the remaining field + // When the fields are laid out on a wide layout + // Then they occupy 3 rows, two full rows of three plus one holding the remaining field, which is what packing + // the fields three to a row rather than stacking them produces expect(screen.getAllByTestId('reportFieldsRow')).toHaveLength(3); }); it('shows one field per row on a narrow layout', async () => { + // Given a narrow layout, where three fields side by side would each be too cramped to read mockShouldUseNarrowLayout = true; + + // When 4 fields are laid out await renderReportFields(4); + // Then each one gets its own row, so the column count drops to 1 rather than staying at 3 expect(screen.getAllByTestId('reportFieldsRow')).toHaveLength(4); }); it('shows one field per row on a wide layout when a single column is requested', async () => { - // A one-expense report passes `shouldUseSingleColumn`, so its fields stack even though the layout is wide + // Given a wide layout and a caller asking for a single column, which is how the one-expense report view + // renders its fields inside a container too narrow for the grid + // When 4 fields are laid out await renderReportFields(4, [], true); + // Then each one gets its own row, proving `shouldUseSingleColumn` overrides the wide layout rather than only + // taking effect when the layout is already narrow expect(screen.getAllByTestId('reportFieldsRow')).toHaveLength(4); }); it('renders every field as an input holding its value', async () => { + // Given a report with 2 fields that already hold values await renderReportFields(2); + // When each field is looked up by its label + // Then it is an input carrying the saved value, because the fields are edited in place now instead of being + // rows that show the value and navigate to the editor page to change it expect(screen.getByLabelText('Field1')).toHaveProp('value', 'Value1'); expect(screen.getByLabelText('Field2')).toHaveProp('value', 'Value2'); }); it('saves a value typed into a field without leaving the report', async () => { + // Given a report with 2 fields await renderReportFields(2); + // When a new value is typed into the first field and the field is left const input = screen.getByLabelText('Field1'); fireEvent.changeText(input, 'Updated value'); fireEvent(input, 'blur'); await waitForBatchedUpdatesWithAct(); + // Then the value is saved from the report itself, with the old value passed along so the optimistic update can + // be rolled back. This is the whole point of inline editing: no navigation to the editor page to change a value expect(updateReportField).toHaveBeenCalledTimes(1); expect(jest.mocked(updateReportField).mock.calls.at(0)?.at(0)).toMatchObject({ reportField: {fieldID: 'field1', value: 'Updated value'}, @@ -257,13 +277,17 @@ describe('MoneyRequestViewReportFields', () => { }); it('saves the empty value when an optional field is cleared', async () => { + // Given a report with 2 optional fields that already hold values await renderReportFields(2); + // When the first field is emptied and left const input = screen.getByLabelText('Field1'); fireEvent.changeText(input, ''); fireEvent(input, 'blur'); await waitForBatchedUpdatesWithAct(); + // Then the empty value is saved, because deliberately clearing an optional field is a real edit and must not + // be mistaken for the no-op case where the user tabbed through without changing anything expect(updateReportField).toHaveBeenCalledTimes(1); expect(jest.mocked(updateReportField).mock.calls.at(0)?.at(0)).toMatchObject({ reportField: {fieldID: 'field1', value: ''}, @@ -272,65 +296,110 @@ describe('MoneyRequestViewReportFields', () => { }); it('rejects clearing a required field instead of saving it', async () => { + // Given a report holding a required field, which the editor page used to guard with its own form validation await renderReportFields(1, [buildRequiredField()]); + // When the field is emptied and left const input = screen.getByLabelText('RequiredField'); fireEvent.changeText(input, ''); fireEvent(input, 'blur'); await waitForBatchedUpdatesWithAct(); + // Then nothing is saved and the error is shown instead, so moving the editing inline did not drop the + // validation that the editor page used to apply before it would submit expect(updateReportField).not.toHaveBeenCalled(); expect(screen.getByText('common.error.fieldRequired')).toBeOnTheScreen(); }); it('does not save when the option a list field already holds is picked again', async () => { + // Given a list field already set to Option1 await renderReportFields(1, [buildListField()]); + // When the dropdown is opened and that same option is picked fireEvent.press(screen.getByLabelText('ListField')); await waitForBatchedUpdatesWithAct(); fireEvent.press(screen.getByTestId('base-list-item-Option1')); await waitForBatchedUpdatesWithAct(); + // Then no update is sent, because a re-pick is not an edit and saving it would put a pointless entry in the + // report history and mark the field as recently used for a value the user never actually changed expect(updateReportField).not.toHaveBeenCalled(); }); it('does not save when the value is left unchanged', async () => { + // Given a report with 2 fields that already hold values await renderReportFields(2); + // When a field is focused and left without typing, which happens constantly as the user tabs through a row const input = screen.getByLabelText('Field1'); fireEvent(input, 'blur'); await waitForBatchedUpdatesWithAct(); + // Then nothing is saved, because blur is the save trigger now and merely passing through a field must not + // cost a write expect(updateReportField).not.toHaveBeenCalled(); }); + it('resets the displayed text when an edit is skipped because it only changed the whitespace', async () => { + // Given a report with 2 fields that already hold values + await renderReportFields(2); + + // When the value is retyped with surrounding whitespace and the field is left + const input = screen.getByLabelText('Field1'); + fireEvent.changeText(input, ' Value1 '); + fireEvent(input, 'blur'); + await waitForBatchedUpdatesWithAct(); + + // Then no update is sent, because the value is unchanged once trimmed + expect(updateReportField).not.toHaveBeenCalled(); + + // Then the field shows the saved value again rather than the whitespace the user typed. The saved value never + // changed, so nothing else would ever correct the display and the text would silently snap back on remount + expect(screen.getByLabelText('Field1')).toHaveProp('value', 'Value1'); + }); + it('keeps the fields of a row aligned at the top so an error grows the row downwards', async () => { + // Given a full row of 3 fields await renderReportFields(3); + // When the row is laid out + // Then its fields are aligned to the top, so one field growing taller to show a validation error pushes its + // own content down instead of re-centering the two fields beside it expect(screen.getAllByTestId('reportFieldsRow').at(0)).toHaveStyle({alignItems: 'flex-start'}); }); it('holds back a field violation until the field has been left', async () => { + // Given a required field that is already empty when the report opens, so its violation is true from the start await renderReportFields(1, [buildEmptyField()]); + // Then the error is not shown yet, because a report full of empty required fields would otherwise open as a + // wall of red before the user has touched anything expect(screen.queryByText('EmptyField is required')).toBeNull(); + // When the user focuses the field and leaves it without filling it in fireEvent(screen.getByLabelText('EmptyField'), 'blur'); await waitForBatchedUpdatesWithAct(); + // Then the error appears, because by now the user has had their chance to fill it in expect(screen.getByText('EmptyField is required')).toBeOnTheScreen(); }); it('renders a list field as a collapsed combobox rather than a row that opens a page', async () => { + // Given a report holding a list field set to Option1 await renderReportFields(1, [buildListField()]); + // When the field is inspected before it has been opened const listInput = screen.getByLabelText('ListField'); + + // Then it exposes itself to assistive tech as a closed combobox showing the current value, which is what a + // dropdown that opens in place must look like, unlike the old row that announced itself as a link to a page expect(listInput).toHaveProp('role', CONST.ROLE.COMBOBOX); expect(listInput).toHaveProp('accessibilityState', {expanded: false}); expect(listInput).toHaveProp('value', 'Option1'); - // The dropdown content is deferred until the field is first opened, so no option row is mounted up front. + // Then no option row is mounted yet, because building the option list for every list field on a report full of + // them would be paid for up front even though most dropdowns are never opened expect(screen.queryByText('Option2')).toBeNull(); }); @@ -349,39 +418,47 @@ describe('MoneyRequestViewReportFields', () => { }); it('opens the option list in place when a list field is pressed', async () => { + // Given a report holding a list field await renderReportFields(1, [buildListField()]); + // When the field is pressed fireEvent.press(screen.getByLabelText('ListField')); await waitForBatchedUpdatesWithAct(); + // Then the options appear over the report and the combobox reports itself as expanded, which is what + // replacing the navigation to the editor page with an in-place dropdown has to produce expect(screen.getByLabelText('ListField')).toHaveProp('accessibilityState', {expanded: true}); expect(screen.getByText('Option2')).toBeOnTheScreen(); }); it('keeps the caret in the same place when the option list opens', async () => { + // Given a closed list field whose caret sits in a container padded on one side only await renderReportFields(1, [buildListField()]); expect(getRenderedStyles()).toEqual(expect.arrayContaining([expect.objectContaining(caretContainer)])); + // When the list is opened and the caret flips to point up fireEvent.press(screen.getByLabelText('ListField')); await waitForBatchedUpdatesWithAct(); - // The caret container is padded on one side only, so `rotate(180deg)` would mirror that padding along with - // the caret and slide it 11px left the moment the list opens. Mirroring on the vertical axis turns the caret - // over while leaving its box — and so the caret's distance from the field's right edge — untouched. + // Then the caret is mirrored on the vertical axis rather than rotated. `rotate(180deg)` would carry the + // asymmetric padding around with it and slide the caret 11px left the instant the list opens, so the caret + // would visibly jump sideways every time the user opened a dropdown const expandedStyles = getRenderedStyles(); expect(expandedStyles).toEqual(expect.arrayContaining([expect.objectContaining({...caretContainer, transform: [{scaleY: -1}]})])); expect(expandedStyles).not.toEqual(expect.arrayContaining([expect.objectContaining({transform: [{rotate: '180deg'}]})])); }); it('sizes the option popover like the Spend dropdowns instead of matching the field width', async () => { + // Given a report holding a list field, which in the grid can be as narrow as a third of a row await renderReportFields(1, [buildListField()]); + // When its option list is opened fireEvent.press(screen.getByLabelText('ListField')); await waitForBatchedUpdatesWithAct(); - // A popover as narrow as the field it is anchored to looked cramped, so it uses the width every Spend - // filter dropdown uses. + // Then the popover takes the standard dropdown width rather than the width of the field it is anchored to, + // because a popover that narrow left the option labels cramped and truncated const popover = screen.UNSAFE_getByType(PopoverWithMeasuredContent); expect(popover.props.popoverDimensions).toEqual(expect.objectContaining({width: CONST.POPOVER_DROPDOWN_WIDTH})); }); diff --git a/tests/unit/ReportFieldOptionsListUtilsTest.ts b/tests/unit/ReportFieldOptionsListUtilsTest.ts index ad2a4b28d8d9..b1a5827f32f0 100644 --- a/tests/unit/ReportFieldOptionsListUtilsTest.ts +++ b/tests/unit/ReportFieldOptionsListUtilsTest.ts @@ -20,43 +20,54 @@ describe('ReportFieldOptionsListUtils', () => { describe('getReportFieldOptionsSection', () => { it('pins the frozen initial value to its own "Selected" section on top and excludes it from the "All" section', () => { + // Given a list opened on "Charlie", with the live selection still matching the value it opened on + // When the sections are built const sections = getReportFieldOptionsSection({ options: OPTIONS, recentlyUsedOptions: [], - // Live selection matches the frozen one on open. selectedOptions: [{text: 'Charlie'}], initiallySelectedValue: 'Charlie', searchValue: '', translate: translateLocal, }); + // Then "Charlie" is pinned to the top in its own checked section, so the user can see what the field + // currently holds without scrolling a long option list to find it const selectedSection = sectionByIndex(sections, 1); expect(selectedSection?.data.map((option) => option.keyForList)).toEqual(['Charlie']); expect(selectedSection?.data.at(0)?.isSelected).toBe(true); - // The pinned value must not be duplicated in the "All" section. + + // Then it is not repeated in the "All" section, because the same option listed twice reads as two + // different options the user has to tell apart expect(sectionByIndex(sections, 3)?.data.map((option) => option.keyForList)).not.toContain('Charlie'); }); it('keeps the frozen value pinned while the live selection drives the checkmark', () => { + // Given a list that opened on "Charlie" and on which the user has since picked "Delta" + // When the sections are rebuilt const sections = getReportFieldOptionsSection({ options: OPTIONS, recentlyUsedOptions: [], - // The user picked "Delta" (live), but the list opened on "Charlie" (frozen). selectedOptions: [{text: 'Delta'}], initiallySelectedValue: 'Charlie', searchValue: '', translate: translateLocal, }); + // Then "Charlie" stays pinned but loses its checkmark. Pinning on the live selection instead would make + // the option the user just clicked jump to the top of the list under their cursor const selectedSection = sectionByIndex(sections, 1); - // "Charlie" stays pinned (frozen) but is no longer checked... expect(selectedSection?.data.map((option) => option.keyForList)).toEqual(['Charlie']); expect(selectedSection?.data.at(0)?.isSelected).toBe(false); - // ...and the live selection "Delta" is checked where it sits in the "All" section. + + // Then the checkmark follows the live selection to where "Delta" naturally sits, so the list still shows + // the user what they picked expect(sectionByIndex(sections, 3)?.data.find((option) => option.keyForList === 'Delta')?.isSelected).toBe(true); }); it('keeps the pinned value at the top of the search results when it matches', () => { + // Given a list pinned to "Rate B" and a search term that every option matches + // When the sections are built const sections = getReportFieldOptionsSection({ options: ['Rate A', 'Rate B', 'Rate C'], recentlyUsedOptions: [], @@ -66,14 +77,20 @@ describe('ReportFieldOptionsListUtils', () => { translate: translateLocal, }); - // Search collapses to a single section; the pinned value leads regardless of its natural position. + // Then searching collapses everything into one section but the pinned value still leads it, so the + // current value does not disappear into the middle of the results the moment the user types const searchData = sectionByIndex(sections, 0)?.data.map((option) => option.keyForList) ?? []; expect(searchData.at(0)).toBe('Rate B'); + + // Then every matching option is still offered exactly once, because pinning reorders the results rather + // than adding a copy of the current value to them expect(searchData).toEqual(expect.arrayContaining(['Rate A', 'Rate B', 'Rate C'])); expect(searchData).toHaveLength(3); }); it('leaves the selected value in place when pinning is turned off', () => { + // Given a caller that opts out of pinning, which the inline field does for a short option list + // When the sections are built const sections = getReportFieldOptionsSection({ options: OPTIONS, recentlyUsedOptions: [], @@ -84,13 +101,20 @@ describe('ReportFieldOptionsListUtils', () => { shouldPinSelectedOption: false, }); - // No "Selected" section, and the value keeps its natural position in the full list while staying checked. + // Then there is no "Selected" section and the options keep their natural order, because a handful of + // options is easier to scan in a stable order than one that moves the current value to the top expect(sectionByIndex(sections, 1)).toBeUndefined(); expect(sectionByIndex(sections, 3)?.data.map((option) => option.keyForList)).toEqual(OPTIONS); + + // Then the current value is still checked, so turning off pinning changes the ordering only and does not + // cost the user the ability to see what the field holds expect(sectionByIndex(sections, 3)?.data.find((option) => option.keyForList === 'Charlie')?.isSelected).toBe(true); }); it('drops the section titles when they are turned off', () => { + // Given a caller that opts out of section titles and data that would otherwise produce two titled + // sections, "Recent" and "All" + // When the sections are built const sections = getReportFieldOptionsSection({ options: OPTIONS, recentlyUsedOptions: ['Delta'], @@ -101,10 +125,14 @@ describe('ReportFieldOptionsListUtils', () => { shouldShowSectionTitles: false, }); + // Then both sections are still built but neither carries a title, so a small popover is not made to spend + // its limited height on headers while the options themselves stay grouped expect(sections.map((section) => section.title)).toEqual(['', '']); }); it('does not force-show the pinned value when it does not match the search', () => { + // Given a list pinned to "Rate B" and a search term that only "Zulu" matches + // When the sections are built const sections = getReportFieldOptionsSection({ options: ['Rate A', 'Rate B', 'Zulu'], recentlyUsedOptions: [], @@ -114,6 +142,8 @@ describe('ReportFieldOptionsListUtils', () => { translate: translateLocal, }); + // Then only "Zulu" is offered. Pinning decides where a matching value sits, so it must not smuggle a + // non-matching option into results the user has explicitly filtered down expect(sectionByIndex(sections, 0)?.data.map((option) => option.keyForList)).toEqual(['Zulu']); }); }); From 15561faded8e215848e304362cf8b250dfdc5f60 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Fri, 25 Sep 2026 04:10:58 +0000 Subject: [PATCH 05/16] Address AI review: drop em dashes from comments and move compact row height into a named style Co-authored-by: daledah --- .../MoneyRequestReportView/ReportFieldInlineInput.tsx | 4 ++-- src/components/ReportActionItem/MoneyReportView.tsx | 4 ++-- src/pages/EditReportFieldDropdown.tsx | 4 +--- src/styles/index.ts | 4 ++++ 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx index 88d7079c50ac..64d107886f04 100644 --- a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx +++ b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx @@ -197,7 +197,7 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error ); return ( - // No `popoverWidth`, so the dropdown falls back to `CONST.POPOVER_DROPDOWN_WIDTH` — the same width every + // No `popoverWidth`, so the dropdown falls back to `CONST.POPOVER_DROPDOWN_WIDTH`, the same width every // Spend filter dropdown uses. Matching the field's own width instead made narrow fields open odd, cramped // popovers and wide ones open oversized. { const {fieldsByName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs); return Object.values(fieldsByName) diff --git a/src/pages/EditReportFieldDropdown.tsx b/src/pages/EditReportFieldDropdown.tsx index 3601e03ae27d..44178633b8bf 100644 --- a/src/pages/EditReportFieldDropdown.tsx +++ b/src/pages/EditReportFieldDropdown.tsx @@ -10,8 +10,6 @@ import useThemeStyles from '@hooks/useThemeStyles'; import {getHeaderMessageForNonUserList} from '@libs/OptionsListUtils'; import {getReportFieldOptionsSection} from '@libs/ReportFieldOptionsListUtils'; -import variables from '@styles/variables'; - import ONYXKEYS from '@src/ONYXKEYS'; import React from 'react'; @@ -105,7 +103,7 @@ function EditReportFieldDropdown({ // Same override the Spend single-select dropdowns use to shorten the default 64px option row. It also makes // the rows match the 52px `getSelectionListPopoverHeight` already assumes, so the popover stops being sized // for less content than it holds. - style={shouldUseCompactRows ? {listItemWrapperStyle: {minHeight: variables.optionRowHeightCompact}} : undefined} + style={shouldUseCompactRows ? {listItemWrapperStyle: styles.optionRowCompactMinHeight} : undefined} shouldShowTextInput={shouldShowTextInput} textInputOptions={textInputOptions} // Re-selecting the value the field already holds submits that same value rather than an empty string. diff --git a/src/styles/index.ts b/src/styles/index.ts index 20662cdcff89..4a05704a8a67 100644 --- a/src/styles/index.ts +++ b/src/styles/index.ts @@ -2183,6 +2183,10 @@ const staticStyles = (theme: ThemeColors) => paddingBottom: 12, }, + optionRowCompactMinHeight: { + minHeight: variables.optionRowHeightCompact, + }, + optionsListSectionHeader: { marginTop: 8, marginBottom: 4, From c7676e34a4487c2b0fa48ba57b5f97a8310b497a Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Fri, 25 Sep 2026 10:07:40 +0000 Subject: [PATCH 06/16] Keep report field labels raised and remount inputs per report so values do not jump between reports Co-authored-by: daledah --- .../MoneyRequestViewReportFields.tsx | 4 +- .../ReportFieldInlineInput.tsx | 6 +++ tests/ui/MoneyRequestViewReportFieldsTest.tsx | 40 +++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx b/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx index 6a15730dd215..1d72272ba78e 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx @@ -74,7 +74,9 @@ function ReportFieldView( ) { return ( )} @@ -247,6 +250,9 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error saveDraftValue(); }} onSubmitEditing={saveDraftValue} + // Keeps the label raised like the date field's, so a value that goes empty and back while the next report + // loads does not drop the label into the field and animate it back up. + forceActiveLabel /> ); } diff --git a/tests/ui/MoneyRequestViewReportFieldsTest.tsx b/tests/ui/MoneyRequestViewReportFieldsTest.tsx index 5cb124a41d9f..ce45a62167c4 100644 --- a/tests/ui/MoneyRequestViewReportFieldsTest.tsx +++ b/tests/ui/MoneyRequestViewReportFieldsTest.tsx @@ -449,6 +449,46 @@ describe('MoneyRequestViewReportFields', () => { expect(expandedStyles).not.toEqual(expect.arrayContaining([expect.objectContaining({transform: [{rotate: '180deg'}]})])); }); + it('keeps the label of text and list fields raised so it does not jump while the next report loads', async () => { + // Given a report holding both a text field and a list field + await renderReportFields(1, [buildListField()]); + + // When the inputs are inspected + const raisedLabels: unknown[] = []; + for (const node of screen.UNSAFE_root.findAll((candidate) => candidate.props.forceActiveLabel === true)) { + raisedLabels.push(node.props.label); + } + + // Then both labels are pinned in the raised position like the date field's. The next/previous arrows keep + // these inputs mounted, and the next report's value can briefly resolve to empty before it loads, so an + // unpinned label would drop into the field and animate back up + expect(raisedLabels).toEqual(expect.arrayContaining(['Field1', 'ListField'])); + }); + + it('does not carry an unsaved edit into the next report', async () => { + // Given a report whose field holds an edit the user has typed but not saved yet + const {rerender} = await renderReportFields(1); + fireEvent.changeText(screen.getByLabelText('Field1'), 'Unsaved edit'); + await waitForBatchedUpdatesWithAct(); + + // When the next report arrow swaps in another report on the same workspace without remounting the screen + const policy = buildPolicy(1); + const nextReport = {...buildReport(), reportID: 'next_report_report_fields_grid'}; + rerender( + + + , + ); + await waitForBatchedUpdatesWithAct(); + + // Then the field shows the next report's value, because the edit belonged to the previous report and the + // field key alone is the same on every report of the workspace + expect(screen.getByLabelText('Field1')).toHaveProp('value', 'Value1'); + }); + it('sizes the option popover like the Spend dropdowns instead of matching the field width', async () => { // Given a report holding a list field, which in the grid can be as narrow as a third of a row await renderReportFields(1, [buildListField()]); From 7cdd0dfa3d393cda81797a9f356f4ef31b635556 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Fri, 25 Sep 2026 11:04:23 +0000 Subject: [PATCH 07/16] Open the list report field dropdown once per tap so Android's duplicate press does not close it Co-authored-by: daledah --- .../ReportFieldInlineInput.tsx | 76 +++++++++++-------- tests/ui/MoneyRequestViewReportFieldsTest.tsx | 18 +++++ 2 files changed, 64 insertions(+), 30 deletions(-) diff --git a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx index 653716a7408e..1a9c6d3c3940 100644 --- a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx +++ b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx @@ -26,8 +26,9 @@ import CONST from '@src/CONST'; import type {PolicyReportField} from '@src/types/onyx'; import {Str} from 'expensify-common'; -import React, {useState} from 'react'; +import React, {useRef, useState} from 'react'; import {View} from 'react-native'; +import type {GestureResponderEvent} from 'react-native'; type ReportFieldInlineInputProps = { reportField: PolicyReportField; @@ -70,6 +71,11 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error // the input is blurred right after it was submitted, is skipped while the update is still in flight. const [lastSavedValue, setLastSavedValue] = useState(value); + // On Android a tap on the list field's input reports the same press twice, once as `onPress` and once as + // `onPressOut`, and the dropdown trigger toggles. Remembering the last press lets each tap open the list once + // instead of opening and then closing it. + const lastOptionsPressEventRef = useRef(null); + // The saved value changes while this input is mounted whenever the field is updated elsewhere, for example from // another device or by an optimistic update, so the draft follows it. if (value !== previousValue) { @@ -202,35 +208,45 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error // popovers and wide ones open oversized. ( - - setHasBeenBlurred(true)} - onSubmitEditing={onPress} - disableKeyboard - // The field is focused so it stays keyboard reachable, but it holds a value picked from a - // list rather than typed text, so the caret would be a caret you cannot type into. - // `disableKeyboard` only suppresses the software keyboard, it does not hide the caret. - caretHidden - // Keeps the label raised like the date field's, so a value that goes empty and back while - // the next report loads does not drop the label into the field and animate it back up. - forceActiveLabel - /> - - )} + renderButton={({onPress, ref, isExpanded}) => { + const openOptions = (event: GestureResponderEvent | KeyboardEvent) => { + if (event === lastOptionsPressEventRef.current) { + return; + } + lastOptionsPressEventRef.current = event; + onPress(); + }; + + return ( + + setHasBeenBlurred(true)} + onSubmitEditing={onPress} + disableKeyboard + // The field is focused so it stays keyboard reachable, but it holds a value picked from a + // list rather than typed text, so the caret would be a caret you cannot type into. + // `disableKeyboard` only suppresses the software keyboard, it does not hide the caret. + caretHidden + // Keeps the label raised like the date field's, so a value that goes empty and back while + // the next report loads does not drop the label into the field and animate it back up. + forceActiveLabel + /> + + ); + }} /> ); } diff --git a/tests/ui/MoneyRequestViewReportFieldsTest.tsx b/tests/ui/MoneyRequestViewReportFieldsTest.tsx index ce45a62167c4..4614e06ad210 100644 --- a/tests/ui/MoneyRequestViewReportFieldsTest.tsx +++ b/tests/ui/MoneyRequestViewReportFieldsTest.tsx @@ -431,6 +431,24 @@ describe('MoneyRequestViewReportFields', () => { expect(screen.getByText('Option2')).toBeOnTheScreen(); }); + it('keeps the option list open when one tap reaches the list field as both a press and a press out', async () => { + // Given a report holding a list field + await renderReportFields(1, [buildListField()]); + const listField = screen.getByLabelText('ListField'); + const tapEvent = {nativeEvent: {}}; + + // When a single tap reaches the field twice with the same event, the way the Android input reports it + // through both `onPressOut` and `onPress` + fireEvent(listField, 'pressOut', tapEvent); + fireEvent.press(listField, tapEvent); + await waitForBatchedUpdatesWithAct(); + + // Then the list stays open. The dropdown trigger toggles, so if the repeated report were not ignored the + // second call would close the list the first one opened, which is the list auto-closing on Android + expect(listField).toHaveProp('accessibilityState', {expanded: true}); + expect(screen.getByText('Option2')).toBeOnTheScreen(); + }); + it('keeps the caret in the same place when the option list opens', async () => { // Given a closed list field whose caret sits in a container padded on one side only await renderReportFields(1, [buildListField()]); From 48fb51515fd5014a42e9346e5971a380c7c76405 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Mon, 28 Sep 2026 08:28:27 +0000 Subject: [PATCH 08/16] Fix: Run oxfmt on ReportFieldInlineInput Co-authored-by: daledah --- .../MoneyRequestReportView/ReportFieldInlineInput.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx index 1a9c6d3c3940..16d7937e69a9 100644 --- a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx +++ b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx @@ -25,10 +25,11 @@ import EditReportFieldDropdown from '@pages/EditReportFieldDropdown'; import CONST from '@src/CONST'; import type {PolicyReportField} from '@src/types/onyx'; +import type {GestureResponderEvent} from 'react-native'; + import {Str} from 'expensify-common'; import React, {useRef, useState} from 'react'; import {View} from 'react-native'; -import type {GestureResponderEvent} from 'react-native'; type ReportFieldInlineInputProps = { reportField: PolicyReportField; From 192d7873f12fdc9531e1db1ee4d56c6656d52ca9 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Mon, 28 Sep 2026 09:21:54 +0000 Subject: [PATCH 09/16] Keep the backdrop behind the Month/Year picker when editing a date report field inline in the RHP Co-authored-by: daledah --- src/components/DatePicker/index.tsx | 2 ++ .../MoneyRequestReportView/ReportFieldInlineInput.tsx | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/src/components/DatePicker/index.tsx b/src/components/DatePicker/index.tsx index 34279f0ec9a7..99abb5c93a80 100644 --- a/src/components/DatePicker/index.tsx +++ b/src/components/DatePicker/index.tsx @@ -54,6 +54,7 @@ function DatePicker({ rightHandSideComponent, onPickerVisibilityChange, shouldHideCalendarIcon = false, + shouldEnableMonthYearBackdropInNarrowPane = false, }: DateInputWithPickerProps) { const icons = useMemoizedLazyExpensifyIcons(['Calendar']); const styles = useThemeStyles(); @@ -276,6 +277,7 @@ function DatePicker({ shouldPositionFromTop={!isInverted} forwardedFSClass={forwardedFSClass} shouldCloseWhenBrowserNavigationChanged + shouldEnableMonthYearBackdropInNarrowPane={shouldEnableMonthYearBackdropInNarrowPane} /> ); diff --git a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx index 16d7937e69a9..502d069fefb2 100644 --- a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx +++ b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx @@ -13,6 +13,7 @@ import TextInput from '@components/TextInput'; import useIsInLandscapeMode from '@hooks/useIsInLandscapeMode'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; import useLocalize from '@hooks/useLocalize'; +import useResponsiveLayout from '@hooks/useResponsiveLayout'; import useThemeStyles from '@hooks/useThemeStyles'; import useWindowDimensions from '@hooks/useWindowDimensions'; @@ -57,6 +58,7 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error const {translate} = useLocalize(); const {windowHeight} = useWindowDimensions(); const isInLandscapeMode = useIsInLandscapeMode(); + const {isInNarrowPaneModal} = useResponsiveLayout(); const icons = useMemoizedLazyExpensifyIcons(['DownArrow']); const [draftValue, setDraftValue] = useState(value); @@ -154,6 +156,9 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error wrapperStyle={styles.mv0} shouldDeferShowUntilPositioned shouldHideClearButton + // The Month/Year pickers are right-docked, and a right-docked modal opened from the RHP drops its + // backdrop by default, so the report behind them would not be dimmed. + shouldEnableMonthYearBackdropInNarrowPane={isInNarrowPaneModal} /> ); } From 07b54dc8f0925556bab7192164709661dc685a6e Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Thu, 1 Oct 2026 09:00:25 +0000 Subject: [PATCH 10/16] Show a delete button on report fields deleted from the workspace Inline report fields no longer open the editor page, so its Delete option was unreachable for a field the admin had deleted. Such a field now shows a segmented trash button on its right edge that confirms and removes it. Co-authored-by: daledah --- src/CONST/index.ts | 1 + src/components/DatePicker/index.tsx | 3 +- .../MoneyRequestViewReportFields.tsx | 44 ++++++++++- .../ReportFieldInlineInput.tsx | 59 +++++++++++++-- src/styles/index.ts | 15 ++++ tests/ui/MoneyRequestViewReportFieldsTest.tsx | 73 ++++++++++++++++++- 6 files changed, 179 insertions(+), 16 deletions(-) diff --git a/src/CONST/index.ts b/src/CONST/index.ts index 30fa3840425c..1fb39483f456 100644 --- a/src/CONST/index.ts +++ b/src/CONST/index.ts @@ -9427,6 +9427,7 @@ const CONST = { MODERATION_BUTTON: 'Report-ModerationButton', MONEY_REQUEST_REPORT_ACTIONS_LIST_SELECT_ALL: 'MoneyRequestReportActionsList-SelectAll', MONEY_REQUEST_REPORT_TRANSACTION_ITEM: 'MoneyRequestReportTransactionItem', + REPORT_FIELD_DELETE_BUTTON: 'Report-ReportFieldDeleteButton', REPORT_ACTION_AVATAR: 'Report-ReportActionAvatar', PARTICIPANTS_ROW: 'Report-ParticipantsRow', ROOM_MEMBERS_ROW: 'Report-RoomMembersRow', diff --git a/src/components/DatePicker/index.tsx b/src/components/DatePicker/index.tsx index 99abb5c93a80..be80fde9d11f 100644 --- a/src/components/DatePicker/index.tsx +++ b/src/components/DatePicker/index.tsx @@ -55,6 +55,7 @@ function DatePicker({ onPickerVisibilityChange, shouldHideCalendarIcon = false, shouldEnableMonthYearBackdropInNarrowPane = false, + textInputContainerStyles, }: DateInputWithPickerProps) { const icons = useMemoizedLazyExpensifyIcons(['Calendar']); const styles = useThemeStyles(); @@ -255,7 +256,7 @@ function DatePicker({ onBlur={onBlur} onSubmitEditing={() => showDatePickerModal()} onKeyPress={handleInputKeyPress} - textInputContainerStyles={isModalVisible ? styles.borderColorFocus : {}} + textInputContainerStyles={[textInputContainerStyles, isModalVisible && styles.borderColorFocus]} shouldHideClearButton={shouldHideClearButton} onClearInput={handleClear} forwardedFSClass={forwardedFSClass} diff --git a/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx b/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx index 1d72272ba78e..7ce575930c49 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx @@ -1,13 +1,16 @@ +import {ModalActions} from '@components/Modal/Global/ModalContext'; import OfflineWithFeedback from '@components/OfflineWithFeedback'; +import useConfirmModal from '@hooks/useConfirmModal'; import {useCurrencyListActions} from '@hooks/useCurrencyList'; import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; +import useLocalize from '@hooks/useLocalize'; import useOnyx from '@hooks/useOnyx'; import useResponsiveLayoutOnWideRHP from '@hooks/useResponsiveLayoutOnWideRHP'; import useSaveReportField from '@hooks/useSaveReportField'; import useThemeStyles from '@hooks/useThemeStyles'; -import {clearReportFieldKeyErrors} from '@libs/actions/Report'; +import {clearReportFieldKeyErrors, deleteReportField} from '@libs/actions/Report'; import {resolveReportFieldValue} from '@libs/Formula'; import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; import { @@ -59,6 +62,9 @@ type MoneyRequestViewReportFieldsProps = { type EnrichedPolicyReportField = { fieldValue: string; isFieldDisabled: boolean; + + /** Whether the field was deleted from the workspace but still holds a value on this report, so it can be removed */ + isFieldDeletable: boolean; fieldKey: string; violation: ReportViolationName | undefined; violationTranslation: string; @@ -70,6 +76,7 @@ function ReportFieldView( policy: OnyxEntry, styles: ThemeStyles, onSaveValue: (reportField: PolicyReportField, value: string) => void, + onDelete: (reportField: EnrichedPolicyReportField) => Promise, pendingAction?: PendingAction, ) { return ( @@ -93,6 +100,13 @@ function ReportFieldView( errorText={reportField.violationTranslation} fieldList={policy?.fieldList} onSaveValue={(value) => onSaveValue(reportField, value)} + onDelete={ + reportField.isFieldDeletable + ? () => { + onDelete(reportField); + } + : undefined + } /> @@ -107,6 +121,25 @@ function MoneyRequestViewReportFields({report, policy, pendingAction, style, sho const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${getNonEmptyStringOnyxID(report?.reportID)}`); const {getCurrencyDecimals} = useCurrencyListActions(); const [rules] = useOnyx(ONYXKEYS.COLLECTION.RULE); + const {translate} = useLocalize(); + const {showConfirmModal} = useConfirmModal(); + + const deleteField = async (reportField: EnrichedPolicyReportField) => { + const result = await showConfirmModal({ + title: translate('workspace.reportFields.delete'), + prompt: translate('workspace.reportFields.deleteConfirmation'), + confirmText: translate('common.delete'), + cancelText: translate('common.cancel'), + buttonVariant: CONST.BUTTON_VARIANT.DANGER, + shouldEnableNewFocusManagement: true, + }); + if (result.action !== ModalActions.CONFIRM || !report?.reportID) { + return; + } + // The failure data restores whatever field is passed here, so it gets the field as stored on the report rather + // than the copy enriched for display. + deleteReportField(report.reportID, report.fieldList?.[reportField.fieldKey] ?? reportField); + }; const sortedPolicyReportFields = useMemo((): EnrichedPolicyReportField[] => { const {fieldValues, fieldsByName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs); @@ -121,6 +154,8 @@ function MoneyRequestViewReportFields({report, policy, pendingAction, style, sho const isFieldDisabled = isReportFieldDisabledForUser(report, field, policy, currentUserAccountID, rules); const isDeletedFormulaField = field.type === CONST.REPORT_FIELD_TYPES.FORMULA && field.deletable; const fieldKey = getReportFieldKey(field.fieldID); + const isFieldOnPolicy = !!(policy?.fieldList?.[fieldKey] ?? policy?.fieldList?.[field.fieldID]); + const isEditable = !isFieldDisabled || isDeletedFormulaField; const violation = isFieldDisabled ? undefined : getFieldViolation(field); const violationTranslation = getFieldViolationTranslation(field, violation); @@ -128,7 +163,10 @@ function MoneyRequestViewReportFields({report, policy, pendingAction, style, sho return { ...field, fieldValue, - isFieldDisabled: isFieldDisabled && !isDeletedFormulaField, + isFieldDisabled: !isEditable, + // The same rule the report field editor page used for its Delete option, limited to fields that are + // no longer on the workspace. + isFieldDeletable: field.deletable && field.fieldID !== CONST.REPORT_FIELD_TITLE_FIELD_ID && !isFieldOnPolicy && isEditable, fieldKey, violation, violationTranslation, @@ -163,7 +201,7 @@ function MoneyRequestViewReportFields({report, policy, pendingAction, style, sho // message grows downwards instead of stretching the cells beside it and shifting their inputs. style={[styles.flexRow, styles.gap3, styles.alignItemsStart]} > - {fieldRow.map((reportField) => ReportFieldView(reportField, report, policy, styles, saveReportField, pendingAction))} + {fieldRow.map((reportField) => ReportFieldView(reportField, report, policy, styles, saveReportField, deleteField, pendingAction))} {/* A partly filled last row is padded out so its fields stay the same width as the rows above it. */} {Array.from({length: columnCount - fieldRow.length}, (_unused, index) => ( void; + + /** + * Removes the field from the report. Passed only for a field that was deleted from the workspace, so the field + * shows a delete button on its right edge. Without it the stale value could never be cleared off the report. + */ + onDelete?: () => void; }; -function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, errorText, fieldList, onSaveValue}: ReportFieldInlineInputProps) { +function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, errorText, fieldList, onSaveValue, onDelete}: ReportFieldInlineInputProps) { + const theme = useTheme(); const styles = useThemeStyles(); const {translate} = useLocalize(); const {windowHeight} = useWindowDimensions(); const isInLandscapeMode = useIsInLandscapeMode(); const {isInNarrowPaneModal} = useResponsiveLayout(); - const icons = useMemoizedLazyExpensifyIcons(['DownArrow']); + const icons = useMemoizedLazyExpensifyIcons(['DownArrow', 'Trashcan']); const [draftValue, setDraftValue] = useState(value); const [previousValue, setPreviousValue] = useState(value); @@ -139,8 +149,38 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error onSaveValue(selectedValue); }; - if (reportField.type === CONST.REPORT_FIELD_TYPES.DATE && !isReadOnly) { + // The input's right corners are squared off so the delete button can sit flush against it, and the input's own + // right border becomes the divider between the two. + const textInputContainerStyles = onDelete ? styles.noRightBorderRadius : undefined; + + const renderWithDeleteButton = (field: React.ReactNode) => { + if (!onDelete) { + return field; + } + return ( + // The button keeps the input's height and stays at the top, so an error message under the input grows the + // field downwards without stretching the button. + + {field} + + + + + ); + }; + + if (reportField.type === CONST.REPORT_FIELD_TYPES.DATE && !isReadOnly) { + return renderWithDeleteButton( + textInputContainerStyles={textInputContainerStyles} + />, ); } @@ -208,7 +249,7 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error ); - return ( + return renderWithDeleteButton( // No `popoverWidth`, so the dropdown falls back to `CONST.POPOVER_DROPDOWN_WIDTH`, the same width every // Spend filter dropdown uses. Matching the field's own width instead made narrow fields open odd, cramped // popovers and wide ones open oversized. @@ -249,15 +290,16 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error // Keeps the label raised like the date field's, so a value that goes empty and back while // the next report loads does not drop the label into the field and animate it back up. forceActiveLabel + textInputContainerStyles={textInputContainerStyles} /> ); }} - /> + />, ); } - return ( + return renderWithDeleteButton( + textInputContainerStyles={textInputContainerStyles} + />, ); } diff --git a/src/styles/index.ts b/src/styles/index.ts index cfeb6d92611b..706ea2a77711 100644 --- a/src/styles/index.ts +++ b/src/styles/index.ts @@ -2199,6 +2199,21 @@ const staticStyles = (theme: ThemeColors) => minHeight: variables.optionRowHeightCompact, }, + // Sits flush against the right edge of a report field whose input has `noRightBorderRadius`, so the input's + // right border reads as a divider and the two look like one segmented field. + reportFieldDeleteButton: { + width: variables.componentSizeLarge, + height: variables.componentSizeLarge, + alignItems: 'center', + justifyContent: 'center', + backgroundColor: theme.appBG, + borderWidth: 1, + borderLeftWidth: 0, + borderTopRightRadius: variables.componentBorderRadiusNormal, + borderBottomRightRadius: variables.componentBorderRadiusNormal, + borderColor: theme.bordersBold, + }, + optionsListSectionHeader: { marginTop: 8, marginBottom: 4, diff --git a/tests/ui/MoneyRequestViewReportFieldsTest.tsx b/tests/ui/MoneyRequestViewReportFieldsTest.tsx index 4614e06ad210..e804a5132ef7 100644 --- a/tests/ui/MoneyRequestViewReportFieldsTest.tsx +++ b/tests/ui/MoneyRequestViewReportFieldsTest.tsx @@ -5,7 +5,7 @@ import MoneyRequestViewReportFields from '@components/MoneyRequestReportView/Mon import OnyxListItemProvider from '@components/OnyxListItemProvider'; import PopoverWithMeasuredContent from '@components/PopoverWithMeasuredContent'; -import {updateReportField} from '@libs/actions/Report'; +import {deleteReportField, updateReportField} from '@libs/actions/Report'; import initOnyxDerivedValues from '@userActions/OnyxDerived'; @@ -65,6 +65,14 @@ jest.mock('@react-navigation/core', () => ({ jest.mock('@libs/actions/Report', () => ({ ...jest.requireActual>('@libs/actions/Report'), updateReportField: jest.fn(), + deleteReportField: jest.fn(), +})); + +const mockShowConfirmModal = jest.fn(() => Promise.resolve({action: 'CONFIRM'})); + +jest.mock('@hooks/useConfirmModal', () => ({ + __esModule: true, + default: () => ({showConfirmModal: mockShowConfirmModal}), })); TestHelper.setupGlobalFetchMock(); @@ -115,6 +123,15 @@ const buildListField = (): OnyxTypes.PolicyReportField => ({ disabledOptions: [false, false], }); +// A field the admin deleted from the workspace. The report keeps its own copy, with `deletable` set, so the value it +// already held is not lost. +const buildDeletedField = (): OnyxTypes.PolicyReportField => ({ + ...buildTextField(1), + name: 'DeletedField', + fieldID: 'deletedField', + value: 'Old value', +}); + /** Every flattened style currently on screen, so a test can assert that some node does (or does not) carry one. */ const getRenderedStyles = (): unknown[] => { const renderedStyles: unknown[] = []; @@ -153,8 +170,9 @@ const buildPolicy = (fieldCount: number, extraFields: OnyxTypes.PolicyReportFiel }; }; -const buildReport = (): OnyxTypes.Report => ({ +const buildReport = (reportOnlyFields: OnyxTypes.PolicyReportField[] = []): OnyxTypes.Report => ({ ...LHNTestUtils.getFakeReport([accountID, 2]), + fieldList: Object.fromEntries(reportOnlyFields.map((field) => [`expensify_${field.fieldID}`, field])), reportID, type: CONST.REPORT.TYPE.EXPENSE, policyID, @@ -165,9 +183,14 @@ const buildReport = (): OnyxTypes.Report => ({ total: 0, }); -const renderReportFields = async (fieldCount: number, extraFields: OnyxTypes.PolicyReportField[] = [], shouldUseSingleColumn = false) => { +const renderReportFields = async ( + fieldCount: number, + extraFields: OnyxTypes.PolicyReportField[] = [], + shouldUseSingleColumn = false, + reportOnlyFields: OnyxTypes.PolicyReportField[] = [], +) => { const policy = buildPolicy(fieldCount, extraFields); - const report = buildReport(); + const report = buildReport(reportOnlyFields); await act(async () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, policy); @@ -520,4 +543,46 @@ describe('MoneyRequestViewReportFields', () => { const popover = screen.UNSAFE_getByType(PopoverWithMeasuredContent); expect(popover.props.popoverDimensions).toEqual(expect.objectContaining({width: CONST.POPOVER_DROPDOWN_WIDTH})); }); + + it('shows a delete button only on a field that was deleted from the workspace', async () => { + // Given a report holding one field that is still on the workspace and one the admin has since deleted. Both + // are `deletable`, so the button has to be decided by the field being gone from the workspace + await renderReportFields(1, [], false, [buildDeletedField()]); + + // When the fields are rendered + // Then the deleted field still shows its value and carries a delete button, because the editor page with its + // Delete option is no longer reachable and this is the only way to clear the stale value off the report + expect(screen.getByLabelText('DeletedField')).toHaveProp('value', 'Old value'); + expect(screen.getByLabelText('workspace.reportFields.delete, DeletedField')).toBeOnTheScreen(); + + // And the field that is still on the workspace has no delete button, since removing it would only bring it back + expect(screen.queryByLabelText('workspace.reportFields.delete, Field1')).not.toBeOnTheScreen(); + }); + + it('deletes the field from the report once the delete is confirmed', async () => { + // Given a report holding a field that was deleted from the workspace + await renderReportFields(1, [], false, [buildDeletedField()]); + + // When its delete button is pressed and the confirmation is accepted + fireEvent.press(screen.getByLabelText('workspace.reportFields.delete, DeletedField')); + await waitForBatchedUpdatesWithAct(); + + // Then the user is asked first, as on the editor page, and the field is removed from this report + expect(mockShowConfirmModal).toHaveBeenCalledTimes(1); + expect(deleteReportField).toHaveBeenCalledTimes(1); + expect(deleteReportField).toHaveBeenCalledWith(reportID, expect.objectContaining({fieldID: 'deletedField'})); + }); + + it('keeps the field when the delete is cancelled', async () => { + // Given a report holding a field that was deleted from the workspace + await renderReportFields(1, [], false, [buildDeletedField()]); + + // When its delete button is pressed but the confirmation is dismissed + mockShowConfirmModal.mockImplementationOnce(() => Promise.resolve({action: 'CLOSE'})); + fireEvent.press(screen.getByLabelText('workspace.reportFields.delete, DeletedField')); + await waitForBatchedUpdatesWithAct(); + + // Then nothing is deleted, so a stray tap on the button cannot wipe the value + expect(deleteReportField).not.toHaveBeenCalled(); + }); }); From 4db00c662967e882daee8154c38a2a78d9a47670 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Thu, 1 Oct 2026 16:01:04 +0000 Subject: [PATCH 11/16] Don't reserve offline indicator space inside FilterPopupButton popover Co-authored-by: daledah --- src/components/Search/FilterDropdowns/FilterPopupButton.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/components/Search/FilterDropdowns/FilterPopupButton.tsx b/src/components/Search/FilterDropdowns/FilterPopupButton.tsx index 5bf8eff98616..351f81a43ee1 100644 --- a/src/components/Search/FilterDropdowns/FilterPopupButton.tsx +++ b/src/components/Search/FilterDropdowns/FilterPopupButton.tsx @@ -67,7 +67,11 @@ function FilterPopupButton({ const isFocused = useIsFocused(); const styles = useThemeStyles(); const {isKeyboardActive} = useKeyboardState(); - const bottomSafeAreaPaddingStyle = useBottomSafeSafeAreaPaddingStyle({addBottomSafeAreaPadding: isSmallScreenWidth && !isKeyboardActive}); + // The style is applied inside the popover, which never renders an offline indicator, so don't reserve space for one + const bottomSafeAreaPaddingStyle = useBottomSafeSafeAreaPaddingStyle({ + addBottomSafeAreaPadding: isSmallScreenWidth && !isKeyboardActive, + addOfflineIndicatorBottomSafeAreaPadding: false, + }); const StyleUtils = useStyleUtils(); const {windowHeight} = useWindowDimensions(); const triggerRef = useRef | null>(null); From 64672651ae17c8c7d2b9802fa8b3168c438ad6f1 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Thu, 1 Oct 2026 16:55:44 +0000 Subject: [PATCH 12/16] Show the delete button on deleted date report fields too Co-authored-by: daledah --- .../MoneyRequestViewReportFields.tsx | 7 ++++--- tests/ui/MoneyRequestViewReportFieldsTest.tsx | 16 ++++++++++++++-- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx b/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx index 7ce575930c49..ca56c843825d 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx @@ -164,9 +164,10 @@ function MoneyRequestViewReportFields({report, policy, pendingAction, style, sho ...field, fieldValue, isFieldDisabled: !isEditable, - // The same rule the report field editor page used for its Delete option, limited to fields that are - // no longer on the workspace. - isFieldDeletable: field.deletable && field.fieldID !== CONST.REPORT_FIELD_TITLE_FIELD_ID && !isFieldOnPolicy && isEditable, + // Only fields that are no longer on the workspace can be deleted. Their `deletable` flag is not + // checked: once the workspace drops a field, whether it was required no longer applies, and the + // report's copy of a date field can still be flagged as not deletable. + isFieldDeletable: field.fieldID !== CONST.REPORT_FIELD_TITLE_FIELD_ID && !isFieldOnPolicy && isEditable, fieldKey, violation, violationTranslation, diff --git a/tests/ui/MoneyRequestViewReportFieldsTest.tsx b/tests/ui/MoneyRequestViewReportFieldsTest.tsx index e804a5132ef7..2e3ece9caad2 100644 --- a/tests/ui/MoneyRequestViewReportFieldsTest.tsx +++ b/tests/ui/MoneyRequestViewReportFieldsTest.tsx @@ -123,8 +123,7 @@ const buildListField = (): OnyxTypes.PolicyReportField => ({ disabledOptions: [false, false], }); -// A field the admin deleted from the workspace. The report keeps its own copy, with `deletable` set, so the value it -// already held is not lost. +// A field the admin deleted from the workspace. The report keeps its own copy, so the value it already held is not lost. const buildDeletedField = (): OnyxTypes.PolicyReportField => ({ ...buildTextField(1), name: 'DeletedField', @@ -559,6 +558,19 @@ describe('MoneyRequestViewReportFields', () => { expect(screen.queryByLabelText('workspace.reportFields.delete, Field1')).not.toBeOnTheScreen(); }); + it('shows a delete button on a deleted date field even when its copy on the report is not deletable', async () => { + // Given a report holding a date field the admin has since deleted from the workspace, whose copy on the report + // is flagged as not deletable + await renderReportFields(1, [], false, [ + {...buildDeletedField(), name: 'DeletedDateField', fieldID: 'deletedDateField', type: CONST.REPORT_FIELD_TYPES.DATE, value: '2026-10-01', deletable: false}, + ]); + + // When the fields are rendered + // Then it still carries a delete button, because the field is gone from the workspace and there is no other + // way to clear its stale value off the report + expect(screen.getByLabelText('workspace.reportFields.delete, DeletedDateField')).toBeOnTheScreen(); + }); + it('deletes the field from the report once the delete is confirmed', async () => { // Given a report holding a field that was deleted from the workspace await renderReportFields(1, [], false, [buildDeletedField()]); From 23f9ff3a64b8f7cc24c33885b5fb4ec8868cfee2 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Thu, 8 Oct 2026 04:00:16 +0000 Subject: [PATCH 13/16] Allow saving an empty value in a required inline text report field Co-authored-by: daledah --- .../ReportFieldInlineInput.tsx | 16 ++++----- tests/ui/MoneyRequestViewReportFieldsTest.tsx | 35 ++++++++++++++----- 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx index 7760e75040c1..89e59d035b89 100644 --- a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx +++ b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx @@ -96,11 +96,14 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error setDraftValue(value); setValidationError(''); setLastSavedValue(value); - setHasBeenBlurred(false); + // When the new value is the one this input just saved, the user has already left the field, so a violation + // the save caused, such as "Field is required" after clearing a required field, stays visible. + if (value !== lastSavedValue) { + setHasBeenBlurred(false); + } } const label = Str.UCFirst(reportField.name); - const isRequired = !reportField.deletable; const isReadOnly = isDisabled || reportField.type === CONST.REPORT_FIELD_TYPES.FORMULA; const violationError = hasBeenBlurred ? errorText : undefined; @@ -118,12 +121,9 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error } if (trimmedValue === '') { - if (isRequired) { - setValidationError(translate('common.error.fieldRequired')); - return; - } - // Clearing an optional field is a real change, so the empty value is saved. Any whitespace the user - // left behind is normalized away first so the stored value is empty rather than blank. + // Clearing a field is a real change, so the empty value is saved even when the field is required. Like + // Classic, a required field left empty is flagged by its violation rather than blocked from saving. Any + // whitespace the user left behind is normalized away first so the stored value is empty rather than blank. setValidationError(''); setDraftValue(''); setLastSavedValue(''); diff --git a/tests/ui/MoneyRequestViewReportFieldsTest.tsx b/tests/ui/MoneyRequestViewReportFieldsTest.tsx index 2e3ece9caad2..56342f413046 100644 --- a/tests/ui/MoneyRequestViewReportFieldsTest.tsx +++ b/tests/ui/MoneyRequestViewReportFieldsTest.tsx @@ -105,7 +105,7 @@ const buildEmptyField = (): OnyxTypes.PolicyReportField => ({ value: '', }); -// A field that cannot be deleted is the one field that must always hold a value, so clearing it is rejected. +// A field that cannot be deleted must hold a value. Leaving it empty raises a " is required" violation. const buildRequiredField = (): OnyxTypes.PolicyReportField => ({ ...buildTextField(1), name: 'RequiredField', @@ -317,9 +317,9 @@ describe('MoneyRequestViewReportFields', () => { }); }); - it('rejects clearing a required field instead of saving it', async () => { - // Given a report holding a required field, which the editor page used to guard with its own form validation - await renderReportFields(1, [buildRequiredField()]); + it('saves the empty value when a required field is cleared and keeps its violation visible', async () => { + // Given a report holding a required field + const rendered = await renderReportFields(1, [buildRequiredField()]); // When the field is emptied and left const input = screen.getByLabelText('RequiredField'); @@ -327,10 +327,29 @@ describe('MoneyRequestViewReportFields', () => { fireEvent(input, 'blur'); await waitForBatchedUpdatesWithAct(); - // Then nothing is saved and the error is shown instead, so moving the editing inline did not drop the - // validation that the editor page used to apply before it would submit - expect(updateReportField).not.toHaveBeenCalled(); - expect(screen.getByText('common.error.fieldRequired')).toBeOnTheScreen(); + // Then the empty value is saved, because like Classic a required field left empty is flagged by a violation + // rather than blocked from saving + expect(updateReportField).toHaveBeenCalledTimes(1); + expect(jest.mocked(updateReportField).mock.calls.at(0)?.at(0)).toMatchObject({ + reportField: {fieldID: 'requiredField', value: ''}, + previousReportField: {fieldID: 'requiredField', value: 'Value1'}, + }); + expect(screen.queryByText('common.error.fieldRequired')).toBeNull(); + + // When the report comes back holding the empty value the input just saved + rendered.rerender( + + + , + ); + await waitForBatchedUpdatesWithAct(); + + // Then the required violation shows right away, because the user already left the field and the save is what + // emptied it, so waiting for a second blur would hide the problem the save just created + expect(screen.getByText('RequiredField is required')).toBeOnTheScreen(); }); it('does not save when the option a list field already holds is picked again', async () => { From e0e41a4702e36a4103b984e40a763f7350c79642 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Thu, 8 Oct 2026 08:31:57 +0000 Subject: [PATCH 14/16] Move compact report field row style to per-item itemStyle after main removed listItemWrapperStyle Co-authored-by: daledah --- src/pages/EditReportFieldDropdown.tsx | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/pages/EditReportFieldDropdown.tsx b/src/pages/EditReportFieldDropdown.tsx index 44178633b8bf..51c8b813737e 100644 --- a/src/pages/EditReportFieldDropdown.tsx +++ b/src/pages/EditReportFieldDropdown.tsx @@ -66,7 +66,7 @@ function EditReportFieldDropdown({ // The live value still drives the checkmark, so tapping a row marks it without reordering the list. The reorder happens only on reopen. const initialFieldValue = useInitialSelection(fieldValue, {resetOnFocus: true}); - const sections = getReportFieldOptionsSection({ + const optionSections = getReportFieldOptionsSection({ searchValue: debouncedSearchValue, // Live value drives the checkmark, so tapping a row marks it immediately. selectedOptions: [ @@ -85,6 +85,13 @@ function EditReportFieldDropdown({ shouldShowSectionTitles, }); + // Same override the Spend single-select dropdowns use to shorten the default 64px option row. It also makes the rows + // match the 52px `getSelectionListPopoverHeight` already assumes, so the popover stops being sized for less content + // than it holds. + const sections = shouldUseCompactRows + ? optionSections.map((section) => ({...section, data: section.data.map((option) => ({...option, itemStyle: styles.optionRowCompactMinHeight}))})) + : optionSections; + const policyReportFieldData = sections.at(0)?.data ?? []; const textInputOptions = { @@ -100,10 +107,6 @@ function EditReportFieldDropdown({ Date: Thu, 8 Oct 2026 08:39:21 +0000 Subject: [PATCH 15/16] Show report field violations on open and stop pinning the label of empty inline fields Co-authored-by: daledah --- .../ReportFieldInlineInput.tsx | 30 +++---------------- tests/ui/MoneyRequestViewReportFieldsTest.tsx | 27 +++++++---------- 2 files changed, 14 insertions(+), 43 deletions(-) diff --git a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx index 89e59d035b89..fdfbe35818c8 100644 --- a/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx +++ b/src/components/MoneyRequestReportView/ReportFieldInlineInput.tsx @@ -75,11 +75,6 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error const [previousValue, setPreviousValue] = useState(value); const [validationError, setValidationError] = useState(''); - // A field violation such as "Field is required" is derived from the value being empty, so it is already true when - // the report opens. Holding it back until the user has left the field keeps a report full of empty fields from - // opening as a wall of red. - const [hasBeenBlurred, setHasBeenBlurred] = useState(false); - // Tracks the value that was last sent to the server so a second save attempt for the same value, for example when // the input is blurred right after it was submitted, is skipped while the update is still in flight. const [lastSavedValue, setLastSavedValue] = useState(value); @@ -96,16 +91,10 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error setDraftValue(value); setValidationError(''); setLastSavedValue(value); - // When the new value is the one this input just saved, the user has already left the field, so a violation - // the save caused, such as "Field is required" after clearing a required field, stays visible. - if (value !== lastSavedValue) { - setHasBeenBlurred(false); - } } const label = Str.UCFirst(reportField.name); const isReadOnly = isDisabled || reportField.type === CONST.REPORT_FIELD_TYPES.FORMULA; - const violationError = hasBeenBlurred ? errorText : undefined; const saveDraftValue = () => { const valueToSave = StringUtils.lineBreaksToSpaces(draftValue); @@ -186,11 +175,10 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error label={label} accessibilityLabel={label} value={value} - errorText={violationError} + errorText={errorText} minDate={CONST.CALENDAR_PICKER.MIN_DATE} maxDate={CONST.CALENDAR_PICKER.MAX_DATE} onInputChange={(selectedDate) => saveSelectedOption(selectedDate)} - onBlur={() => setHasBeenBlurred(true)} // The grid puts its own gap between the fields, so the default vertical margin would push a date // input below the plain text inputs sharing its row. wrapperStyle={styles.mv0} @@ -273,23 +261,19 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error role={CONST.ROLE.COMBOBOX} accessibilityState={{expanded: isExpanded}} value={value} - errorText={violationError} + errorText={errorText} inputStyle={styles.pointerEventsNone} icon={icons.DownArrow} // `pr0` makes the icon container's padding asymmetric, so it has to flip in place. // `flipUpsideDown` would rotate that padding too and shift the caret left when open. iconContainerStyle={[styles.pr0, isExpanded && styles.flipUpsideDownInPlace]} onPress={openOptions} - onBlur={() => setHasBeenBlurred(true)} onSubmitEditing={onPress} disableKeyboard // The field is focused so it stays keyboard reachable, but it holds a value picked from a // list rather than typed text, so the caret would be a caret you cannot type into. // `disableKeyboard` only suppresses the software keyboard, it does not hide the caret. caretHidden - // Keeps the label raised like the date field's, so a value that goes empty and back while - // the next report loads does not drop the label into the field and animate it back up. - forceActiveLabel textInputContainerStyles={textInputContainerStyles} /> @@ -306,17 +290,11 @@ function ReportFieldInlineInput({reportField, fieldKey, value, isDisabled, error accessibilityLabel={label} role={CONST.ROLE.PRESENTATION} value={isReadOnly ? value : draftValue} - errorText={validationError || violationError} + errorText={validationError || errorText} disabled={isReadOnly} onChangeText={setDraftValue} - onBlur={() => { - setHasBeenBlurred(true); - saveDraftValue(); - }} + onBlur={saveDraftValue} onSubmitEditing={saveDraftValue} - // Keeps the label raised like the date field's, so a value that goes empty and back while the next report - // loads does not drop the label into the field and animate it back up. - forceActiveLabel textInputContainerStyles={textInputContainerStyles} />, ); diff --git a/tests/ui/MoneyRequestViewReportFieldsTest.tsx b/tests/ui/MoneyRequestViewReportFieldsTest.tsx index 56342f413046..bd7cd54540ec 100644 --- a/tests/ui/MoneyRequestViewReportFieldsTest.tsx +++ b/tests/ui/MoneyRequestViewReportFieldsTest.tsx @@ -347,8 +347,7 @@ describe('MoneyRequestViewReportFields', () => { ); await waitForBatchedUpdatesWithAct(); - // Then the required violation shows right away, because the user already left the field and the save is what - // emptied it, so waiting for a second blur would hide the problem the save just created + // Then the required violation shows, so the empty value the save created is flagged instead of blocked expect(screen.getByText('RequiredField is required')).toBeOnTheScreen(); }); @@ -410,19 +409,13 @@ describe('MoneyRequestViewReportFields', () => { expect(screen.getAllByTestId('reportFieldsRow').at(0)).toHaveStyle({alignItems: 'flex-start'}); }); - it('holds back a field violation until the field has been left', async () => { + it('shows a field violation as soon as the report opens', async () => { // Given a required field that is already empty when the report opens, so its violation is true from the start await renderReportFields(1, [buildEmptyField()]); - // Then the error is not shown yet, because a report full of empty required fields would otherwise open as a - // wall of red before the user has touched anything - expect(screen.queryByText('EmptyField is required')).toBeNull(); - - // When the user focuses the field and leaves it without filling it in - fireEvent(screen.getByLabelText('EmptyField'), 'blur'); - await waitForBatchedUpdatesWithAct(); - - // Then the error appears, because by now the user has had their chance to fill it in + // When the report is shown, before the user has touched the field + // Then the error is already visible, like the report field rows before inline editing, so a report with empty + // required fields tells the user what still needs filling in expect(screen.getByText('EmptyField is required')).toBeOnTheScreen(); }); @@ -508,7 +501,7 @@ describe('MoneyRequestViewReportFields', () => { expect(expandedStyles).not.toEqual(expect.arrayContaining([expect.objectContaining({transform: [{rotate: '180deg'}]})])); }); - it('keeps the label of text and list fields raised so it does not jump while the next report loads', async () => { + it('does not pin the label of text and list fields in the raised position', async () => { // Given a report holding both a text field and a list field await renderReportFields(1, [buildListField()]); @@ -518,10 +511,10 @@ describe('MoneyRequestViewReportFields', () => { raisedLabels.push(node.props.label); } - // Then both labels are pinned in the raised position like the date field's. The next/previous arrows keep - // these inputs mounted, and the next report's value can briefly resolve to empty before it loads, so an - // unpinned label would drop into the field and animate back up - expect(raisedLabels).toEqual(expect.arrayContaining(['Field1', 'ListField'])); + // Then neither label is pinned, so an empty field shows its label inside the input like a placeholder and + // reads as empty instead of looking like a filled field + expect(raisedLabels).not.toEqual(expect.arrayContaining(['Field1'])); + expect(raisedLabels).not.toEqual(expect.arrayContaining(['ListField'])); }); it('does not carry an unsaved edit into the next report', async () => { From b81a593302c104439b4f30e5019a02ac086fe678 Mon Sep 17 00:00:00 2001 From: "daledah (via MelvinBot)" Date: Thu, 8 Oct 2026 09:26:53 +0000 Subject: [PATCH 16/16] Remove useMemo around the report field list in MoneyReportView Co-authored-by: daledah --- src/components/ReportActionItem/MoneyReportView.tsx | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/components/ReportActionItem/MoneyReportView.tsx b/src/components/ReportActionItem/MoneyReportView.tsx index 2013e0266a15..bfd03faed323 100644 --- a/src/components/ReportActionItem/MoneyReportView.tsx +++ b/src/components/ReportActionItem/MoneyReportView.tsx @@ -124,12 +124,10 @@ function MoneyReportView({ // Only used to decide whether the report field block is worth rendering. // `MoneyRequestViewReportFields` builds and resolves the fields it displays itself. - const sortedPolicyReportFields = useMemo(() => { - const {fieldsByName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs); - return Object.values(fieldsByName) - .filter((field) => isReportFieldTargetMatchingReport(report, field)) - .sort(({orderWeight: a}, {orderWeight: b}) => a - b); - }, [policy?.fieldList, report, reportNameValuePairs]); + const {fieldsByName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs); + const sortedPolicyReportFields = Object.values(fieldsByName) + .filter((field) => isReportFieldTargetMatchingReport(report, field)) + .sort(({orderWeight: a}, {orderWeight: b}) => a - b); const isOnlyTitleFieldEnabled = sortedPolicyReportFields.every(shouldHideSingleReportField); const isClosedExpenseReportWithNoExpenses = isClosedExpenseReportWithNoExpensesReportUtils(report);