Skip to content

Add workspace rooms list rows#91056

Open
sumo-slonik wants to merge 22 commits into
Expensify:mainfrom
software-mansion-labs:kuba-nowakowski/feature/workspace-rooms-list
Open

Add workspace rooms list rows#91056
sumo-slonik wants to merge 22 commits into
Expensify:mainfrom
software-mansion-labs:kuba-nowakowski/feature/workspace-rooms-list

Conversation

@sumo-slonik
Copy link
Copy Markdown
Contributor

Explanation of Change

Adds rows to the WorkspaceRoomsPage table. Behind the workspaceRoomsPage beta.

Fixed Issues

$ #89596

Tests

  1. Open a workspace's Rooms page with the workspaceRoomsPage beta enabled.
  2. Verify the layout matches the Figma in the linked issue.
  • Verify that no errors appear in the JS console

Offline tests

Unnecessary

QA Steps

Same as tests.

  • 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 shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • 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)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • 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)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • 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.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • 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

MacOS: Chrome / Safari
Screen.Recording.2026-05-18.at.14.27.22.mov

- Use styles.flex3/flex2/flex1 instead of inline {flex: N} so column widths share one source of truth between header and row
- Move column header into FlatList ListHeaderComponent
- Use textLabelSupporting for column titles
- Replace per-row PERSONAL_DETAILS_LIST subscription with getPersonalDetailsByIDs
Filter archived/hidden rooms, derive room name from report attributes, and
exclude hidden participants from the member count. Lift personal-details
lookup to the page and pass per-row data into WorkspaceRoomsListItem.

Switch FlatList to FlashList, disable the placeholder Create button, add
the persistent row background, hover state, 8px row gap, and the column
flex constants shared between the header and rows.
Tell useOnyx to invalidate its selector cache when policyID changes
instead of relying on selector reference identity, matching the
SuggestionMention pattern.
Use the existing styles.flex1/flex2/flex3 helpers directly in the header
and row instead of wrapping them in a separate columnFlex module.
- Extract policy chat rooms selector to src/selectors/Report.ts and add unit tests
- Extract list header into a dedicated ListHeader component
- Add JSDoc to WorkspaceRoomsRow and WorkspaceRoomsListItemProps
@sumo-slonik sumo-slonik requested review from a team as code owners May 19, 2026 10:11
@melvin-bot melvin-bot Bot requested review from heyjennahay and ikevin127 May 19, 2026 10:11
@sumo-slonik
Copy link
Copy Markdown
Contributor Author

Initial review took place on the fork PR: software-mansion-labs#338

@melvin-bot melvin-bot Bot removed request for a team May 19, 2026 10:11
@melvin-bot
Copy link
Copy Markdown

melvin-bot Bot commented May 19, 2026

@ikevin127 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]

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e3c5d404a9

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

Comment thread src/selectors/Report.ts
if (!report || report.policyID !== policyID) {
continue;
}
if (!isChatRoom(report) && !isPolicyExpenseChat(report)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude policy expense chats from the rooms list

Because this selector explicitly admits isPolicyExpenseChat(report), any workspace member expense chat that is already in Onyx for this policy will be rendered as a row on the Workspace Rooms page and navigate like a room. Those chats are not rooms — the existing isChatRoom() helper is the repo's room predicate and it intentionally does not include policy expense chats (src/libs/ReportUtils.ts:1770). This will pollute the Rooms table for users who have opened policy expense chats in the workspace.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't technically true. We only send the ones that are accessible by the user and we want to see the policy expense chats.

However, @dylanexpensify should admins/auditors viewing Rooms should see every member's expense chat listed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, @dylanexpensify should admins/auditors viewing Rooms should see every member's expense chat listed?

IMO we should surface any rooms these users have access to, including expense chats. We can leverage filtering/sorting to make it more digestible for admins down the road. Thoughts?

@grgia grgia self-requested a review May 19, 2026 15:10
import CONST from '@src/CONST';
import type {PersonalDetails, Report} from '@src/types/onyx';

const ROW_MIN_HEIGHT = 64;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there not a norm CONST for rows? why is it hardcoded here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I fixed it

@github-actions
Copy link
Copy Markdown
Contributor

🚧 @grgia has triggered a test Expensify/App build. You can view the workflow run here.

@github-actions
Copy link
Copy Markdown
Contributor

🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
Built from App PR #91056.

Android 🤖 iOS 🍎
⏩ SKIPPED ⏩ ⏩ SKIPPED ⏩
The build for Android was skipped The build for iOS was skipped
Web 🕸️
https://91056.pr-testing.expensify.com
Web

👀 View the workflow run that generated this build 👀

Comment thread src/pages/workspace/rooms/WorkspaceRoomsListItem.tsx Outdated
@ikevin127
Copy link
Copy Markdown
Contributor

@grgia I was auto-assigned for C+ review, checking the parent issue looks like this PR is for @shubham1206agra to review - I'll unsubscribe from the issue, tag me in case my review is needed 🙌

Screenshot 2026-05-19 at 13 48 38

@codecov
Copy link
Copy Markdown

codecov Bot commented May 20, 2026

Codecov Report

❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.

Files with missing lines Coverage Δ
src/components/Table/TableSearchBar.tsx 100.00% <100.00%> (ø)
...sTable/WorkspaceCompanyCardsTableHeaderButtons.tsx 0.00% <ø> (ø)
src/libs/API/types.ts 100.00% <ø> (ø)
src/styles/variables.ts 100.00% <ø> (ø)
src/components/Table/TableHeader.tsx 89.74% <83.33%> (-2.15%) ⬇️
src/components/Table/TableRow.tsx 0.00% <0.00%> (ø)
src/selectors/Report.ts 55.55% <93.75%> (+21.07%) ⬆️
src/libs/actions/Policy/Room.ts 0.00% <0.00%> (ø)
src/pages/workspace/rooms/WorkspaceRoomsPage.tsx 0.00% <0.00%> (ø)
...les/WorkspaceRoomsTable/WorkspaceRoomsTableRow.tsx 0.00% <0.00%> (ø)
... and 1 more
... and 46 files with indirect coverage changes

@sumo-slonik sumo-slonik force-pushed the kuba-nowakowski/feature/workspace-rooms-list branch from 5d21048 to d0be423 Compare May 20, 2026 11:15
@shawnborton
Copy link
Copy Markdown
Contributor

The gap between the avatar and the label should be 12px
CleanShot 2026-05-22 at 10 55 46@2x

Can we also give Members column a fixed width since it should never be super long? Perhaps something like 80px.

@sumo-slonik
Copy link
Copy Markdown
Contributor Author

The gap between the avatar and the label should be 12px CleanShot 2026-05-22 at 10 55 46@2x

Can we also give Members column a fixed width since it should never be super long? Perhaps something like 80px.

Of course, I’m taking care of it.

@sumo-slonik sumo-slonik requested a review from a team as a code owner May 22, 2026 09:30
@sumo-slonik
Copy link
Copy Markdown
Contributor Author

It should already be fixed.

Screen.Recording.2026-05-22.at.11.30.08.mov

@shawnborton
Copy link
Copy Markdown
Contributor

Members column seems too wide, and the spacing is uneven between these two
CleanShot 2026-05-22 at 11 36 43@2x

@grgia
Copy link
Copy Markdown
Contributor

grgia commented May 22, 2026

Let me know when this is ready for a fresh build

@sumo-slonik
Copy link
Copy Markdown
Contributor Author

@shawnborton Do you have any other suggestions on what I should improve?

image

JS00001
JS00001 previously approved these changes May 22, 2026
Copy link
Copy Markdown
Contributor

@JS00001 JS00001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Table changes look good to me. A lot of your changes (The 'width' prop for columns, the 'label' prop for the searchbar, the additional styling for the search bar, etc) are similar to changes being made here: #90204

So you may need to fix conflicts if that PR merges before this one

@sumo-slonik
Copy link
Copy Markdown
Contributor Author

Table changes look good to me. A lot of your changes (The 'width' prop for columns, the 'label' prop for the searchbar, the additional styling for the search bar, etc) are similar to changes being made here: #90204

So you may need to fix conflicts if that PR merges before this one

I agree, in that case I’ll wait until that one is merged, resolve the conflicts with it, and then we’ll proceed with merging this

@shawnborton
Copy link
Copy Markdown
Contributor

Yup, visually that's looking good to me. For mobile, if we don't have a value for the Created by column, we should just omit that label entirely.

@shubham1206agra
Copy link
Copy Markdown
Contributor

@sumo-slonik Can we put a rush on this? As we want to finish this as soon as possible.

@sumo-slonik
Copy link
Copy Markdown
Contributor Author

sumo-slonik commented May 25, 2026

@sumo-slonik Can we put a rush on this? As we want to finish this as soon as possible.

@shubham1206agra From my side, I’m happy to do everything I can to get this done as quickly as possible, but as agreed in the message above with @JS00001 , we were supposed to wait until the second table migration is ready to avoid any conflicts. However, everything here seems to be ready, so maybe we can merge it sooner in that case.
It seems to me that we’re now waiting for your review, since the remaining comments have already been resolved.

@grgia
Copy link
Copy Markdown
Contributor

grgia commented May 25, 2026

@sumo-slonik would you add HOLD and the link to the PR we're holding on to the title or description?

@sumo-slonik sumo-slonik changed the title Add workspace rooms list rows [HOLD #90204 ] Add workspace rooms list rows May 25, 2026
@sumo-slonik sumo-slonik changed the title [HOLD #90204 ] Add workspace rooms list rows Add workspace rooms list rows May 27, 2026
@sumo-slonik
Copy link
Copy Markdown
Contributor Author

@shubham1206agra It’s all yours

Comment thread src/components/Table/TableSearchBar.tsx Outdated
tableMethods: {updateSearchString},
} = useTableContext();

const inputLabel = label ?? translate('workspace.companyCards.findCard');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets just require a label, we shouldnt fallback to 'find card'

Comment thread src/libs/actions/Policy/Room.ts Outdated
import Log from '@libs/Log';

function openPolicyRoomsPage(policyID: string) {
if (!policyID) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an issue that you ran into? Is this guard necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added just in case it'd be needed later, but I can drop it this is inside a workspace flow so policyID should always be set.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just implemented the same way as in openWorkspaceRoomsPage

[policyID],
);

const rooms: WorkspaceRoomRowData[] = (policyReports ?? [])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be better to make this the selector instead of filtering later on?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move the it into policyChatRoomsSelector and update the tests

Comment thread tests/unit/ReportSelectorTest.ts Outdated
import type {Report} from '@src/types/onyx';

describe('policyChatRoomsSelector', () => {
const R = ONYXKEYS.COLLECTION.REPORT;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nab can we give this a meaningful name please?

@shubham1206agra
Copy link
Copy Markdown
Contributor

Yup, visually that's looking good to me. For mobile, if we don't have a value for the Created by column, we should just omit that label entirely.

@sumo-slonik Can you please address this?

- Require label prop on TableSearchBar; drop the Find card fallback
- Move archived filter into policyChatRoomsSelector and update tests
- Drop redundant policyID guard in openPolicyRoomsPage
- Rename const R to REPORT_KEY_PREFIX in ReportSelectorTest
- Omit Created by label on narrow layout when owner is missing
@sumo-slonik
Copy link
Copy Markdown
Contributor Author

Do you think we should change the label from “members” to “member” on the narrow screen when there is only 1 member?

image

@dannymcclain
Copy link
Copy Markdown
Contributor

Do you think we should change the label from “members” to “member” on the narrow screen when there is only 1 member?

I personally do!

@sumo-slonik
Copy link
Copy Markdown
Contributor Author

Do you think we should change the label from “members” to “member” on the narrow screen when there is only 1 member?

I personally do!

In that case, I’ll do it that way

@sumo-slonik
Copy link
Copy Markdown
Contributor Author

@shubham1206agra I think that all comments have already been resolved here

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants