Skip to content

Migrate group members page to the new Table layout#95224

Draft
MelvinBot wants to merge 2 commits into
mainfrom
claude-migrateGroupMembersTable
Draft

Migrate group members page to the new Table layout#95224
MelvinBot wants to merge 2 commits into
mainfrom
claude-migrateGroupMembersTable

Conversation

@MelvinBot

@MelvinBot MelvinBot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Migrates the group chat Members page (DynamicReportParticipantsPage) off the legacy SelectionListWithModal / TableListItem and onto the new Table framework 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/design in the issue.

This also resolves the originally reported bug (email tooltip only appearing on admin rows): the old inconsistency came from the Admin badge narrowing only admin rows' text column. Every row now shares an identical layout, so hover/tooltip behavior is consistent.

New components (modeled on WorkspaceMembersTable):

  • src/components/Tables/ReportParticipantsTable/index.tsx — wraps Table, defines the member / role (group chats only) / actions columns, sorts by name, and searches over name/email/login via tokenizedSearch.
  • src/components/Tables/ReportParticipantsTable/ReportParticipantsTableRow.tsx — the narrow row (avatar + name + subtitle + ArrowRight caret).

Decisions applied (confirmed by mkhutornyi in the issue):

  1. Role display — only admins surface a role (Admin • email in the subtitle); non-admins show just their email, matching production. Members do not show a "Member" label.
  2. Search bar — only rendered when there are STANDARD_LIST_ITEM_LIMIT or more active members, preserving the previous gating.
  3. Selection continues to work through the Table's built-in selection middleware (checkboxes on wide screens, long-press → bottom-docked "Select" modal in mobile selection mode). Invite, remove, and make-admin/make-member bulk actions are unchanged and still live in the page header.

Intentional behavior changes worth a reviewer's attention:

  • The persisted find-member search phrase (ROOM_MEMBERS_USER_SEARCH_PHRASE) is dropped; the Table owns search state (same as the workspace members table).
  • The custom empty-search message ("member not found — use the invite button") is replaced by the Table's standard "No results found" empty state.

Verification performed by MelvinBot (static checks only): prettier, lint-changed, full tsc typecheck (exit 0), tsgo (0 errors), and React Compiler compliance check (all three files COMPILED). No automated UI test exists for this page; visual/interaction QA is still needed (see Tests/QA).

Fixed Issues

$ #93322
PROPOSAL: #93322 (comment)

Tests

// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"

  1. Open a group chat with 3+ members (as an admin), then open the chat header → Members.
  2. Verify the list renders as the new table (single "Members" header, avatar + name + Admin • email for admins / email for non-admins + caret), matching the workspace members table on mobile.
  3. Hover a non-admin member's email and verify the tooltip behaves the same as for admins.
  4. Verify invite, select (long-press on mobile / checkboxes on web), remove, and make-admin/make-member all still work.
  5. Open a non-group participant list (e.g. a chat room / thread) and verify it still renders read-only with no role column or selection.
  6. Verify that no errors appear in the JS console.

Offline tests

// TODO: The human co-author must fill out the offline tests.

  1. Go offline, remove/add a member, and verify the pending (offline) row styling still shows.

QA Steps

// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • 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 followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • 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 new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • 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 Avatar is modified, I verified that Avatar is 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 Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
- [ ] I verified that similar component doesn't exist in the codebase - [ ] I verified that all props are defined accurately and each prop has a `/** comment above it */` - [ ] I verified that each file is named correctly - [ ] I verified that each component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone - [ ] I verified that the only data being stored in component state is data necessary for rendering and nothing else - [ ] In component if we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes - [ ] For Class Components, any internal methods passed to components event handlers are bound to `this` properly so there are no scoping issues (i.e. for `onClick={this.submit}` the method `this.submit` should be bound to `this` in the constructor) - [ ] I verified that component internal methods bound to `this` are necessary to be bound (i.e. avoid `this.submit = this.submit.bind(this);` if `this.submit` is never passed to a component event handler like `onClick`) - [ ] I verified that all JSX used for rendering exists in the render method - [ ] I verified that each 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

Screenshots/Videosundefined

Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
@MelvinBot MelvinBot requested a review from a team July 2, 2026 17:18
@MelvinBot

Copy link
Copy Markdown
Contributor Author

I created this PR from an upstream branch since mkhutornyi doesn't have push access to a fork of Expensify/App.

To take ownership of this branch and push updates from your own fork, run:

git remote add upstream https://github.com/Expensify/App.git
git fetch upstream claude-migrateGroupMembersTable
git checkout -b claude-migrateGroupMembersTable upstream/claude-migrateGroupMembersTable
git push -u origin claude-migrateGroupMembersTable

Then you can continue the work from your fork (open a new PR from it if you prefer).

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
src/components/Table/TableHeader.tsx 95.52% <100.00%> (ø)
src/components/Table/middlewares/selection.ts 59.15% <100.00%> (ø)
src/components/Table/TableRow.tsx 74.32% <75.00%> (ø)
src/pages/DynamicReportParticipantsPage.tsx 0.00% <0.00%> (ø)
...omponents/Tables/ReportParticipantsTable/index.tsx 0.00% <0.00%> (ø)
...rtParticipantsTable/ReportParticipantsTableRow.tsx 0.00% <0.00%> (ø)
... and 15 files with indirect coverage changes

@mkhutornyi

Copy link
Copy Markdown
Contributor

@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>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

Fixed and pushed (5a7ca47). Selection was broken because this page is the first Table consumer that lives in the RHP, and the Table framework gated its selection UX on shouldUseNarrowLayout — which is always true inside a narrow pane modal regardless of screen size.

Root cause

Table decided checkbox/long-press visibility from shouldUseNarrowLayout:

  • TableRow.tsx — long-press: shouldEnableMobileSelectionLongPress = shouldUseNarrowLayout && !isInNarrowPaneModal → in the RHP isInNarrowPaneModal is true, so long-press was always disabled → "nothing happens on long-press mobile".
  • TableRow.tsx / TableHeader.tsx — checkbox: isSelectionCheckboxVisible = selectionEnabled && (isMobileSelectionEnabled || !shouldUseNarrowLayout) → in the RHP shouldUseNarrowLayout is true even on wide web, and mobile selection mode can only be turned on via the (disabled) long-press → no checkbox on web.

The old SelectionListWithModal deliberately gated selection on isSmallScreenWidth (real mobile), not shouldUseNarrowLayout, and never excluded isInNarrowPaneModal — which is exactly why selection worked in the RHP before the migration.

Fix — key the selection UX (checkbox visibility, long-press, and the mobile selection-mode auto-sync) off isSmallScreenWidth instead of shouldUseNarrowLayout, in TableRow, TableHeader, and the selection middleware. Visual layout still uses shouldUseNarrowLayout, so the row keeps its narrow appearance in the RHP.

Result:

  • Desktop RHP (isSmallScreenWidth false) → checkboxes render directly.
  • Mobile RHP (isSmallScreenWidth true) → long-press opens the bottom-docked "Select" modal, then checkboxes.
Why this is safe for existing (central-pane) tables

shouldUseNarrowLayout = isSmallScreenWidth || isInNarrowPaneModal. Outside a narrow pane modal isInNarrowPaneModal is always false, so shouldUseNarrowLayout === isSmallScreenWidth. Every existing Table consumer (workspace/domain members, categories, tags, etc.) renders in the central pane, so this swap is a no-op for them — the two flags only diverge inside the RHP, which no other consumer uses. Since the shared Table is touched, worth a reviewer's confirmation on that.

Verification: prettier (clean), tsc/tsgo typecheck (0 errors), and React Compiler compliance (all 3 files COMPILED). ⚠️ Full type-aware eslint could not complete in this environment (JS heap OOM at the default 2 GB; larger-heap invocations needed interactive approval) — but the change is a mechanical shouldUseNarrowLayoutisSmallScreenWidth rename using the same eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth pattern already lint-clean in SelectionListWithModal. Please confirm lint/selection behavior on device during QA.

@mkhutornyi

Copy link
Copy Markdown
Contributor

@codex review

@mkhutornyi

Copy link
Copy Markdown
Contributor

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Review

Overall this is a clean migration that closely mirrors WorkspaceMembersTable. With the selection fix pushed in 5a7ca47, the logic looks correct. A few nits and QA callouts below — nothing blocking.

Correctness (checked against the code):

  • Selection round-trip is sound — selectedKeys = selectedMembers.map(String) out, onRowSelectionChange maps back via Number, and it's a stable useCallback, so the selection middleware won't re-clear on every render. ✓
  • Current user is correctly non-selectable via isSelectionDisabled: accountID === currentUserAccountID, and removeUsers also filters self out. ✓
  • The role column is only added for group chats, and in the RHP's narrow layout TableHeader renders only the title + select-all — so you correctly get the single "Members" header with no stray "Role" column. ✓
  • Non-admin / non-group path stays read-only (selectionEnabled false) and the row press routes to the profile. ✓

Nits (optional):

  1. ReportParticipantRowData.isGroupChat duplicates the table-level isGroupChat onto every row. Harmless, but it could be passed to the row as a prop (like WorkspaceMembersTableRow receives its shouldShow* flags) instead of embedding it in each item.
  2. compareItems calls .toLowerCase() before localeCompare; localeCompare already handles case, and WorkspaceMembersTable doesn't lowercase. Redundant, not wrong.
  3. The row omits the sentryLabel that WorkspaceMembersTableRow sets — consider adding one for observability parity.

Shared Table change (from the selection fix): tests/ui/TableTest.tsx mocks useResponsiveLayout without isSmallScreenWidth. Behavior is unchanged (an undefined isSmallScreenWidth is falsy exactly like the mocked shouldUseNarrowLayout: false, and the test asserts nothing about selection), but the mock should add isSmallScreenWidth: false for clarity. Non-blocking.

QA / behavior changes to confirm on device (all intentional per the description):

  • Selection on all platforms — desktop web RHP shows checkboxes directly; mobile uses long-press → bottom-docked "Select" → checkboxes. Verify invite / remove / make-admin / make-member all still work in both modes.
  • Empty-search state now shows the generic "No results found" instead of the old "member not found — use the invite button". Worth a @Expensify/design / Bug Zero confirmation.
  • The persisted find-member search phrase (ROOM_MEMBERS_USER_SEARCH_PHRASE) is dropped.
Local checks I could run

tsc/tsgo typecheck (0 errors), prettier (clean), and React Compiler compliance (all files COMPILED) pass. Full type-aware eslint and jest could not run in this environment (JS heap OOM at the default heap; larger-heap invocations are approval-gated here) — please rely on CI for those.

I'm not approving (that's for a human reviewer) — this is an automated review pass.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 5a7ca47556

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants