Skip to content
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

[$250] Android - LHN flickers when scrolled down and switching between Search and Inbox #53605

Open
2 of 8 tasks
lanitochka17 opened this issue Dec 4, 2024 · 63 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Dec 4, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.71-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch ND or hybrid app
  2. Scroll down LHN list
  3. Tap Search
  4. Tap Inbox

Expected Result:

LHN will not flicker when scrolled down and switching between Search and Inbox

Actual Result:

LHN flickers when scrolled down and switching between Search and Inbox

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6684654_1733343491984.Screen_Recording_20241205_041553_Expensify.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021867321297737486613
  • Upwork Job ID: 1867321297737486613
  • Last Price Increase: 2025-01-09
Issue OwnerCurrent Issue Owner: @hungvu193
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 4, 2024
Copy link

melvin-bot bot commented Dec 4, 2024

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@truph01
Copy link
Contributor

truph01 commented Dec 5, 2024

🚨 Edited by proposal-police: This proposal was edited at 2025-01-20 02:48:25 UTC.

🚨 Edited by proposal-police: This proposal was edited at 2024-12-27 11:25:11 UTC.

Edited by proposal-police: This proposal was edited at 2024-12-27 11:25:11 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

  • LHN flickers when scrolled down and switching between Search and Inbox

What is the root cause of that problem?

  • We have logic to scroll to the previous scroll position in:

flashListRef.current.scrollToOffset({offset});

  • With a large list, during rapid scrolling, FlashList may encounter situations where it needs to render multiple items simultaneously to keep up with the fast-changing offsets, which can result in temporary blank spaces being visible.

  • As a result, we can see the LHN flickers when going back from inbox.

What changes do you think we should make in order to solve the problem?

  • When on the sidebar screen, store the index of the first visible item as firstVisibleItemIndex. If the user navigates to another screen and then returns to the sidebar, display a skeleton loader if the item corresponding to firstVisibleItemIndex has not been rendered. Detailed steps are provided below:

1. Store the index of the currently first visible item

    // Saves the scroll offset of the first visible item for the given route.
    saveFirstVisibleIndex: (route: PlatformStackRouteProp<ParamListBase>, scrollOffset: number) => void;

    // Retrieves the previously saved scroll offset (first visible index) for the given route.
    getFirstVisibleIndex: (route: PlatformStackRouteProp<ParamListBase>) => number | undefined;

    const saveFirstVisibleIndex: ScrollOffsetContextValue['saveFirstVisibleIndex'] = useCallback((route, scrollOffset) => {
        firstVisibleIndexRef.current[getKey(route)] = scrollOffset;
    }, []);

    const getFirstVisibleIndex: ScrollOffsetContextValue['getFirstVisibleIndex'] = useCallback((route) => {
        if (!firstVisibleIndexRef.current) {
            return;
        }
        return firstVisibleIndexRef.current[getKey(route)];
    }, []);

via the contextValue.

    const handleViewableItemsChanged = ({viewableItems}) => {
        if (viewableItems.length > 0) {
            const currentIndex = viewableItems[0].index; // Get the first visible item
            saveFirstVisibleIndex(route, currentIndex); // Save the index
        }
    };
  • Update:

const {saveScrollOffset, getScrollOffset} = useContext(ScrollOffsetContext);

    const {saveScrollOffset, getScrollOffset, getFirstVisibleIndex, saveFirstVisibleIndex} = useContext(ScrollOffsetContext);
    const firstVisibleItem = useRef(getFirstVisibleIndex(route));

2. Display a skeleton loader if the first visible item has not been rendered yet

  • In here introduce new state:
const [initialScrollIndexVisible, setInitialScrollIndexVisible] = useState(false);
  • Update the condition to display skeleton:

{!!isLoading && optionListItems?.length === 0 && (

                {(!initialScrollIndexVisible || (!!isLoading && optionListItems?.length === 0)) && (

3. Remove the skeleton loader once the first visible item is rendered

  • Update:

                    onInitialScrollIndexVisible={() => {
                        setInitialScrollIndexVisible(true);
                    }}

Function onInitialScrollIndexVisible will be triggered when the first visible item is rendered.

                    onLayout={() => {
                        onLayoutItem();
                        if (Math.abs(index - (firstVisibleItem.current ?? 0)) < 15) {
                            onInitialScrollIndexVisible?.();
                        }
                    }}

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

  • We can utilize firstVisibleItem.current in the main solution and make the necessary updates:

                    initialScrollIndex={firstVisibleItem.current}
  • Remove the !initialScrollIndexVisible which is added in main solution from:
                {(!initialScrollIndexVisible || (!!isLoading && optionListItems?.length === 0)) && (

Result

Screen.Recording.2024-12-27.at.18.25.49.mov

@saifelance
Copy link

Proposal

Please re-state the problem that we are trying to solve in this issue.

The problem occurs when switching between the "Search" and "Inbox" tabs in the LHN (Left-Hand Navigation) while the list is scrolled down. The LHN flickers, creating a poor user experience.

LHN flickering occurs due to:

  1. Re-rendering: FlashList unmounts/remounts when switching tabs due to optionMode or data changes.
  2. Scroll Position Restoration: Inefficient handling of offsets causes layout jitter.
  3. Layout Recalculation: Style updates during tab switches alter visible areas.
  4. Key/Item Size Issues: Incorrect estimatedItemSize or key management disrupts rendering.
  5. Transition Timing: Asynchronous updates or animations destabilize layout.

What is the root cause of that problem?

  1. Preserve Scroll Position: Ensure efficient offset handling and avoid redundant renders.
  2. Minimize Re-renders: Use React.memo, useCallback, and optimize extraData.
  3. Optimize FlashList: Match estimatedItemSize with actual item heights; ensure consistent keys.
  4. Debounce Updates: Batch layout changes to prevent disruptions.
  5. Smooth Transitions: Add animations to reduce abrupt visual changes.
  6. Debug Shifts: Investigate unnecessary style toggles.

What changes do you think we should make in order to solve the problem?

Updates

  1. React.memo for Component Memoization
  2. useCallback and useMemo for Optimization
  3. Improved onScroll and onLayout
  4. Updated Scroll-to-Top Logic
  5. Updated Render Logic for Empty State
  6. General Cleanup

App/src/components/LHNOptionsList/LHNOptionsList.tsx

import React, {useRef, useCallback, useEffect, useMemo} from 'react';
import {StyleSheet, View} from 'react-native';
import {FlashList} from '@shopify/flash-list';
import {useRoute} from '@react-navigation/native';
import {useOnyx} from 'react-native-onyx';
import OptionRowLHNData from './OptionRowLHNData'; // Assuming this is your custom component
import BlockingView from './BlockingView';
import TextBlock from './TextBlock';
import Icon from './Icon';
import useLocalize from '../hooks/useLocalize';
import useTheme from '../hooks/useTheme';
import useThemeStyles from '../hooks/useThemeStyles';
import useResponsiveLayout from '../hooks/useResponsiveLayout';
import usePrevious from '../hooks/usePrevious';
import * as CONST from '../constants';
import * as OptionsListUtils from '../utils/OptionsListUtils';
import * as ReportUtils from '../utils/ReportUtils';
import * as ReportActionsUtils from '../utils/ReportActionsUtils';
import * as DraftCommentUtils from '../utils/DraftCommentUtils';
import * as SidebarUtils from '../utils/SidebarUtils';
import {ScrollOffsetContext} from '../contexts/ScrollOffsetContext';

function LHNOptionsList({
    style,
    contentContainerStyles,
    data,
    onSelectRow,
    optionMode,
    shouldDisableFocusOptions = false,
    onFirstItemRendered = () => {},
}) {
    const {saveScrollOffset, getScrollOffset} = React.useContext(ScrollOffsetContext);
    const flashListRef = useRef(null);
    const route = useRoute();

    const [reports] = useOnyx(CONST.ONYXKEYS.COLLECTION.REPORT);
    const [reportActions] = useOnyx(CONST.ONYXKEYS.COLLECTION.REPORT_ACTIONS);
    const [policy] = useOnyx(CONST.ONYXKEYS.COLLECTION.POLICY);
    const [personalDetails] = useOnyx(CONST.ONYXKEYS.PERSONAL_DETAILS_LIST);
    const [transactions] = useOnyx(CONST.ONYXKEYS.COLLECTION.TRANSACTION);
    const [draftComments] = useOnyx(CONST.ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT);
    const [transactionViolations] = useOnyx(CONST.ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);

    const theme = useTheme();
    const styles = useThemeStyles();
    const {translate, preferredLocale} = useLocalize();
    const {shouldUseNarrowLayout} = useResponsiveLayout();
    const shouldShowEmptyLHN = shouldUseNarrowLayout && data.length === 0;

    const onLayoutItem = useCallback(() => {
        onFirstItemRendered();
    }, [onFirstItemRendered]);

    const renderItem = useCallback(
        ({item: reportID}) => {
            const fullReport = reports?.[reportID];
            const policyData = policy?.[fullReport?.policyID];
            const lastMessageText = OptionsListUtils.getLastMessageTextForReport(fullReport, personalDetails, policyData);

            return (
                <OptionRowLHNData
                    reportID={reportID}
                    fullReport={fullReport}
                    policy={policyData}
                    personalDetails={personalDetails}
                    lastMessageText={lastMessageText}
                    onSelectRow={onSelectRow}
                    preferredLocale={preferredLocale}
                    hasDraftComment={DraftCommentUtils.isValidDraftComment(draftComments?.[reportID])}
                />
            );
        },
        [reports, policy, personalDetails, onSelectRow, preferredLocale, draftComments],
    );

    const extraData = useMemo(
        () => [reports, policy, personalDetails, data.length, draftComments, optionMode, preferredLocale],
        [reports, policy, personalDetails, data.length, draftComments, optionMode, preferredLocale],
    );

    const previousOptionMode = usePrevious(optionMode);

    useEffect(() => {
        if (previousOptionMode === optionMode || !flashListRef.current) {
            return;
        }
        flashListRef.current.scrollToOffset({offset: 0});
    }, [previousOptionMode, optionMode]);

    const onScroll = useCallback(
        (e) => {
            if (e.nativeEvent.layoutMeasurement.height > 0) {
                saveScrollOffset(route, e.nativeEvent.contentOffset.y);
            }
        },
        [route, saveScrollOffset],
    );

    const onLayout = useCallback(() => {
        const offset = getScrollOffset(route);
        if (offset && flashListRef.current) {
            requestAnimationFrame(() => {
                flashListRef.current.scrollToOffset({offset});
            });
        }
    }, [route, getScrollOffset]);

    return (
        <View style={[style ?? styles.flex1, shouldShowEmptyLHN ? styles.emptyLHNWrapper : undefined]}>
            {shouldShowEmptyLHN ? (
                <BlockingView
                    animation={CONST.LottieAnimations.Fireworks}
                    animationStyles={styles.emptyLHNAnimation}
                    title={translate('common.emptyLHN.title')}
                    shouldShowLink={false}
                />
            ) : (
                <FlashList
                    ref={flashListRef}
                    data={data}
                    renderItem={renderItem}
                    keyExtractor={(item) => item}
                    extraData={extraData}
                    onScroll={onScroll}
                    onLayout={onLayout}
                    estimatedItemSize={optionMode === CONST.OPTION_MODE.COMPACT ? CONST.variables.optionRowHeightCompact : CONST.variables.optionRowHeight}
                    showsVerticalScrollIndicator={false}
                    keyboardShouldPersistTaps="always"
                />
            )}
        </View>
    );
}

export default React.memo(LHNOptionsList);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  1. Verify smooth scroll restoration during tab switches.
  2. Test with long lists to ensure no flickers.
  3. Validate transitions in both empty and populated LHN states.
  4. Simulate rapid tab switching for stability.
  5. Test across devices and orientations.
  6. Check offset handling during mid-scroll tab switches.

What alternative solutions did you explore? (Optional)

  • Use a static LHN component to avoid unmounting.
  • Manage LHN state globally with Redux/Context for smoother transitions.
  • Leverage native optimizations for Android rendering.

@slafortune
Copy link
Contributor

slafortune commented Dec 5, 2024

I am not able to reproduce this - reached out to C+ here

Screen_Recording_20241205_145204_New.Expensify.mp4

@slafortune slafortune added the Needs Reproduction Reproducible steps needed label Dec 5, 2024
@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@melvin-bot melvin-bot bot added the Overdue label Dec 9, 2024
@truph01
Copy link
Contributor

truph01 commented Dec 9, 2024

@slafortune You can try the below steps to reproduce the bug:

  1. Open the ND app and log in using an account with more than 100 reports in the LHN.
  2. Scroll to the bottom of the LHN list.
  3. Tap on the Search option.
  4. Tap on Inbox.

Copy link

melvin-bot bot commented Dec 9, 2024

@slafortune Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Copy link

melvin-bot bot commented Dec 11, 2024

@slafortune Eep! 4 days overdue now. Issues have feelings too...

@slafortune
Copy link
Contributor

Alright - thank, I was able to recreate this now.

@melvin-bot melvin-bot bot removed the Overdue label Dec 12, 2024
@slafortune slafortune added External Added to denote the issue can be worked on by a contributor and removed Needs Reproduction Reproducible steps needed labels Dec 12, 2024
@melvin-bot melvin-bot bot changed the title Android - LHN flickers when scrolled down and switching between Search and Inbox [$250] Android - LHN flickers when scrolled down and switching between Search and Inbox Dec 12, 2024
Copy link

melvin-bot bot commented Dec 12, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021867321297737486613

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 12, 2024
Copy link

melvin-bot bot commented Dec 12, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @hungvu193 (External)

Copy link

melvin-bot bot commented Dec 16, 2024

@slafortune, @hungvu193 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Dec 16, 2024
@hungvu193
Copy link
Contributor

Not overdue

@melvin-bot melvin-bot bot removed the Overdue label Dec 16, 2024
@hungvu193
Copy link
Contributor

@truph01 Can you provide more details on your solution? Maybe a test branch so I can test it?

@truph01
Copy link
Contributor

truph01 commented Dec 17, 2024

@hungvu193 The test branch here

@rezkiy37
Copy link
Contributor

Hi, I am Michael (Mykhailo) from Callstack, an expert agency and I can work on this issue.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 15, 2025
@rezkiy37
Copy link
Contributor

Working on the issue.

@rezkiy37
Copy link
Contributor

I can confirm the bug is replicable.

@rezkiy37
Copy link
Contributor

My investigations:

  1. The bug happens only when the app scrolls LHN. If the LHN is on the top it works properly.
  2. The project uses scrollToOffset of FlashList. It supports the animated param. It is false currently.
    flashListRef.current.scrollToOffset({offset});
  3. When I set the param to true, it works a bit smoother and there are no flickers. However, there is a blank area while rows are not rendered.
Video

Test.mp4

  1. I am testing in dev mode so it is much slower than production for sure.
  2. Going to build an APK.

@rezkiy37
Copy link
Contributor

There is a useful prop of FlashList - initialScrollIndex. I am testing to implement it.

@truph01
Copy link
Contributor

truph01 commented Jan 17, 2025

There is a useful prop of FlashList - initialScrollIndex. I am testing to implement it.

@rezkiy37 I’ve already tried using this prop, but occasionally an empty screen is displayed, as noted here. I’m not sure how you’re using it, but if you’ve encountered the same bug, we’re on the same page.

@melvin-bot melvin-bot bot added the Overdue label Jan 17, 2025
@huult
Copy link
Contributor

huult commented Jan 18, 2025

#53605 (comment)

There is a useful prop of FlashList - initialScrollIndex. I am testing to implement it.

@rezkiy37 that is my suggestion

@hungvu193
Copy link
Contributor

Still in progress

@melvin-bot melvin-bot bot removed the Overdue label Jan 20, 2025
@truph01
Copy link
Contributor

truph01 commented Jan 20, 2025

Proposal updated

  • I just restored the solution that I mistakenly removed (added to the What alternative solutions did you explore? (Optional) section)

@rezkiy37
Copy link
Contributor

I take your comments into account, appreciate it 🙂

@muttmuure muttmuure moved this to MEDIUM in [#whatsnext] #quality Jan 20, 2025
@rezkiy37
Copy link
Contributor

I am preparing a proposal.

Copy link
Contributor

⚠️ @rezkiy37 Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

@rezkiy37
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

The problem occurs when opening the "Inbox" tab during switching between the tabs. The LHN flickers. It rerenders. Moreover, it does not always restore the previous scroll position.

What is the root cause of that problem?

FlashList rerenders during scrolling to the previous position or even stays at the top.

What changes do you think we should make in order to solve the problem?

I am proposing to add one more important property to FlashList - estimatedListSize. It allows FlashList to avoid auto-measuring by setting static values. They say:

Estimated visible height and width of the list. It is not the scroll content size. Defining this prop will enable the list to be rendered immediately. Without it, the list first needs to measure its size, leading to a small delay during the first render.

const {windowHeight, windowWidth} = useWindowDimensions();
const listHeight = windowHeight - variables.bottomTabHeight;

....

<FlashList
  ...
  estimatedListSize={{
    height: listHeight,
    width: windowWidth,
  }}
/>

I can easily obtain the window dimensions and the defined tab height. I understand that the header height is dynamic, but I don’t see any issue if the list height is slightly larger than the actual. Considering that getting the header height could be an overhead, I believe this is a reasonable compromise.

Additionally, it resolves an issue related to restoring the previous scroll position. It now functions flawlessly.

I've attached a couple of videos to demonstrate the difference ⬇️

Video before

Before.mp4

Video after

After.mp4

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  1. Verify that the previous scroll position has been restored properly during tab switches.
  2. Test long and short lists.

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

N/A

@hungvu193
Copy link
Contributor

Thanks for the update @rezkiy37, I'll take a look 👀

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 23, 2025
@rezkiy37
Copy link
Contributor

I've opened #55486 for review.

Copy link

melvin-bot bot commented Jan 28, 2025

Triggered auto assignment to @puneetlath, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@rezkiy37
Copy link
Contributor

#55486 (comment)

I am going to work on a follow-up to find a web solution.

@rezkiy37
Copy link
Contributor

rezkiy37 commented Feb 4, 2025

Working on the issue today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: MEDIUM
Development

No branches or pull requests

9 participants