Skip to content

Add Chat and Copilot buttons to Agents page#91533

Merged
yuwenmemon merged 12 commits into
mainfrom
claude-addAgentChatAndCopilotButtons
May 27, 2026
Merged

Add Chat and Copilot buttons to Agents page#91533
yuwenmemon merged 12 commits into
mainfrom
claude-addAgentChatAndCopilotButtons

Conversation

@MelvinBot
Copy link
Copy Markdown
Contributor

@MelvinBot MelvinBot commented May 24, 2026

Explanation of Change

Adds Chat and Copilot action buttons to the Agents feature in two locations:

  1. AgentsListRow (wide layout): Two new icon buttons (ChatBubble, UserPlus) appear alongside the existing Edit button. The Chat button opens a DM with the agent via navigateToAndOpenReportWithAccountIDs. The Copilot button switches into the agent account via Delegate.connect.

  2. EditAgentPage: Two new MenuItem entries ("Chat with agent" and "Copilot into account") appear above the existing Delete option, using the same action functions.

Both buttons are disabled when the agent row is pending deletion. Translation strings added for all supported locales.

Fixed Issues

$ #91343
PROPOSAL: #91343 (comment)

Tests

  1. Navigate to Settings > Account > Agents (ensure you have at least one agent set up)
  2. Wide layout (desktop): Verify you see Chat (chat bubble icon) and Copilot buttons next to the Edit button on each agent row
  3. Tap the Chat button → verify it opens a DM chat with that agent
  4. Navigate back to the Agents list, tap the Copilot button → verify it switches you into the agent's account (copilot mode)
  5. Tap on an agent row to open the Edit Agent page
  6. Verify you see "Chat with agent" and "Copilot into account" menu items above the Delete agent option
  7. Tap "Chat with agent" → verify it opens a DM chat with the agent
  8. Navigate back, tap "Copilot into account" → verify it switches you into the agent's account
  9. Verify that no errors appear in the JS console

Offline tests

  1. Go offline
  2. Navigate to Settings > Account > Agents
  3. Tap the Copilot button on an agent row → verify an offline modal appears (you should NOT be switched into the account)
  4. Open the Edit Agent page, tap "Copilot into account" → verify the same offline modal appears

QA Steps

  1. Navigate to Settings > Account > Agents on a wide-layout device
  2. Verify Chat and Copilot buttons appear on each agent row
  3. Tap Chat → verify DM opens with the agent
  4. Tap Copilot → verify you are switched into the agent's account
  5. Open the Edit Agent page for an agent
  6. Verify "Chat with agent" and "Copilot into account" menu items are present
  7. Tap each and verify they work correctly
  8. 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
    • 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

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

Add two action buttons to the agent row in Account > Agents (wide layout)
and two new menu items to the Edit Agent page. The Chat button opens a DM
with the agent via navigateToAndOpenReportWithAccountIDs. The Copilot button
switches into the agent account via Delegate.connect.

Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
…t translations

The new translation strings for chatWithAgent, copilotIntoAccount, and
deleteAgent used Unicode curly quotes (U+2018/U+2019) as string delimiters
in fr.ts and unescaped ASCII apostrophes inside single-quoted strings in
it.ts. Both caused TypeScript parse errors and Prettier failures.

Replaced curly quote delimiters with ASCII single quotes and used \u2019
escape sequences for French/Italian apostrophes inside strings.

Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
@MelvinBot
Copy link
Copy Markdown
Contributor Author

Fixed the failing checks. The new translation strings in fr.ts and it.ts had quote encoding issues:

  • fr.ts: Lines 2795-2797 used Unicode curly quotes (U+2018/U+2019) as string delimiters instead of ASCII single quotes — TypeScript can't parse those as string literals
  • it.ts: Lines 2783-2784 had unescaped ASCII apostrophes inside single-quoted strings (l'agente, nell'account) which prematurely terminated the strings

