-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix console errors related to forward ref batch9 #78270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix console errors related to forward ref batch9 #78270
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
…-forwardRef-batch9
…-forwardRef-batch9
…-forwardRef-batch9
…-forwardRef-batch9
|
Just to confirm: this is last cleanup PR which removes all deprecated forwardRef / unused ref usages throughout the app? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2e2e6a89a
ℹ️ 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".
|
@aimane-chnaif It's mostly cleanup but it also takes care of the last few (I believe 8) occurrences of |
|
No product considerations, removing my review |
|
ESLINT check fail unrelated - fails on main |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppDatePicker-ios.movFlatListWithScrollKey-ios.moviOS: mWeb SafariMacOS: Chrome / SafariContextMenu.movDatePicker.movFlatListWithScrollKey.mov |
|
Please merge main and fix lint |
|
@aimane-chnaif on it |
…-forwardRef-batch9
| * FlatList component that handles initial scroll key. | ||
| */ | ||
| function FlatListWithScrollKey<T>(props: FlatListWithScrollKeyProps<T>, ref: ForwardedRef<RNFlatList>) { | ||
| function FlatListWithScrollKey<T>({ref, ...props}: FlatListWithScrollKeyProps<T>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: I think we can simplify this component.
import BaseFlatListWithScrollKey from './BaseFlatListWithScrollKey';
export default BaseFlatListWithScrollKey;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can’t just re-export it because this wrapper forwards the ref that's used to call scroll methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should {...props} forward already as props contains ref?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yeah, actually the concern with access to scroll methods isn't relevant, but now I am not sure how major is the difference in props since BaseFlatListWithScrollKey and FlatListWithScrollKey have different prop types.
src/components/ValidateCodeActionModal/ValidateCodeForm/BaseValidateCodeForm.tsx
Outdated
Show resolved
Hide resolved
src/components/ValidateCodeActionModal/ValidateCodeForm/index.android.tsx
Outdated
Show resolved
Hide resolved
src/components/ValidateCodeActionModal/ValidateCodeForm/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx
Lines 421 to 425 in d748024
| description={contextAction.getDescription?.(selection) ?? ''} | |
| isAnonymousAction={contextAction.isAnonymousAction} | |
| isFocused={focusedIndex === index} | |
| shouldPreventDefaultFocusOnPress={contextAction.shouldPreventDefaultFocusOnPress} | |
| onFocus={() => setFocusedIndex(index)} |
When navigating the report action context menu with the keyboard (arrow keys), focusedIndex is still tracked and applied to items, but the Enter-key shortcut that previously triggered the focused item was removed along with the imperative ref. As a result, on web/desktop keyboard users can focus items but pressing Enter no longer invokes onPress, so the menu is not operable without a mouse. Consider re‑adding an Enter handler (e.g., useKeyboardShortcut) or otherwise wiring focused items to activate on Enter.
ℹ️ 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".
|
To use Codex here, create a Codex account and connect to github. |
|
@aimane-chnaif I've got carried away with those displayNames and forgot to remove them 😆 |
mountiny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, lets give it a go, but let's be on a look out for the issues in the flows using the arrows
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.9-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.9-2 🚀
|
Explanation of Change
The whole issue comes from
forwardRefbeing deprecated in the near future of react thus we will no longer be able to access the passedrefbyelement.refasforwardRefis needed for passingrefas a separate attribute. Now in order to pass there reference it has to be passed as one of thepropsso we can access it directly or like that ->element.props.ref.In order to adjust the codebase to new react recommendations in every file that was using
forwardRefthe ref had to be moved to thepropsinstead of being a separate attribute andforwardRefhad to be completely removed.In response to the number of files that needed adjusting the changes are made in batches with this one being
the ninth one.NOTE: This PR removes the custom react
forwardRefas it is no longer needed since all the usages offorwardRefhave been removed -> removal ofreact.d.tsfile[NO QA] NOTE: This PR also takes care of unused
ref's noticed in the previous batches -> they are mostly NO QA as it's a simple code cleanup (some will require tests)UNUSED COMPONENTS (@mountiny): There are 2 components which are used in the code but never activated in App:
KeyboardDismissibleFlatListonly renders when @components/FlatList is givenenableAnimatedKeyboardDismissal=true, but that prop isn’t set toTrueanywhere today, so the component never mounts in normal app use.AnimatedFlatListWithCellRendereris used only inside KeyboardDismissibleFlatList, so it likewise never mounts.The decision what should be done with those components has to be made (there is also a
KeyboardDismissibleFlatListContextso it's usefulness also needs to checked)Modified files are grouped below by the components they are related to. In order to make testing easier there are also screenshots of places in app where you can find those components.
FlatListWithScrollKey
src/components/FlatList/FlatListWithScrollKey/BaseFlatListWithScrollKey.tsxsrc/components/FlatList/FlatListWithScrollKey/index.ios.tsxsrc/components/FlatList/FlatListWithScrollKey/index.tsxsrc/components/FlatList/FlatListWithScrollKey/types.tsValidateCodeCountdown
src/pages/signin/ValidateCodeCountdown/index.tsxsrc/pages/signin/ValidateCodeCountdown/types.tsUnused ref's removal requiring testing
ContextMenu
src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsxsrc/components/ContextMenuItem.tsxDatePicker
src/pages/EditReportFieldDate.tsxsrc/components/DatePicker/index.tsxsrc/components/DatePicker/types.tsOptionRowRendererComponent
src/components/LHNOptionsList/OptionRowRendererComponent/index.native.tsx[NO QA] files are listed below:
Files with removed forwardRef's
AnimatedFlatListWithCellRenderer -> not used anywhere in App
src/components/AnimatedFlatListWithCellRenderer.tsxKeyboardDismissibleFlatList -> not used anywhere in App
src/components/KeyboardDismissibleFlatList/index.ios.tsxsrc/components/KeyboardDismissibleFlatList/index.tsxUnused ref's removal
src/types/modules/react.d.ts- COMPLETELY REMOVEDsignin/ValidateCodeForm
src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.tsxsrc/pages/signin/ValidateCodeForm/index.android.tsxsrc/pages/signin/ValidateCodeForm/index.tsxValidateCodeActionForm
src/components/ValidateCodeActionForm/type.tsPopoverWithoutOverlay
src/components/PopoverWithoutOverlay/index.tsxsrc/components/PopoverWithoutOverlay/types.tsTimePicker
src/components/TimePicker/TimePicker.tsxtests/ui/components/TimePickerTest.tsxsrc/components/AmountTextInput.tsx- clearly not a TimePicker but the testID was added to it to allow for proper testing of the TimePicker componentHighlightableMenuItem
src/components/HighlightableMenuItem.tsxAvatarCapture
src/pages/settings/Profile/Avatar/AvatarCapture/types.tsLottie
src/components/Lottie/index.tsxAccountValidatePage
src/pages/settings/Security/MergeAccounts/AccountValidatePage.tsxFixed Issues
$ #67033
PROPOSAL: N/A
Tests
Request a new code in 00:30countdown works properlyReportsand turn onReport fields-> pressAdd fieldand chooseTypeDateEditReportFieldDatePageandDatePickerfield on this page render and work properly. Try changing the date of the report and verify that it changed properlyOn mobile
InboxOffline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Screen.Recording.2026-01-19.at.03.50.04.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-01-19.at.03.47.28.mov