Skip to content

Remove REPORT_ACTIONS Onyx reference from hasReportBeenForwardedSinceLastSubmit - 3 - #99817

Merged
mollfpr merged 12 commits into
Expensify:mainfrom
cretadn22:refactor/66419-hasReportBeenForwardedSinceLastSubmit-3
Sep 16, 2026
Merged

mollfpr merged 12 commits into
Expensify:mainfrom
cretadn22:refactor/66419-hasReportBeenForwardedSinceLastSubmit-3

Conversation

@cretadn22

@cretadn22 cretadn22 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Fixed Issues

$ #66419
PROPOSAL: #66419

Tests

  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  1. Create several expenses within the same report
  2. Try editing an expense inline
  3. Select multiple expenses or reports and attempt a bulk edit
  4. Verify that all actions execute successfully
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-09-04.at.13.15.37.mov

@cretadn22
cretadn22 requested review from a team as code owners August 29, 2026 07:04
@melvin-bot
melvin-bot Bot requested review from garrettmknight and situchan and removed request for a team and garrettmknight August 29, 2026 07:04
@melvin-bot

melvin-bot Bot commented Aug 29, 2026

Copy link
Copy Markdown

@situchan Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@melvin-bot
melvin-bot Bot removed the request for review from a team August 29, 2026 07:04
@codecov

codecov Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
...nents/ReportActionItem/MoneyRequestReceiptView.tsx 71.87% <ø> (ø)
...c/components/ReportActionItem/MoneyRequestView.tsx 82.06% <ø> (ø)
src/hooks/useSelectedTransactionsActions.ts 78.29% <100.00%> (+0.08%) ⬆️
src/libs/ReportUtils.ts 87.19% <100.00%> (+<0.01%) ⬆️
src/libs/actions/IOU/BulkEdit.ts 83.04% <100.00%> (ø)
src/libs/actions/TransactionInlineEdit.ts 93.90% <ø> (ø)
...arch/SearchEditMultiple/SearchEditMultipleUtils.ts 55.69% <100.00%> (+11.39%) ⬆️
...arch/SearchEditMultiple/SearchEditMultiplePage.tsx 0.00% <0.00%> (ø)
... and 11 files with indirect coverage changes

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 890d9b9841

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/libs/ReportUtils.ts Outdated
…or policy, report, and transaction initialization

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd8a30e9f8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/hooks/useSelectedTransactionsActions.ts Outdated
@MelvinBot

Copy link
Copy Markdown
Contributor

Review

One change to make before merge, in src/libs/ReportUtils.ts:5345. Everything else looks correct — the other eight call sites all pass the report actions belonging to the same report the IOU action lives on, which is what hasReportBeenForwardedSinceLastSubmit needs.

canEditMultipleTransactions doesn't actually get migrated

