Skip to content

Fix table search visibility when filtering scrolled lists - #99451

Open
KJ21-ENG wants to merge 11 commits into
Expensify:mainfrom
KJ21-ENG:KJ21-ENG/99373-table-search-visibility
Open

KJ21-ENG wants to merge 11 commits into
Expensify:mainfrom
KJ21-ENG:KJ21-ENG/99373-table-search-visibility

Conversation

@KJ21-ENG

@KJ21-ENG KJ21-ENG commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Focused query changes reset a scrolled table after its list ref is attached, keeping search visible through matching results, no results, and clearing.

On Android, scrolling-header searches also avoid the delayed focus scroll that opposed the first swipe and temporarily suppress native caret-rectangle scrolling that caused a clipped intermediate stop. Native child-focus navigation remains enabled. The FlashList patch hides the stale sticky copy as scrolling crosses its release boundary. Focus ownership is scoped to each table and released on blur, removal, or menu suppression. Regression coverage and patch documentation accompany these changes.

Fixed Issues

$ #99373
PROPOSAL: N/A (deploy-blocker follow-up to #93877; no proposal flow)

Tests

  1. Open Workspace settings > Categories in a workspace with enough loaded categories to scroll.
  2. Focus search and immediately swipe up as the keyboard opens. Verify the list follows the gesture without snapping back.
  3. Scroll until search is above the viewport, then type a query such as a that still returns several categories. Verify search returns fully into view in one movement, retains its value and focus, and shows one column header without a duplicate or blank header strip.
  4. Scroll down again and clear search. Verify the complete list returns with search visible and focused.
  5. Enter a query with no matches, then clear it. Verify the no-results state and list recovery preserve the input. Repeat with whitespace-only input and verify all categories remain available.
  6. On Android, use forward Tab and modifier-held Shift+Tab from a scrolled search through nearby controls and rows. Verify each focused control is revealed and search can regain focus.
  7. Open and dismiss a table menu, then refocus search. Verify there is no stale focus or selection state.
  8. Repeat search, clear, and selection checks on Workspace Tags and Workspace Members. Repeat the relevant search and scrolling steps on iOS and web, preserving each screen's existing selection behavior.
  • Verify that no errors appear in the JS console

Offline tests

Load the category list, disconnect the device's network, and repeat search, no-results, clear, and scroll checks against the loaded data. Search and focus should behave the same. This change does not alter API calls, stored data, or optimistic updates.

QA Steps

Same as tests.

  • Verify that no errors appear in the JS console

PR Author Checklist

Platform scope note: this regression is specific to native keyboard and FlashList behavior. Android and iOS native are covered by the recordings below. The mWeb and desktop platform items are marked complete as reviewed and not applicable to this native-only fix, not as claims of additional platform recordings.

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • Offline behavior reviewed. Dedicated offline testing is not applicable because the change only adjusts a local list offset and does not touch network, API, or Onyx behavior.
    • High Traffic account testing reviewed. It is not applicable because the fix does not depend on account volume, API behavior, or server data shape.
  • I included screenshots or videos for all applicable platforms. Android and iOS native recordings are included; mWeb and desktop are not applicable to this native-only regression.
  • I reviewed the behavior on all platforms relevant to this change:
    • Android: Native
    • Android: mWeb Chrome (not applicable to this native-only regression)
    • iOS: Native
    • iOS: mWeb Safari (not applicable to this native-only regression)
    • MacOS: Chrome / Safari (not applicable to this native-only regression)
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • No new code pattern is added; the change extends the existing table offset-reset pattern.
  • I followed the guidelines as stated in the Review Guidelines
  • I tested the shared table search behavior on the affected native surfaces and added focused shared-component regression coverage.
  • No CSS style is added:
    • No new style duplication is introduced.
    • No new style requires a StyleUtils helper.
  • No assets are added or modified:
    • Asset optimization is not applicable.
    • Asset loading validation is not applicable.
  • Message editing, sending, and markdown rendering are not modified by this PR.
  • The generic table search component is covered by focused tests for non-empty, empty, clear, whitespace, filtered-empty, value, identity, and focus behavior.
  • No related Storybook story is modified or affected.
  • No deeplink behavior is modified by this PR.
  • The fix changes scroll behavior without adding or changing visual styles or form alignment:
    • Input alignment is unchanged.
    • Design review is not applicable because no visual style or layout specification changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • The branch was synchronized with current main before the final native recordings and validation.

Screenshots/Videos

Android: Native
Record_2026-08-25-18-50-42_40d2c9e2d8fb5b5aba3904834590021e.compressed.mp4
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-08-25.at.7.25.38.PM.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
646286470-222d5177-65ac-474b-bfbb-ffe83e1dcaed.1.mov

@KJ21-ENG
KJ21-ENG marked this pull request as ready for review August 25, 2026 18:13
@KJ21-ENG
KJ21-ENG requested review from a team as code owners August 25, 2026 18:13
@melvin-bot
melvin-bot Bot requested review from JmillsExpensify and Krishna2323 and removed request for a team August 25, 2026 18:14
@melvin-bot

melvin-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

@Krishna2323 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]

@melvin-bot
melvin-bot Bot removed the request for review from a team August 25, 2026 18:14
@KJ21-ENG

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: dd4570b86d

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@JmillsExpensify JmillsExpensify left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Krishna2323 Krishna2323 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on web, and this doesn't fully fix it there. The scroll still jumps up and down sometimes while typing, the search input can still end up out of view, and clearing the field doesn't work correctly either.

Monosnap.screencast.2026-08-29.06-13-54.mp4

@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
...components/Table/TableFilterBar/TableSearchBar.tsx 100.00% <100.00%> (ø)
... and 1677 files with indirect coverage changes

@KJ21-ENG

KJ21-ENG commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@Krishna2323 Kind ping here, Thanks!

@Krishna2323

Copy link
Copy Markdown
Contributor

On my radar for today.

@Krishna2323

Copy link
Copy Markdown
Contributor

On my radar for today

@Krishna2323 Krishna2323 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KJ21-ENG This grew quite a bit from the original fix. Most of that is handling local input state vs. activeSearchString, which feels like a lot of complexity for a scroll bug.

Could we try a single effect instead?

useLayoutEffect(() => {
    if (!isTextInputFocused(inputRef)) {
        return;
    }
    listRef.current?.scrollToOffset({offset: 0, animated: false});
}, [activeSearchString, listRef]);

This covers typing, clearing, and imperative updates with one timing, which might also avoid the web jitter. The only gap is whitespace-only edits, but those don't refilter anyway.

The tests are good, but many are covering the extra batching/whitespace/imperative behavior introduced by this PR. Simplifying the fix should let us drop most of those.

@KJ21-ENG

KJ21-ENG commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

@Krishna2323 Thanks, this was a good call. I switched the fix to the single focused useLayoutEffect, removed the extra state/batching machinery, and kept the existing no-results safeguard intact.

The deep-scroll edit and clear flow now stays fully visible and focused on Android, and the remaining whitespace edge is clean on web. Attaching both latest recordings below.

Web:

pr99451-web-whitespace.mov

Android:

pr99451-android-deep-scroll.mp4

@Krishna2323 Krishna2323 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KJ21-ENG The issue is still reproducible on android:

android_hybrid.mp4

@KJ21-ENG

KJ21-ENG commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Strange, checking it 🏃🏻‍♂️

@KJ21-ENG

KJ21-ENG commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@Krishna2323 Fixed.

PR99451-Android-fixed-search.mp4

@KJ21-ENG
KJ21-ENG requested a review from Krishna2323 September 9, 2026 08:31
@Krishna2323

Copy link
Copy Markdown
Contributor

@KJ21-ENG, I can see a double-header flicker:

Monosnap.screencast.2026-09-10.02-31-08.mp4

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ This PR is possibly changing native code and/or updating libraries, it may cause problems with HybridApp. Please check if any patch updates are required in the HybridApp repo and run an AdHoc build to verify that HybridApp will not break. Ask Contributor Plus for help if you are not sure how to handle this. ⚠️

@KJ21-ENG

KJ21-ENG commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

@Krishna2323

  • Fixed double-header flicker, first-swipe snapback, and clipped search resets; preserved Tab/Shift+Tab.
  • Added regression coverage, separated focus actions, and cleaned up patch notes.

Latest after-fix video.

native-fix-on-edit-clear.mp4

@Krishna2323

Copy link
Copy Markdown
Contributor

@KJ21-ENG two patches here touch core RN and FlashList native code. Can you explain what specifically forced patching (vs. a JS-only fix), and have you filed/considered filing these upstream?

@KJ21-ENG

KJ21-ENG commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@Krishna2323 The patches address two separate behaviors that remained after the JS search-reset fix:

  1. React Native: Android can scroll to reveal the focused input’s caret before our JS reset returns the full search header to the top. In a controlled comparison with the other fixes unchanged, allowing native rectangle scrolling produced 600 → 87 → 0; suppressing it produced 600 → 0, removing the visibly clipped intermediate position.

    I tried the existing scrollsChildToFocus=false control, but it also disabled native focus scrolling and left the next focused control offscreen during Tab navigation. The patch separates rectangle requests from child-focus scrolling so we can suppress the former while preserving the latter. It defaults to the existing behavior and is disabled only while the corresponding Android Table’s scrolling-header search owns focus.

  2. FlashList: One clarification: this patch changes TypeScript/JavaScript using RN’s native-driven Animated value, rather than Java/Kotlin. During the reset, the normal header can reappear before JS updates FlashList’s sticky index, briefly showing both copies. The patch hides the stale overlay using FlashList’s existing scroll value and header geometry.

    An app-side workaround that hid the normal cell through the JS sticky callback removed duplication but introduced a blank header-sized strip. The patch avoided that artifact. An app-owned native-driven wrapper remains an untested alternative, so I cannot claim that patching FlashList is the only possible implementation.

Neither change has been filed upstream yet; both patch notes state that. I agree both warrant upstream discussion with minimal reproductions: the RN case as a more granular scrolling control, and the FlashList case as a sticky-overlay release timing issue.

Supporting evidence

1. Existing RN control: the rejected focus-scoped workaround broke Tab reveal

With the scrollsChildToFocus=false prototype, the viewport remained at offset 524.71 after Tab. A diagnostic scroll to zero revealed the eye control still holding focus. With native focus scrolling enabled, Tab automatically revealed that control at offset 74.12.

Rejected prototype: after Tab Diagnostic scroll to reveal focus Control: native focus scrolling enabled
Viewport remains on deep rows after Tab in the rejected prototype Manual scroll to zero reveals the eye control still holding focus Native focus scrolling automatically reveals the focused eye control

Rejected prototype sequence, 18 seconds. The diagnostic reveal above is a separate manual step, not automatic scrolling. The recording has no keypress overlay; the Tab result is corroborated by the recorded runtime snapshots and these focus/control screenshots.

rn-broad-flag-rejected-trial.mp4
2. FlashList: the tested JS Cell-opacity workaround replaced duplication with a blank strip
Original duplicate-header baseline at 00:06.200 Rejected JS Cell-opacity workaround at 00:18.000
Two Categories headers visible during the original reset transition Empty header-sized strip below the sticky Categories header in the rejected JS workaround

Original duplicate transition, 4 seconds:

flashlist-original-duplicate.mp4

Rejected JS workaround, 5 seconds. The blank-strip frame is at 00:02 in this excerpt:

flashlist-js-workaround-gap.mp4

This is the Table-owned normal-Cell opacity workaround. It is not footage of a hideRelatedCell failure; that alternative was rejected through source-based regression review. It also does not test the proposed app-owned native-driven wrapper.

3. Matched RN rectangle-scroll comparison with the other fixes held constant

Same rebuilt Android binary and JavaScript, with the sticky-header correction and delayed-focus-helper skip active in both conditions. Native child-focus scrolling remains enabled. Only the Table's declarative rectangle-suppression ownership is toggled.

Rectangle requests enabled: the input stops at the clipped intermediate position before the JS reset. The first edit shows that position around 00:02, followed by the full description/search position around 00:03.

rn-rectangle-enabled.mp4

Rectangle requests suppressed: the corresponding edit and clear return directly to the full search-header position.

rn-rectangle-suppressed.mp4

@KJ21-ENG

Copy link
Copy Markdown
Contributor Author

@Krishna2323 Kind ping here. Thanks!

@Krishna2323

Copy link
Copy Markdown
Contributor

Reviewing today...

@Krishna2323

Copy link
Copy Markdown
Contributor
  • I still see double headers:
Monosnap.screencast.2026-09-18.04-37-43.mp4

@JS00001

JS00001 commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

lmk when this is ready please

@KJ21-ENG

Copy link
Copy Markdown
Contributor Author

@Krishna2323 I replayed the sequence from your video on 5421ab1: scroll with the search focused, enter L, scroll and add k, clear both characters, repeat the scrolled K/clear flow twice, then enter A, scroll, and clear it. I couldn't reproduce the double-header flash in this recording.

This was on a physical realme RMX1971 running Android 11/API 30.

Could you confirm the commit you tested and whether both dependency patches were applied and the Android app rebuilt? That will help me match your setup and trace the remaining difference. Thanks!

pr-99451-android-reviewer-sequence.mp4

@Krishna2323

Copy link
Copy Markdown
Contributor

@JS00001, could you please run an ad hoc build for native?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants