Repository navigation
Keep the Account screen unfrozen on native so the status avatar updates - #105306
Conversation
…es after clearing status Co-authored-by: Nyoman Jyotisa <nyomanjyotisa@users.noreply.github.com>
|
The failing check is unrelated to this PR, so I didn't push a code change. Analysis: Evidence:
Next step: rerun the failed
|
|
🤖 Web test steps verification results: The PR's Tests section is still empty, so I ran the repro steps from the linked issue on a narrow web viewport. The Account avatar showed the status emoji after saving a status and went back to the plain avatar after clearing it. I couldn't read the browser console, so the "no JS console errors" step is not fully verified.
view run · no recording available |
|
@MelvinBot please replace Updated PR descriptionTests
Offline tests
QA StepsSame as Tests and Offline tests.
PR Author Checklist
Screenshots/VideosAndroid: NativeAndroid-pr105306-after-live.mp4Android: mWeb ChromeAndroid-Chrome-pr105306-after-clear-live.mp4iOS: NativeiOS-pr105306-after-clear-verified.mp4iOS: mWeb SafariiOS-Safari-pr105306-after-clear-live.mp4MacOS: Chrome / SafariChrome-pr105306-after-clear-verified.mp4Safari-pr105306-after-clear-verified.mp4 |
|
🤖 I replaced |
|
@MelvinBot please rerun the Reassure Performance Tests workflow, including fresh measurements. Keep this PR in draft; no code changes. |
|
I couldn't rerun the Reassure Performance Tests workflow. My run environment blocked the rerun command, so no new run started. The PR is still a draft, and I made no code changes. To get fresh measurements, open the run and choose Re-run all jobs. Don't choose Re-run failed jobs. That option reruns only Why the last run failedThe run tested the current PR head ( |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid-pr105306-after-live.mp4Android: mWeb ChromeAndroid-Chrome-pr105306-after-clear-live.mp4iOS: HybridAppiOS-pr105306-after-clear-verified.mp4iOS: mWeb SafariiOS-Safari-pr105306-after-clear-live.mp4MacOS: Chrome / SafariChrome-pr105306-after-clear-verified.mp4Safari-pr105306-after-clear-verified.mp4 |
|
@MelvinBot please mark this PR ready for review. No code or description changes. |
|
🤖 I marked this PR ready for review. I didn't change any code or the description. |
|
@nyomanjyotisa 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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
The Analysis: The failing measurement is Evidence: today, the same measurement fails the same way on unrelated PRs:
This test is currently unstable across PRs. It needs a fix on Next step: you can rerun the failed job, but it may fail again until the test is fixed on
|
|
@arosiclair While working on this PR, I found an existing accessibility bug: the Status Message field uses Removing its explicit May I handle this in a separate PR under the C+ small-fix process? |
|
@arosiclair Could you rerun the full Reassure workflow? The failure looks unrelated, and Melvin couldn’t rerun it. |
|
Why the rerun didn't help: the second attempt reran only Evidence that it's unrelated
|
This is still failing after a retry. Can you reproduce the failure locally?
I'm not familiar with the "C+ small-fix process". Is that written down somewhere? |
|
Analysis: The rerun failed on the same Reassure measurement, Evidence: Today, unrelated PRs fail on the same
Next step: The
|
|
@arosiclair I reran the report-name benchmark locally at I also checked the other failures directly:
The process I meant is documented in the C+ small-fix guidelines: the C+ asks the assigned internal engineer first; if approved, the engineer creates an issue and the C+ opens a PR with a full diff under 20 lines. My request was for the Status Message field’s accessible name, separately from this CI failure. |
|
Thanks for the link. Let's make another issue for that.
@MelvinBot create a new issue to address this bug. Use the standard issue template and assign me and @nyomanjyotisa |
|
Thanks for looking into Reassure Performance Test as well. Looks like it's flakey, so let's ignore that for now. |
|
I created Status page - Message field announces "statusText" instead of its translated label to screen readers from the standard template, and assigned It has the template's The bug was found by reading the code. No one has reproduced it yet, so the staging and platform checkboxes are empty. |
|
🚧 arosiclair 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/arosiclair in version: 9.5.27-0 🚀
|
|
No help site update needed. This PR fixes a bug where the Account avatar kept showing an old status after you cleared it, so the app now does what users already expect. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.27-2 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
On native, split navigators freeze every covered screen by default, including the sidebar. The Account screen (
SCREENS.SETTINGS.ROOT) shows the user's status in the top bar avatar, but the status is edited from Profile > Status, which are stacked on top of it. So the status change always reaches the Account screen while it is frozen, and on Android the avatar keeps showing the old emoji after you clear the status.This PR sets
nonTopScreenBehavior: 'none'on theSETTINGS.ROOTscreen only, so it stays unfrozen while covered and the avatar updates.persistentScreensalready keeps this screen unfrozen there.AI Tests (run by MelvinBot)
npm run lint-changed: passednpm run typecheck: passednpm run spell-changed: passednpm test -- tests/unit/Navigation: 46 suites, 738 tests passednpm run react-compiler-compliance-check check-changed: could not run in the CI sandbox (it couldn't resolve the base branch ref). The change only spreads an options object, so compiler compliance is not expected to change.Fixed Issues
$ #105198
PROPOSAL: #105198 (comment)
Tests
/settingsand/settings/profile/statussigned in and out; verify the page or sign-in opens.Offline tests
QA Steps
Same as Tests and Offline tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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
Android-pr105306-after-live.mp4
Android: mWeb Chrome
Android-Chrome-pr105306-after-clear-live.mp4
iOS: Native
iOS-pr105306-after-clear-verified.mp4
iOS: mWeb Safari
iOS-Safari-pr105306-after-clear-live.mp4
MacOS: Chrome / Safari
Chrome-pr105306-after-clear-verified.mp4
Safari-pr105306-after-clear-verified.mp4