reportActions: reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report?.reportID}`],

Two lines above, ReportUtils.ts:5313-5315 already builds the correct value and uses it to resolve reportAction:

const actionsForReport = {...(searchSnapshotData?.[reportActionsKey] ?? {}), ...(reportActions?.[reportActionsKey] ?? {})};

The new argument drops the searchSnapshotData half. The Search caller — useSearchBulkActions.ts:2159 — passes searchResults?.data precisely because report actions for reports the user hasn't opened live only in the snapshot. There, reportActions?.[key] is undefined, so hasReportBeenForwardedSinceLastSubmit silently falls back to the module-level allReportActions this refactor exists to remove. It also keys off report?.reportID rather than the reportActionsKey already built from transaction.reportID, so it resolves to reportActions_undefined whenever the report isn't in reports or the snapshot.

Suggested fix:

reportActions: actionsForReport,

SearchEditMultiplePage.tsx:108 already does this correctly with mergedReportActions, so this is just bringing the two Search paths in line. A test for the snapshot path would be worth adding alongside the new reportActions describe block, since that's the case the current code misses.

Everything else I checked

Call sites are correctly paired. canEditMoneyRequest resolves moneyRequestReport as report ?? getReportOrDraftReport(reportAction.reportID), so the actions passed must be that report's. Verified each:

  • MoneyRequestView / MoneyRequestReceiptView / TransactionInlineEditparentReportActions is useOnyx(REPORT_ACTIONS + parentReportID) and parentReportAction is looked up from it. Matches. MoneyRequestView already passed it to canEditMoneyRequest at line 393, so this is consistent with the existing pattern.
  • useExpenseActions — the variable naming reads oddly (reportAction comes from reportActions, but moneyRequestReportActions is passed). It's correct: reportActions there is the transaction-thread hook's value, and moneyRequestReportActions is useOnyx(REPORT_ACTIONS + reportID) for the expense report. That's the right one.
  • useSelectedTransactionsActions / ReportSecondaryActionUtils:1133 — both are report-scoped (only used from SelectionToolbar and MoneyReportHeaderSelectionDropdown), so all selected transactions belong to the report whose actions are passed.
  • BulkEdit.ts:212 — correctly omits the ?? {} that line 150 uses. An empty object would defeat the ?? fallback in hasReportBeenForwardedSinceLastSubmit; leaving it undefined preserves it. Worth a comment so nobody "fixes" it later.

Array overload. Object.values() handles both branches, and the union type is narrow enough that an OnyxCollection<ReportActions> won't type-check through by mistake. The union now appears in three signatures — a named alias would be tidier, but that's a nit.

Still un-migrated (fine for a -3 PR, just noting the remaining surface): selectionBuilders.ts:80, ReportSecondaryActionUtils.ts:1249 (needs a new param threaded through getSecondaryTransactionThreadActions), useShouldAddOrReplaceReceipt.ts:39, and TransactionReceiptModalContent.tsx:166-167.

I couldn't run the Jest suites in this environment, so CI is the check on those.

Smoke test — web

Pass. Amount, merchant and date stayed editable for the submitter on a submitted, not-forwarded report, and the report action menu rendered cleanly.

Step Status Observation
Amount editable for submitter on non-forwarded report pass Opened the Amount edit screen with the field pre-filled at 1500.00
Merchant editable for submitter on non-forwarded report pass Opened the Merchant edit screen pre-filled with the test merchant name
Date editable for submitter on non-forwarded report pass Opened the Date edit screen pre-filled with 2026-08-31
Report is in the not-forwarded state pass Banner read "Outstanding — Waiting for you to approve expenses" right after submit, so still at the first approver
Expense More menu renders without error pass All 15 actions listed, no error text or blank dialog
No JS console errors pass No console capture exists for web in this agent-device build, so this wasn't asserted programmatically. No error banners, blank screens or crash UI appeared anywhere in the flow
Evidence (3)

Amount edit screen opens from a submitted, not-forwarded expense report

Amount edit screen opens from a submitted, not-forwarded expense report

Date edit screen opens from a submitted, not-forwarded expense report

Date edit screen opens from a submitted, not-forwarded expense report

Report More menu renders the full action list with no errors

Report More menu renders the full action list with no errors

view run

Comment thread src/libs/actions/IOU/BulkEdit.ts Outdated
Comment thread src/pages/Search/SearchEditMultiple/SearchEditMultiplePage.tsx Outdated
Comment thread src/libs/ReportUtils.ts Outdated
@MelvinBot

Copy link
Copy Markdown
Contributor

Regressions

One real regression, at the two call sites that pass a ReportAction[] instead of the Onyx object:

Both arrays are a paginated, display-filtered window of the report's actions. The fallback they replace read the complete Onyx entry. Where the window omits the FORWARDED action, hasReportBeenForwardedSinceLastSubmit now returns false, so a corporate-policy submitter keeps edit and "Move expense" permission on a report that was already forwarded past its first approver. It fails silently and in the permissive direction.

The chain. Both trace back to getFilteredReportActionsForReportView(usePaginatedReportActions(...))MoneyRequestReportActionsList.tsx:141-142 feeding SelectionToolbar, and useTransactionThreadReportID.ts:32 feeding MoneyReportHeaderSelectionDropdown and MoneyReportHeaderSecondaryActions. usePaginatedReportActions returns getContinuousChain(...), which returns [] when a reportActionID deep-link anchor isn't in Onyx (PaginationUtils.ts:409-411) and a page slice once REPORT_ACTIONS_PAGES is populated. MoneyRequestReportActionsList.tsx:141 passes route?.params?.reportActionID, so the deep-link path is live.

Then at ReportUtils.ts:2071:

const reportActionsArray = Object.values(reportActions ?? allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`] ?? {});

