Skip to content

Move track-expense rows and the report guard out of Report Details page - #103319

Merged
mjasikowski merged 9 commits into
Expensify:mainfrom
callstack-internal:refactor/rd-08-track-guard
Oct 9, 2026
Merged

mjasikowski merged 9 commits into
Expensify:mainfrom
callstack-internal:refactor/rd-08-track-guard

Conversation

@sosek108

@sosek108 sosek108 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

This is the last PR of the Report Details page decomposition, PR 8 of 8.

The four track-expense rows and their 11 Onyx subscriptions move out of ReportDetailsMenuItems into a new ReportDetailsMenuTrackExpenseItems component. It only mounts for a track expense that has request data and a parent action that is not deleted. ReportDetailsMenuItems now keeps only the self DM / archived gate and the track-expense check. The LeaveItem part of the issue already landed in #102988.

The report-or-not-found decision moves out of withReportOrNotFound into ReportOrNotFoundGuard, which exports getReportOrNotFoundDecision and a guard component that takes children. The deeplink OpenReport effect moves into a useReportDeepLinkOnOpen hook. withReportOrNotFound calls both and keeps its own subscriptions, its contentShown ref and its prop forwarding, so the 20 other screens that use it do not change. The issue asks to move the body of withReportOrNotFound into a guard. I moved the decision instead, because a guard that takes children cannot inject props into 20 screens without cloning elements.

The page entry now reads reportID from the route and renders ReportDetailsContent inside ReportOrNotFoundGuard. The entry has no Onyx subscription. ReportDetailsContent is the former page shell and reads report and the report loading state itself. The guard's subscriptions sit one level above the content, so the content does not re-render when they change.

The guard records that content was shown with state set during render, not with a ref written during render. This lints clean and compiles with React Compiler.

One deviation: ReportDetailsContent uses the composed HeaderWithBackButtonAndTitle instead of the restricted HeaderWithBackButton. The title and back handler are the same, and this follows batch 1 of the Header migration. It keeps the lint seatbelt baseline untouched.

The two page tests and the perf test only lose the 5 HOC props that no longer exist, plus one report object that was only used for those props. A new withReportOrNotFound test covers the HOC and the guard: loading, not found, content, a blank screen while navigating away unfocused, not found while focused, the deeplink OpenReport call, and the HOC passing Onyx values over caller props. There was no test for the wrapper before. One unit page test never had a seeded report and always rendered the loading indicator. That was already the case and it still is.

This is a refactor and does not change behaviour.

image

Fixed Issues

$ #102183
PROPOSAL:

Tests

  1. Open Report details for a chat room, a group chat, a task, an expense report and a tracked expense in the self DM. Verify the header, rows and menu order are the same as on prod
  2. Open a report's details page directly with a deeplink. Verify the page loads
  3. Open an expense, open its Report details and delete it. Verify there is no "Not here" page flash after the delete
  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as tests

  • 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 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 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)
  • 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 the PR adds or modifies the UI:
    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • 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
Nagranie.z.ekranu.2026-10-7.o.13.04.16.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Nagranie.z.ekranu.2026-10-7.o.13.01.37.mov
Nagranie.z.ekranu.2026-10-8.o.11.06.23.mov

@codecov

codecov Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Coverage Δ
.../DynamicReportDetailsPage/ReportDetailsContent.tsx 100.00% <100.00%> (ø)
...ynamicReportDetailsPage/ReportDetailsMenuItems.tsx 100.00% <100.00%> (+5.33%) ⬆️
src/pages/DynamicReportDetailsPage/index.tsx 100.00% <100.00%> (ø)
src/pages/inbox/report/useReportDeepLinkOnOpen.ts 100.00% <100.00%> (ø)
src/pages/inbox/report/withReportOrNotFound.tsx 100.00% <100.00%> (+2.27%) ⬆️
src/components/ReportOrNotFoundGuard.tsx 97.29% <97.29%> (ø)
...DetailsPage/ReportDetailsMenuTrackExpenseItems.tsx 92.59% <92.59%> (ø)
... and 9 files with indirect coverage changes

@sosek108

sosek108 commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Performance impact of this PR

image

@sosek108
sosek108 marked this pull request as ready for review October 8, 2026 09:19
@sosek108
sosek108 requested review from a team as code owners October 8, 2026 09:19
@melvin-bot
melvin-bot Bot requested a review from nkdengineer October 8, 2026 09:19
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@melvin-bot
melvin-bot Bot requested review from trjExpensify and removed request for a team October 8, 2026 09:19
@melvin-bot

melvin-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

@nkdengineer 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]

Comment thread src/pages/inbox/report/useReportDeepLinkOnOpen.ts
Comment thread src/components/ReportOrNotFoundGuard.tsx
Comment thread src/pages/DynamicReportDetailsPage/ReportDetailsContent.tsx
@nkdengineer

Copy link
Copy Markdown
Contributor

@MelvinBot Review

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Code review

I found no blocking issues. The refactor keeps the existing behaviour, and CI passes except for the human checklist jobs.

