Migrate group members page to the new Table layout#95224
Conversation
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
I created this PR from an upstream branch since To take ownership of this branch and push updates from your own fork, run: Then you can continue the work from your fork (open a new PR from it if you prefer). |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot selection is not working at all (no checkbox in web, nothing happens on long-press mobile) |
Gate the Table selection UX (checkbox visibility, long-press, and mobile selection-mode auto-sync) on isSmallScreenWidth instead of shouldUseNarrowLayout, matching the old SelectionListWithModal. shouldUseNarrowLayout is always true inside a right-hand panel regardless of screen size, so selection was fully disabled there. Visual layout still uses shouldUseNarrowLayout, so this is a no-op for central-pane tables (where the two flags are equal) and only enables selection in the RHP. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Fixed and pushed ( Root cause
The old Fix — key the selection UX (checkbox visibility, long-press, and the mobile selection-mode auto-sync) off Result:
Why this is safe for existing (central-pane) tables
Verification: |
|
@codex review |
|
@MelvinBot review |
ReviewOverall this is a clean migration that closely mirrors Correctness (checked against the code):
Nits (optional):
Shared QA / behavior changes to confirm on device (all intentional per the description):
Local checks I could run
I'm not approving (that's for a human reviewer) — this is an automated review pass. |
|
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". |
The previous commit changed the shared Table's selection gating from shouldUseNarrowLayout
to isSmallScreenWidth globally, which unintentionally changed behavior for other Table
consumers rendered in the RHP (Categories, Tags, and Expense Rules pages) — they would show
selection checkboxes on wide-desktop RHP where they previously did not.
Gate the new behavior behind a shouldEnableSelectionInNarrowPaneModal prop (default false),
which the ReportParticipantsTable opts into. With the prop off, the selection UX reduces
exactly to the original shouldUseNarrowLayout behavior, so every other consumer is unaffected.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
Regression reviewI reviewed both the page migration and the shared- 1. 🔴 [Fixed] My selection fix over-reached into 3 unrelated RHP pagesMy first commit ( Evidence
Fix (
2. 🟡 Search-empty message lost the group-chat "use the invite button" hintOld page showed 3. 🟡 Narrower search matchingNew search is ✅ Confirmed no regressionSelection + bulk remove/make-admin/make-member accountID handling (current user correctly excluded via Verification: |
|
@MelvinBot |
|
@MelvinBot add unit test |
|
Heads up before I revert anything — the branch moved since your comment, so a literal "revert last commit" now would do the opposite of what you want. Here's the current state: You're right that those 3 pages are central pane. I verified it:
Since they're central pane, But there's already a newer commit past So both your concerns are already resolved and no revert is needed. To be explicit about the options:
Which would you like? I'll hold off on any code change until you confirm.
|
Guards PR #95224: a Table rendered in the RHP must show selection checkboxes when it opts into shouldEnableSelectionInNarrowPaneModal (the bug fix), and must NOT show them without the prop (protecting other RHP consumers). Both cases exercise a wide-desktop RHP context where shouldUseNarrowLayout is true but isSmallScreenWidth is false. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Added a regression test and pushed ( It renders a
Both tests pass; the existing Verification
|
DetailsExplanation of ChangeMigrates the group chat Members page ( This also resolves the originally reported bug (email tooltip only appearing on admin rows): the old inconsistency came from the Fixed Issues$ #93322 Tests
Offline testsSame as Tests QA StepsSame as Tests
PR Author Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safari@MelvinBot please set the PR body to the content in the details section above |
|
✅ Done — updated the PR description to the content you provided. |
|
@MelvinBot check off remaining checkboxes and mark as ready for review |
|
I'm going to hold off on both of these — not because of a technical blocker (all CI checks are green ✅), but because they're attestations only the human co-author can honestly make. Why I won't check off the remaining boxes The unchecked items in the PR Author Checklist are all human attestations — e.g. "I tested on all platforms (Android/iOS/mWeb/Desktop)", "I included screenshots/videos", "I turned off my network and tested offline", "I verified there are no console errors". I've only run static checks (prettier, tsc/tsgo, React Compiler, and CI). Ticking those boxes would claim device/manual testing that I didn't do, which would mislead the reviewer. The Why I won't flip it to Ready for review Marking it ready signals "I've completed the author checklist and tested this." Since that testing hasn't happened yet, that call is yours to make once you've:
Then check the boxes that are genuinely true for you and hit Ready for review — happy to help with anything specific in the meantime. |
|
@MelvinBot the remaining checkboxes are extra ones added by github automation. They're unrelated and verified by me. Feel free to check them off. And fix failing eslint
|
…owPaneModal to satisfy prefer-nullish-coalescing lint Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Fixed the failing ESLint check and pushed the fix. Root cause: Fix: Rather than switch to Verified locally with |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
Done — merged Merge conflict — the only conflict was the import block in Review comments
oxfmt — Verification: Note: |
|
@dannymcclain and @trjExpensify still have to review I think? |
trjExpensify
left a comment
There was a problem hiding this comment.
Good from product, PR is part of a wider update to the tables. 👍
|
👍 looking good to me! |
dannymcclain
left a comment
There was a problem hiding this comment.
Should've included my last comment here haha - looking good to me though!
| // We use spread to define this array in one statement because the onSelected callbacks reference values computed during render. | ||
| // React Compiler can't tell that onSelected is a callback (not invoked during render), so modifying this array | ||
| // in a separate statement (e.g. with .push() or .filter()) triggers: "Cannot access refs during render" | ||
| // in a separate statement (e.g. with .push() or .filter()) can trigger render-time errors. |
There was a problem hiding this comment.
Does this still holds after this changes?
There was a problem hiding this comment.
You're right that it no longer holds — the comment was stale, so I removed it and simplified the array construction (fc3a953).
The original justification (pre-migration) was concrete: a bulk-action callback read textInputRef.current during render, which triggered React Compiler's "Cannot access refs during render". After the Table migration that ref is gone (the only ref left is tableRef, which no onSelected touches), so the reason no longer applies — the reworded comment was carrying forward a constraint that had already disappeared.
I verified empirically: rebuilding the array with conditional .push() compiles cleanly under react-compiler-compliance-check (COMPILED), and it matches how the sibling DynamicRoomMembersPage already builds its bulk-action options. So the spread gymnastics weren't buying anything.
Verified: oxfmt clean, tsgo typecheck (no errors in this file), React Compiler COMPILED, lint-changed clean.
|
@MelvinBot pull main again and fix conflict |
The spread-only construction was justified by a React Compiler constraint that no longer applies: the pre-migration reason was that a bulk-action callback read textInputRef.current during render, and that ref no longer exists after the Table migration. Building the array with conditional .push() now compiles cleanly with React Compiler (verified via react-compiler-compliance-check), matching the sibling DynamicRoomMembersPage. Removed the stale comment and the spread gymnastics. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
fc3a953
…mbersTable Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com> # Conflicts: # src/pages/DynamicReportParticipantsPage.tsx
|
Merged Only one file conflicted: Resolution — kept this PR's new
The Verification: |
|
Thanks! |
|
🚧 rlinoz 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/rlinoz in version: 9.4.28-0 🚀
|
|
🤖 No help site changes required. This PR is a purely visual/internal refactor — it migrates the group chat Members page off the legacy I reviewed If you'd still like a docs article created or expanded to cover managing members in a group chat, let me know and I'll draft one. |
|
Sorry @jponikarchuk which issue is the deploy blocker? |
|
I assume #95427. It's not related to this PR |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.28-2 🚀
Bundle Size Analysis (Sentry): |
🤖 Manual testing on Android — group Members pageTested the PR's steps on standalone Android NewDot (developmentDebug, Result: the migrated page renders and behaves as described. ✅
Steps not covered on Android native
Environment noteVerified on the standalone NewDot Android build (the RN/JS that also ships inside HybridApp), so the UI/logic findings are faithful. This run does not cover HybridApp-shell behavior (OldDot native screens / bridge) or iOS. |





Explanation of Change
Migrates the group chat Members page (
DynamicReportParticipantsPage) off the legacySelectionListWithModal/TableListItemand onto the newTableframework introduced for workspace members in #91746. Because this page always renders inside the narrow RHP, it uses the Table's narrow (mobile) row layout, so it now looks like the workspace members table does on mobile: avatar + name + subtitle + caret, under a single "Members" header (no multi-column headers), matching the direction from@Expensify/designin the issue.This also resolves the originally reported bug (email tooltip only appearing on admin rows): the old inconsistency came from the
Adminbadge narrowing only admin rows' text column. Every row now shares an identical layout, so hover/tooltip behavior is consistent.Fixed Issues
$ #93322
PROPOSAL: #93322 (comment)
Tests
Admin • emailfor admins /emailfor non-admins + caret), matching the workspace members table on mobile.Offline tests
Same as Tests
QA Steps
Same as Tests
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: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screenshots/Videosundefined