?? only falls back on null/undefined. A truthy [] — or a partial page — short-circuits the complete Onyx read that allReportActions holds (ReportUtils.ts:1181-1190, an unpaginated, unfiltered connect).

Fix. Pass the full Onyx entry at both sites rather than the render array — useExpenseActions.ts:115 already does exactly this with useOnyx(REPORT_ACTIONS + reportID), and that call site is correct as a result. Guarding on a non-empty array would only patch the [] case, not the paginated-window one.

Also worth noting: the new assertion at useSelectedTransactionsActions.test.ts:882 (expect(lastCall.reportActions).toBe(reportActions)) asserts the array is forwarded, so it passes with the bug in place. It locks in plumbing, not correctness.

Call sites I checked and cleared

canEditMoneyRequest resolves moneyRequestReport as report ?? getReportOrDraftReport(reportAction.reportID), so the actions passed have to be that report's complete set. Comparing each against what the old fallback would have read:

  • MoneyRequestView.tsx, MoneyRequestReceiptView.tsx, TransactionInlineEdit.tsparentReportActions is useOnyx(REPORT_ACTIONS + parentReportID), the complete object, same key the fallback used. No change in behavior. MoneyRequestView already passed it to canEditMoneyRequest at line 393 before this PR.
  • useExpenseActions.ts:215moneyRequestReportActions is the complete Onyx object for the expense report. The naming is confusing (reportAction is resolved from a different reportActions variable), but the value passed is the right one.
  • BulkEdit.ts:212 and SearchEditMultiplePage.tsx:108withSnapshotReportActions merges per-action ({...snapshot, ...onyx}), so the result is a superset of what the fallback read. Strictly better, not a regression. BulkEdit.ts:212 also correctly omits the ?? {} that line 150 uses — {} would defeat the ?? fallback. Worth a comment so nobody "fixes" it later.
  • ReportUtils.ts:5345 (canEditMultipleTransactions) — not a regression. reportActions there is the same Onyx collection the fallback reads, and the key resolves the same way (or to reportActions_undefined, which falls back). But as flagged in my earlier review, it should use the actionsForReport merged two lines up, otherwise the Search bulk-edit path isn't actually migrated.

getFilteredReportActionsForReportView itself is not the lossy step — isVisiblePreviewOrMoneyRequest only drops deleted money requests and hidden report previews, and getSortedReportActionsForDisplay is called with shouldIncludeInvisibleActions = true. SUBMITTED and FORWARDED survive both. The loss is pagination.

Smoke test — web

Pass, but it could not reach the affected state. The session is a single user who is both submitter and self-approver, so the report is never forwarded past a first approver and hasReportBeenForwardedSinceLastSubmit never evaluates true. Confirming the regression needs a two-approver workflow.

Step Status Observation
Selection toolbar offers Move to report with 2+ expenses selected on open report pass Dropdown listed Edit multiple, Hold, Export, Merge, Move to report, Duplicate expenses, Delete — all enabled
Bulk-edit options offered and not greyed out pass "Edit multiple expenses" panel opened with Amount, Description, Merchant, Date, Category, Reimbursable all enabled
Report header More menu on single-expense report lists a move option pass Listed as "Move to report" (current wording), enabled
Search page multi-select offers the same options pass Identical set to the report view, all enabled
Repeat the above after submitting the report pass Same options plus Retract, Reject, Change approver; Move to report still enabled
Evidence (3)

Selection toolbar with 2 expenses selected on an open report — Move to report and Edit multiple offered

Selection toolbar with 2 expenses selected on an open report — Move to report and Edit multiple offered

Search page with 2 expenses selected — same options offered

Search page with 2 expenses selected — same options offered

More menu on the submitted single-expense report still listing Move to report

More menu on the submitted single-expense report still listing Move to report

view run

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 5ec460c8d2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@MelvinBot

Copy link
Copy Markdown
Contributor

Review

No blocking bugs. The canEditMultipleTransactions issue from my last pass is fixed — it now passes actionsForReport (snapshot + Onyx merged), and canEditMultipleTransactionsTest.ts covers the snapshot-only path. All nine migrated call sites pass the actions of the report canEditMoneyRequest resolves, and every one of them is a superset of what the module-level fallback would have read, so there's no behavior change.

One thing I'd change before merge.

The ReportAction[] overload has no production caller and invites a wrong migration

ReportUtils.ts:2096 and ReportUtils.ts:5455 widen the param to OnyxEntry<ReportActions> | ReportAction[], but nothing in src/ passes an array — only the two new 'should accept the report actions as an array' tests.

That matters because of which arrays are in scope at the call sites still to be migrated. getSecondaryReportActions, useExpenseActions, and this PR's own useSelectedTransactionsActions all hold a ReportAction[] that traces back to usePaginatedReportActions + getFilteredReportActionsForReportView. Those are view arrays — pagination means they can be missing the SUBMITTED/FORWARDED actions this check exists to find. getSecondaryReportActions already guards against exactly this at ReportSecondaryActionUtils.ts:540.

So the overload makes reportActions: reportActions type-check at precisely the places where it silently returns false and lets a submitter edit a forwarded report. I'd drop the union until a caller actually needs it, or keep it and say in the doc comment that the value must be the report's complete action set, not a paginated view array.

Related nit, same root cause

useSelectedTransactionsActions.ts:461 deliberately re-derives the report ID and reads the full Onyx collection instead of reusing the reportActions prop one line above. That's the right call for the reason above, but nothing says so — and after this PR the "simplification" compiles. A one-line comment would protect it.

The same line also re-implements the reportID ?? originalMessage.IOUReportID resolution from canEditMoneyRequest without the #93882 comment that explains it. If those two ever diverge, the caller passes a different report's actions than the callee resolves — a wrong answer, not a fallback. Worth a pointer back to the original.

Everything else I checked

Call-site pairing. canEditMoneyRequest resolves moneyRequestReport = report ?? getReportOrDraftReport(reportAction.reportID), so the actions passed must belong to that report. Verified each:

  • MoneyRequestView / MoneyRequestReceiptView / TransactionInlineEditparentReportActions is useOnyx(REPORT_ACTIONS + parentReportID), and moneyRequestReport is parentReport. Same key the module fallback would have used, so identical.
  • canEditMultipleTransactions / BulkEdit / getTransactionEditContext — all key off transaction.reportID, and all three merge snapshot data (or receive it already merged), making them supersets of the module-level collection. No path where the caller-supplied set is narrower than the fallback.
  • useSelectedTransactionsActions — derives the same ID the callee does, from the same Onyx collection. Equal.

Empty-object semantics. Object.values(reportActions ?? allReportActions?.[...] ?? {}) — an empty {} is truthy, so the ?? {} in getTransactionEditContext and BulkEdit:227 permanently disables the Onyx fallback for those paths. Benign here since both collections are supersets of Onyx, but it means the fallback is already dead on the Search paths, not just deprecated.

Search snapshot gap in useSelectedTransactionsActions. It reads raw allReportActions while the sibling Search paths merge searchResults.data. For a report the user hasn't opened, both the new code and the old fallback see nothing, so this is pre-existing, not a regression — but it's the one migrated call site that doesn't get the snapshot treatment.

Tests. The new reportActions describe block in canEditFieldOfMoneyRequestTest.ts is well-built: the policy's submitsTo resolves to report.managerID, so isForwarded collapses to hasReportBeenForwardedSinceLastSubmit alone and the assertions really do exercise it. 'should read the passed reportActions rather than the report actions stored in Onyx' is the right test to have. The transactionIDIOUTransactionID fixture fix in useSelectedTransactionsActions.test.ts is correct — getIOUActionForTransactionID reads IOUTransactionID.

