Fix: Side Panel composer steals focus back after opening a new chat - #101217
Conversation
…cus fires The Side Panel renders a second ComposerWithSuggestions instance that runs the same modal-close refocus effect as the main composer. That effect reads the Side Panel's focus claim (sidePanelComposerRef) when scheduling a delayed focus, but the actual .focus() call lands 150ms later against whatever composer holds focus by then. If the main composer took focus in the meantime, the Side Panel's stale timer still fires and steals it back. focusComposerWithDelay now accepts an optional predicate re-evaluated inside the setTimeout, right before focusing. Only the Side Panel composer supplies one, so every other caller is unaffected. The main composer's own modal-close refocus is also guarded to yield when the Side Panel still holds the claim, and the claim is cleared when the Side Panel composer unmounts, so the guard cannot outlive the panel that set it.
|
@ikevin127 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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Explain why the unsafe cast is needed, per review feedback.
Reviewer Checklist
Screenshots/Videos100601.mov |
Screen.Recording.2026-09-15.at.16.13.58.movSteps, continuing from a state where the Side Panel composer holds focus:
The reason is that step 5 never mounts anything.
This is why it works when you pick a different person and not when you pick the one already open. On The re-check answers "another composer took focus during the delay". It has no answer for "the user explicitly navigated but nothing mounted to take focus". Clearing the claim in // Selecting a chat destination hands composer-focus ownership to the main pane, so release the Side Panel's
// claim before the dismiss. Without this, picking a chat that is already open never mounts a composer to
// release it, and the Side Panel's refocus wins by default.
ReportActionComposeFocusManager.sidePanelComposerRef.current = null;
Navigation.dismissModal();
|
Yield to the Side Panel composer only while the panel is actually on screen. The claim outlives a closing panel by the length of its exit animation, so on narrow screens, where navigating closes the panel and dismisses the RHP at the same moment, the guard could block the main composer's refocus and reproduce the original symptom at a different width. Extract the guard into shouldYieldFocusToSidePanelComposer so it is covered directly, and keep focus on an empty dependency list by reading isInSidePanel through a ref synced in an effect. Assigning the ref during render would exceed this file's react-hooks/refs seatbelt budget. The test now uses a real textarea instead of a cast, and the composer focus delay constant instead of a hardcoded 150. jest/setupAfterEnv.ts switches every suite back to real timers, so the explicit useFakeTimers() call stays.
dylanexpensify
left a comment
dylanexpensify
left a comment
There was a problem hiding this comment.
Looks good from a product perspective
Status of my previous comments
|
| /** Stays false for the length of the Side Panel's exit animation, while its composer is still mounted */ | ||
| shouldHideSidePanel: boolean; |
There was a problem hiding this comment.
🟡 shouldHideSidePanel's param doc is inverted, and it inverts the meaning of the fix.
It is the opposite. SidePanelActions.dismissSidePanel does Onyx.merge(ONYXKEYS.NVP_SIDE_PANEL, {open: false, openNarrowScreen: false, ...}) (src/libs/actions/SidePanel.ts:31), so isSidePanelVisible flips false immediately and shouldHideSidePanel becomes true the moment the panel starts closing.
The thing that stays false for the length of the exit animation is isSidePanelTransitionEnded, which is why SidePanel/index.tsx keeps the composer mounted until both are satisfied.
That distinction is the entire reason this parameter exists. The panel goes logically hidden while its composer is still mounted holding the claim, and shouldHideSidePanel going true is what stops the main composer from yielding to it.
The code and the test (shouldHideSidePanel: true asserts no yield) are both correct, so this is only the doc, but it reads as a contradiction of the behaviour it documents and it is exactly the kind of comment that gets someone to "correct" the logic later:
/** True as soon as the Side Panel starts closing, while its composer is still mounted for the exit animation */
shouldHideSidePanel: boolean;There was a problem hiding this comment.
oh right, thanks. updated
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟠 The already-open-chat case has no reply and no change
This is the one I actually need an answer on before approving.
To be clear, I am not asking you to prove it is unrelated, because I already established that: on main the same flow ends in Concierge too, since both composers schedule a 150ms timer and the panel's is queued last. It is a pre-existing gap, not something this PR introduced.
The risk is QA: The issue's repro is "FAB > Start chat > enter email > select the user" with the expected result "chat focus will be on the newly opened chat". Nothing in that wording says the chat must be new, so a tester picking someone whose chat is already open in the central pane will see focus stay in Concierge and file it against this PR.
Either resolution works for me:
- Fix it here. Release the claim in
navigateToReportbeforeNavigation.dismissModal(). That covers "the user explicitly navigated but nothing mounted to take focus", which the fire-time re-check structurally cannot see.#86658is unaffected, since Cmd+K and Esc never reachnavigateToReportand an LHN switch mounts a composer that clears the claim anyway. - Or scope it out explicitly. Say so in a comment, and add a line to the PR description noting the already-open-chat variant is pre-existing and tracked separately, so QA has something to read before filing.
What I do not want is silence on it, because then it surfaces after deploy with no written history.
Selecting a chat that is already open in the central pane mounts no composer, so nothing released the Side Panel's claim and the panel won the refocus that follows the dismiss. Releasing it in NewChatPage covers that alongside the new-chat and self-DM branches, all of which dismiss the modal themselves without going through navigateToReport. Also correct the shouldHideSidePanel doc, which had the flag inverted: closing the panel merges open: false immediately, so the flag goes true at the start of the close while isSidePanelTransitionEnded is what lags for the exit animation. Document why the main composer has to yield at all, since the Side Panel only wins that race by effect-flush order.
ah sorry, I kinda confused it. it should be fixed now |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Thanks for addressing all comments
|
Conflict :( |
|
cc @mukhrr for conflict resolution - so we can merge this 🙌 |
|
@stitesExpensify resolved |
|
🚧 stitesExpensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 9.4.91-0 🚀
|
|
🤖 No help site changes are required for this PR. This is an internal focus-race fix. It changes no feature, setting, label, button, or user-facing workflow — it only makes the Side Panel composer stop stealing focus back after a modal closes. Focus behavior between the Side Panel composer and the main composer is not documented anywhere in What I checkedChanged files — all implementation and tests, no copy or
Help site search — I searched The remaining "focus" hits are unrelated uses of the word (SmartScan troubleshooting, playbooks, analytics). Per the naming conventions and authoring guidelines, help articles document what a user does and sees, not internal timing behavior. A bug fix that restores the already-expected behavior — typing lands in the chat you just opened — doesn't warrant a new or amended article. @mukhrr, I didn't open a help site PR because none is needed, so there's nothing to mark |
6 participants
Explanation of Change
The Side Panel renders a second composer alongside the main report composer. When a modal closes, both composers run a "refocus after modal close" effect. The Side Panel composer's guard for that effect only checks whether it previously held focus, read at the moment the effect runs, not when the delayed
.focus()actually lands 150ms later. If the newly opened chat's composer takes focus in that window, the Side Panel's stale timer still fires afterward and steals it back.focusComposerWithDelaynow accepts an optional predicate, re-evaluated inside the timer right before focusing. Only the Side Panel composer supplies one, re-checking its own focus claim at fire time instead of trusting the value from when focus was scheduled. The main composer's own modal-close refocus is also guarded to yield when the Side Panel still holds that claim, and the claim is cleared when the Side Panel composer unmounts, so it can never outlive the panel that set it.Fixed Issues
$ #100601
PROPOSAL: #100601 (comment)
Tests
Preconditions: Side Panel (Concierge anywhere) enabled, window wider than 1300px.
Offline tests
This is a client-side focus race with no network calls, offline state cannot affect it. The Tests steps above are unaffected by connectivity.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
web only bugAndroid: mWeb Chrome
web only bugiOS: Native
web only bugiOS: mWeb Safari
web only bugMacOS: Chrome / Safari
web.mp4