[No-QA] refactor: Navigation code in SelectionList component - #92662
MonilBhavsar merged 16 commits into
Conversation
…vigation-code-in-SelectionList-component
|
@mkhutornyi 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04144e2b06
ℹ️ 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!
|
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Please merge main |
ReviewReviewed the SelectionList navigation/focus consolidation. The extraction is faithful — the imperative ref API, props interface, arrow-key/scroll-suppression logic, Enter/Ctrl+Enter shortcuts, and focus-restore-aware setters all match the originals after substitution. No correctness regressions found. Two minor behavioral deltas worth a look (posted inline):
Things I checked that are fine
Next Steps: Reply with |
…vigation-code-in-SelectionList-component
Done. |
…vigation-code-in-SelectionList-component
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c31046bbe
ℹ️ 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".
Regression scan — no functional regressions foundI diffed every removed inline block against its extracted replacement across both list components. The consolidation is faithful: focus-restore scroll suppression, Two things to be aware of (neither is user-facing today): 1. The refactor leans entirely on React Compiler for memoization — make sure compliance stays green.
→ Confirm 2. Minor telemetry attribute change (not a bug). What I verified equivalent (old → new)
Note: my initial pass flagged a style-precedence flip in Next Steps: Reply with |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db307d93f6
ℹ️ 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".
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
…vigation-code-in-SelectionList-component
|
@mkhutornyi please fix reviewer checklist when you have a moment |
Regression reviewI diffed each consolidated module against both original forks ( Verdict: no functional regressions found. The per-list differences that mattered are all preserved, and all CI ( What I checked (and why each is safe)Keyboard shortcuts —
Arrow-key focus —
Scroll —
Empty state —
Text input — refs/handlers move into Types: the removed Residual risk (manual QA worth doing): the unit tests cover the hooks in isolation, but the consolidated focus/scroll/highlight behavior is inherently runtime-only. Since the PR has no screenshots/videos and the test steps are just "no console errors," I'd manually verify on web: (1) keyboard arrow/Tab navigation highlights rows and scrolls correctly in a long list, (2) Next Steps: Reply with |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
🚧 @MonilBhavsar has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.4.17-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. I reviewed the changes in this PR against Expensify's help site content under
Since the help site documents product behavior (features, settings, workflows) rather than internal component architecture, no documentation updates are needed and no draft docs PR has been created. @TaduJR, if you believe any user-facing behavior changed here that I missed, let me know and I'll re-evaluate the help site impact. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.17-3 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.17-3 🚀
|
Explanation of Change
BaseSelectionList(flat) andBaseSelectionListWithSections(sectioned) carried near-identical, copy-pasted forks of the same focus/navigation logic, which had already started to drift. This PR consolidates that duplicated navigation/interaction logic into shared, single-source modules undersrc/components/SelectionList/:useSelectionListKeyboardFocus— the navigation core: wrapsuseArrowKeyFocusManagerand owns the scroll-suppression ref, the focus-restore-aware cursor setters (setFocusedIndexFromRowFocus/setFocusedIndexWithoutScrollOnChange),suppressNextFocusScroll, and the keyboard-navigation modality (thehasKeyBeenPressedflag + Tab listener).useSelectionListScroll— the bounds-checkedscrollToIndexand its debounced variant.useSelectionListShortcuts— theEnter/Ctrl+Entershortcuts anddisableEnterShortcutgating.useSelectionListTextInput— the search-input refs and key handlers.SelectionListEmptyState— the shared loading-skeleton / empty-state rendering.Both
BaseSelectionListandBaseSelectionListWithSectionsnow consume these instead of maintaining their own copies; the per-list differences (data source,isActiveexpression, section flattening) are passed in as parameters.Fixed Issues
$ #91033
PROPOSAL:
Tests
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
iOS: mWeb Safari
MacOS: Chrome / Safari