Still un-migrated (fine for a -3 PR, listing what #66419 has left): selectionBuilders.ts:80, ReportSecondaryActionUtils.ts:1157 and :1272, useExpenseActions.ts:213, TransactionReceiptModalContent.tsx:166-167, and the internal isAwaitingFirstLevelApproval, which still calls hasReportBeenForwardedSinceLastSubmit(report) with no actions.

Verification limits. I reviewed statically. Browser verification wasn't available in this run, and I couldn't execute the Jest suites locally — CI is green on 5ec460c8 for typecheck, ESLint, all 8 test shards, and React Compiler compliance, so the automated coverage is accounted for. The manual QA steps in the PR description (inline edit, bulk edit across reports) are still worth running by hand, particularly the corporate-workspace submitter case after a forward.


view run

@MelvinBot

Copy link
Copy Markdown
Contributor

⚠️ Claude's API is currently overloaded (HTTP 529). This is a temporary issue on Anthropic's side. Try again in a few minutes, or check status.anthropic.com.

Run details

@cretadn22

Copy link
Copy Markdown
Contributor Author

@situchan This is ready

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 5ec460c8d2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@MelvinBot

Copy link
Copy Markdown
Contributor

Review

No blocking bugs. Head is still 5ec460c8 — unchanged since my last pass — so this is a re-verification plus a deeper look at the two things I flagged. One of them I'd now soften, one I'd now sharpen.

Keep the ReportAction[] union — but document that it must be the complete action set

Last time I suggested dropping the union at ReportUtils.ts:2096 / :5455. That was wrong on the type: OnyxEntry<ReportActions> | ReportAction[] is the established convention in this file — isExported, hasExportError, hasReportBeenReopened, hasReportBeenRetracted, and canSubmitAndIsAwaitingForCurrentUser all take it. Matching them is right.

What I got right is the hazard, and it's now better evidenced. isExported has a real production array caller — useExportActions.ts:58-59 feeds it getFilteredReportActionsForReportView(usePaginatedReportActions(...)). That is exactly the kind of array that is in scope at the call sites #66419 still has to migrate (getSecondaryReportActions, useExpenseActions, this PR's own useSelectedTransactionsActions), and it is a paginated, display-filtered window, not the report's full action set.

The two functions tolerate that differently. A missing export action makes isExported render one fewer menu entry. A missing FORWARDED action makes hasReportBeenForwardedSinceLastSubmit return false, which is a permission gate failing open — a corporate-policy submitter keeps edit and "Move expense" rights on a report that was already forwarded past its first approver.

So: keep the union, and extend the doc comment at ReportUtils.ts:5454 to say the value must be the report's complete action set, never a paginated view array. Right now nothing tells the next migration that, and the type checks either way.

Two small consistency nits

  • ReportUtils.ts:2101 uses Object.values(reportActions ?? …) to normalize. It works on arrays, but the sibling helpers use Array.isArray(reportActions) ? reportActions : Object.values(reportActions) (:13874). Worth matching so the array support is obvious at a glance.
  • useSelectedTransactionsActions.ts:461 deliberately re-derives the report ID and reads the full Onyx collection instead of reusing the reportActions prop one line above, and it re-implements the reportID ?? originalMessage.IOUReportID resolution from canEditMoneyRequest without the #93882 comment that explains it. Both are the right calls; neither says so. A one-line comment protects them from a future "simplification" that now compiles.
Everything I verified this pass

All nine migrated call sites pair correctly, and none is narrower than the fallback they replace. canEditMoneyRequest resolves moneyRequestReport = report ?? getReportOrDraftReport(reportAction.reportID ?? originalMessage.IOUReportID), so the actions passed must belong to that report:

  • MoneyRequestView / MoneyRequestReceiptViewparentReportActions is useOnyx(REPORT_ACTIONS + parentReportID) and moneyRequestReport is parentReport. Same key the module fallback used.
  • TransactionInlineEdit — actions keyed on effectiveParentReportID, report is effectiveParentReport. Consistent for both the reported and unreported (self-DM) branches.
  • canEditMultipleTransactions / BulkEdit / getTransactionEditContext — all key off transaction.reportID and all three see snapshot ∪ Onyx, so they are supersets.
  • useSelectedTransactionsActions — derives the same ID the callee does, from the same collection. Equal.

