Repository navigation
Conversation
Seatbelt-Operation: adc93f02-11ed-4ef2-bace-381d00d62b1f
Seatbelt-Operation: add2564d-f961-4201-85ce-d47084230597
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
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.
KJ21-ENG
changed the title
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-10-19.at.17.49.10.mov |
| return acc; | ||
| }, | ||
| [] as Array<OnyxCollectionValuesMapping[T]>, | ||
| [] as Array<OnyxCollectionValuesMapping[T] | Record<string, Pick<OnyxTypes.PolicyCategory, 'name'>>>, |
There was a problem hiding this comment.
🟡 src/libs/SearchQueryUtils.ts:1615 This is still a type assertion ([] as Array<...>), and this PR is meant to remove them. Pass the type to reduce instead:
return Object.keys(policyData).reduce<Array<OnyxCollectionValuesMapping[T] | Record<string, Pick<OnyxTypes.PolicyCategory, 'name'>>>>((acc, curr) => {
...
}, []);There was a problem hiding this comment.
Removed the empty-array assertion and supplied TValue[] directly to reduce. The accumulator is now typed without a cast.
| policies?: OnyxCollection<OnyxTypes.Policy>, | ||
| ): Array<OnyxCollectionValuesMapping[T]> { | ||
| ): Array<OnyxCollectionValuesMapping[T]>; | ||
| function getAllPolicyValues( |
There was a problem hiding this comment.
🟡 src/libs/SearchQueryUtils.ts:1587 Adding a second overload and widening the main signature just for the category projection makes this hard to follow. The function only uses key as a prefix, and isPolicyCollection already takes data: unknown, so it can simply be generic over the value type.
That also removes the cast above:
function getAllPolicyValues<TValue>(
policyID: Filter | undefined,
key: OnyxCollectionKey,
policyData: OnyxCollection<TValue>,
policies?: OnyxCollection<OnyxTypes.Policy>,
): TValue[] {
...
return Object.keys(policyData).reduce<TValue[]>((acc, curr) => { ... }, []);
}The full-model callers keep their types, and buildFilterFormValuesFromQuery gets the name-only projection without a special overload.
There was a problem hiding this comment.
Replaced the overloads with one TValue-based getAllPolicyValues function. Full-model callers retain their inferred result types, and the name-only category projection works without a special overload.
| expect(projected.at(0)).toBe(names[`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}1`]); | ||
| expect(negated).toEqual([{travelKey: {name: 'Travel'}}]); | ||
| expect(all).toEqual([...projected, ...negated]); | ||
| // @ts-expect-error -- name projections do not promise the omitted enabled metadata. |
There was a problem hiding this comment.
🟠 tests/unit/Search/SearchQueryUtilsTest.ts:5574 Like on #103920, I'd rather not add new @ts-expect-errors in a PR that removes type workarounds. The three suppressions plus the IsEqual checks at line 5564 test the overloads' types, not their behaviour.
The calls under @ts-expect-error also really run with the wrong data. If you switch to the generic version above, this block can shrink to the runtime checks (projected, negated, all).
There was a problem hiding this comment.
Removed all three @ts-expect-error suppressions, their invalid calls, and the IsEqual block. Kept the runtime checks for projected, negated, and all-policy values, including ordering and reference identity.
| // Then no reconstruction drops the undefined member and no key/model correlation is lost | ||
| expect(result.current).toBe(cards); | ||
| expect(result.current).toHaveProperty(`${ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST}2`, undefined); | ||
| const sessionType: IsEqual<ReturnType<typeof useSession>, UseOnyxResult<OnyxValue<typeof ONYXKEYS.SESSION>>[0]> = true; |
There was a problem hiding this comment.
🟢 tests/unit/createOnyxContextTest.tsx:93 expect([sessionType, betasType, cardsType]).toEqual([true, true, true]) can never fail at runtime. The types are already checked by tsc when the IsEqual constants are assigned.
Please drop the IsEqual block, since the hydrate/update tests above already cover the hook's behaviour.
There was a problem hiding this comment.
Removed the IsEqual block and its always-true assertion. The four hydration and update tests remain and pass.
| act(() => ref.current?.focusLastSelected()); | ||
| act(() => ref.current?.resetFocus()); | ||
| act(() => ref.current?.blur()); | ||
| act(() => ref.current?.clear()); |
There was a problem hiding this comment.
🟢 tests/ui/ValidateCodeInputEventTest.tsx:216 onChangeText was already last called with '' by the backspace step above, so toHaveBeenLastCalledWith('') passes even if clear() does nothing.
Please call onChangeText.mockClear() before clear(), so the assertion actually checks it.
There was a problem hiding this comment.
Cleared the onChangeText mock immediately before clear(), then asserted exactly one new callback with an empty string. The earlier backspace callback can no longer satisfy this assertion.
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
PR is not ready yet. Most of the code change is sound, but the description fails one of its own test steps, and a few test files add type-check workarounds of the kind I asked you to remove on #103920.
Nobody has left other inline comments yet. Codex found nothing, and CI fails Check independent approval and checklist, which wait on your approval and your unfinished checklist.
The 🟠 Shift+Tab item: the recording shows Shift+Tab leaving the code input, which the test step says shouldn't happen. The Tab logic is the same as on main; only the shift check changed. React's synthetic keyboard events carry shiftKey directly, so 'shiftKey' in event is true on web.
The bug is not present on staging, I checked - meaning it'd be a regression caused by this PR. QA will run that step as written and fail it.
Screen.Recording.2026-10-19.at.18.09.57.mov
@KJ21-ENG Please tag me once the regression and comments are addressed.
Restore baseline Shift modifier access with a typed web modifier. Seatbelt-Operation: reviewer-shift-rollback-sign-104082-49f03569-71bc-4ad9-88a4-d61534f2a9f9
|
@ikevin127 Restored the original direct event.shiftKey check and described the optional web modifier using React’s keyboard-event type, without a cast. Please check this when you get a chnace |
|
@KJ21-ENG Update on the 🟠 Shift+Tab item from my review: I'm withdrawing it as a regression from this PR. I pulled the latest head ( But the same thing happens on a local build of latest Two things before I approve:
|
This branch has not been deployed
3 participants
Explanation of Change
Remove unsafe assertions from shared text/code inputs, Onyx contexts, Search filter projections and web CSS insertion. Use the actual input ref and checked focus/modifier properties while preserving supported input behavior, avatar fallbacks and permission loading states.
Fixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Tests
Current-head upstream Jest and typecheck passed. Focused checks:
npx jest --runInBand TextInputWebRefAndFocusTest ValidateCodeInputEventTest DomUtilsWebTest createOnyxContextTest.Setup: Use an email address you control to reach normal sign-in code entry.
Platforms: Android native, Android mWeb Chrome, iOS native, iOS mWeb Safari and macOS Chrome/Safari. Run hardware-key steps where a keyboard is available.
Paste
12. Verify only the first two slots fill.Keyboard navigation
Offline tests
No additional offline case: submission/reconnect handling is unchanged; focused event tests cover blocked offline Enter and online submission.
QA Steps
Same as tests on staging containing this PR.
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: paste, forward Tab and boundary checks were observed, but interior Shift+Tab left the code input instead of moving back. Attribution remains unconfirmed without a base comparison. Native, mobile-browser and macOS results are unverified.
pr104082-code-paste-focus-desktop.mp4