Skip to content

Fix expense rule selection being cancelled when opened via the Concierge link - #99717

Open
mukhrr wants to merge 3 commits into
Expensify:mainfrom
mukhrr:fix/95132
Open

mukhrr wants to merge 3 commits into
Expensify:mainfrom
mukhrr:fix/95132

Conversation

@mukhrr

@mukhrr mukhrr commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Mobile selection mode is a single global Onyx flag. The Concierge rules link pushes a fullscreen tab entry rather than an RHP, so the report you came from stays mounted underneath, and two of its subscribers clear the flag as soon as the Rules page sets it:

  • MoneyReportHeader clears it in its render body when the report has one visible transaction
  • useMobileSelectionMode clears it on mount, and SelectionToolbarGate mounts SelectionToolbar exactly when the flag flips

Focus-gating both clears fixes it, verified red/green on iOS. The hook cleanup is deferred rather than skipped, so the existing stale-selection cleanup still runs once the screen is focused.

The existing MoneyReportHeaderSelectionModeTest mocks navigation without useIsFocused, so it now stubs it as focused. The new focus-gating tests live in MoneyReportHeaderFocusSelectionModeTest.

The focus guard has to cover the render, not just the turn-off. An unfocused report mounted behind the Rules page otherwise still paints the Select multiple header the moment selection mode turns on, and iOS swipe-back reveals it for about 0.6s until the report regains focus. Gating the whole branch on focus removes that, verified frame by frame on the iOS simulator. The inner focus check on the single-transaction turn-off is folded into the outer one, so an unfocused report still never clears the flag, same as before.

Fixed Issues

$ #95132
PROPOSAL: #95132 (comment)

Tests

Precondition: two or more personal expense rules, and an expense created through a rule so the Concierge message with the personal expense rules link exists.

  1. On iOS or Android, open the report containing that expense, open the expense, and tap the personal expense rules link in the Concierge message
  2. Long-press any rule and tap Select
  3. Verify selection mode stays on: Select multiple header, 1 selected, and checkboxes visible
  4. Repeat on a report with two or more expenses and verify selection is kept there too
  5. Open Settings > Expense rules directly and verify long-press to select still works
  6. Enter selection mode on any list, navigate away, come back, and verify the selection is cleared
  7. On iOS, with selection mode on in Expense rules, swipe back to the report and verify the report header is its normal header the whole way, never Select multiple
  • Verify that no errors appear in the JS console

Offline tests

Selection mode is a RAM-only Onyx value and the flows above make no API calls, so behaviour is identical offline.

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 verified there are no new alerts related to the canBeMissing param for useOnyx
  • 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
android_web.mp4
iOS: Native
IOS_app.mp4
iOS: mWeb Safari
ios_web.mp4
MacOS: Chrome / Safari
web.mp4

Mobile selection mode is a single global Onyx flag. Opening Expense rules via the
Concierge link from an expense pushes a fullscreen tab entry, so the report screen
stays mounted underneath and clears the flag as soon as the Rules page sets it.

Focus-gate both clears: the mount cleanup in useMobileSelectionMode, and the
single-transaction auto-exit in MoneyReportHeader.
@mukhrr
mukhrr marked this pull request as ready for review August 28, 2026 01:09
@mukhrr
mukhrr requested review from a team as code owners August 28, 2026 01:09
@melvin-bot
melvin-bot Bot requested review from ChavdaSachin and removed request for a team August 28, 2026 01:09
@melvin-bot

melvin-bot Bot commented Aug 28, 2026

Copy link
Copy Markdown

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

@ChavdaSachin

ChavdaSachin commented Aug 31, 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 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.
  • 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-09-15.at.6.18.43.AM.mov
Android: mWeb Chrome
iOS: HybridApp
Screen.Recording.2026-09-15.at.6.37.27.AM.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-09-15.at.5.53.30.AM.mov

@mukhrr

mukhrr commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@ChavdaSachin kindly bump

@ChavdaSachin

Copy link
Copy Markdown
Contributor

Reviewing ♻️

@ChavdaSachin ChavdaSachin 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 Julesssss September 15, 2026 01:09
@mukhrr

mukhrr commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

@Julesssss kindly bump

# Conflicts:
#	tests/ui/components/MoneyReportHeaderSelectionModeTest.tsx
The focus guard only covered the turn-off, so a report mounted behind the
Expense rules page still painted the "Select multiple" header the moment
selection mode was enabled there. Swiping back revealed it for ~0.6s until
the report regained focus and cleared the flag.

Gate the whole selection branch on focus, so an unfocused report renders its
normal header and never shows selection UI another screen owns.
@mukhrr

mukhrr commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

@Julesssss I found one related bug here. Do you think it makes sense to fix it in a separate folow up issue?

@mukhrr

mukhrr commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Preconditions

  • iOS, narrow layout
  • At least one personal expense rule (Account > Expense rules)
  • A report containing exactly one expense that was created with a merchant matching that rule, so its Concierge message has the personal expense rules link

Steps

  1. Open the report from the Inbox chat (tap the report card) so it's the focused screen
  2. Scroll to the Concierge message and tap the personal expense rules link
  3. On the Expense rules page, long-press any rule and tap Select
  4. Confirm the page shows "Select multiple" and "1 selected"
  5. Swipe back from the left edge (do not use the back chevron)
  6. Watch the report header as it's revealed

Expected
The report header reads "Expense Report …" with Submit / More from the first moment it's revealed.

Actual
The report is revealed with a "Select multiple" header, which holds for ~0.6s before switching to the normal header.

95132-pathC-before.mp4
image

@Julesssss

Julesssss commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

@Julesssss I found one related bug here. Do you think it makes sense to fix it in a separate folow up issue?

@mukhrr to confirm, that is existing on main? If so I think that is fine, but it's worth fixing if introduced with these changes.

Okay yeah looks like it was introduced here, so lets try and resolve before this PR is merged please.

@mukhrr

mukhrr commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

@Julesssss I found one related bug here. Do you think it makes sense to fix it in a separate folow up issue?

@mukhrr to confirm, that is existing on main? If so I think that is fine, but it's worth fixing if introduced with these changes.

Okay yeah looks like it was introduced here, so lets try and resolve before this PR is merged please.

@Julesssss nice. it is already handled here. so we are good to merge

@Julesssss

Copy link
Copy Markdown
Contributor

Thanks @mukhrr, unfortunately there are new conflicts please fix and we can merge

This branch has not been deployed

No deployments
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.

3 participants