Search-snapshot behaviour — the thing I most wanted to rule out. App's useOnyx does not merge snapshot data, it replaces the value with the snapshot slice when rendering inside SearchScopeProvider, and reportActions_ is a snapshot key. A migrated call site inside that scope would therefore hand hasReportBeenForwardedSinceLastSubmit a snapshot subset and fail open. It doesn't happen here: SearchScopeProvider only wraps the Search list and EmptySearchView, none of MoneyRequestView / MoneyRequestReceiptView / the bulk-edit pages render under it, and the one path that does run in search scope — useTransactionInlineEdit — already reads its actions with useOnyxWithoutSnapshots. Worth keeping in mind for the remaining migrations though.

Empty-object semantics. Object.values(reportActions ?? allReportActions?.[…] ?? {}){} and [] are both truthy, so any caller passing ?? {} permanently disables the Onyx fallback. getTransactionEditContext and BulkEdit:227 both do. Benign here because both receive withSnapshotReportActions(...) supersets, but it means the fallback is already dead on those paths, not merely deprecated.

Tests. The new reportActions block in canEditFieldOfMoneyRequestTest.ts is well built — the policy's submitsTo resolves to report.managerID, so isForwarded collapses to hasReportBeenForwardedSinceLastSubmit alone and the assertions genuinely exercise it. 'should read the passed reportActions rather than the report actions stored in Onyx' is the right test to have. canEditMultipleTransactionsTest.ts covers the snapshot-only path. The transactionIDIOUTransactionID fixture fix in useSelectedTransactionsActions.test.ts is correct.

Still un-migrated, fine for a -3 PR, listing what #66419 has left: selectionBuilders.ts:80, ReportSecondaryActionUtils.ts:1157, useExpenseActions.ts:213, TransactionReceiptModalContent.tsx:166-167, useShouldAddOrReplaceReceipt.ts:39, and the internal isAwaitingFirstLevelApproval, which still calls hasReportBeenForwardedSinceLastSubmit(report) with no actions at all.

CI. Green on 5ec460c8 — typecheck, ESLint, React Compiler compliance, all 8 test shards, perf tests. The only red check is Check independent approval, which is a process gate, not a code failure.

Verification I could not complete

I tried to run the PR's QA steps on web and could not: after CI's sign-in the dev build stayed on a blank page (0 accessibility nodes, unresponsive to waits and clicks) and never reached Home, so I never got to a report or the Search page. The dev server itself built fine, and I have no console capture on web to say what happened. I am not attributing this to the PR — the session stalled at onboarding, well before any code this PR touches, and CI is green on the UI test suites. But it does mean the manual steps are still unverified by me.

The case most worth a human running by hand is the one the tests cover only synthetically: on a corporate workspace, submit a report, have the first approver forward it, then confirm the submitter can no longer edit amount / merchant / date — from the report view, from inline edit, and from a Search bulk edit.

@MelvinBot

Copy link
Copy Markdown
Contributor

Regression hunt

No permissive regressions. I traced all nine migrated call sites against what the module-level fallback used to read, and every one passes a set that is equal to or a superset of it — so nothing that was blocked before becomes editable now.

I did find one user-visible behavior change that isn't described anywhere in the PR, and one latent footgun.

Search bulk edit gets newly blocked on forwarded reports — intended, but undocumented and untested by QA

Three of the migrated sites feed the forwarded check the merged search snapshot ∪ Onyx action set, where before it only saw Onyx:

The snapshot really does carry FORWARDED actions — SearchUIUtils.ts:1941 scans snapshot report actions for exactly that — so this is live, not theoretical. Concretely: a corporate submitter who lands on Search after a hard refresh, whose report was forwarded past its first approver, previously saw "Edit multiple" and editable fields (Onyx was empty, so the check silently returned false). Now the option disappears.