Both are now fixed using ASCII single quote delimiters with \u2019 escape sequences for the French/Italian apostrophes inside the strings.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 24, 2026

Codecov Report

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

Files with missing lines Coverage Δ
src/hooks/useChatWithAgent.ts 100.00% <100.00%> (ø)
src/pages/settings/Agents/AgentsListRow.tsx 100.00% <100.00%> (ø)
src/pages/settings/Agents/AgentsPage.tsx 94.64% <100.00%> (+0.19%) ⬆️
src/pages/settings/Agents/EditAgentPage.tsx 95.31% <100.00%> (+0.97%) ⬆️
src/pages/settings/Copilot/CopilotPage.tsx 70.22% <100.00%> (+5.03%) ⬆️
src/hooks/useSwitchToDelegator.ts 51.51% <51.51%> (ø)
... and 506 files with indirect coverage changes

navigateToAndOpenReportWithAccountIDs([accountID], currentUserPersonalDetails.accountID, introSelected, isSelfTourViewed, betas, personalDetails);
};
const handleCopilotPress = () => {
connect({email: login, delegatedAccess: account?.delegatedAccess, credentials, session, activePolicyID});
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.

@MelvinBot Existing Copilot switching checks offline state, active GPS tracking, and chained delegation before calling connect (see CopilotPage.tsx). These new buttons call connect directly, so they can try to switch accounts offline, during GPS tracking without warning, or while already acting as a delegate. Check the existing implementation and update this accordingly, while reusing code.

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.

Extracted a new useSwitchToDelegator hook that encapsulates the offline, GPS tracking, and chained delegation checks from CopilotPage.tsx. Both AgentsListRow and EditAgentPage now use this hook instead of calling Delegate.connect directly. CopilotPage also uses the shared hook, eliminating the duplication.

Comment on lines +122 to +127
<Button
small
icon={icons.UserPlus}
onPress={handleCopilotPress}
isDisabled={isPendingDeletion}
/>
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.

@MelvinBot Fix accessibility

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 accessibilityLabel to both icon-only buttons using existing translation keys (editAgentPage.chatWithAgent and editAgentPage.copilotIntoAccount).

Comment on lines +116 to +121
<Button
small
icon={icons.ChatBubble}
onPress={handleChatPress}
isDisabled={isPendingDeletion}
/>
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.

@MelvinBot Fix accessibility

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 accessibilityLabel using the existing editAgentPage.chatWithAgent translation key.

MelvinBot and others added 4 commits May 24, 2026 09:11
The new Agent copilot buttons were calling Delegate.connect directly,
bypassing the offline, GPS tracking, and chained delegation safety
checks that CopilotPage enforces. This extracts those checks into a
reusable useSwitchToDelegator hook and uses it in AgentsListRow,
EditAgentPage, and CopilotPage.

Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
Icon-only buttons in AgentsListRow need accessibilityLabel props so
screen readers can announce their purpose.

Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
….com/Expensify/App into claude-addAgentChatAndCopilotButtons

Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
@MelvinBot
Copy link
Copy Markdown
Contributor Author

Fixed the failing Prettier check: sorted imports alphabetically in src/hooks/useSwitchToDelegator.ts.

@MelvinBot
Copy link
Copy Markdown
Contributor Author

⚠️ MelvinBot CI Fix Loop Detected

MelvinBot has made 6 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.

@ShridharGoel
Copy link
Copy Markdown
Contributor

@MelvinBot In the row, it should be "Co-pilot" instead of an icon

@ShridharGoel
Copy link
Copy Markdown
Contributor

@MelvinBot Copilot icon should be the same one as the one used in MenuItem for copilot

Use the existing 'delegate.copilot' translation ("Copilot") as a text
button instead of the UserPlus icon in the wide-layout agent row.

Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
@MelvinBot
Copy link
Copy Markdown
Contributor Author

Changed the Copilot button in the wide-layout agent row from an icon-only (UserPlus) button to a text button showing "Copilot", using the existing delegate.copilot translation key.

Change the copilot MenuItem icon in EditAgentPage from UserPlus to
Users, matching the icon used in the Copilot settings page.

Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
@MelvinBot
Copy link
Copy Markdown
Contributor Author

Changed the copilot MenuItem icon in EditAgentPage from UserPlus to Users, matching the icon used for the Copilot page in settings (InitialSettingsPage).

@ShridharGoel
Copy link
Copy Markdown
Contributor

@MelvinBot Mark this as ready for review

@ShridharGoel
Copy link
Copy Markdown
Contributor

ShridharGoel commented May 24, 2026

@MelvinBot Fill all boxes in author checklist and add tests

@MelvinBot
Copy link
Copy Markdown
Contributor Author

I'm unable to mark this PR as ready for review due to permission restrictions. Please mark it as ready for review manually using the GitHub UI or by running gh pr ready 91533.

@ShridharGoel
Copy link
Copy Markdown
Contributor

@MelvinBot Mark this pull request as ready for review

@MelvinBot
Copy link
Copy Markdown
Contributor Author

I'm unable to mark this PR as ready for review due to permission restrictions in my environment. You can do it yourself by clicking the "Ready for review" button on the PR page or running:

gh pr ready 91533 --repo Expensify/App

@ShridharGoel
Copy link
Copy Markdown
Contributor

ShridharGoel commented May 24, 2026

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 there are no new alerts related to the canBeMissing param for useOnyx
  • I verified proper code patterns were followed (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
    • 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 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)
  • 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
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • 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
    • 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)
    • Any 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)
    • All JSX used for rendering exists in the render method
    • 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 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 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.
  • 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
Android: mWeb Chrome
Screen.Recording.2026-05-24.at.6.07.30.PM.mov
iOS: HybridApp
Screen.Recording.2026-05-24.at.5.56.35.PM.mov
iOS: mWeb Safari
Screen.Recording.2026-05-24.at.5.57.45.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-05-24.at.4.23.44.PM.mov
Screen.Recording.2026-05-24.at.9.19.22.PM.mov

@ShridharGoel
Copy link
Copy Markdown
Contributor

@MelvinBot Mark as ready

@MelvinBot
Copy link
Copy Markdown
Contributor Author

I'm unable to mark this PR as ready for review due to permission restrictions. Please mark it as ready manually using the GitHub UI or by running gh pr ready 91533.

@ShridharGoel
Copy link
Copy Markdown
Contributor

@MelvinBot Try again to mark it as ready for review. You have the permissions.

const icons = useMemoizedLazyExpensifyIcons(['DotIndicator']);
const icons = useMemoizedLazyExpensifyIcons(['DotIndicator', 'ChatBubble']);
const currentUserPersonalDetails = useCurrentUserPersonalDetails();
const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED);
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.

❌ CLEAN-REACT-PATTERNS-2 (docs)

AgentsListRow fetches introSelected, isSelfTourViewed, betas, personalDetails, and currentUserPersonalDetails solely to pass them as arguments to navigateToAndOpenReportWithAccountIDs. The component does not use any of these values for its own rendering. This makes the component a data intermediary for the chat navigation concern.

Encapsulate this data fetching and the navigateToAndOpenReportWithAccountIDs call inside a dedicated hook (e.g., useChatWithAccount) that only needs an accountID parameter. This removes five hooks/subscriptions from this component.


Reviewed at: b1dd748 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

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.

Addressed — the chat navigation data fetching is now encapsulated in the useChatWithAgent hook. AgentsListRow no longer subscribes to introSelected, isSelfTourViewed, betas, personalDetails, or currentUserPersonalDetails directly.

const [allPersonalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
const {showConfirmModal} = useConfirmModal();
const currentUserPersonalDetails = useCurrentUserPersonalDetails();
const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED);
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.

❌ CLEAN-REACT-PATTERNS-2 (docs)

EditAgentPage fetches introSelected, isSelfTourViewed, betas, allPersonalDetails, and currentUserPersonalDetails solely to pass them as arguments to navigateToAndOpenReportWithAccountIDs. None of these values are used for the component's own rendering logic.

Encapsulate this data fetching and the navigation call inside a dedicated hook (e.g., useChatWithAccount) that only needs an accountID parameter.


Reviewed at: b1dd748 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

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.

Addressed — same as above. EditAgentPage now uses the shared useChatWithAgent hook and no longer subscribes to these values directly. The unselected useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST) subscription has been removed.

const accountID = route.params.accountID;
const [agent, agentMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_AGENT_PROMPT}${accountID}`);
const [personalDetails, personalDetailsMetadata] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {selector: (list) => list?.[accountID]});
const [allPersonalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
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.

❌ PERF-11 (docs)

useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST) subscribes to the entire personal details collection without a selector. This will cause a re-render every time any user's personal details change. Line 38 already subscribes to the same key with a selector for the specific agent's details.

If this subscription is needed for navigateToAndOpenReportWithAccountIDs, consider encapsulating it in a dedicated hook (as suggested in the CLEAN-REACT-PATTERNS-2 comment). Otherwise, use a selector to narrow the subscription.


Reviewed at: b1dd748 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

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.

@MelvinBot Address this

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.

Addressed — the unselected useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST) has been moved into the useChatWithAgent hook, removing it from EditAgentPage.

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.

Done — see reply above.

const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED);
const [isSelfTourViewed] = useOnyx(ONYXKEYS.NVP_ONBOARDING, {selector: hasSeenTourSelector});
const [betas] = useOnyx(ONYXKEYS.BETAS);
const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
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.

❌ PERF-11 (docs)

useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST) subscribes to the entire personal details collection without a selector. This causes re-renders whenever any user's personal details change, even though this component only needs to open a chat for a single agent.

Consider extracting the chat navigation logic into a hook that owns this subscription, or narrow the data with a selector if possible.


Reviewed at: b1dd748 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

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.

@MelvinBot Address this

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.

Addressed — the unselected useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST) has been moved into the useChatWithAgent hook, removing it from AgentsListRow.

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.

Done — see reply above.

Comment thread src/hooks/useSwitchToDelegator.ts Outdated
const {isActingAsDelegate} = useDelegateNoAccessState();
const {showDelegateNoAccessModal} = useDelegateNoAccessActions();

const [account] = useOnyx(ONYXKEYS.ACCOUNT);
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.

❌ PERF-11 (docs)

useOnyx(ONYXKEYS.ACCOUNT) subscribes to the entire account object, but only account?.delegatedAccess is used (line 82). Any change to any field in the account object will trigger a re-render of every component that uses this hook.

Use a selector to narrow the subscription:

const [delegatedAccess] = useOnyx(ONYXKEYS.ACCOUNT, {
    selector: (account) => account?.delegatedAccess,
});

Reviewed at: b1dd748 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

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.

@MelvinBot Address this

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.

Addressed — now using a selector to subscribe only to delegatedAccess instead of the full account object:

const [delegatedAccess] = useOnyx(ONYXKEYS.ACCOUNT, {selector: (account: Account | undefined) => account?.delegatedAccess});

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.

Done — see reply above.

…ctors, remove useCallback

- Created useChatWithAgent hook to deduplicate chat navigation logic (CONSISTENCY-3, CLEAN-REACT-PATTERNS-2)
- Used selector for useOnyx(ONYXKEYS.ACCOUNT) to subscribe only to delegatedAccess (PERF-11)
- Removed redundant useCallback wrappers in useSwitchToDelegator (CLEAN-REACT-PATTERNS-0)
- Removed unneeded useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST) from AgentsListRow and EditAgentPage (PERF-11)
- Added accessibilityLabel to Copilot button in AgentsListRow

Co-authored-by: ShridharGoel <35566748+ShridharGoel@users.noreply.github.com>
Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
@ShridharGoel
Copy link
Copy Markdown
Contributor

@MelvinBot AgentsListRow now creates global subscriptions for every row. Shouldn't we call call these hooks once in AgentsPage and pass onChatPress(accountID) / onCopilotPress(login) into each row?

What do you think?

@MelvinBot
Copy link
Copy Markdown
Contributor Author

Yes, you're right — this should be lifted. Each AgentsListRow currently creates 12 global Onyx subscriptions (8 from useSwitchToDelegator + 4 from inline hooks like BETAS, PERSONAL_DETAILS_LIST, NVP_INTRO_SELECTED, NVP_ONBOARDING). With N agents, that's 12N duplicate subscriptions to the same global keys.

CopilotPage already does this correctly — it calls useSwitchToDelegator() once at the page level and passes the callback into each delegate row.

Recommendation: Lift both useSwitchToDelegator() and the chat-related hooks (BETAS, PERSONAL_DETAILS_LIST, NVP_INTRO_SELECTED, NVP_ONBOARDING) to AgentsPage, then pass onChatPress(accountID) / onCopilotPress(login) callbacks into AgentsListRow.

@ShridharGoel
Copy link
Copy Markdown
Contributor

@MelvinBot Update accordingly

…ow to AgentsPage

Moves the hook calls to AgentsPage so they run once, and passes
onChatPress / onCopilotPress callbacks into each AgentsListRow.
This avoids duplicating global Onyx subscriptions per agent row.

Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
@MelvinBot
Copy link
Copy Markdown
Contributor Author

Lifted useChatWithAgent and useSwitchToDelegator from AgentsListRow up to AgentsPage. The hooks now run once and onChatPress/onCopilotPress callbacks are passed into each row, eliminating duplicate Onyx subscriptions per agent.

NicolasBonet
NicolasBonet previously approved these changes May 26, 2026
@NicolasBonet
Copy link
Copy Markdown
Contributor

@Expensify/product can you please confirm we are good to go?

@shawnborton
Copy link
Copy Markdown
Contributor

shawnborton commented May 27, 2026

Can we use 8px for the gap between buttons?
CleanShot 2026-05-27 at 20 50 36@2x

cc @Expensify/design I think we usually do that.

@dannymcclain
Copy link
Copy Markdown
Contributor

Yes! 8px gap for horizontal button groupppppps

@NicolasBonet
Copy link
Copy Markdown
Contributor

@MelvinBot do the change requested in #91533 (comment)

@MelvinBot
Copy link
Copy Markdown
Contributor Author

The fix is to change styles.gap3 (12px) to styles.gap2 (8px) on line 96 of AgentsListRow.tsx.

I can only make code changes when directed by the PR author, an assignee, or an existing co-author. To have me implement this, the PR assignee or author should reply with a request tagging me.

@ShridharGoel
Copy link
Copy Markdown
Contributor

@MelvinBot do the change requested in #91533 (comment)

Co-authored-by: Shridhar Goel <ShridharGoel@users.noreply.github.com>
@MelvinBot
Copy link
Copy Markdown
Contributor Author

Changed the button gap from gap3 (12px) to gap2 (8px) in the wide-layout agent row.

@NicolasBonet
Copy link
Copy Markdown
Contributor

@ShridharGoel can you post screenshots or videos so product can approve?

Copy link
Copy Markdown
Contributor

@yuwenmemon yuwenmemon left a comment

Choose a reason for hiding this comment

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

LGTM - we can handle any pixel-level adjustments in a followup.

@yuwenmemon yuwenmemon merged commit cab9799 into main May 27, 2026
40 checks passed
@yuwenmemon yuwenmemon deleted the claude-addAgentChatAndCopilotButtons branch May 27, 2026 21:44
@ShridharGoel
Copy link
Copy Markdown
Contributor

Videos were already present here

@github-actions
Copy link
Copy Markdown
Contributor

🚧 @yuwenmemon 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.

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.

7 participants