Repository navigation
ReportActionAvatars — WorkspaceAvatar + policyID migration - #98973
roryabraham merged 2 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 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". |
|
@sobitneupane 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] |
sobitneupane
left a comment
sobitneupane
left a comment
There was a problem hiding this comment.
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 Issuessection 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
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - 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 verified the steps for local testing are in the
- 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 usingAvatarhave 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
Avataris modified, I verified thatAvataris 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
Designlabel and/or tagged@Expensify/designso the design team can review the changes.
- 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
mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps. - I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.
Screenshots/Videos
Android: HybridApp
Screen.Recording.2026-08-21.at.18.03.26.mov
Android: mWeb Chrome
Screen.Recording.2026-08-21.at.18.01.35.mov
iOS: HybridApp
iOS: mWeb Safari
Screen.Recording.2026-08-21.at.18.21.38.mov
MacOS: Chrome / Safari
Screen.Recording.2026-08-21.at.17.51.17.mov
|
🚧 roryabraham 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/roryabraham in version: 9.4.59-0 🚀
|
Help site review: no changes requiredI reviewed the changes in this PR and no updates to the help site ( Why: This is a purely internal front-end refactor with no user-facing behavior change. It migrates workspace avatars in list items to resolve from a What I checked
Help articles under No draft PR was created since there is nothing to document. |
|
@jmusial @sobitneupane Could you please confirm if we should use an account with 2+ workspaces with no default WS? Workspace selection list does not appear when creating a report if one of the WS is set as default. Bug7240352_1787436080684.bugg.mp4 |
Hey I'm not sure if there are more preconditions. In my setup I have more workspaces + default set. Assumed 2+ should be enough. Screen.Recording.2026-08-24.at.15.26.58.mov |
|
This PR failing because of the issue #99334 |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.4.59-5 🚀
Bundle Size Analysis (Sentry): |
|
Validation passed Web/Chrome, v9.4.67-0, iPhone13/iOS26.2, v9.4.65-3 Recording.3467.mp4ScreenRecording_09-02-2026.17-59-43_1.MP4 |
|
@m-natarajan I heard that there were still issues with getting workspace list step of repro working. Please see the vid below hope that'd help 😄 Screen.Recording.2026-09-04.at.10.06.22.mov |
|
@jmusial Thanks for the video, It didn't work for me. Is this domain related? Am I missing anything here? Recording.3472.mp4 |
|
@jmusial I just created workspace and set as default for the invited account, Able to see select workspace. Can you check on your side? Recording.3475.mp4 |
|
@m-natarajan maybe it's applauses domain thing? Just checked with regular gmail account and got the selector. Can you try following:
Screen.Recording.2026-09-07.at.14.40.19.mov |
|
Can you please clarify the preconditions for this "default workspace unset / personal / deleted / not eligible" Selector appears when default workspace is billing-restricted (owner past due + past grace period) Recording.3489.mp4
Able to reproduce this Recording.3490.mp4 |
|
@ vid 1 interesting - meybe because it's 3 workspaces it falls under scenario 3 ? TBH I'm just trying to deduce those steps from the code - I think would be best to get here someone that knows when the list should show from business or design perspective ? cc @Expensify/design is this sth you're able to help with ? Or know someone who can ? |
|
What is the exact ask here? Happy to try to help, but I'm not entirely sure what you need us to do. |
|
@shawnborton The issue is with the PR steps: “Press FAB and select ‘Create report’ and verify the workspace selection list opens.” I’m unable to identify clear steps to consistently reproduce this behavior. I’m trying different scenarios to determine the required conditions. |
|
I think you need to have multiple workspaces in your account. Then, go to the create button and create a report, and you should see the report selection screen. |
|
Tried that and not working. Workspace list not displayed when user is invited to multiple workspaces. 646411794-36553561-b627-4144-92b4-d42712b7f066.mp4 |
|
Hmm any idea @jmusial ? |
|
@m-natarajan can you check if changing the default workspace in this account will result in you getting the list ? @shawnborton that's why I started looking for someone who will know how it should work 😄 This PR just replaced avatar components didn't change any of the flow. |
|
@shawnborton @jmusial When I changed the default workspace to second in the list, Workspace list not displayed. For the third one the workspace list displayed. Recording.3524.mp4 |
Thx I think this is the bug then. In this case it should be a list regardless which one is |
|
I'm actually not too sure, let's see what @Expensify/product thinks (cc Tom) |
|
But I would think we should show the list regardless unless you are locked down on domain control. |
|
The original logic for this is was:
So it was essentially just a fallback mechanism for that "personal workspace is the default and we need to know which of 1+ group workspaces to create the report on" case. It sounds like we've had to accommodate billing restriction and domain securityGroup's preferred workspace restrictions at some point, which make sense. So I'd expect that to work as follows:
|
|
@trjExpensify cc. @m-natarajan
✅ Matches code.
✅ Matches code.
🔴 IN CODE: the app does not look at domain restrictions at all when creating a report. A domain-restricted user gets the same behaviour as an unrestricted one: the selector still shows up in the same situations, and it lists every workspace they belong to.
🔴 IN CODE: if the user belongs to more than one workspace, the app shows the workspace selector instead of the billing restriction modal. The modal only appears when the user belongs to exactly one workspace.
🔴 IN CODE: if the default is an eligible group workspace but it is the 3rd+ entry in the policy list, the FAB shows the selector instead of creating directly. FAB only, search router and Reports empty view are not affected. I'm gonna prep a follow up fix to align all the 🔴. |
Nice one, thanks! 👍 |
|
@MelvinBot please create a followup issue based on this comment and assign me to it |
|
🤖 Follow-up issue created and assigned to you: Create report workspace selector ignores domain restrictions and misfires on the FAB It captures all three 🔴 items from your comment, with
No existing open or recently closed issue covers this. |
Explanation of Change
policyIDinstead of each page hand-building aniconsarray.PolicyAvatar+usePolicyIcons(subscribes toavatarURL/nameonly), mirroring the existingAccountAvatar+useAccountIconspair.Fixed Issues
$ #94587
PROPOSAL:
Tests
Precondition: Account with 2+ workspaces, but not created from that account (Testing account must never create own workspace,
creating one would set it as default and kill the repro)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.Screenshots/Videos
Android: Native
Screen.Recording.2026-08-20.at.17.44.54.mov
Android: mWeb Chrome
Screen.Recording.2026-08-20.at.17.46.32.mov
iOS: Native
Screen.Recording.2026-08-20.at.17.36.52.mov
iOS: mWeb Safari
Screen.Recording.2026-08-20.at.17.38.35.mov
MacOS: Chrome / Safari
Screen.Recording.2026-08-20.at.16.42.50.mov