Fix blank background when copiloting into an agent from the agent-DM RHP - #97869
Conversation
Co-authored-by: Michał Jasikowski <mjasikowski@users.noreply.github.com>
Co-authored-by: Michał Jasikowski <mjasikowski@users.noreply.github.com>
|
The two failing checks are review/process gates, not code failures — there's nothing to fix by pushing code.
Every actual code check is green (Bun tests, ESLint, Oxfmt, React Compiler, Storybook, unused styles, knip, all builds, So no code fix is warranted here — the PR just needs the author checklist completed and an independent review/approval. |
|
Re-checked — nothing has changed since my previous comment. The two remaining failing checks aren't code failures, so there's nothing to push:
All code checks are green (Bun tests, ESLint, Oxfmt, React Compiler, Storybook, typecheck, unused styles, knip, all builds, |
|
🚧 mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@MelvinBot fix conflicts |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
…ankBackground Co-authored-by: Michał Jasikowski <mjasikowski@users.noreply.github.com> # Conflicts: # src/libs/Navigation/AppNavigator/withAgentAccessDenied.tsx
|
🤖 Merged What conflicted: How I resolved it: adopted Test fix from the merge
Verified locally:
|
|
I looked into the failing checks — there's no code fix to push here. The only two failing checks are process gates, not CI/code failures:
Every actual CI check is green: To get this mergeable: complete the PR Author Checklist and fill in the |
|
@eVoloshchak 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] |
|
Re-checked — nothing has changed since my earlier comment. The only two failing checks are still the non-code gates:
All real CI is green, including There's no code fix to push. To make this mergeable: complete the PR Author Checklist + |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // isAgent transition here too so the background is corrected immediately. Skip when focused since | ||
| // useFocusEffect already covers that case. | ||
| useEffect(() => { | ||
| if (isFocused) { |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This newly added comment uses em dashes (—) to set off clauses in its own sentences (lines: mounted but NOT focused — e.g. the owner taps ... and over this guarded central pane — useFocusEffect never runs). Comments should read as plain, natural sentences without em/en dashes.
Rewrite using ordinary punctuation, for example:
// useFocusEffect only fires while this screen is focused. When the session flips to an agent while
// this guarded screen is mounted but NOT focused, for example the owner taps "Copilot into account" from
// an unguarded RHP (the agent DM) sitting over this guarded central pane, useFocusEffect never runs,
// so the pane renders null (blank background) until the RHP is closed. Drive the redirect off the
// isAgent transition here too so the background is corrected immediately. Skip when focused since
// useFocusEffect already covers that case.Reviewed at: 15f43a8 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed in 7bec3bc — replaced the em dashes with plain punctuation (, for example ...,) in this comment.
| }); | ||
| }, [isAgent]), | ||
| ); | ||
| // useFocusEffect only fires while this screen is focused. When the session flips to an agent while |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This comment uses an arrow (->) in its own sentence (the initial false->true transition) instead of writing the relationship in words. Comments should express the relationship in plain English.
Rewrite, for example:
// Redirect on every focus (not just the initial transition from false to true) so navigating backReviewed at: 15f43a8 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed in 7bec3bc — rewrote false->true as transition from false to true.
|
Update: the Every actual CI check is green (typecheck, all |
|
@MelvinBot address reviewer comments |
Co-authored-by: Michał Jasikowski <mjasikowski@users.noreply.github.com>
|
@chuckdries @mjasikowski One of you needs to 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] |
|
🤖 Addressed both reviewer comments (CONSISTENCY-16, from the automated code-review bot) in
The pre-existing Verified |
|
Re-checked — status is unchanged, and there's still no code fix to push. The only two failing checks are non-code gates:
Every real CI check is green ( To clear these: the PR needs a reviewer to complete the Reviewer Checklist and give an independent approval. No branch changes are needed from me. |
|
Still no code fix to push — the two failing checks are both human-gate checks, not CI/code failures:
All actual CI checks are green ( Both remaining failures clear once a reviewer reviews and approves the PR — nothing for me to change on the branch. |
|
@eVoloshchak would you mind reviewing and checklisting this? thanks |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppscreen-20260818-133213-1787052676932.mp4Android: mWeb ChromeScreen.Recording.2026-08-18.at.13.13.47.moviOS: HybridAppScreen.Recording.2026-08-18.at.13.11.46.moviOS: mWeb SafariScreen.Recording.2026-08-18.at.13.09.36.movMacOS: Chrome / SafariScreen.Recording.2026-08-18.at.13.08.10.movScreen.Recording.2026-08-18.at.13.07.39.mov |
|
🤖 There's nothing to fix in the code here — every real CI check is passing ( The only failing check is What's going on: Since I can't resolve an approval gate with a code change, I've made no changes to the branch. Let me know if you actually want me to look at something in the diff instead. |
|
🚧 chuckdries 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/chuckdries in version: 9.4.56-0 🚀
|
|
🤖 Help site review: no changes required I reviewed the changes in this PR against Expensify's help site articles under Why: This is a purely internal rendering/navigation bug fix. It adds a focus-independent There is:
Help articles document product behavior, not internal bug fixes for visual glitches, so nothing here needs to be reflected on the help site. @mjasikowski, since no help site changes are required, there's no linked docs PR to review or mark |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.4.56-3 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
When an owner creates an agent on a wide/split layout, #95631 opens the owner↔agent DM in a dedicated unguarded RHP (
AGENT_REPORT) sitting over the guarded Agents settings pane. If the owner then taps Copilot into account from that RHP, the session email flips to the agent, soisAgentbecomestrueand the guardedAgentsPagecentral pane returnsnull— a blank background — until the RHP is closed.The redirect that should send the guarded pane to Profile lived only inside
useFocusEffect, which never fires here because the central pane is mounted but not focused (the RHP holds focus). Closing the RHP finally re-focused the pane and ran the effect, which is why "Profile appears" only after closing the RHP.This adds a focus-independent
useEffect(keyed on theisAgenttransition) that runs the same redirect when the guarded pane is mounted-but-unfocused, so the background is corrected immediately instead of rendering blank. The redirect logic is factored into a sharedredirectAgentAwaycallback reused by bothuseFocusEffectand the new effect; the new effect is skipped while focused sinceuseFocusEffectalready covers that case. Because the agent-DM RHP is the topmost modal, the existingdismissModal+ deferred-redirect path dismisses it and lands the user on Profile — matching the behavior of the already-guarded copilot path.Fixed Issues
$ #97843
PROPOSAL:
Tests
Offline tests
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.