Repository navigation
fix: skip participant picker for group workspace share sheet - #100309
nabi-ebrahimi wants to merge 50 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa6e03c7f7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // to the picker if the locked policy's expense chat isn't in Onyx yet, so we never navigate to an empty report. | ||
| const lockedExpenseChatReportID = | ||
| isSubmitFlow && isRestrictedToPreferredPolicy && preferredPolicyID ? getPolicyExpenseChat(currentUserAccountID, preferredPolicyID)?.reportID : undefined; | ||
| const defaultExpenseChatReportID = canUseDefaultPolicy ? getPolicyExpenseChat(currentUserAccountID, defaultExpensePolicy?.id)?.reportID : undefined; |
There was a problem hiding this comment.
When an unrestricted user's qualifying default group policy and personal policy both have autoReporting disabled, this always resolves that policy's expense chat and submits the shared receipt as a workspace expense. The normal global-create path explicitly selects the self-DM/Track destination in this scenario (NavigateGlobalCreateContext.tsx lines 99-102), so the share flow now bypasses the user's disabled auto-reporting configuration and sends expenses to the wrong destination. Choose the self-DM unless either policy enables auto-reporting.
Useful? React with 👍 / 👎.
|
@dmkt9, I verified the expected flow on iOS (both app open and closed) and on Android (app already open). In all these scenarios, sharing a photo with an eligible default workspace opens the Submit expense flow correctly. However, on an Android cold launch, the app does not consistently reach the Submit flow. This occurs before the share participant selector runs: Android receives ACTION_SEND, and native code immediately dispatches new-expensify://share/root via onNewIntent() while React Native and navigation are still initializing. Consequently, the route or shared-file handoff is lost before the JS share flow is ready. This is a pre-existing Android native cold-start handoff defect, unrelated to this PR's changes regarding default workspace selection. How should we proceed with this pre-existing native bug? Should we handle this in a follow-up PR so we can keep this PR focused on its original scope? cc @NikkiWines |
Yes. I noticed this issue while testing this PR. It seems that |
|
I agree with you both that's a separate bug and should be handled in a separate issue/PR 👍 |
@NikkiWines Could you please create an issue for that and assign it to both @nabi-ebrahimi and me? Thanks |
| const canUseDefaultPolicy = | ||
| isSubmitFlow && | ||
| !isRestrictedToPreferredPolicy && | ||
| shouldUseDefaultExpensePolicy(CONST.IOU.TYPE.CREATE, defaultExpensePolicy, amountOwed, userBillingGracePeriodEnds, ownerBillingGracePeriodEnd, currentUserAccountID); |
There was a problem hiding this comment.
Add a comment explaining why we use CONST.IOU.TYPE.CREATE instead of CONST.IOU.TYPE.SUBMIT, because we only want to check whether the default policy can be used.
There was a problem hiding this comment.
Thanks for the feedback. Added a comment explaining why we intentionally pass CONST.IOU.TYPE.CREATE: shouldUseDefaultExpensePolicy is reused only for default-policy eligibility, and passing SUBMIT would always return false.
| const {isRestrictedToPreferredPolicy, preferredPolicyID} = usePreferredPolicy(); | ||
| const {isLoadingSecurityGroup} = useUserSecurityGroup(); | ||
| const defaultExpensePolicy = useDefaultExpensePolicy(); | ||
| const personalPolicy = usePersonalPolicy(); |
There was a problem hiding this comment.
If the user's default workspace is set to a group workspace
I don't think we want to use personalPolicy here
There was a problem hiding this comment.
Thanks for the feedback. I kept personalPolicy intentionally to match the existing Global Create behavior in NavigateGlobalCreateContext and useDefaultParticipants, both of which use defaultExpensePolicy.autoReporting || personalPolicy.autoReporting when choosing the automatic destination.
Removing it here would make Share Submit behave differently: when the default group workspace has auto-reporting disabled but the personal policy enables it, Share Submit would route to self-DM while Global Create routes to the default workspace. Please let me know if Share Submit is intended to diverge from that established behavior.
There was a problem hiding this comment.
Hmm. Please read the request in the original post. Do we use useDefaultParticipants in the current share submit flow?
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
@codex review |
| // calling setState synchronously in the effect body) avoids the react-hooks/set-state-in-effect violation. | ||
| Navigation.navigate(detailsPageRouteObject.getRoute(lockedExpenseChatReportID.toString()), { | ||
| afterTransition: () => setHasAutoNavigatedToLockedReport(true), | ||
| Navigation.navigate(detailsPageRouteObject.getRoute(autoNavigateReportID.toString()), { |
There was a problem hiding this comment.
I think we should set selectedReportID here as well
There was a problem hiding this comment.
Thanks for the suggestion. I now store autoNavigateReportID in selectedReportID after the transition completes, so the automatically selected workspace remains selected if the user navigates back to the participant picker.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b5dc07c80
ℹ️ 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".
| const shouldAutoReport = !!defaultExpensePolicy?.autoReporting || !!personalPolicy?.autoReporting; | ||
| let defaultReportID: string | undefined; | ||
| if (canUseDefaultPolicy) { | ||
| defaultReportID = shouldAutoReport ? getPolicyExpenseChat(currentUserAccountID, defaultExpensePolicy?.id)?.reportID : selfDMReport.reportID; |
There was a problem hiding this comment.
When the account has no loaded self-DM yet, useSelfDMReport() returns a newly generated optimistic report, but this component only takes its ID and never saves that report. For a default workspace with auto-reporting disabled, the effect therefore navigates to a random report ID; SubmitDetailsPage cannot resolve it as a self-DM, derives an empty Submit destination, and cannot submit until the user backs out and selects again. Use useResolvedSelfDMReport() to wait for hydration, and if the account genuinely has no self-DM, persist the optimistic report before navigating.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Let's remove selfDMReport.reportID
There was a problem hiding this comment.
Thanks for the clarification. Removed the self-DM fallback and the personalPolicy dependency. Share Submit now auto-navigates only when the qualifying default group workspace has auto-reporting enabled; otherwise, it falls back to the participant picker.
| const isDestinationReady = | ||
| isPersonalDetailsReady && | ||
| !isLoadingOnyxValue(activePolicyIDMetadata, policiesMetadata, reportsMetadata, amountOwedMetadata, userBillingGracePeriodEndsMetadata, ownerBillingGracePeriodEndMetadata); |
There was a problem hiding this comment.
On a cold share-extension start, the listed Onyx values can finish loading before the separate PERSONAL_POLICY_ID subscription used by usePersonalPolicy(). In that window isDestinationReady becomes true while personalPolicy is still undefined, so when the default workspace has autoReporting disabled but the personal policy enables it, the flow incorrectly selects the self-DM and sets the one-shot resolution guard; the later personal-policy update cannot redirect to the workspace chat. Include the personal-policy ID/value loading metadata in shouldWaitForDestination.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for the feedback. This was applicable when personalPolicy participated in destination selection. Per the later guidance, I removed the personalPolicy dependency and now auto-navigate only when the default group workspace itself has auto-reporting enabled. Therefore, there is no personal-policy hydration state to include in destination readiness.
I was seeing some errors on my side |
|
The issue is here: #101194 - please comment on it so i can assign you both 🙇 |
I fixed the focus restoration issue when changing the workspace from the Share confirmation page. The previously focused field is now restored after returning to Confirm details in both portrait and landscape modes. The landscape fix is scoped to the pending Share-field restoration, so it does not change the application-wide input focus behavior. fixed Demo 1792992490436.mp4 |
|
I found and fixed an additional issue in the editable Share destination flow. When a shared workspace expense had a negative amount, the destination picker still allowed selecting a personal 1:1 chat. P2P money requests do not support negative amounts, so this could carry an invalid negative workspace expense into a personal destination. Reproduction:
The Share picker now reuses the standard expense-flow behavior by excluding P2P and new-email recipients whenever the preserved draft amount is negative. Workspace destinations remain available, and the entered amount and other draft fields are preserved. After the fix, reopening the destination picker for a negative expense displays only supported workspace destinations. I also added focused regression coverage. Reproduction and fixed-demo recordings are available for review. Reproduced Demo bug-negative-amount.mp4Fixed Demo fix-negative-amount.mp4 |
|
@dmkt9, I’ve addressed all your feedback. Please take another look when you have a chance. Thanks! |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@nabi-ebrahimi Sorry for the delay. Reviewing... |
|
Bug: The negative amount filter excludes Self DM, even though Self DM supports negative amounts. However, I'm not sure excluding P2P is the right behavior either. Previously, the amount was cleared instead of preserved, so P2P was always available for selection. 2026-11-06.15-54-39.mp4 |
|
Bug: Workspace defaults aren't applied when opening Submit for the first time
2026-11-06.16-08-14.mp4 |
|
Bug: P2P isn't marked as selected after selection (also reproducible on staging) 2026-11-06.16-12-38.mp4 |
|
@MelvinBot regression review |
Addressed the negative-amount destination-picker issue. Root Cause: The picker previously treated any negative amount as Fix: Updated Updated Fixed DEMO fix.1.mp4 |
Fixed the initial Share Submit workspace-default issue. Root Cause: While the policy defaults resolved correctly, Fix: Added fixed DEMO fix.2.mp4 |
Fixed the missing selection indicator for P2P destinations. Root Cause: Newly selected P2P chats use an optimistic Fix: Updated the picker selection matching logic to check against both Fixed DEMO fix.3.mp4 |
|
@dmkt9, I Identified and resolved a destination editing issue found during testing: While testing destination switching in the Share flow, I noticed that selecting Self DM shifted the internal IOU type to Fix: Enabled participant editing for the Share confirmation flow regardless of whether the destination is Reproduced DEMO bug.4.mp4Fixed DEMO fix.4.mp4 |
|
@dmkt9 Thanks for the review! I’ve addressed all the issues you reported, and I also caught and fixed an additional bug while testing. Please take another look when you get a chance |
|
Claude posting on behalf of Matt: Closing this PR. Work on the linked issue continues in a new PR. Use this one as a reference. |
Explanation of Change
This PR makes the Share Submit flow wait until all destination data is fully available before resolving or automatically navigating the user.
It now verifies that the current user’s personal details are loaded, including a valid account ID and login, and waits for report collection metadata alongside policy, billing, and security-group data. This prevents the flow from prematurely committing to the participant picker or navigating to an incomplete/default destination while Onyx data is still loading.
The loading logic is consolidated into
shouldWaitForDestinationand used consistently by both destination-resolution effects and the loading UI, ensuring restricted-workspace and default-policy auto-navigation only run after the required data is ready.Fixed Issues
$ #98805
PROPOSAL: #98805 (comment)
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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-09-09.at.3.48.09.PM.mov
Android: mWeb Chrome
N/A
iOS: Native
Screen.Recording.2026-09-03.at.7.46.48.PM.mov
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
N/A