I checked these points:

  • The guard's getReportOrNotFoundDecision matches the old HOC branches exactly, including the !isFocused || deleteTransactionNavigateBackUrl suppression.
  • ReportDetailsMenuTrackExpenseItems is a straight move of the old block, and its mount condition is unchanged.
  • The deeplink OpenReport effect keeps the same deps and arguments.
  • HeaderWithBackButtonAndTitle keeps the same title and back handler. The old header's help button defaulted to off, so nothing visible is lost.
  • The guard returns the same children element, so ReportDetailsContent skips re-renders when only the guard's subscriptions change.

Suggestions (non-blocking):

  1. Duplicated input wiring. The guard (ReportOrNotFoundGuard.tsx:87-99) and the HOC (withReportOrNotFound.tsx:81-96) still repeat the same subscriptions, the derived flags, the deeplink hook call and the three-branch render. You could move this into one useReportOrNotFoundDecision({reportID, shouldRequireReportID, hasShownContent}) hook. Then a future fix to the access rules only needs to change one place.
  2. Naming. ReportOrNotFoundGuard sits next to the existing ReportNotFoundGuard, which differs by two letters. The decision value 'null' (ReportOrNotFoundGuard.tsx:23) also reads like a missing value. Consider a clearer name, such as 'blank'.
  3. Test gap. The new suite does not test the focused case where NVP_DELETE_TRANSACTION_NAVIGATE_BACK_URL is set, which is test step 3 (no "Not here" flash after delete). Since getReportOrNotFoundDecision is pure and exported, you can add a direct unit test with little effort.

@trjExpensify

Copy link
Copy Markdown
Contributor

PR doesn’t need product input as a performance refactor PR. Unassigning and unsubscribing myself.

@trjExpensify
trjExpensify removed their request for review October 8, 2026 18:24
@sosek108

sosek108 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

About Melvin's review:

I applied 2 (renamed the decision to 'blank') and 3 (added a test for a focused screen with a delete-transaction navigation in flight). I kept the ReportOrNotFoundGuard name because it matches the withReportOrNotFound HOC it replaces.

On 1: the access rules already live in one place, getReportOrNotFoundDecision, so a change to them touches one function. What's left is subscription wiring, and the two differ on purpose. The HOC also reads policy and reportMetadata for the screen it wraps, and it tracks shown content in a ref instead of state. A shared hook would also change every screen that uses the HOC, which is outside this Report Details slice.

@nkdengineer

@sosek108

sosek108 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

@MelvinBot review again

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 Code review (follow-up)

The PR looks good to me, with no blocking issues. The only failing check besides the approval job is validate-perf-tests, and it looks like runner noise. Re-run it before merging.

Changes since my last review:

  • 26389e0 renames the decision value 'null' to 'blank' in the guard and the HOC. It also adds the focused delete-transaction test, which asserts that no content, not-found page or loading indicator shows. That covers test step 3.
  • a1c3460 removes the old HOC props from a test that came in from main. policy and report are still used to seed Onyx, so nothing is left unused.
  • Both main merges left the source files in this PR unchanged.

Your reasons for keeping the guard name and not adding a shared hook make sense. The access rules live only in getReportOrNotFoundDecision, and changing the HOC wiring would affect screens outside this slice.

Why the perf failure looks like noise
  • The job failed on a1c3460. That commit only changes a unit test file. The same job passed 6 minutes earlier on 8bbc14f, which has the same source code.
  • Render counts went down by 1 in every DynamicReportDetailsPage measurement. Only durations went up, by about 10 ms. The unrelated SelectionList test also slowed down by 13.7% in the same run.

@nkdengineer

Copy link
Copy Markdown
Contributor

@sosek108 Change looks good. Can you commit something and revert to re-run the perf-test before approving.

@nkdengineer

Copy link
Copy Markdown
Contributor

@sosek108 Look like the perf failure is not a noise

@sosek108

sosek108 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor Author

I'm veryfing this. Problem is, it works on my machine so it's hard to diagnose. I'll be back w/ update.

btw. it's flaky because it failed on different test

@nkdengineer

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 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
    • 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
    • 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 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.
  • If the PR adds or modifies the UI:
    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • 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
Screen.Recording.2026-10-09.at.14.37.59.mov
Android: mWeb Chrome
Screen.Recording.2026-10-09.at.14.33.19.mov
iOS: HybridApp
Screen.Recording.2026-10-09.at.14.34.00.mov
iOS: mWeb Safari
Screen.Recording.2026-10-09.at.14.35.00.mov
MacOS: Chrome / Safari
Screen.Recording.2026-10-09.at.14.31.24.mov
Screen.Recording.2026-10-09.at.14.31.46.mov
Screen.Recording.2026-10-09.at.14.32.10.mov

@nkdengineer nkdengineer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@melvin-bot
melvin-bot Bot requested a review from mjasikowski October 9, 2026 09:41
@mjasikowski
mjasikowski merged commit 5f13502 into Expensify:main Oct 9, 2026
35 of 37 checks passed
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

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

@OSBotify OSBotify mentioned this pull request Oct 9, 2026
74 tasks done
@OSBotify

OSBotify commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.5.7-0 🚀

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

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 No help site update needed. This PR is an internal refactor of the Report details page that keeps the same rows, labels, and behaviour, so no article sentence becomes incorrect.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/mountiny in version: 9.5.7-3 🚀

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

Bundle Size Analysis (Sentry):

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.

6 participants