Make Copilot switching discoverable with a dedicated Switch button on the Accounts page (v2)#95769
Open
wildan-m wants to merge 14 commits into
Open
Make Copilot switching discoverable with a dedicated Switch button on the Accounts page (v2)#95769wildan-m wants to merge 14 commits into
wildan-m wants to merge 14 commits into
Conversation
…button on the Accounts page" This reverts commit 658bde1, restoring the change reverted while the deploy blockers it surfaced were investigated. The following commits fix those blockers.
The tooltip measured its anchor once and never again. On an orientation change the anchor is laid out twice: once at an intermediate position, then at its final one. measure() only reports the new position after the native layout has landed, so the single early measurement left the tooltip anchored to a position the component had already left. Anchors on the left edge hid this, since their x doesn't depend on the container width. The Switch button sits on the right edge, so the stale x placed the right-aligned tooltip off the left of the screen, and made the bounds check read the button as overflowing in landscape, hiding the tooltip entirely. Re-measure on every layout of the anchor once the tooltip has been displayed, so it tracks the component instead of a snapshot of where it used to be.
The tooltip is anchored to the Switch button, which scrolls with the settings list, but it is drawn in a portal at a fixed position. It stayed put while the list moved, floating over unrelated rows once the button had scrolled away. BaseEducationalTooltip already hides and re-measures on scroll, but two things stopped it here: the tooltip never opted in via shouldHideOnScroll, and the settings page never emitted a scrolling event for anything to listen to. Opt in and emit, so the tooltip hides while the list moves and, once it settles, either follows the button or stays hidden when the button has scrolled out of view.
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
…t-switch-button-v2
…e tooltip
In landscape the educational tooltip stayed hidden after rotating, and only appeared
once you left the page and came back.
Dimensions.get('window') already excludes the horizontal safe-area insets, but the
bounds check added the right inset back to the element's right edge. On a device with
a landscape display cutout that counted it twice: the Switch button's right edge is
842.28 inside an 862.48 window, yet the check computed 894.09 and read it as
overflowing, so it hid the tooltip.
Leaving the page and returning worked because that remounts the switcher, and the
first-display path shows the tooltip without consulting the bounds check at all.
Rotation re-runs only the bounds-checked path, so there was nothing to undo the hide.
Anchor both edges to the inset-adjusted left so they share the window's origin, and
pull the arithmetic into a helper so it can be tested against the measured values.
…t-switch-button-v2
On mWeb the educational tooltip stayed hidden after rotating to landscape, and only came back after leaving the page and returning. The debounced reset closed over isResizing, so the callback created by the first resize event still saw it as false and took the early return instead of settling. The effect's cleanup then cancelled that pending call as soon as isResizing flipped true, leaving a second resize event as the only thing that could ever reset it. Dragging a window emits a stream of events so it settled; an orientation change can emit just one, so it stayed true forever and BaseEducationalTooltip kept hiding the tooltip without ever re-rendering it. Track the settle deadline in the effect instead, so a lone resize event still settles.
The pending "scrolling ended" event was cancelled on unmount without being sent, so navigating away mid-scroll left every listener believing the scroll was still going: educational tooltips stayed hidden, hovers stayed suppressed, and the composer kept swallowing wheel events. The scroll is over once the view is gone, so say so. Also cover the behaviour the surrounding code was built for, so it stays covered: a tooltip re-measures once resizing settles, a target resting against the left edge is still in bounds, and one that scrolls out of view horizontally is not.
…t-switch-button-v2
The previous commit pulled the bounds arithmetic into a helper so it could be unit tested, which meant reworking how the component reads its insets and moving code that was never at fault. Fix the one wrong expression in place instead: dropping the right inset also makes it an unused dependency, so it goes too.
…t-switch-button-v2
wildan-m
marked this pull request as ready for review
July 18, 2026 15:33
melvin-bot
Bot
requested review from
a team,
eh2077 and
joekaufmanexpensify
and removed request for
a team
July 18, 2026 15:33
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d516241c3a
ℹ️ 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 on lines
+211
to
+212
| if (hasDisplayedTooltipRef.current) { | ||
| renderTooltip(); |
There was a problem hiding this comment.
When an educational tooltip is mounted inside an animated container and the anchor fires a second onLayout before CONST.TOOLTIP_ANIMATION_DURATION elapses, hasDisplayedTooltipRef is already true because it is set as soon as shouldMeasure flips, before the delayed show.current runs. This new path therefore calls renderTooltip() immediately and skips the existing settle delay, so the first render can be measured against a transient animated layout and appear in the wrong place; gate this on the delayed show actually completing instead.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Good catch, fixed in c319d19. hasDisplayedTooltipRef was set as soon as shouldMeasure flipped — before the delayed first render actually ran — so a second onLayout inside the animation window re-measured against a transient layout. Moved the flag into the delayed show.current(), so both the layout and scroll-gating re-measure paths only fire after the first render settles. Added a regression test for a second layout arriving before the delay elapses.
The re-measure-on-layout path checked hasDisplayedTooltipRef, but that ref was set as soon as shouldMeasure flipped — before the delayed first render actually ran. So a second onLayout arriving within the animation window (e.g. an animated container still settling) re-measured against a transient layout and could place the tooltip wrongly. Set the ref inside the delayed show instead, so both the layout and the scroll-gating re-measure paths only run after the first render has genuinely settled.
joekaufmanexpensify
previously approved these changes
Jul 20, 2026
joekaufmanexpensify
left a comment
Contributor
There was a problem hiding this comment.
V2 attempt after fixing deploy blockers
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Contributor
|
sorry for the delay, will finish test and review by tmr. |
eh2077
reviewed
Jul 24, 2026
Comment on lines
+43
to
+49
| // Unmounting mid-scroll drops the pending "scrolling ended" event, leaving every listener | ||
| // stuck as if the scroll never finished. The scroll is over either way, so say so. | ||
| if (!isScrollingRef.current) { | ||
| return; | ||
| } | ||
| DeviceEventEmitter.emit(CONST.EVENTS.SCROLLING, false); | ||
| isScrollingRef.current = false; |
Contributor
There was a problem hiding this comment.
I don't prefer playing trick about scrolling. Why don't use ProductTrainingTooltipInner?
Contributor
Author
There was a problem hiding this comment.
Good point — reverted useScrollEventEmitter to main; the copilot tooltip now hides on scroll purely via EducationalTooltip's shouldHideOnScroll, the same mechanism ProductTrainingTooltipInner uses, with the Accounts page emitting scroll events like LHNOptionsList does.
…t-switch-button-v2 # Conflicts: # src/languages/de.ts # src/languages/en.ts # src/languages/es.ts # src/languages/fr.ts # src/languages/it.ts # src/languages/ja.ts # src/languages/nl.ts # src/languages/pl.ts # src/languages/pt-BR.ts # src/languages/zh-hans.ts
Per review: hiding the copilot tooltip on scroll doesn't need the shared scroll emitter touched. The tooltip uses EducationalTooltip's shouldHideOnScroll and the settings page emits scroll events the same way the LHN list does — matching the ProductTrainingTooltip pattern. The emit-on-unmount hardening was a defensive edge-case fix unrelated to this feature, so revert useScrollEventEmitter to main and drop its test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
This re-lands #93393 (reverted in #95748) together with fixes for the two deploy blockers it surfaced.
1. Reapply the feature — revert of the revert (
658bde1c7ee), no conflicts.2.
#95693— no tooltip in landscape, wrong position after rotating backThe educational tooltip measured its anchor once and never again. On an orientation change the anchor is laid out twice: first at an intermediate position, then at its final one.
measure()only reports the new position after the native layout has landed, so that single early measurement left the tooltip anchored to a position the component had already left.Instrumented on an Android emulator, rotating back to portrait:
Anchors on the left edge hid this, because their
xdoesn't depend on the container width. The Switch button sits on the right edge, so withANCHOR_ORIGIN_HORIZONTAL.RIGHTthe stalexput the tooltip atleft: -5.76dp— off the screen — and made the safe-area bounds check read the button as overflowing in landscape, hiding the tooltip entirely.Fix: re-measure on every layout of the anchor once the tooltip has been displayed, so it tracks the component instead of a snapshot of where it used to be. No timers, no delay constants — the layout event itself is the trigger.
3.
#95717— tooltip stays in place when the page is scrolledBaseEducationalTooltipalready knows how to hide and re-measure on scroll, but two independent gaps stopped it here:AccountSwitchernever passedshouldHideOnScroll, so it defaulted tofalseand the tooltip never subscribed to the scroll event.InitialSettingsPage'sScrollViewnever calleduseScrollEventEmitter, soCONST.EVENTS.SCROLLINGwas never emitted on that page at all — there was nothing to subscribe to. (LHNOptionsListandBaseSelectionListWithSectionsboth emit it; the settings page didn't.)Fix: opt in and emit. The tooltip now hides while the list moves and, once it settles, either follows the button or stays hidden when the button has scrolled out of view.
Fixed Issues
$ #92253
$ #95693
$ #95717
PROPOSAL: #92253 (comment)
Tests
Requires an account that is a copilot for at least one other account, and that has not yet dismissed the account-switcher tooltip.
Rotation (#95693), Android/iOS native:
Scrolling (#95717), all platforms:
5. Back in portrait at the top of the Account list, with the tooltip visible, scroll the page down.
6. Verify the tooltip is hidden once the Switch button scrolls out of view — it must not float over the other rows.
7. Scroll back to the top. Verify the tooltip reappears anchored to the Switch button.
Feature (re-land of #93393):
8. Verify the Switch button opens the account switcher.
9. Verify the avatar and account name at the top of Account are no longer pressable.
10. Verify Status now lives under Profile.
Offline tests
The tooltip, the Switch button and the account switcher are all rendered from local Onyx data, so behaviour is unchanged offline. Repeat the Tests above with the network disabled and verify the tooltip still appears, hides on scroll, and repositions on rotation.
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
Kapture.2026-07-16.at.20.51.46.mp4
Android: mWeb Chrome
Kapture.2026-07-16.at.13.18.39.mp4
iOS: Native
Kapture.2026-07-16.at.13.14.09.mp4
iOS: mWeb Safari
Kapture.2026-07-16.at.13.18.39.mp4
MacOS: Chrome / Safari
Kapture.2026-07-16.at.13.06.32.mp4