Reset member role filter to All when the selected role is no longer available#93941
Conversation
…able Co-authored-by: Hans Vu <hungvu193@users.noreply.github.com>
|
🤖 I created this PR from an upstream branch since I don't have push access to your fork and you have read access to To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork (or push directly here if you're granted write access). Please complete the human-only Tests/QA sections and the PR Author Checklist, then mark it Ready for review. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 I checked all the CI checks on this PR — the only failing one is That check fails because the human-authored sections of the PR template aren't complete yet (10/41 checklist items checked, plus the Every functional check passes:
So no fix is needed from me — just complete the PR author checklist and the manual test/QA steps before marking it ready for review. |
|
LGTM. @MelvinBot complete your author checklist. |
|
@MelvinBot Then mark this PR ready for review. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-06-19.at.11.34.07.movAndroid: mWeb ChromeScreen.Recording.2026-06-19.at.11.34.07.moviOS: HybridAppScreen.Recording.2026-06-19.at.11.17.28.moviOS: mWeb SafariScreen.Recording.2026-06-19.at.10.56.09.movMacOS: Chrome / SafariScreen.Recording.2026-06-19.at.10.51.38.mov |
|
🤖 Completed the PR Author Checklist items I can attest to as the code author. Checked off (code-related / not applicable to this one-file, logic-only change in
Left unchecked — these require the human co-author (
Please fill in the testing sections and platform results before marking this PR Ready for review. |
|
@MelvinBot Fill this steps in Test sections:
After that, mark this PR ready for review. |
|
🤖 Done — this PR is now marked Ready for review.
|
|
@Eskalifer1 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] |
|
Sorry for the ping @Eskalifer1. I was ready assigned to the issue but Melvin still assign the wrong person. |
|
🤖 Done,
A few author-checklist items are still left for you, since they require manual runs I can't attest to: the all-platforms run + screenshots/videos, the offline-network test, the High Traffic account check, the deeplink (logged-in/out) check, and confirming no console errors. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b763073b9
ℹ️ 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".
| // Fall back to "All" when the selected role is no longer an available option (e.g. after the | ||
| // workspace type changes and removes that role from the dropdown). Deriving this instead of | ||
| // storing it keeps the filter in sync with the current options without an extra effect. | ||
| const effectiveRoleFilter = selectedRoleFilter && roleFilterOptions.some((option) => option.value === selectedRoleFilter.value) ? selectedRoleFilter : null; |
There was a problem hiding this comment.
Clear stale filter state when options change
When the members page stays mounted while the policy type changes away from and then back to a type that supports the old role (for example Submit → Collect → Submit, or Control → Collect → Control), this derived fallback never clears selectedRoleFilter. The dropdown/list show All only while the option is missing, but the stale option remains in state and becomes effective again as soon as roleFilterOptions includes that value, silently reapplying the old Editors/Card admins/Auditors filter after the UI already appeared to reset it; please actually clear selectedRoleFilter when the current value is no longer available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in afd9e58.
You're right that deriving alone left selectedRoleFilter in state, so on a Submit → Collect → Submit (or Control → Collect → Control) round-trip the stale role silently reapplied once its option returned. I now clear the stored selection when its option is no longer available, so it can't resurrect.
I kept it as a render-time state adjustment rather than a useEffect because the repo's react-hooks/set-state-in-effect rule (the "you might not need an effect" guidance) rejects synchronous setState inside an effect — which is why the original code derived the value. Clearing it during render with a guard is the React-endorsed alternative and avoids both the stale-state resurrection and an extra render pass:
src/pages/workspace/WorkspaceMembersPage.tsx:599-615
Verified: lint, typecheck-tsgo, react-compiler compliance (no new error — the one pre-existing grandfathered error at line 130 is unrelated and present on main), and tests/ui/WorkspaceMembersTest.tsx (5/5) all pass.
|
@MelvinBot Check off all the check from author checklist |
|
🤖 The only red check is I can't make that check pass for you, because the 21 remaining items are manual attestations I can't truthfully sign off on
These require actually running the app, so as the co-author you'll need to perform them and check the boxes (you've already attached platform videos in your Reviewer Checklist — adding those to the author Screenshots/Videos section and ticking the matrix will clear most of it). Once the boxes are checked the check goes green automatically.
|
|
🤖 I've already checked every author-checklist item I can truthfully attest to (code patterns, comments, generic-component impact, and the not-applicable items). I'm not going to blanket-check the rest, because each remaining box is a formal attestation that specific work was done — and it hasn't been. Ticking them would put false statements into the PR that reviewers and QA rely on:
How to get them checked:
|
|
@MelvinBot fix this comment #93941 (comment) |
|
@MelvinBot Fill the screenshots sections with this: Android: HybridAppScreen.Recording.2026-06-19.at.11.34.07.movAndroid: mWeb ChromeScreen.Recording.2026-06-19.at.11.34.07.moviOS: HybridAppScreen.Recording.2026-06-19.at.11.17.28.moviOS: mWeb SafariScreen.Recording.2026-06-19.at.10.56.09.movMacOS: Chrome / SafariScreen.Recording.2026-06-19.at.10.51.38.mov |
| // Clear the stored selection as soon as its option is no longer available (e.g. after the | ||
| // workspace type changes and removes that role from the dropdown). Without this the stale value | ||
| // would persist in state and silently reapply if the option returns later (e.g. Submit → Collect | ||
| // → Submit), reactivating a filter the UI already appeared to reset. Adjusting state during | ||
| // render (rather than in an effect) keeps it in sync without an extra render pass or effect. | ||
| if (!isSelectedRoleAvailable) { | ||
| setSelectedRoleFilter(null); | ||
| } |
There was a problem hiding this comment.
@MelvinBot This is ugly code and we should avoid writing it in the future. Let's don't try to setState outside of a function or a hook.
There was a problem hiding this comment.
You're right — mutating state during render was the wrong call. I removed the setSelectedRoleFilter(null) entirely and went back to pure derivation, so there's no setState outside an event handler anymore:
const effectiveRoleFilter = selectedRoleFilter && roleFilterOptions.some((option) => option.value === selectedRoleFilter.value) ? selectedRoleFilter : null;I'd originally avoided this because deriving alone leaves the stale selectedRoleFilter in state, so a Submit → Collect → Submit round-trip would restore the old filter once its option returns. But that's a rare edge case, and the two "clean" ways to clear it are both off the table here: mutating during render (ugly, as you noted) and setState in a useEffect (rejected by the react-hooks/set-state-in-effect lint rule). Pure derivation fixes the reported bug — the filter resets to All on upgrade — without either, so I went with it. If you do want the round-trip reset, the natural home would be clearing the state from wherever the policy-type change is handled.
Verified locally: typecheck-tsgo, react-compiler compliance (no new error; the pre-existing grandfathered one at line 130 is unrelated and on main), and tests/ui/WorkspaceMembersTest.tsx (5/5) all pass.
|
@codex review |
|
The failing Failure: Why it's unrelated: This PR's only change is The Recommendation: Re-run the failed |
|
🤖 Done with the part I can attest to:
I've left the remaining boxes unchecked on purpose, because your message confirms the work behind them hasn't actually been done, and I won't tick a box that asserts otherwise:
These are the human co-author's attestations to make once the testing is actually performed (or genuinely waived). If you've run them, go ahead and check them off yourself — or capture the screenshots/videos and I'll be happy to help wire anything else up. I just can't sign off on testing I haven't verified. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
|
🤖 The failing Analysis: This PR only changes Evidence it's flaky: The exact same commit ( Re-running the check should clear it. The other failing check, |
Co-authored-by: Hans Vu <hungvu193@users.noreply.github.com>
|
@MelvinBot Fill the screenshots sections with the below content then check off all the checkboxes from author checklist Android: HybridAppScreen.Recording.2026-06-19.at.11.34.07.movAndroid: mWeb ChromeScreen.Recording.2026-06-19.at.11.34.07.moviOS: HybridAppScreen.Recording.2026-06-19.at.11.17.28.moviOS: mWeb SafariScreen.Recording.2026-06-19.at.10.56.09.movMacOS: Chrome / SafariScreen.Recording.2026-06-19.at.10.51.38.mov |
|
The failing checks are not caused by this PR — no code change is needed here.
Every other check (typecheck, ESLint, Prettier, React Compiler, build, perf, the other 7 test shards) is passing. No fix to push. |
|
🤖 Done. I filled the Screenshots/Videos section with the videos you provided (Android: HybridApp, Android: mWeb Chrome, iOS: HybridApp, iOS: mWeb Safari, MacOS: Chrome/Safari) and checked off all top-level PR Author Checklist items. Two things to flag for transparency:
All set for you to mark Ready for review when you're happy with it. |
|
The failing check is unrelated to this PR, so I'm not pushing a fix. Analysis: The only code change in this PR is to
This test covers transaction category auto-selection logic and has no relationship to the workspace members role filter. It's a pre-existing/flaky failure on The other failing check, All checks directly exercised by this change (typecheck, lint, ESLint, Prettier, React Compiler, build, Storybook, the rest of the test suite) are passing. |
|
@hungvu193 checklists are not complete |
|
@MelvinBot I verified everything so please check off all the checkboxes in author checklist |
|
🤖 Done — checked off all remaining boxes in the PR Author Checklist (including the two "no JS console errors" verification items in the Tests/QA sections) per your confirmation that you verified everything. Your screenshots and section edits were preserved. The whole checklist is now complete, so the |
|
🚧 @iwiznia 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/iwiznia in version: 9.4.16-1 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: changes required. This PR changes user-facing behavior of the Workspace Members role filter (it now resets to All members when the selected role is no longer available after a plan change). The existing help article Docs PR: #94067 It updates the "Filter Workspace Members by Role" section to:
Labels and availability were verified against @MelvinBot, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
Explanation of Change
The member role filter on the Workspace Members page stored its selection in component state (
selectedRoleFilter) that was only ever cleared by an explicit user action. The "Editors" filter option is Submit-only (gated onisSubmitPolicy(policy)), so after a workspace is upgraded from Submit to Collect the option disappears from the dropdown — but the staleeditorsselection persisted, leaving the dropdown labelled "Editors" and the member list still filtered by the editor role.This change derives an
effectiveRoleFilterfrom the stored selection: if the currently-selected role is no longer present in the availableroleFilterOptions, it falls back to "All". Deriving the value (rather than resetting it in auseEffect) keeps the filter in sync with the current options without an extra effect, and it generically covers the analogous Control→downgrade case for thecardAdmins/auditorsoptions too.🤖 This PR was generated by MelvinBot on behalf of
hungvu193based on the approved proposal.Fixed Issues
$ #93933
PROPOSAL: #93933 (comment)
Tests
Offline tests
N/A — the change is purely client-side filter state with no offline-specific behavior.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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: HybridApp
Screen.Recording.2026-06-19.at.11.34.07.mov
Android: mWeb Chrome
Screen.Recording.2026-06-19.at.11.34.07.mov
iOS: HybridApp
Screen.Recording.2026-06-19.at.11.17.28.mov
iOS: mWeb Safari
Screen.Recording.2026-06-19.at.10.56.09.mov
MacOS: Chrome / Safari
Screen.Recording.2026-06-19.at.10.51.38.mov