Add pagination to NewChatPage#88452
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.
|
32a8f31 to
f020be6
Compare
| if (skipPagination) { | ||
| return {paginatedData: data, loadMore: () => {}, hasMore: false}; | ||
| } | ||
|
|
||
| if (pageSize < 1) { | ||
| return {paginatedData: [], loadMore: () => {}, hasMore: false}; | ||
| } |
There was a problem hiding this comment.
Could these early returns can go above the if (resetKey !== prevResetKey) condition? 🤔
There was a problem hiding this comment.
at first, after reading this question, I though it would be better to move it down. But actually it needs to stay in place.
Example:
usePaginatedData(data, pageSize, { resetKey: "false", skipPagination:false})
// some time later
usePaginatedData(data, pageSize, { resetKey: "true", skipPagination:true})
// some time later
usePaginatedData(data, pageSize, { resetKey: "false", skipPagination:false})
If we move the condition down, the change in resetKey would go unnoticed; the pagination would not reset to the first page.
| paginatedData: personalDetails, | ||
| loadMore: loadMorePersonalDetails, | ||
| hasMore: hasMorePersonalDetails, | ||
| } = usePaginatedData(allPersonalDetailOptions, PAGINATION_SIZE, {skipPagination: isSearching, resetKey: String(isSearching)}); |
There was a problem hiding this comment.
If isSearching return a boolean so doesn't String(isSearching) return "true" or "false"? I think it might be a mistake
There was a problem hiding this comment.
That's an area I was also unsure about. It looks a little bit off, and I'm open to suggestions how to do it better.
Returning to your question, I intentionally type casted boolean to get "true"/"false" as the resetKey.
What case am I trying to handle here?
- Initial load -> use this pagination
- User starts typing in search field -> skip pagination in this place
- User clears search query -> pagination is back, but page is reset
isSearching is a perfect flag for when we should reset pagination, but since key is typed as a string, I casted the boolean value here.
I tried to do something similar to React Component key, which is defined as:
interface Attributes {
key?: Key | null | undefined;
}
type Key =
| string
| number
| bigint
It does not accept a boolean value, and it makes sense, especially here with the name resetKey, which is a key that on change would cause a reset to internal state. If it also accepted a boolean value, it becomes a bit hard to understand the intent of the prop. What does resetKey: true mean? So I'd stick with a string type.
Ideally I'd change the key only once when we go back from isSearching: true -> false. And it needs to a string value for reasons explained above.
The simple String(isSearching) does it well, wouldn't cause unnecessary re-renders when the searchTerm is changing like a -> ab -> abc -> abcd.
The casting may look strange, but it does what it set out to do, causes only one additional re-render vs potentially many more by passing searchTerm
It could always be handled more explicitly (we can write out explicit logic to track changes between renders to isSearching and change the key only on the flip from true to false), but imo String(isSearching) is enough.
There was a problem hiding this comment.
@staszekscp I agree with your point that it might be confusing, so I used a more explicite string key here, so that next devs would not have to wonder if this was done intentionally.
Now is smth like: isSearching ? 'OneKey' : 'AnotherKey'
| import isLoadingOnyxValue from '@src/types/utils/isLoadingOnyxValue'; | ||
| import type SelectedOption from './types'; | ||
|
|
||
| function useGroupDraftRestore( |
There was a problem hiding this comment.
I wonder if this hook should be renamed of have at least some comment to explain what it does, because it doesn't seem intuitive to me 😄
There was a problem hiding this comment.
Yeah, totally makes sense! This was also flagged by a reviewer on a previous PR that the current one is based from.
Now that I rebased to only include the relevant commits to this PR, js doc is in place.
| const shouldRestoreSelectedOptionsRef = useRef(true); | ||
| const isScreenInBackgroundRef = useRef(false); |
There was a problem hiding this comment.
Why are these values treated as refs?
There was a problem hiding this comment.
To avoid causing re-renders.
They control the returned value from the selector. And the selector itself causes re-renders. But we normally don't need those values (only in the few exceptions). Normally, selector's supposed to return undefined (nothing) and not react to changes in the draft from Onyx.
| if (!isSubscriptionActive) { | ||
| return undefined; | ||
| } | ||
| return draft?.participants; |
There was a problem hiding this comment.
Wouldn't eg. empty array be more natural here so we can avoid nullish coalescing in its usage?
There was a problem hiding this comment.
| isScreenInBackgroundRef.current = false; | ||
|
|
||
| return () => { | ||
| isScreenInBackgroundRef.current = true; |
There was a problem hiding this comment.
Isn't there any utility function from Navigation to check if the screen is backgrounded? 🤔
There was a problem hiding this comment.
Great catch!
The useIsFocused, but it causes re-renders. I need a ref here, but even then, there's a hook useIsFocusedRef, which I myself refactored out from some component some time ago because I needed it.
Thanks, I'll be able to shorten boilerplate code here.
68fe534 to
9e86bb0
Compare
c15ac76 to
d1abdd0
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1abdd0b1f
ℹ️ 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".
|
|
||
| if (resetKey !== prevResetKey) { | ||
| setPrevResetKey(resetKey); | ||
| setCurrentPage(1); |
There was a problem hiding this comment.
Reset pagination state before slicing data
When resetKey changes after the user has already loaded multiple pages, this hook queues setCurrentPage(1) but still computes limit from the previous currentPage in the same render. That means one render returns too many rows for the new context (for example, after changing the search term), which can briefly show stale pagination and trigger extra onEndReached work before the next render corrects it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
When you call the set function during render, React will re-render that component immediately after your component exits with a return statement, and before rendering the children.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ 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". |
bebd537 to
c00fd1e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
App/src/pages/NewChatPage/index.tsx
Lines 109 to 111 in c00fd1e
Paginating allPersonalDetailOptions before calling getValidOptions means we sort and filter only an arbitrary raw slice (Object.values order) instead of the full contact set, so the first page can exclude contacts that should be at the top (e.g., alphabetically) and then reorder/jump when more pages are loaded. getValidOptions/filterAndOrderOptions are where personal details are actually ordered, so slicing needs to happen after those steps to preserve stable, correct list ordering.
ℹ️ 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".
c00fd1e to
0a2cfc1
Compare
|
@codex review |
|
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". |
sort alphabetically before paginating add tests
0a2cfc1 to
6d23e93
Compare
|
@codex review |
|
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". |
|
@NikkiWines |
|
Yeah, i think there's some potential for conflicts if we reimplement the changes in the linked PR - just want to make sure nothing overlaps before we move forward with this one 🙇 |
|
Confirmed we're keeping this on hold for now |
|
Still on hold pending this |
|
helloooo 👋 sorry for the delay, but it looks like we can now move forward with this PR 🙇 |
|
@staszekscp, @NikkiWines, @dukenv0307 PR is ready. Conflicts've been resolved. |
|
Looks good to me Screen.Recording.2026-06-02.at.10.13.19.mov |
staszekscp
left a comment
There was a problem hiding this comment.
Looking good! Good job!
|
|
||
| const handleEndReached = () => { | ||
| if (!hasMore || !areOptionsInitialized || !isScreenFocusedRef.current) { | ||
| if ((!hasMoreFilteredPersonalDetails && !hasMorePersonalDetails && !hasMore) || !areOptionsInitialized || !isScreenFocusedRef.current) { |
There was a problem hiding this comment.
NAB: I had to think for a moment to understand this condition 😄 Maaaaybe this could become a separate hasMoreData variable instead of (!hasMoreFilteredPersonalDetails && !hasMorePersonalDetails && !hasMore)
There was a problem hiding this comment.
@staszekscp yep, it took me a moment to remember what this condition checked too 😅. Which is a good indicator that changing it would help.
NikkiWines
left a comment
There was a problem hiding this comment.
looks good, couple very small things
|
@sharabai friendly bump here 🙇 |
|
Hey @NikkiWines! On behalf of @sharabai I wanted to let you know that Sergei is on sick leave and will handle the review after he's back in shape! Also, sorry for not responding since Wednesday, but Thursday and Friday there was a bank holiday in Poland and we all were OOO 😄 |
|
No worries, thanks for the update @staszekscp 🙇 Hope you get well soon @sharabai, and I hope you all had a good holiday weekend |
|
@NikkiWines thanks for the suggestions! Looks cleaner with them. |
…reparation/pagination
|
🚧 @NikkiWines 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/NikkiWines in version: 9.4.5-0 🚀
Bundle Size Analysis (Sentry): |
Help site review — no docs changes requiredI reviewed the changes in this PR against the help articles under Why: This PR is an internal performance/architecture refactor of how
Files reviewedAll 12 changed files are TypeScript source/tests ( If you believe a customer-facing behavior did change in a way that should be documented, let me know what the user would observe and I'll draft the docs update. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.5-6 🚀
|



Explanation of Change
Note
#87881 must be merged first
Add pagination to
NewChatPage(usePaginatedData)Problem: Pagination used to live inside
SelectionListWithSections. The screen prepared all personal details up front and the list just revealed more already-computed rows on scroll — purely visual, no real work saved. The screen had no control or visibility over it; you'd normally expect pagination at the data-preparation level, and the data flow inNewChatPageis complicated enough that it was easy to miss that pagination wasn't happening there at all. The new list doesn't carry this forward, so New Chat lost even the visual pagination while still processing the full list.Approach: Move pagination up to the screen (
NewChatPage) using a small, generic hook (usePaginatedData), applied at two points:Important
the two usages serve different purposes and are mutually exclusive:
Why: Pagination at the list level hid both work and intent. Owning it at the screen with a dedicated hook makes the data-prep flow traceable and controllable — on initial load (and whenever the user is not searching) "seeing pagination" now actually matches "pagination is happening."
Note
A purely visual pagination is still kept while the user is searching — search itself runs against the full dataset, but filtered rows are still fed into the list in chunks so the list doesn't mount everything at once.
Sort and dedupe before slicing (alphabetical pagination)
Problem: With pagination at the screen, page 1 was the first
PAGINATION_SIZEofObject.values(personalDetails)—accountIDorder, not alphabetical.filterAndOrderOptionsthen alphabetized only that arbitrary slice, soloadMorecould reveal a name that sorted earlier than already-visible rows. Imported phone contacts had the same issue; an Onyx-PD/contact login overlap also wasted page slots before the late dedupe inOptionsListUtilsstripped one.Approach: Dedupe + alphabetically sort the union of Onyx personal details and imported phone contacts upstream of pagination, in a small pure helper (
mergeAndSortPersonalDetailsWithContacts). Dedupe key:addSMSDomainIfPhoneNumber(login).toLowerCase(). Onyx PDs are spread first, so on collision the real-accountIDcopy wins over the optimistic contact copy. The latepersonalDetails.concat(contacts)is removed (contacts are already merged upstream). While here,orderPersonalDetailsOptionsis generic-ised and its sort key aligned withpersonalDetailsComparator, so the upstream sort agrees with the heap re-sort insidegetValidOptions.Why: A paginated slice should match the user's mental model of "first page." Sorting + deduping upstream makes page 1 the alphabetical prefix and
loadMorestrictly append-only under a stable data snapshot.Performance gains on opening New Chat
With this change, opening
NewChatPageon accounts with a large contact list is noticeably faster — +75.88 ms(+22.2%) without the pagination.
Measured by forcing
skipPagination: trueon the firstusePaginatedDatausage (the one before data preparation) to simulate the previous behavior, and comparing against the current code with pagination enabled.Fixed Issues
$ #86089
PROPOSAL:
Tests
Load-flow pagination (initial open)
Visual pagination while searching
Sort + dedupe correctness
accountIDhas an early alphabetical name (e.g. "Aaron")Offline tests
QA Steps
Same as tests
Offline 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
iOS: mWeb Safari
MacOS: Chrome / Safari
pagination.mp4