Repository navigation
Conversation
…nt account components Seatbelt-Operation: e7694751-9deb-4d30-b155-87815525cae0
Seatbelt-Operation: 77793e37-f1a1-48d2-89b3-bae480b8b95a
|
@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] |
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. |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| label={translate('newRoomPage.visibility')} | ||
| items={visibilityOptions} | ||
| onValueChange={(value) => setVisibility(value as typeof visibility)} | ||
| onValueChange={(value) => { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This onValueChange handler duplicates the exact logic introduced for the writeCapability picker just above it: find the option whose value matches, and if it exists, set state to selectedOption.value. The two handlers differ only in the options array (visibilityOptions vs writeCapabilityOptions) and the setter (setVisibility vs setWriteCapability), so the shared shape should be factored out instead of copied.
Extract a small helper and reuse it for both pickers:
const getMatchingOptionValue = <T,>(options: Array<{value: T}>, value: string): T | undefined =>
options.find((option) => option.value === value)?.value;
// writeCapability picker
onValueChange={(value) => {
const next = getMatchingOptionValue(writeCapabilityOptions, value);
if (next) {
setWriteCapability(next);
}
}}
// visibility picker
onValueChange={(value) => {
const next = getMatchingOptionValue(visibilityOptions, value);
if (next) {
setVisibility(next);
}
}}Reviewed at: 73eb411 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
.find() guard in WorkspaceNewRoomPage.tsx is a valid find by the bot.
There was a problem hiding this comment.
Extracted the repeated lookup into one local typed findRoomOption helper used by both pickers. It returns only offered option values; missing and unmatched selections leave the current state unchanged.
There was a problem hiding this comment.
Removed the duplicated lookup through one local typed helper shared by both handlers. Added tests for valid, missing, and unmatched picker selections.
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-10-17.at.17.10.43.mov |
| act(() => refreshEmojiWindow?.()); | ||
| const {heading, host} = getEmojiHeaderHost(natureIndex, 'web'); | ||
| mockFocusDescriptors.push({host, descriptor: Object.getOwnPropertyDescriptor(host, 'focus')}); | ||
| // @ts-expect-error The external web host can lack focus. All remaining native host fields and ref identity stay real. |
There was a problem hiding this comment.
🟠 tests/ui/EmojiPickerMenuComponentTest.tsx:700
This @ts-expect-error manufactures a host the type says can't exist. headerRef is ComponentRef<typeof View> (index.tsx:68), and on web that host is a DOM element, so focus is always there. The comment "The external web host can lack focus" is not accurate.
This leg only exists to cover the ?. in headerRef.current.focus?.() (index.tsx:86), which is a leftover from the old HTMLElement cast.
Can we call headerRef.current.focus() directly and drop this part of the test ? In a PR that removes type bypasses I'd rather not add a new one in the tests.
There was a problem hiding this comment.
Removed the unsupported host-without-focus test and its suppression. The web handler now calls focus directly on the supported View ref.
| bankCountry: CONST.COUNTRY.US, | ||
| accountData: {type: CONST.BANK_ACCOUNT.TYPE.BUSINESS, allowDebit: true, state: CONST.BANK_ACCOUNT.STATE.SETUP, addressName: 'Chase Bank'}, | ||
| }); | ||
| type SettlementPageProps = PlatformStackScreenProps<SettingsNavigatorParamList, typeof SCREENS.WORKSPACE.DYNAMIC_WORKSPACE_EXPENSIFY_CARD_SETTINGS_ACCOUNT>; |
There was a problem hiding this comment.
🟠 tests/ui/DynamicWorkspaceSettlementAccountPageTest.tsx:132
SettlementPageProps is the same type as WorkspaceSettlementAccountPageProps in DynamicWorkspaceSettlementAccountPage.tsx:43.
That file is already part of the PR, so can we export it from there and import it here instead of re-deriving ?
There was a problem hiding this comment.
Exported the production WorkspaceSettlementAccountPageProps type and imported it in the test, removing the copied props definition.
| type BankIconParams = { | ||
| styles: ThemeStyles; | ||
| bankName?: BankName; | ||
| bankName?: AccountData['addressName']; |
There was a problem hiding this comment.
🟡 src/components/Icon/BankIconsUtils.ts:10
AccountData['addressName'] ties a shared icon helper to one Onyx field, but the other getBankIcon callers pass card?.accountData?.bank, withdrawalIDItem.bankName and reimbursementAccount?.achData?.bankName. getBankNameKey in this same file already takes bankName: string.
Can we use bankName?: string here ?
There was a problem hiding this comment.
Changed the shared icon helper's bankName input to an optional string, removing its dependency on one Onyx field's type.
|
|
||
| function BankAccountListItemLeftElement({bankName}: {bankName: BankName}) { | ||
| type BankAccountListItemLeftElementProps = { | ||
| bankName: NonNullable<BankIconParams['bankName']>; |
There was a problem hiding this comment.
🟢 src/components/SettlementAccountSelector.tsx:34
With string above, this can be just bankName: string instead of NonNullable<BankIconParams['bankName']>.
There was a problem hiding this comment.
Simplified the selector prop to bankName: string and removed the derived BankIconParams type dependency.
|
@KJ21-ENG Dropped (4) comments, please tag me once addressed 🙌 |
Seatbelt-Operation: reviewer-fix-103920-148a7b7c-46f3-4e1d-92e3-d55572edf948
Seatbelt-Operation: upstream-ci-repair-103920-7e4b3ccd-cb71-4f37-8701-8f35e50c8125
|
@ikevin127 Could you please check this once you get a chance, Thanks! |
| // Upstream adds an optional pending-state argument. These settings have no pending update. | ||
| for (const call of settlementCalls) { | ||
| expect(call.slice(6)).toStrictEqual(call.length === 6 ? [] : [false]); | ||
| } |
There was a problem hiding this comment.
🟡 tests/ui/DynamicWorkspaceSettlementAccountPageTest.tsx:191 These assertions are written to pass whether updateSettlementAccount gets 6 or 7 arguments (call.length === 6 ? [] : [false]), because the branch is behind main. Once merged, the 6-argument case can never happen, and the conditional only makes it unclear what the page is expected to pass.
Please merge main and assert the exact call instead, here and at line 222 below:
expect(updateSettlementAccount).toHaveBeenNthCalledWith(1, 'example.com', fundID, policyID, programKey, 22, 11, false);
expect(updateSettlementAccount).toHaveBeenNthCalledWith(2, 'example.com', fundID, policyID, programKey, 33, 11, false);This branch has not been deployed
3 participants
Explanation of Change
Use catalog-owned emoji header keys, the existing typed header ref, option lookups for room posting/visibility, and bank-name types that accept actual account names. Room callbacks ignore values outside the offered options, and reconciliation route lookup skips an absent connection; supported selections, focus, and fallback icons are preserved.
Fixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Tests
Relevant upstream TypeScript and Jest checks passed.
tests/ui/EmojiPickerMenuComponentTest.tsx,tests/ui/WorkspaceNewRoomPageTest.tsxandtests/ui/DynamicWorkspaceSettlementAccountPageTest.tsxcover focus, room submission values, settlement IDs/icons and reconciliation-link visibility.Room settings
Setup: Use an admin account allowed to create a room in the intended workspace.
Offline tests
No separate offline case is needed: the changed option callbacks use the already offered values and change no room-creation request, storage, or reconnect behavior.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
Recordings (1)
Windows desktop Chrome: room-picker selection/dismissal and saved settings after reload were observed. Console errors occurred on reload without confirmed attribution to this PR. Native Android/iOS, mobile-web and macOS Chrome/Safari results are unverified.
01-room-picker-persistence-desktop.mp4