Skip to content

[Refactor] Move remaining PolicyUtils member, domain, and travel helpers into members.ts, domains.ts, and travel.ts - #104331

Merged
blimpich merged 5 commits into
mainfrom
claude-policyUtilsPhase9MembersDomainsTravel
Oct 19, 2026
Merged

blimpich merged 5 commits into
mainfrom
claude-policyUtilsPhase9MembersDomainsTravel

Conversation

@MelvinBot

@MelvinBot MelvinBot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

Phase 9, the last phase of the PolicyUtils split. It moves everything left in PolicyUtils/index.ts into submodules, so index.ts is now only the barrel. index.ts drops from 591 to 23 lines, which keeps the folder well under the 4000-line ESLint limit.

This is a pure move. Nothing is renamed, no signature changes, and behavior is the same. Callers stay on @libs/PolicyUtils.

  • members.ts: membership checks, Expensify-team filtering, invite exclusions, guides and account managers, member account IDs, and bank-account share recipients (19 helpers). The MemberEmailsToAccountIDs type moved here too, and export * re-exports it. It imports from ./permissions and ./policy.
  • domains.ts: the policy domain name, admin private domains, the most frequent domain, and domain-card lookups (6 helpers). It imports isExpensifyTeam from ./members.
  • travel.ts: travel provisioning, accepted terms, non-USD workspaces, and the travel step (4 helpers). The local TravelStep type moved with getTravelStep, its only user. It imports from ./policy, ./domains, and ./policyType.
  • approvals.ts: isNonMemberApprover moved next to getDefaultApprover. approvals.ts now imports isPolicyMember from ./members.
  • None of the new modules imports approvals.ts or index.ts, so there is no cycle and no import/no-cycle disable.
  • index.ts keeps the export * comment and list, and adds ./members, ./domains, and ./travel.
  • Five test files mock @libs/PolicyUtils with a factory that runs while the barrel is still loading inside an import cycle. Before this PR, the moved helpers were named exports on index.ts, so they were already on the module. Now they come through export *, which is not populated yet at that point. Those mocks now spread jest.requireActual('@libs/PolicyUtils/members'), the same fix earlier phases used for other submodules: ReportPrimaryActionUtilsTest, ReportSecondaryActionUtilsTest, ReportUtilsTest, SidebarUtilsTest, and CreateDraftTransactionTest.

AI tests run locally

  • npm run typecheck: passed
  • npm run lint-changed: passed
  • npm run fmt (oxfmt): no changes
  • npm run spell-changed: passed
  • Git move detection (--color-moved) shows every removed function line from index.ts landing unchanged in the new files. Only header, import, and export lines differ.
  • Jest: all 55 suites that mock @libs/PolicyUtils passed after the mock fix (3557 tests), plus 31 suites that cover the moved helpers (3376 tests), including PolicyUtilsTest, CardUtilsTest, createFilteredMemberCountSelectorTest, OptionsListUtilsTest, TravelUtilsTest, and WorkflowUtilsTest. The full npm test run doesn't finish within the local time limit, so CI is the full-suite signal.
  • A web smoke test of the Members, Invite, Workflows, and Travel pages could not run, because the local web test session was blank. It was not caused by this change.

Fixed Issues

$ #102675
PROPOSAL: #102675 (comment)

Tests

Members list, invites and removing a member

Prerequisite: a Control workspace where User A is the admin. User B and User C are members, and Add approvals is on with User C as User B's approver.

  1. As User A, go to Workspaces. Verify that the member count on the workspace row matches the number of members.
  2. Open Workspace > Members. Verify that User A, User B and User C are listed with the right roles.
  3. Tap Invite member and search for User B. Verify that User B doesn't show up in the suggestions, because they're already a member.
  4. Invite a new user, User D. Verify that User D shows up in Members and the member count goes up by one.
  5. Go offline. Remove User D. Verify that User D shows with a strikethrough as pending delete.
  6. Go back online. Verify that User D disappears and the member count goes back down.
  7. Remove User C from the workspace. Open Workflows. Verify that User B's workflow shows the approver as no longer a member.

Expensify team and guide filtering

Prerequisite: a workspace that has an assigned guide, or an @expensify.com / @team.expensify.com member

  1. As a non-Expensify admin, open Workspace > Members. Verify that Expensify team members are hidden, and the member count on the Workspaces list leaves them out.

Travel

Prerequisite: Account is public domain email

  1. Go Workspace > More features
  2. Enable travel
  3. Go Travel > click on CTA button
  4. Verify that the public domain error page appears
  • Verify that no errors appear in the JS console

Offline tests

Same as test

QA Steps

Same as test

  • 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 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 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 the PR adds or modifies the UI:
    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • 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
Screen.Recording.2026-10-19.at.22.22.14.mov
Android: mWeb Chrome
Screen.Recording.2026-10-19.at.22.21.10.mov
iOS: Native
Screen.Recording.2026-10-19.at.22.21.20.mov
iOS: mWeb Safari
Screen.Recording.2026-10-19.at.22.21.48.mov
MacOS: Chrome / Safari
Screen.Recording.2026-10-19.at.22.16.12.mov
Screen.Recording.2026-10-19.at.22.17.09.mov
Screen.Recording.2026-10-19.at.22.17.32.mov

MelvinBot and others added 2 commits October 19, 2026 07:34
@MelvinBot
MelvinBot requested a review from a team October 19, 2026 07:53
@melvin-bot melvin-bot Bot added Melvin-Test-Android Melvin-Test-Web Triggers Melvin to run the testing steps of the PR on web labels Oct 19, 2026
@MelvinBot

Copy link
Copy Markdown
Contributor Author

I created this PR from an upstream branch since I don't have push access to your fork.

To take ownership of this branch and be able to push updates, run:

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

Then you can close this PR and open a new one from your fork.

@codecov

codecov Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Coverage Δ
src/libs/Fullstory/utils.ts 89.53% <ø> (ø)
src/libs/PolicyUtils/approvals.ts 96.96% <100.00%> (+0.03%) ⬆️
src/libs/PolicyUtils/permissions.ts 100.00% <100.00%> (ø)
src/libs/PolicyUtils/travel.ts 100.00% <100.00%> (ø)
src/libs/telemetry/TelemetrySynchronizer.ts 98.80% <ø> (ø)
src/libs/PolicyUtils/domains.ts 87.50% <87.50%> (ø)
src/libs/PolicyUtils/members.ts 88.75% <88.75%> (ø)
... and 10 files with indirect coverage changes

@MelvinBot

MelvinBot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Android test steps verification results: ✅ pass

On standalone Android NewDot, the Members, Invite, Workflows, and Travel pages all opened and rendered correctly, with no red-box errors or crashes. The PR's Tests section only lists "Verify that no errors appear in the JS console", so I also smoke-tested the pages the PR description names as affected.
Workspace Travel page with Book travel onboarding

Step Status
The app loads after sign-in with no red-box error or crashAfter sign-in and onboarding, the app reached the Home/Inbox screen with no red-box or crash. The app stayed in the foreground for the whole run.
App loaded on Home screen after sign-in
✅
Workspace > Members page opens and lists the current user as a member, with no errorMembers page showed "Total workspace members: 1" with [email protected] listed as Owner, with no error banners.
Workspace Members page listing the current user as Owner
✅
Workspace > Members > Invite member page opens and shows the invite search, with no error"Invite new members" opened with the workspace name header and a "Name, email, or phone number" search field. Next was disabled until input, as expected.
Invite member page with search field
✅
Workspace > Workflows page opens and shows approval settings, with no errorThe Approvals tab showed the default approval workflow (Approver = current user, "Expenses from: Everyone") and an "Add approval workflow" control.
Workflows Approvals tab showing default approver settings
✅
The Travel page opens, with no errorAfter enabling Travel in More features, the workspace Travel page showed the "Get started with Expensify Travel" card with a "Book travel" button.
Workspace Travel page with Book travel onboarding
✅
No JS errors, red-box, or crash during the whole runNo red-box or crash appeared in about 15 snapshots across sign-in, onboarding, Members, Invite, Workflows, and Travel. The raw logcat/Metro log could not be searched in this run, so this verdict is based on on-screen inspection only.
✅

view run · view recording

@MelvinBot

MelvinBot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Web test steps verification results: ✅ pass

On the dev web build, the Members, Invite, Workflows, and Travel pages all opened and rendered correctly, with no error pages or crashes. The PR's Tests section only lists "Verify that no errors appear in the JS console", so I also smoke-tested the pages the PR description names as affected.
Travel page get-started screen

