102436: Concierge - RHP closes when dismissing an attachment with Esc or expanding a chart by abbasifaizan70 · Pull Request #102937 · Expensify/App

abbasifaizan70 · 2026-10-03T22:02:24Z

Explanation of Change

Two fixes for the Side Panel when it is shown as an RHP overlay (below the extra-large breakpoint):

Esc closing both the attachment preview and the panel: SidePanelModal now disables its Escape shortcut while a modal (willAlertModalBecomeVisible) is open on top of it, so the Esc that closes the attachment preview (or expanded chart) no longer also closes the panel. An RHP beneath the panel doesn't set that flag, so Esc still closes the panel in that case.
Expanding a chart closing the panel: useSyncSidePanelWithHistory now skips modal back-guard history entries (CUSTOM_HISTORY-MODAL:*) when reading the last history entry, so a shouldHandleNavigationBack modal opened from the panel is no longer mistaken for the panel being closed. Browser Back closes the modal first, then the panel.

Fixed Issues

$ #102436
PROPOSAL: #102436

Tests

Verify that no errors appear in the JS console

Bug 1: Esc closes both the attachment preview and Concierge RHP

Send any attachment in the Concierge chat.
Open any other report and click the Help icon in the header to open Concierge in the RHP.
Open the attachment preview.
Press Esc.
Verify that only the attachment preview closes. Concierge RHP remains open.

Bug 2: Expanding a chart closes Concierge RHP

Generate a chart in Concierge.
Open any other report and click the Help icon in the header to open Concierge in the RHP.
Expand the chart.
Press Esc.
Verify the expanded chart modal opens on top of the Concierge RHP, which remains open underneath.

Offline tests

Same as tests.

QA Steps

Same as tests.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Pressing the ESC button is not applicable here.

Android: mWeb Chrome

Pressing the ESC button is not applicable here.

iOS: Native

Pressing the ESC button is not applicable here.

iOS: mWeb Safari

Pressing the ESC button is not applicable here.

MacOS: Chrome / Safari

Screen.Recording.2026-10-04.at.2.15.39.PM.1.mov

… or expanding a chart

codecov · 2026-10-03T22:06:04Z

Codecov Report

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

Files with missing lines	Coverage Δ
...nts/SidePanel/useSyncSidePanelWithHistory/index.ts	`0.00% <0.00%> (ø)`
src/components/SidePanel/SidePanelModal/index.tsx	`0.00% <0.00%> (ø)`
... and 198 files with indirect coverage changes

melvin-bot · 2026-10-04T11:39:53Z

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

trjExpensify · 2026-10-05T13:26:03Z

Dropping off, not sure why it put two product review on here. 👍

luacmartins

LGTM

luacmartins · 2026-10-05T18:04:45Z

@situchan all yours

luacmartins · 2026-10-06T16:16:42Z

@situchan bump

situchan · 2026-10-06T20:57:11Z

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

Android: mWeb Chrome

iOS: HybridApp

iOS: mWeb Safari

MacOS: Chrome / Safari

test.mov

situchan · 2026-10-07T11:10:52Z

Both fixes target the right root causes. NAB suggestions:

Esc fix (SidePanelModal/index.tsx#L52): willAlertModalBecomeVisible is one flag shared by all modals, so closing a nested modal turns it off while the outer one is still open. I suggest keeping the shortcut always registered and checking the list of open modals inside the callback, when the key is pressed, as BulkActionBar already does:

useKeyboardShortcut(
    CONST.KEYBOARD_SHORTCUTS.ESCAPE,
    () => {
        if (!areAllModalsHidden()) {
            return;
        }
        closeSidePanel();
    },
    {isActive: !isExtraLargeScreenWidth, shouldBubble: false},
);

areAllModalsHidden() stays correct when modals are stacked, and it's checked at the moment the key is pressed, so there's no gap while the Onyx write is pending.

History fix (useSyncSidePanelWithHistory/index.ts#L30): Looks good. Optional alternative: check whether the panel's entry is in history at all, like useSyncMfaModalNavigatorWithHistory. That covers any entry added after the panel's later on, but the panel would then stay open under the MFA flow instead of closing.

MelvinBot · 2026-10-07T11:17:52Z

Code review

Both fixes target the right causes. I recommend one change to the Esc fix before merge. The history fix looks correct.

1. Esc fix can still close the panel when modals are stacked (recommended change)

SidePanelModal/index.tsx:52 gates the shortcut on willAlertModalBecomeVisible. That is one shared flag, and BaseModal.tsx:142 sets it to false whenever any modal hides, even when another modal is still open.

Example on web, below the XL breakpoint:

Open Concierge in the side panel and open an attachment preview.
Open the preview's three-dot menu, then close it.
Press Esc.

The menu's hideModal cleared the flag, so the panel shortcut is active again. Esc closes both the preview and the panel, which is the original bug.

I agree with situchan's suggestion: keep the shortcut active and check areAllModalsHidden() inside the callback. It counts each open modal, so stacked modals don't break it. It's read when the key is pressed, so there's no wait for Onyx. The attachment Modal and the chart modal both pass onClose, so both are counted.

2. History fix: looks good

useSyncSidePanelWithHistory/index.ts:30 now skips CUSTOM_HISTORY_ENTRY_MODAL:* entries. Opening a modal no longer reads as the side panel entry being popped, and Back closes the modal first. findLast returns the entry from state, so the !== reference check still works for route entries.

3. Minor

Neither change has a unit test. You could add a test that puts a CUSTOM_HISTORY_ENTRY_MODAL:<id> entry after CUSTOM_HISTORY_ENTRY_SIDE_PANEL and checks that closeSidePanel is not called. This would catch a regression.
In the Bug 2 test steps, step 4 ("Press Esc") comes before step 5, which checks that the expanded chart opened. Move step 5 to follow step 3, then check that Esc closes only the chart.

I reviewed the code only. I didn't run the stacked-modal example in the app.

view run

luacmartins · 2026-10-07T14:57:46Z

@abbasifaizan70 wanna address this comment before we merge? #102937 (comment)

…ertModalBecomeVisible flag

abbasifaizan70 · 2026-10-07T16:15:46Z

@situchan @luacmartins Done — the Esc handler now checks areAllModalsHidden() when the key is pressed, as in BulkActionBar. Kept the history fix as is, since the alternative would keep the panel open under the MFA flow.

OSBotify · 2026-10-07T20:03:18Z

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

github-actions · 2026-10-07T20:23:26Z

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

github-actions · 2026-10-07T20:29:16Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
Built from App PR #102937.

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com/rock-artifacts/ad-hoc/rock-android-Adhoc-7986d29-a57d040-245dd6fcadf4477f52977b9c9c7da4be5d500f51/index.html	https://ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com/rock-artifacts/ad-hoc/rock-ios-device-AdHoc-7986d29-a57d040-ccdc075a04f7221062c0b69eb154b58fcff94ca1/index.html

Web 🕸️
https://102937.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

OSBotify · 2026-10-08T19:45:16Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 9.5.6-0 🚀

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

MelvinBot · 2026-10-08T19:49:33Z

No help site update is needed. This PR is a bug fix: the Concierge side panel now stays open when you close an attachment preview with Esc or expand a chart, which is what users already expect, and no sentence in an existing article (including Concierge Basics) describes the old behavior.

view run

OSBotify · 2026-10-09T20:30:54Z

🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.6-6 🚀

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

Bundle Size Analysis (Sentry):

Android
iOS

102436: Concierge - RHP closes when dismissing an attachment with Esc…

5f01d67

… or expanding a chart

melvin-bot Bot assigned abbasifaizan70 Oct 3, 2026

abbasifaizan70 changed the title ~~102436: Concierge - RHP closes when dismissing an attachment with Esc…~~ 102436: Concierge - RHP closes when dismissing an attachment with Esc or expanding a chart Oct 4, 2026

abbasifaizan70 marked this pull request as ready for review October 4, 2026 11:39

abbasifaizan70 requested review from a team as code owners October 4, 2026 11:39

melvin-bot Bot requested review from a team, situchan and trjExpensify and removed request for a team October 4, 2026 11:39

melvin-bot Bot requested review from garrettmknight and removed request for a team October 4, 2026 11:39

abbasifaizan70 mentioned this pull request Oct 4, 2026

[Due for payment 2026-10-16] [$175] Concierge - RHP closes when dismissing an attachment with Esc or expanding a chart #102436

Open

7 tasks

garrettmknight previously approved these changes Oct 5, 2026

View reviewed changes

trjExpensify removed their request for review October 5, 2026 13:26

luacmartins self-requested a review October 5, 2026 14:58

luacmartins approved these changes Oct 5, 2026

View reviewed changes

situchan approved these changes Oct 7, 2026

View reviewed changes

melvin-bot Bot requested a review from luacmartins October 7, 2026 11:11

luacmartins previously approved these changes Oct 7, 2026

View reviewed changes

abbasifaizan70 added 2 commits October 7, 2026 18:58

Merge branch 'main' into 102436

89c88d2

fix: check open-modal registry on Escape instead of the shared willAl…

3d2a409

…ertModalBecomeVisible flag

abbasifaizan70 dismissed stale reviews from garrettmknight and luacmartins via 3d2a409 October 7, 2026 16:16

luacmartins approved these changes Oct 7, 2026

View reviewed changes

luacmartins merged commit 7986d29 into Expensify:main Oct 7, 2026
36 of 37 checks passed

OSBotify mentioned this pull request Oct 8, 2026

Deploy Checklist: New Expensify 2026-10-08 #103502

Closed

90 tasks done

applause-bot Bot mentioned this pull request Oct 8, 2026

[$175] Concierge - Image appears cut on its preview on the Concierge RHP #103533

Open

1 of 7 tasks

Conversation

abbasifaizan70 commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

codecov Bot commented Oct 3, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melvin-bot Bot commented Oct 4, 2026

Uh oh!

trjExpensify commented Oct 5, 2026

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

luacmartins commented Oct 5, 2026

Uh oh!

luacmartins commented Oct 6, 2026

Uh oh!

situchan commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

situchan commented Oct 7, 2026

Uh oh!

MelvinBot commented Oct 7, 2026

Code review

Uh oh!

luacmartins commented Oct 7, 2026

Uh oh!

abbasifaizan70 commented Oct 7, 2026

Uh oh!

Uh oh!

OSBotify commented Oct 7, 2026

Uh oh!

github-actions Bot commented Oct 7, 2026

Uh oh!

github-actions Bot commented Oct 7, 2026

Uh oh!

OSBotify commented Oct 8, 2026

Uh oh!

MelvinBot commented Oct 8, 2026

Uh oh!

OSBotify commented Oct 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

abbasifaizan70 commented Oct 3, 2026 •

edited

Loading

codecov Bot commented Oct 3, 2026 •

edited by github-actions Bot

Loading

situchan commented Oct 6, 2026 •

edited

Loading