That's the right outcome and the PR's own test covers it (canEditMultipleTransactionsTest.ts:269). But the PR's Explanation of Change is empty, the Tests section is empty, and the QA steps only say "attempt a bulk edit … verify all actions execute successfully" — which is the opposite of the case that changed. QA will not exercise this, and a support report of "Edit multiple disappeared on Search" would look like a regression to whoever triages it. Please describe it in the PR body and add a QA step for it.

Edge worth testing by hand while you're there: hasReportBeenForwardedSinceLastSubmit derives lastSubmittedAt from the same set it scans (ReportUtils.ts:2101). On a retract → resubmit report, if the snapshot's action set carries the FORWARDED from the first cycle but a missing or stale SUBMITTED, the comparison flips true and bulk edit is wrongly blocked. I could not confirm the snapshot's exact completeness from client code, so treat this as unverified rather than a known bug.

Latent footgun: ?? {} permanently kills the Onyx fallback

ReportUtils.ts:2101 falls back to the module collection with reportActions ?? allReportActions?.[…]. {} and [] are both truthy, so any caller that passes ?? {} opts out of the fallback entirely and gets a silent false. SearchEditMultipleUtils.ts:62, BulkEdit.ts:227 and ReportUtils.ts:5351 all do.

Harmless today — all three receive merged supersets, so {} only happens when Onyx is empty too and the fallback would have returned false anyway. Worth knowing that the fallback is already dead on those paths rather than merely deprecated, because the remaining #66419 migrations will copy this shape.

Every call site I checked, and why none of them regress

canEditMoneyRequest resolves moneyRequestReport = report ?? getReportOrDraftReport(reportAction.reportID ?? originalMessage.IOUReportID), so a regression requires the passed actions to belong to a different report, or to be a strict subset of allReportActions[thatReportID].

Site Passed value vs. old fallback
MoneyRequestView (7 fields + canEdit) useOnyx(REPORT_ACTIONS + parentReportID), report is parentReport Identical key
MoneyRequestReceiptView same, report is parentReport Identical key
TransactionInlineEdit useOnyxWithoutSnapshots(REPORT_ACTIONS + effectiveParentReportID), report is effectiveParentReport Identical key, both branches
canEditMultipleTransactions actionsForReport = snapshot ∪ Onyx Superset
BulkEdit transactionReportActions from withSnapshotReportActions(...) Superset
SearchEditMultiplePage reportActions from getTransactionEditContext(mergedReportActions, …) Superset
useSelectedTransactionsActions allReportActions[REPORT_ACTIONS + moneyRequestReportID] Same collection, same derived ID

Search-snapshot substitution — the main thing I tried to break it with. App's useOnyx replaces a value with the snapshot slice (it does not merge) when rendering inside SearchScopeProvider, and reportActions_ is a snapshot key. Any migrated site inside that scope would hand the check a subset and fail open. It doesn't happen: SearchScopeProvider only wraps the Search list in components/Search/index.tsx and EmptySearchView. useSelectedTransactionsActions is reached only from SelectionToolbar and MoneyReportHeaderSelectionDropdown, SearchEditMultiplePage is an RHP screen, and the one path that does run in search scope — useTransactionInlineEdit — already reads with useOnyxWithoutSnapshots. All get live Onyx.

No paginated view array reaches the check. useSelectedTransactionsActions holds a ReportAction[] prop from usePaginatedReportActions, and this PR deliberately does not pass it — :464 re-derives the report ID and reads the full collection instead. That's the correct choice (a paginated window can omit the FORWARDED action), and it's the reason there's no regression at the two sites I was most suspicious of.

Test changes don't mask anything. The useSelectedTransactionsActions.test.ts fixture fix (transactionIDIOUTransactionID) is required — getIOUActionForTransactionID reads IOUTransactionID, so the old fixture never resolved an action. The test mocks canEditFieldOfMoneyRequest and asserts only on arguments, so it can't hide a behavior change. MoneyRequestViewTest.tsx only adds merchant/date assertions to two existing cases.

CI is green on 5ec460c8 — typecheck, ESLint, React Compiler, all 8 shards, perf. Only Check independent approval is red, which is a process gate.

Browser verification

I couldn't run any of it. The prestarted web session's test account has no workspaces and no expenses at all (Workspaces → "No workspaces yet", Spend → Expenses → "No expenses yet"), so there was nothing to open, inline-edit, multi-select, or bulk-edit. The findings above are static only.

The scenario most worth running by hand is the one that actually changed: as a corporate submitter, submit a report, have the first approver forward it, hard-refresh onto Search, then check whether "Edit multiple" and the per-field edit affordances behave as you intend.

@situchan

Copy link
Copy Markdown
Contributor

Please pull main

@situchan

situchan commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Android: mWeb Chrome
iOS: HybridApp
ios.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov

@melvin-bot
melvin-bot Bot requested a review from mollfpr September 10, 2026 06:05
@mollfpr

mollfpr commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

@cretadn22 could you resolve the conflict? Thanks!

@cretadn22

Copy link
Copy Markdown
Contributor Author

@mollfpr Resolved all conflicts

@cretadn22

Copy link
Copy Markdown
Contributor Author

The failing test is existing from main and will be resolved via #101016

@cretadn22

Copy link
Copy Markdown
Contributor Author

@mollfpr Resolved all conflicts

@cretadn22

Copy link
Copy Markdown
Contributor Author

@mollfpr Does everything look good to go? If so, could we get this merged soon to prevent any conflicts?

@mollfpr
mollfpr merged commit 560c78a into Expensify:main Sep 16, 2026
34 of 35 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🚧 mollfpr has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/mollfpr in version: 9.4.81-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 No help site changes are required for this PR. I did not create a docs PR.

Why: this PR is an internal Onyx refactor with no user-facing change. It threads reportActions through canEditFieldOfMoneyRequestcanEditMoneyRequesthasReportBeenForwardedSinceLastSubmit as an explicit parameter instead of relying on the module-level Onyx.connect() cache, per Remove Onyx.connect() for the key: ONYXKEYS.COLLECTION.REPORT_ACTIONS in src/libs/ReportUtils.ts.

Nothing in the diff adds, removes, or renames a feature, setting, tab, button, or any user-facing copy, so no article under docs/articles can go stale because of it.

What I checked

Source changes (all signature/plumbing only):

  • src/libs/ReportUtils.ts:2063hasReportBeenForwardedSinceLastSubmit now accepts ReportAction[] in addition to OnyxEntry<ReportActions>, and still falls back to the Onyx cache when nothing is passed. Same result either way.
  • canEditMoneyRequest and canEditFieldOfMoneyRequest gained an optional reportActions param, documented in the diff as temporary until every caller is migrated.
  • Callers updated to pass the report actions they already hold: MoneyRequestView.tsx, MoneyRequestReceiptView.tsx, useExpenseActions.ts, useSelectedTransactionsActions.ts, ReportSecondaryActionUtils.ts, BulkEdit.ts, TransactionInlineEdit.ts, SearchEditMultiplePage.tsx, SearchEditMultipleUtils.ts.

Test changes: the added assertions in tests/ui/MoneyRequestViewTest.tsx cover merchant and date alongside amount for the existing "forwarded since last submit locks editing" rule. That rule already existed and already shipped — the tests just widen coverage of it, they don't introduce new behavior to document.

No copy changes: the diff touches no src/languages/* file and no UI string.

Docs scan: I checked docs/articles for content describing what a submitter can edit after a report is submitted or forwarded. Nothing there describes the per-field edit permissions this code governs, so there is no article to correct.

No docs PR was created, so there is nothing for you to review on the help site side. If you think a specific article should describe which expense fields stay editable after a report is forwarded, reply with @MelvinBot and the article you have in mind and I'll draft it.


view run

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.81-1 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 failure ❌
🍎 iOS 🍎 failure ❌

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.81-1 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 failure ❌
🍎 iOS 🍎 failure ❌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants