Repository navigation
Conversation
|
@hoangzinh 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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const canReadVendors = canMemberRead(policy, currentUserLogin ?? '', CONST.POLICY.POLICY_FEATURE.VENDORS); | ||
| usePolicyConnectionsPrefetch(policy, canReadVendors); | ||
|
|
||
| const {agents: policyAgents} = useWorkspaceAgents(policy, false); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
useWorkspaceAgents(policy, false) subscribes (via useAgents) to the whole AGENT and POLICY collections without selectors and builds the full workspaceAgents row array (joined with workspace members, with per-agent closures) on every render. WorkspaceInitialPage only forwards policyAgents to getWorkspaceMenuItems, which reduces it to a single boolean hasAgentsError = policyAgents?.some((agent) => agent.errors). Materializing the entire agents array just to compute one brick-road boolean forces an expensive recompute on every write to those collections.
Derive the boolean with a scalar selector over the AGENT collection (scoped to the policy's agents) and pass that boolean into getWorkspaceMenuItems instead of the full array, so Onyx only compares a primitive:
const [hasAgentsError] = useOnyx(ONYXKEYS.COLLECTION.AGENT, {
selector: (agents) => Object.values(agents ?? {}).some((agent) => /* agent belongs to this policy && has errors */),
});Reviewed at: 2fe4a2f | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
The agent collection is not an issue. The policy collection is an issue indeed. I will handle that here https://github.com/Expensify/Expensify/issues/694677
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e8b9024b5
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| const canReadVendors = canMemberRead(policy, currentUserLogin ?? '', CONST.POLICY.POLICY_FEATURE.VENDORS); | ||
| usePolicyConnectionsPrefetch(policy, canReadVendors); | ||
|
|
||
| const {agents: policyAgents} = useWorkspaceAgents(policy, false, false); |
There was a problem hiding this comment.
The false, false flags only skip the API calls, so the whole derivation still runs here on every render:
useWorkspaceMemberssubscribes touseAllPersonalDetails()+usePersonalDetailsByLogins(employeeList)and loops over the entireemployeeListuseAgentssubscribes toCOLLECTION.AGENT,COLLECTION.POLICY, personal details, and mountsuseChatWithAgent,useSwitchToDelegator,useConfirmModal,useRuleBotGuardModalplus a highlight effect on atableRefthat is never attacheduseWorkspaceAgentsdoesworkspaceMembers.find(...)once per agent, so O(agents × members)
This also runs for users who never see the Agents row, since it's gated by isGroupPolicy && canReadPolicyFeature(AGENTS) && isCompanyAgentsBetaEnabled && !isAgentAccount in getWorkspaceMenuItems.
On a workspace with thousands of members, any personal details or policy collection update now re-renders this page and redoes the member join. Before, this page didn't subscribe to any of that.
The RBR only needs a boolean, and the read-only rows that useWorkspaceAgents pushes never carry errors. Could we extract the error merge from useAgents into a pure helper (e.g. getAgentErrors(agent) in a lib) and use a boolean selector instead?
const [hasAgentsError] = useOnyx(ONYXKEYS.COLLECTION.AGENT, {
selector: (agents) =>
Object.entries(agents ?? {}).some(([key, agent]) => {
const accountID = Number(key.slice(ONYXKEYS.COLLECTION.AGENT.length));
const isInWorkspace = agent?.ownerAccountID === workspaceAccountID || memberAccountIDs.has(accountID);
return isInWorkspace && !!getAgentErrors(agent);
}),
});useAgents would call getAgentErrors too, so there's still a single source of truth.
There was a problem hiding this comment.
I have updated the hooks, now useWorkspaceAgents use less data. Only biggest part now is useWorkspaceMembers subscribing to the personalDetailsList. Not sure if it's an issue given that the workspace members page already do the same or if we should split the manageable/non-manageable agents hooks
I still want to keep useAgents instead of using useOnyx to avoid duplicating the "deleted agents" filter
| const [hasAgentErrors] = useOnyx(ONYXKEYS.COLLECTION.AGENT, { | ||
| selector: (agents) => Object.values(agents ?? {}).some((agent) => !isEmptyObject(agent?.nameErrors) || !isEmptyObject(agent?.promptErrors) || !isEmptyObject(agent?.avatarErrors)), | ||
| }); | ||
| const {agents} = useAgents(false); |
There was a problem hiding this comment.
The old selector returned a boolean, so this hook only re-rendered when the boolean flipped. With useAgents(false), it now re-renders on every COLLECTION.AGENT / COLLECTION.POLICY / personal details change and rebuilds all agent rows (with closures). This hook backs the Account tab, which is always mounted on wide layouts.
There are also two behavior changes the description doesn't mention:
useAgentsdrops agents without personal details (if (!details) return [];), so an agent withnameErrors/promptErrorswhose personal details aren't loaded no longer shows an RBR here.- Generic
agent.errorsnow counts (create failureagentsPage.error.genericAdd, delete failurecommon.genericErrorMessageinAgent.ts). The old selector ignored these. This is probably desirable, but can you confirm it's intended?
With a shared getAgentErrors(agent) helper (see my comment on WorkspaceInitialPage), this can stay a cheap selector:
const [hasAgentErrors] = useOnyx(ONYXKEYS.COLLECTION.AGENT, {
selector: (agents) => Object.values(agents ?? {}).some((agent) => !!getAgentErrors(agent)),
});There was a problem hiding this comment.
Updated the hook, now useAgents only subscribes to the agent collection. I think this is still fine as agents are not supposed to be that many or change often
| screenName: SCREENS.WORKSPACE.AGENTS, | ||
| sentryLabel: CONST.SENTRY_LABEL.WORKSPACE.INITIAL.AGENTS, | ||
| badgeTranslationKey: 'common.beta', | ||
| brickRoadIndicator: hasAgentsError ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined, |
There was a problem hiding this comment.
Question: getPolicyBrickRoadIndicatorStatus (PolicyUtils/errors.ts) doesn't include agent errors, so this RBR shows on the Agents row but not on the workspace row in the Workspaces list or the Workspaces tab. Categories behave the same way today, so this isn't blocking. Is that the expected scope for this issue?
There was a problem hiding this comment.
I wanted to fix this but it requires subscribing to policy members too. Probably NAB. I have added RBR to the Account tab (since agents can be found in Account > Agents too) so the agents errors are not hidden (even if the policy does not include them).
This reverts commit 6ad2a3a.
s77rt
changed the title
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
I have done some refactor. I think I will also remove |
Explanation of Change
useAgentsanduseWorkspaceAgentstouseAgents/useAgentsTableanduseWorkspaceAgents/useWorkspaceAgentsTableFixed Issues
$ https://github.com/Expensify/Expensify/issues/694594
https://github.com/Expensify/Expensify/issues/694677
PROPOSAL:
Tests
(Not for QA)
Onyx.merge('agent_7363', {promptErrors: {"1": "test"}})Offline tests
n/a
QA Steps
n/a
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari