Pin initially-selected items to the top of long BankAccount and Card filter lists - #96031
Conversation
…lector filters Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Both failing checks are unrelated to this PR — no code fix is needed.
This PR only touches
Everything under this PR's control is green — Evidence
|
|
Re-checked — the situation is unchanged, and there's still nothing in this PR to fix or push.
Everything under this PR's control ( |
…rd/BankAccount selectors Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Update — the checks re-ran and there's now a second failure, but all three failing checks are unrelated to this PR, which touches only 1. 2. 3. This PR's own code is green: Evidence
|
|
Re-checked — status is unchanged from my previous comment, and there is still nothing in this PR to fix or push. This PR touches only
I can't resolve these by pushing to this branch — they clear once (1) the ONYX key owner buckets |
|
Both failing checks are unrelated to this PR (which only touches
Recommended: once #96037 lands, merge Evidence
|
|
@MelvinBot add unit test. And pull main |
…orPinInitial Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
…ctors Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Done — added unit tests and merged Unit tests — added
Merged Checks run locally
The |
|
Update after the latest re-run: the two failing test jobs are both pre-existing failures on
Everything under this PR's control is green: Evidence
|
|
The two real failures (
Fix owner: #95099's author needs to bucket
Everything under this PR's control is green — Evidence
|
|
@MelvinBot Update PR title and Explanation of Change based on latest changes |
|
Updated the PR title and Explanation of Change to match the latest changes.
Everything else in the body (Tests, QA, checklist, screenshots) is unchanged; I also dropped a malformed leftover block at the very bottom (a stray duplicate checklist under a |
|
@MelvinBot update tests steps based on latest changes |
|
Both remaining test failures are unrelated to this PR and are pre-existing on
This PR only touches the two Search filter selectors and their new tests, and its own tests pass: Evidence
|
|
Updated the The key change: pinning only kicks in when the list has ≥
I left the |
|
@codex review |
|
@MelvinBot review |
|
All three failing checks are unrelated to this PR — there's nothing here for me to fix. The two real test failures are pre-existing on
This PR changes only Evidence
|
|
Reviewed the diff — the approach is sound and matches the already-shipped A couple of minor, non-blocking points: 1. 2. Tiny DRY nit (optional). In Behavior note (looks intentional, just confirming): deselecting an initially-selected row keeps it in the pinned top section, unchecked, until the filter is reopened — consistent with "don't make rows jump between sections." |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fc780acf3
ℹ️ 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".
| const unselectedIndividualCardsData = individualCardsSectionData.filter((item) => (!shouldPinInitialSelection || !wasInitiallySelected(item)) && searchFunction(item)); | ||
| const unselectedClosedCardsData = closedCardsSectionData.filter((item) => (!shouldPinInitialSelection || !wasInitiallySelected(item)) && searchFunction(item)); |
There was a problem hiding this comment.
Disable auto-scroll when new card selections stay in place
When selecting an unselected card, this new grouping now leaves the row in unselectedIndividualCardsData, but SelectionListWithSections still auto-scrolls multi-section multi-select lists to index 0 before calling onSelectRow (BaseSelectionListWithSections.tsx:132-135) unless shouldPreventAutoScrollOnSelect is set. In long card lists, choosing a card below the fold will therefore jump the viewport to the top even though the row no longer moves there, which preserves the original disruptive jump in a different form.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by setting shouldPreventAutoScrollOnSelect = true
| const unselectedOpenData = openItems.filter((item) => (!shouldPinInitialSelection || !wasInitiallySelected(item)) && searchFunction(item)); | ||
| const unselectedClosedData = closedItems.filter((item) => (!shouldPinInitialSelection || !wasInitiallySelected(item)) && searchFunction(item)); |
There was a problem hiding this comment.
Disable auto-scroll when new bank selections stay in place
When selecting an unselected bank account, this new grouping now leaves the row in unselectedOpenData/unselectedClosedData, but SelectionListWithSections still auto-scrolls multi-section multi-select lists to index 0 before calling onSelectRow (BaseSelectionListWithSections.tsx:132-135) unless shouldPreventAutoScrollOnSelect is set. In bank-account lists, choosing an account below the fold will therefore jump the viewport to the top even though the row no longer moves there, so the no-jump behavior is not actually achieved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by setting shouldPreventAutoScrollOnSelect = true
|
@MelvinBot pass |
|
Marked this PR as ready for review ✅ All completed checks are green — the earlier |
| // Only float the initial selection when the list is long enough to warrant it (>= STANDARD_LIST_ITEM_LIMIT), mirroring | ||
| // the shared moveInitialSelectionToTop gate; for short lists items stay in their natural order so nothing is pinned. | ||
| const initialSelectedValues = useInitialValue(() => value); | ||
| const wasInitiallySelected = (item: BankAccountFilterItem) => initialSelectedValues.includes(item.value); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
shouldPinInitialSelection computes the exact same expression (openItems.length + closedItems.length >= CONST.STANDARD_LIST_ITEM_LIMIT) that shouldShowSearchInput already computes a few lines above. This duplicates the same list-length gate in two named constants, so any future change to the threshold logic has to be made in both places.
Reuse the already-computed value instead of recalculating it:
const shouldShowSearchInput = openItems.length + closedItems.length >= CONST.STANDARD_LIST_ITEM_LIMIT;
// ...
const shouldPinInitialSelection = shouldShowSearchInput;or, if the two concepts should stay distinct, hoist the shared comparison into one named boolean (e.g. isLongList) that both derive from.
Reviewed at: 7e11554 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Done in 7924ff1 — shouldPinInitialSelection now just aliases the already-computed shouldShowSearchInput instead of recomputing openItems.length + closedItems.length >= CONST.STANDARD_LIST_ITEM_LIMIT, so the list-length gate lives in one place. Kept the distinct name since it documents the pinning intent at its use sites.
| // Only float the initial selection when the list is long enough to warrant it (>= STANDARD_LIST_ITEM_LIMIT), mirroring | ||
| // the shared moveInitialSelectionToTop gate; for short lists items stay in their natural order so nothing is pinned. | ||
| const initialSelectedValues = useInitialValue(() => value); | ||
| const wasInitiallySelected = (item: CardFilterItem) => !!item.keyForList && initialSelectedValues.includes(item.keyForList); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
shouldPinInitialSelection computes the exact same expression (individualCardsSectionData.length + closedCardsSectionData.length >= CONST.STANDARD_LIST_ITEM_LIMIT) that shouldShowSearchInput already computes on the line above. This duplicates the same list-length gate in two named constants, so the threshold logic must be kept in sync in two places.
Reuse the already-computed value instead of recalculating it:
const shouldShowSearchInput = individualCardsSectionData.length + closedCardsSectionData.length >= CONST.STANDARD_LIST_ITEM_LIMIT;
// ...
const shouldPinInitialSelection = shouldShowSearchInput;or, if the two concepts should stay distinct, hoist the shared comparison into one named boolean (e.g. isLongList) that both derive from.
Reviewed at: 7e11554 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Done in 7924ff1 — shouldPinInitialSelection now just aliases the already-computed shouldShowSearchInput instead of recomputing individualCardsSectionData.length + closedCardsSectionData.length >= CONST.STANDARD_LIST_ITEM_LIMIT, so the list-length gate lives in one place. Kept the distinct name since it documents the pinning intent at its use sites.
|
@MelvinBot address #96031 (comment) and #96031 (comment) |
…STENCY-3) Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@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] |
|
🚧 marcaaron has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
…fy#96031 Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.4.36-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site files under Why: This PR is a purely internal UI-behavior fix. It changes how the Bank account and Card filters (under Search filters) order their rows — snapshotting which items were selected when the filter first opened so pre-selected items stay pinned to the top of long lists (12+ items) and don't jump between sections when toggled. It adds no new user-facing feature, setting, or workflow, and doesn't rename or relabel anything. The help articles document what the Search filters do, not the row-ordering/pinning mechanics of individual filter lists, so there's nothing to update. Since no changes are required, no draft help site PR was created.
|
|
@marcaaron @parasharrajat Applause can't connect 12 bank accounts. Could this be verified internally? |
yes, just test Card list. The same logic applied to Bank account |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.4.38-0 🚀
|
|
🤖 I reviewed the changes in this PR against the help site articles under Why: This PR is a purely internal UX/ordering refinement. It snapshots the initially-selected items when the Bank account and Card filters first open so they stay floated to the top of long lists (≥ 12 items), and prevents toggled rows from jumping between sections. It introduces no new feature, tab, setting, button, or copy. The public help articles that touch Search filtering — Use Search Operators to Filter and Analyze and Search and Download Expenses — document filter syntax and workflows. Neither describes the internal ordering of items within a filter's selection list, so nothing there becomes inaccurate as a result of this change. No draft PR was created. @mkhutornyi, please confirm you agree no help site updates are needed here. If you believe a doc should call out this behavior, let me know and I'll draft the PR. |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.4.40-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site files under No help site changes are required. This PR is a purely internal UI-ordering refinement of the Bank account and Card filters on the Search (Spend) page. It snapshots which accounts/cards were selected when a filter first opened so those items stay pinned to the top of long lists (12+ items) instead of jumping between sections when toggled. There is no new feature, no new setting, and no change to any feature name, tab, button label, or documented workflow. The relevant help articles (e.g. No draft PR was created since there is nothing to update. @mkhutornyi, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.4.40-0 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
BankAccountSelector/CardSelectorfilters render their rows throughSelectionListWithSectionsand group selected rows into a pinned top section.Previously that section was keyed on the live
isSelectedflag, so toggling a row's selection while the filter was open made it immediately jump between the "selected" and "unselected" sections.This snapshots the values that were selected when the filter first opened (via
useInitialValue) and keys section membership on that snapshot instead of the live selection, while each row's checkbox still reflects the current selection.So the initially-selected accounts/cards stay floated at the top on first render, and selecting/deselecting a row no longer makes it jump between sections.
The pinning is gated on list length: pre-selected items are only floated to the top when the list has at least
CONST.STANDARD_LIST_ITEM_LIMIT(12) items.For shorter lists the items stay in their natural order and nothing is pinned. Applies this
< 12gate to these two section-grouping selectors.Fixed Issues
$ #61414
Tests
Long list (pinning applies — 12 or more items):
STANDARD_LIST_ITEM_LIMIT(12) that enables pinning.Short list (pinning does NOT apply — fewer than 12 items):
8. Repeat with an account that has fewer than 12 bank accounts / cards.
9. Select one or more items, switch to another filter, then return.
10. Verify the selected items are not pinned to the top — every item stays in its natural order and the selected ones are simply marked in place.
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)