Repository navigation
Conversation
Seatbelt-Operation: 2facc752-58f4-4853-995b-5cf055d3ff11
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@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. |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-10-16.at.18.35.11.mov |
| import React, {useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react'; | ||
| import {DeviceEventEmitter, StyleSheet} from 'react-native'; | ||
|
|
||
| type WebMarkdownInput = HTMLDivElement & Pick<TextInput, 'clear' | 'isFocused'> & {value?: string}; |
There was a problem hiding this comment.
src/components/Composer/implementation/index.tsx L30 / L295
🟠 value?: string on WebMarkdownInput is what forces onClear to become (text: string | undefined) => void across types.ts, ComposerWithSuggestions and useComposerSubmit, and that's why validateAndSubmitDraft now has to throw by hand. Can we normalize it at the source instead ?
const currentText = textInputRef.current.value ?? '';Then onClear can stay (text: string) => void everywhere, and the undefined branch plus the hand-written TypeError in useComposerSubmit can go.
Right now, a clear fired before live-markdown sets value still crashes the report screen. The only difference is that the crash is now deliberate.
| // Live-markdown may clear before it initializes value. Keep that failure before submission effects. | ||
| if (draftMessage === undefined) { | ||
| throw new TypeError("Cannot read properties of undefined (reading 'trim')"); | ||
| } |
There was a problem hiding this comment.
Compose/useComposerSubmit.ts` L63-66**
🟠 Throwing a hand-written TypeError that copies the V8 message ("Cannot read properties of undefined (reading 'trim')") to preserve a crash isn't something we want in the codebase. It also makes the stack trace in Sentry point at a fake error.
If we fix the value ?? '' above, this block goes away. Otherwise, return early here.
| // Preserve the failed focus invocation after Onyx records focus. | ||
| if (ref === null) { | ||
| throw new TypeError("Cannot read properties of null (reading 'focus')"); | ||
| } |
There was a problem hiding this comment.
src/libs/actions/InputFocus/index.web.ts L20-23
🟠 Same pattern here. If ref is null, throwing a hand-written TypeError from inside a useEffect in ReportActionItemMessageEdit takes down the edit row. Let's just do ref?.focus().
The test preserves the null TypeError after that update in ComposerImperativeRefTest should then assert that it doesn't throw.
| type ComposerWithSuggestionsRef = Pick<ComposerRef, 'clear' | 'blur' | 'isFocused'> & { | ||
| readonly setSelection: ComposerRef['setSelection']; | ||
| readonly setNativeProps: ComposerRef['setNativeProps']; | ||
| readonly preventFocusOnFirstResponderOnce: ComposerRef['preventFocusOnFirstResponderOnce']; | ||
| readonly scrollTop: number | undefined; |
There was a problem hiding this comment.
src/pages/inbox/report/ReportActionCompose/ComposerWithSuggestions.tsx L84-89 / L960-1030
🟡 The only consumers of ComposerWithSuggestionsRef (ComposerProvider, useComposerFocus, useEditMessage, ComposerContext) use focus, blur, isFocused, clearWorklet, replaceSelectionWithText and resetHeight.
Nothing reads setSelection, setNativeProps, preventFocusOnFirstResponderOnce, scrollTop or clear from the outer handle, so the Object.defineProperties block is about 40 lines of dead getters.
Can we drop those from the type and the handle ?
useImperativeHandle(ref, (): ComposerWithSuggestionsRef => ({
focus,
replaceSelectionWithText,
getCurrentText,
clearWorklet,
resetHeight,
blur: () => composerRef.current?.blur(),
isFocused: () => !!composerRef.current?.isFocused(),
}));This also gets rid of the throw new TypeError('Composer ref is not available') wrappers. A side note in case someone wires the setNativeProps getter up later: it returns an unbound host-instance method, and ReactNativeElement.setNativeProps relies on this.
The old Proxy covered for that, but this plain object won't.
| defaultSelectedTab?: Extract<TTabName, string>; | ||
| onTabSelected?: (newTabName: Extract<TTabName, string> | undefined) => void; |
There was a problem hiding this comment.
src/libs/Navigation/OnyxTabNavigator.tsx L44-45 / L399
🟡 Widening onTabSelected to | undefined spreads into useResetIOUType (where tabSelectedTypeRef is now IOURequestType | undefined | null) and useOdometerDraftHydrator.
Passing undefined into resetIOUTypeIfChanged would call initMoneyRequest with newIouRequestType: undefined and wipe the draft, so it shouldn't be possible through the public API.
Can we guard it in the navigator and keep the callback narrow ?
if (!newSelectedTab) {
return;
}
Tab.setSelectedTab(id, newSelectedTab);
onTabSelected(newSelectedTab);onTabSelected?: (newTabName: TTabName) => void;Also, Extract<TTabName, string> is redundant since TTabName extends string already; plain TTabName is enough.
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
@KJ21-ENG I found no blockers. There are 5 actionable comments I'd like addressed though, and the two most important ones are the same problem: the PR adds hand-written crashes (throw new TypeError(...)) to copy the old null/undefined crashes, and adds new tests that lock them in.
The Composer and tab-navigator files are already identical to the PR head on this branch. To check that the createOnyxTabNavigator factory doesn't stop React Compiler from compiling OnyxTabNavigator, I ran Babel and OXC on the main and PR-head versions.
Both still compile and memoize it, so that's fine. The old Proxy forwarding on ComposerWithSuggestions doesn't break any current caller either: the outer handle is only used through focus, blur, isFocused, clearWorklet, replaceSelectionWithText and resetHeight, and setNativeProps is always called on the inner ref.
dylanexpensify
left a comment
dylanexpensify
left a comment
There was a problem hiding this comment.
refactor, product input not required
Seatbelt-Operation: reviewer-fix-103910-06791fa4-9c10-4ba3-8718-b35d312fac7c
Seatbelt-Operation: upstream-ci-repair-103910-0f9fc68a-d352-4633-96c8-b2525d1538d3
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Thanks for the changes! All five of your comments are fixed in code, and the only red CI check is a flaky Jest job (a worker killed by the CI host) that needs a re-run.
|
Jest tests seem to be failing? |
This branch has not been deployed
4 participants
Explanation of Change
Replace asserted composer and DOM shapes with platform-specific ref contracts and DOM checks, and bind expense navigators to their supported tab names. Forwarding handles read the current input, DOM writes require a real DOM element, and browser IME confirmation is checked through its native keyboard event.
Changed files:
src/components/Composer/implementation/index.native.tsxsrc/components/Composer/implementation/index.tsxsrc/components/Composer/types.tssrc/hooks/useHtmlPaste/index.tssrc/hooks/useHtmlPaste/types.tssrc/hooks/useOdometerDraftHydrator.tssrc/hooks/useResetIOUType.tssrc/libs/ComponentUtils/index.native.tssrc/libs/ComponentUtils/index.tssrc/libs/InputUtils/index.tssrc/libs/InputUtils/types.tssrc/libs/Navigation/OnyxTabNavigator.tsxsrc/libs/ReportActionComposeFocusManager.tssrc/libs/actions/InputFocus/index.tssrc/libs/actions/InputFocus/index.web.tssrc/libs/focusComposerWithDelay/types.tssrc/libs/preventTextInputFocusOnFirstResponderOnce/index.ios.tssrc/libs/preventTextInputFocusOnFirstResponderOnce/types.tssrc/libs/updateMultilineInputRange/types.tssrc/pages/home/ForYouSection/ConciergePromptBox.tsxsrc/pages/inbox/report/ReportActionCompose/ComposerContext.tssrc/pages/inbox/report/ReportActionCompose/ComposerWithSuggestions.tsxsrc/pages/inbox/report/ReportActionCompose/ReportActionComposeUtils.tssrc/pages/inbox/report/ReportActionCompose/Suggestions.tsxsrc/pages/inbox/report/ReportActionCompose/getScrollPosition/types.tssrc/pages/inbox/report/ReportActionCompose/updateNativeTextInputValue/index.ios.tssrc/pages/inbox/report/ReportActionCompose/useComposerSubmit.tssrc/pages/inbox/report/ReportActionCompose/useComposerSuggestions.tssrc/pages/inbox/report/ReportActionCompose/useEditMessage.tssrc/pages/inbox/report/ReportActionItemMessageEdit.tsxsrc/pages/iou/request/DistanceRequestStartPage.tsxsrc/pages/iou/request/IOURequestStartPage.tsxtests/ui/ComposerImperativeRefTest.tsxtests/ui/DistanceRequestStartPageTest.tsxtests/ui/IOURequestStartPageManualTabTest.tsxtests/ui/OnyxTabNavigatorTest.tsxtests/ui/hooks/useHtmlPasteTest/index.tsxtests/unit/ComposerSelectionNotificationTest.tsxtests/unit/InputUtilsTest.tstests/unit/focusComposerWithDelayTest.tstests/unit/hooks/useEditMessageComposerCommands.test.tstests/unit/useComposerSubmitClearValueTest.tsxtests/unit/useResetIOUTypeTabSelectionTest.tsxFixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Tests
Focused repository lint, formatting, spelling, and checked-in Jest tests passed, including all 15 selected suites and 154 tests.
QA Steps
1. Composer lifecycle.
2. Message editing.
3. Paste and scroll.
4. Expense-tab persistence.
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 (5)
01-composer-lifecycle-desktop.mp4
02-message-edit-desktop.mp4
03-message-edit-narrow.mp4
04-paste-scroll-desktop.mp4
05-manual-scan-persistence-desktop.mp4