Step Status
The app loads after sign-in with no error page or crashAfter onboarding, the app landed on the Workspace Overview page with no error banner or crash screen.
Workspace Overview page after onboarding completes
✅
Workspace > Members page opens and lists the current user as a member, with no errorMembers page showed "Total workspace members: 1" with [email protected] listed as Owner, with no error text.
Workspace Members page listing current user as Owner
✅
Workspace > Members > Invite member page opens and shows the invite search, with no error"Invite new members" opened with a "Name, email, or phone number" search field and a Next button.
Invite new members panel with search field
✅
Workspace > Workflows page opens and shows approval settings, with no errorAfter enabling Workflows in More features, the Submissions tab showed the schedule toggle. The Approvals tab showed approvals on, the default workflow "Expenses from Everyone" with an approver, and "Add approval workflow".
Workflows Submissions tab
Workflows Approvals tab with approval settings
✅
The Travel page opens, with no errorAfter enabling Travel in More features, the Travel page showed "Get started with Expensify Travel" with a "Let's go" button.
Travel page get-started screen
✅
No JS console errors during the whole runNo error banners, crash screens, or broken renders appeared on any visited page. The test tool can't read the browser console on web, so this verdict is based on on-screen inspection only, not a direct console read.
✅

view run · view recording

@nkdengineer

Copy link
Copy Markdown
Contributor

@MelvinBot Let's add unit tests for isExpensifyTeam, getMemberAccountIDsForWorkspace, getTravelStep. After that wrap new moved functions into related describe in PolicyUtilsTest.

…space, getTravelStep and group moved helpers by submodule

Co-authored-by: nkdengineer <[email protected]>
@MelvinBot

Copy link
Copy Markdown
Contributor Author

I added the unit tests and grouped the moved helpers by submodule in PolicyUtilsTest (commit bf41ddf).

New tests

  • isExpensifyTeam: covers expensify.com and team.expensify.com emails, other domains (including a subdomain of expensify.com), and missing or empty emails.
  • getMemberAccountIDsForWorkspace: covers the email-to-accountID map, members without personal details, includeMemberWithErrors, includeMemberWithPendingDelete, and an undefined employee list.
  • getTravelStep: covers all 3 steps. It also tests each early return (public-only admin domains, no active paid workspace, non-paid workspace) with a pending signup request, so a skipped guard would fail the test.

Regrouping (moved tests are unchanged apart from indentation)

  • members: isPolicyMemberWithoutPendingDelete, getEligibleBankAccountShareRecipientEmails, hasEligibleBankAccountShareRecipient, getExpensifyTeamExclusions, getExcludedUsers, hasActiveExpensifyCard, isMemberInHomeAndOfficeWorkspace, plus the new tests. The 2 duplicate hasEligibleBankAccountShareRecipient describes had the same hooks, so I merged them into one.
  • domains: getPolicyIDFromDomainName and getPolicyForAssignedCard. These were top-level describes outside PolicyUtils before.
  • travel: the new getTravelStep tests.
  • isNonMemberApprover now sits in approvals, next to getDefaultApprover.

⚠️ I couldn't run Jest, typecheck, lint, or the formatter this time, because this environment blocked those commands. CI is the first real check of this commit. Before pushing, I compared the diff while ignoring indentation: every line I moved matches its original, and only the new tests are new content.


view run

@nkdengineer

Copy link
Copy Markdown
Contributor

@MelvinBot, fill out the Test section

  • Members list, invites and removing a member

Prerequisite: a Control workspace where User A is the admin. User B and User C are members, and Add approvals is on with User C as User B's approver.

  1. As User A, go to Workspaces. Verify that the member count on the workspace row matches the number of members.
  2. Open Workspace > Members. Verify that User A, User B and User C are listed with the right roles.
  3. Tap Invite member and search for User B. Verify that User B doesn't show up in the suggestions, because they're already a member.
  4. Invite a new user, User D. Verify that User D shows up in Members and the member count goes up by one.
  5. Go offline. Remove User D. Verify that User D shows with a strikethrough as pending delete.
  6. Go back online. Verify that User D disappears and the member count goes back down.
  7. Remove User C from the workspace. Open Workflows. Verify that User B's workflow shows the approver as no longer a member.
  • Expensify team and guide filtering

Prerequisite: a workspace that has an assigned guide, or an @expensify.com / @team.expensify.com member

  1. As a non-Expensify admin, open Workspace > Members. Verify that Expensify team members are hidden, and the member count on the Workspaces list leaves them out.
  • Travel

Prerequisite: Account is public domain email

  1. Go Workspace > More features
  2. Enable travel
  3. Go Travel > click on CTA button
  4. Verify that the public domain error page appears

Then update Offline tests and QA Steps to Same as test text

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Updated the PR description:

  • Tests: I added your three scenarios: members list, invites and removal; Expensify team and guide filtering; and Travel. I removed the TODO placeholder and kept the JS console checkbox.
  • Offline tests: Set to "Same as test".
  • QA Steps: Set to "Same as test" and kept the JS console checkbox.

view run

@nkdengineer

Copy link
Copy Markdown
Contributor

@MelvinBot Let's move getHighestPolicyRole util to permissions.ts file and remove getHighestPolicyRole.ts file.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

I moved getHighestPolicyRole into permissions.ts and deleted getHighestPolicyRole.ts in 39dfcca.

  • POLICY_ROLE_RANKING and the function moved without changes. It is now a named export, so the barrel re-exports it.
  • TelemetrySynchronizer.ts and Fullstory/utils.ts now import it from @libs/PolicyUtils.
  • TelemetrySynchronizerTest fully mocks @libs/PolicyUtils. I added the real getHighestPolicyRole from @libs/PolicyUtils/permissions to that mock.
Checks
  • npm run typecheck: passed
  • ESLint on the changed files: passed
  • Jest: TelemetrySynchronizerTest, PolicyUtilsTest, FullstoryTest, FullstoryUtilsTest, and FullstoryCommonTest passed (750 tests)
  • I couldn't run oxfmt or cspell locally. CI will check those.

@nkdengineer

Copy link
Copy Markdown
Contributor

@MelvinBot, fill the Screenshots/Videos

Screenshots/Videos

Android: Native
Screen.Recording.2026-10-19.at.22.22.14.mov
Android: mWeb Chrome
Screen.Recording.2026-10-19.at.22.21.10.mov
iOS: Native
Screen.Recording.2026-10-19.at.22.21.20.mov
iOS: mWeb Safari
Screen.Recording.2026-10-19.at.22.21.48.mov
MacOS: Chrome / Safari
Screen.Recording.2026-10-19.at.22.16.12.mov
Screen.Recording.2026-10-19.at.22.17.09.mov
Screen.Recording.2026-10-19.at.22.17.32.mov

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Filled out the Screenshots/Videos section in the PR description with your 7 videos:

  • Android: Native: 1 video
  • Android: mWeb Chrome: 1 video
  • iOS: Native: 1 video
  • iOS: mWeb Safari: 1 video
  • MacOS: Chrome / Safari: 3 videos

The rest of the description is unchanged.


view run

@nkdengineer

Copy link
Copy Markdown
Contributor

@MelvinBot, Update the Author Checklist section. Also, I verified there's no console error; then let's check for Verify that no errors appear in the JS console.. After that mark the PR as ready.

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 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 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 the PR adds or modifies the UI:
    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • 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.

@MelvinBot
MelvinBot marked this pull request as ready for review October 19, 2026 15:59
@MelvinBot
MelvinBot requested a review from a team as a code owner October 19, 2026 15:59
@melvin-bot
melvin-bot Bot requested a review from nkdengineer October 19, 2026 15:59
@MelvinBot

Copy link
Copy Markdown
Contributor Author

Done. I updated the PR description and marked the PR as ready for review.

  • PR Author Checklist: Replaced with your checklist. Every item is checked.
  • JS console checks: Checked "Verify that no errors appear in the JS console" in both Tests and QA Steps.
  • Screenshots/Videos: Kept the videos you added.

view run

@melvin-bot
melvin-bot Bot removed the request for review from a team October 19, 2026 15:59
@melvin-bot

melvin-bot Bot commented Oct 19, 2026

Copy link
Copy Markdown

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

@melvin-bot
melvin-bot Bot requested review from dylanexpensify and removed request for a team October 19, 2026 15:59
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-19T16:06:18.351133Z 39dfcca Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@nkdengineer

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible 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 checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • 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 verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • The 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
  • 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 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 the PR adds or modifies the UI:
    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this 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.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Screen.Recording.2026-10-19.at.22.22.14.mov
Android: mWeb Chrome
Screen.Recording.2026-10-19.at.22.21.10.mov
iOS: HybridApp
Screen.Recording.2026-10-19.at.22.21.20.mov
iOS: mWeb Safari
Screen.Recording.2026-10-19.at.22.21.48.mov
MacOS: Chrome / Safari
Screen.Recording.2026-10-19.at.22.16.12.mov
Screen.Recording.2026-10-19.at.22.17.09.mov
Screen.Recording.2026-10-19.at.22.17.32.mov

@nkdengineer nkdengineer left a comment

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.

LGTM! @blimpich All yours.

@melvin-bot
melvin-bot Bot requested a review from blimpich October 19, 2026 16:08
@MelvinBot

Copy link
Copy Markdown
Contributor Author

The failing ESLint check is unrelated to this PR. main breaks it, so I didn't push a fix here.

Analysis: The only lint error is in a file this PR doesn't touch:

src/libs/Navigation/types.ts:1:1  error  Unused eslint-disable directive (no problems were reported from 'max-lines').

Move Categories & Tags settings into the header More menu merged into main today and removed 16 lines from that file. The file is now exactly 4,000 lines, which is the max-lines limit. That makes the /* eslint-disable max-lines */ directive unused, so ESLint fails for every PR that runs against current main.

Fix in progress: [No QA] Split SettingsNavigatorParamList out of Navigation/types.ts to fix max-lines lint already fixes this on main. After it merges, merge main into this branch or rerun the check.

Evidence
  • Failing run: ESLint check. It reports 1 error, in src/libs/Navigation/types.ts.
  • This PR changes only src/libs/PolicyUtils/*, src/libs/Fullstory/utils.ts, src/libs/telemetry/TelemetrySynchronizer.ts, and tests. It doesn't touch src/libs/Navigation/types.ts.
  • src/libs/Navigation/types.ts:1 on main is 4,000 lines and still has the directive. The rule is set at config/eslint/eslint.config.mjs:748: 'max-lines': ['error', 4000].
  • All other checks on this PR pass, including TypeScript and every Jest shard.

view run

@MelvinBot

Copy link
Copy Markdown
Contributor Author

Nothing has changed since my earlier comment, so I still haven't pushed a fix.

After that PR merges, rerun ESLint or merge main into this branch. If you need this PR unblocked sooner, reply and I'll merge main and remove the directive here. That would likely conflict with the fix PR.


view run

@MelvinBot

Copy link
Copy Markdown
Contributor Author

I still haven't pushed a fix, because nothing has changed since my earlier comment.

After that fix PR merges, merge main into this branch or rerun ESLint. Repeating this request won't change the result. If you want me to work around it here instead, say so explicitly, for example: "merge main and remove the directive in this PR."


view run

@nkdengineer

Copy link
Copy Markdown
Contributor

@MelvinBot The fix was merged, please merge main.

@MelvinBot
MelvinBot requested a review from a team as a code owner October 19, 2026 18:10
@MelvinBot

Copy link
Copy Markdown
Contributor Author

Merged main into this branch and pushed it as 7a84e78. There were no conflicts.

  • main no longer has the unused max-lines directive in src/libs/Navigation/types.ts, so the ESLint failure should clear.
  • From main, the only PolicyUtils change is in errors.ts, so index.ts is still just the barrel.
  • I couldn't run typecheck or lint locally this time. CI will run both on the new commit.

view run

@melvin-bot
melvin-bot Bot requested review from blimpich and nkdengineer and removed request for a team October 19, 2026 18:10
@melvin-bot

melvin-bot Bot commented Oct 19, 2026

Copy link
Copy Markdown

@nkdengineer @blimpich One of you needs to 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]

@nkdengineer

Copy link
Copy Markdown
Contributor

@blimpich Fixed lint.

@blimpich
blimpich merged commit 31b5428 into main Oct 19, 2026
42 checks passed
@blimpich
blimpich deleted the claude-policyUtilsPhase9MembersDomainsTravel branch October 19, 2026 18:26
@github-actions

Copy link
Copy Markdown
Contributor

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

@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/blimpich in version: 9.5.15-1 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor Author

No help site update needed. This PR only moves PolicyUtils helpers into new internal modules. It renames nothing and keeps behavior the same, so no user-facing text or workflow changed.


view run

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/grgia in version: 9.5.15-3 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

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

Labels

Melvin-Test-Android Melvin-Test-Web Triggers Melvin to run the testing steps of the PR on web

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants