Restore workspace list scroll position after navigation#86158
Restore workspace list scroll position after navigation#86158
Conversation
Integrate WorkspacesListPage with ScrollOffsetContext to save and restore scroll position when navigating to/from workspace or domain pages. Uses useLayoutEffect to restore position before first paint, and dynamically calculates initialNumToRender based on the saved offset and estimated row height instead of data.length to avoid the performance regression that caused the previous fix (PR #77313) to be reverted. Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Fixed the failing Prettier check: ran |
|
@MelvinBot Implement the refined proposal |
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.
|
Use onLayout to dynamically measure the actual workspace row height, with layout-aware fallback estimates for narrow (100px) and wide (88px) layouts. Compute initialNumToRender from the saved scroll offset and measured/estimated item height plus a viewport buffer, avoiding the performance regression from rendering all items. Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Implemented the refined proposal. Here's what changed: 1. Dynamic row height measurement (primary source): Added a 2. Layout-aware fallback estimates: Replaced the single
3. Smarter
For a user scrolled to 3000px in narrow layout: |
|
@MelvinBot For Layout-aware fallback estimates, let us use style constants(e.g. variables.avatarSizeNormal) to arrive at the estimate instead of hardcoding to a constant value (i.e. 88) |
Replace hardcoded height values with style constants from variables.ts (avatarSizeNormal, spacing2) and clearly-named local constants for the spacing values that cannot be imported at module level (p5 padding, mb3 margin). Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Updated the fallback height estimates to use style constants instead of hardcoded values: const WORKSPACE_ROW_PADDING = 20; // styles.p5 padding per side
const WORKSPACE_ROW_NARROW_MARGIN = 12; // styles.mb3 internal margin in narrow layout
const ESTIMATED_ITEM_HEIGHT_WIDE = variables.avatarSizeNormal + WORKSPACE_ROW_PADDING * 2 + variables.spacing2;
const ESTIMATED_ITEM_HEIGHT_NARROW = variables.avatarSizeNormal + WORKSPACE_ROW_PADDING * 2 + WORKSPACE_ROW_NARROW_MARGIN + variables.spacing2;
|
@MelvinBot Avoid hardcoding for the above as well |
…dding/margin Add spacing3 (12) and spacing5 (20) to style variables, following the existing spacing2 (8) pattern, and use them in the estimated workspace row height calculation to avoid hardcoded magic numbers. Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Added const ESTIMATED_ITEM_HEIGHT_WIDE = variables.avatarSizeNormal + variables.spacing5 * 2 + variables.spacing2;
const ESTIMATED_ITEM_HEIGHT_NARROW = variables.avatarSizeNormal + variables.spacing5 * 2 + variables.spacing3 + variables.spacing2;No more hardcoded values — everything derives from style variables now. |
@MelvinBot Instead of using generic
|
Replace variables.spacing2/spacing3/spacing5 with values extracted from the actual styles (styles.mb2, styles.p5, styles.mb3) so the height computation clearly reflects which styles it derives from. Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Updated — now extracting spacing values from const rowMarginBottom = (styles.mb2 as StyleNumericValue<'marginBottom'>)?.marginBottom ?? 0;
const rowPaddingVertical = (styles.p5 as StyleNumericValue<'padding'>)?.padding ?? 0;
const narrowInternalMargin = (styles.mb3 as StyleNumericValue<'marginBottom'>)?.marginBottom ?? 0;
const estimatedItemHeightWide = variables.avatarSizeNormal + rowPaddingVertical * 2 + rowMarginBottom;
const estimatedItemHeightNarrow = variables.avatarSizeNormal + rowPaddingVertical * 2 + narrowInternalMargin + rowMarginBottom;Each variable name now clearly represents which style it derives from ( |
rojiphil
left a comment
There was a problem hiding this comment.
@MelvinBot Please address these review comments.
| const fallbackHeight = shouldUseNarrowLayout ? estimatedItemHeightNarrow : estimatedItemHeightWide; | ||
| // eslint-disable-next-line react-hooks/refs -- Reading the measured height ref during render is intentional; the value is only an optimization hint for initialNumToRender and stale reads are acceptable. | ||
| const itemHeight = measuredItemHeight.current ?? fallbackHeight; | ||
| const viewportItems = Math.ceil(Dimensions.get('window').height / itemHeight); |
There was a problem hiding this comment.
Let's use useWindowDimensions hook here.
There was a problem hiding this comment.
@/tmp/reply-2978910740.md
| // available (from a previous render), otherwise falls back to a layout-aware estimate. | ||
| const savedScrollOffset = getScrollOffset(route) ?? 0; | ||
| const computedInitialNumToRender = (() => { | ||
| if (savedScrollOffset <= 0) { |
There was a problem hiding this comment.
Let's return undefined also when data.length is 0
There was a problem hiding this comment.
@/tmp/reply-2978912770.md
| if (savedScrollOffset <= 0) { | ||
| return undefined; | ||
| } | ||
| const fallbackHeight = shouldUseNarrowLayout ? estimatedItemHeightNarrow : estimatedItemHeightWide; |
There was a problem hiding this comment.
Since this is fallback height, we can rely on a single height. Let us use wide height and remove the usage of narrow height.
There was a problem hiding this comment.
@/tmp/reply-2978921217.md
src/styles/variables.ts
Outdated
| inputComponentSizeNormal: 40, | ||
| componentSizeLarge: 52, | ||
| spacing2: 8, | ||
| spacing3: 12, |
There was a problem hiding this comment.
Remove spacing3 and spacing5 as they are redundant now.
There was a problem hiding this comment.
@/tmp/reply-2978923760.md
| if (measuredItemHeight.current) { | ||
| return; | ||
| } | ||
| measuredItemHeight.current = e.nativeEvent.layout.height; |
There was a problem hiding this comment.
When the user comes back again to the workspace view with a preserved scrollOffset value, the measuredItemHeight would be empty. So, let's preserve the measured item height in ScrollOffsetContextProvider with support of additional method saveAverageItemLength
There was a problem hiding this comment.
@/tmp/reply-2978963254.md
| ); | ||
|
|
||
| const flatlistRef = useRef<FlatList | null>(null); | ||
| const measuredItemHeight = useRef<number | undefined>(undefined); |
There was a problem hiding this comment.
And, if available, let's retrieve the measured item height in ScrollOffsetContextProvider with support of additional method getAverageItemLength. Otherwise, let it be undefined
There was a problem hiding this comment.
@/tmp/reply-2978967274.md
- Use useWindowDimensions hook instead of Dimensions.get('window')
- Return undefined when data.length is 0 in initialNumToRender
- Simplify to single fallback height (wide layout estimate only)
- Remove spacing3 and spacing5 from variables.ts (no longer needed)
- Persist measured item height in ScrollOffsetContextProvider via
saveAverageItemLength/getAverageItemLength so it survives navigation
- Extract fallback height values from styles instead of hardcoding
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@MelvinBot In |
Delete averageItemLengthsRef.current entries in both the priority mode change effect and the cleanScrollOffsets helper, matching the existing cleanup of scrollOffsetsRef.current to prevent stale measured heights. Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Added
This prevents stale measured heights from persisting after their associated scroll offsets are cleaned up. |
| shouldShowErrorMessages={item.policyID !== policyIDToDelete} | ||
| shouldHideOnDelete={false} | ||
| onLayout={(e) => { | ||
| if (getAverageItemLength(route)) { |
There was a problem hiding this comment.
@MelvinBot Let us return early if e.nativeEvent.layout.height is 0 as it does not make sense to set the average length.
There was a problem hiding this comment.
Done — the onLayout callback now returns early when height <= 0, before updating either the local ref or the context.
Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
Added an early return when |
| shouldShowErrorMessages={item.policyID !== policyIDToDelete} | ||
| shouldHideOnDelete={false} | ||
| onLayout={(e) => { | ||
| if (e.nativeEvent.layout.height === 0 || getAverageItemLength(route)) { |
There was a problem hiding this comment.
@MelvinBot Two comments here:
- Let make the condition
e.nativeEvent.layout.height <= 0as that is safer - Also, we would run into problems if the measured height of item reduces as compared to the saved one. In this case, the reduced height would not get saved and this can cause scrolling issues. Let's save the measured height locally as a ref and also use it wherever needed. Also, we will allow updation only if local and saved one are not same.
There was a problem hiding this comment.
Both addressed:
- Changed guard to
height <= 0for safer handling - Added a local
measuredItemHeightref that always gets updated with the latest measured height fromonLayout, syncs toScrollOffsetContextonly when the value differs from the saved one, and is used as the primary source incomputedInitialNumToRender(falls back to context value, then to estimated height)
- Change onLayout condition to `height <= 0` for safer guard - Store measured height in a local ref and sync to ScrollOffsetContext only when values differ, ensuring height reductions are captured - Use local ref as primary source in initialNumToRender calculation Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
rojiphil
left a comment
There was a problem hiding this comment.
@MelvinBot Few more comments to address.
| } | ||
| measuredItemHeight.current = height; | ||
| if (height !== getAverageItemLength(route)) { | ||
| saveAverageItemLength(route, height); |
There was a problem hiding this comment.
Let us keep the measuredItemHeight and averageItemLength within ScrollOffsetContextProvider in sync by assigning height to measuredItemHeight.current here.
There was a problem hiding this comment.
Done — moved the ref assignment inside the conditional block so both the local ref and context value are updated together, keeping them in sync.
| ); | ||
|
|
||
| const flatlistRef = useRef<FlatList | null>(null); | ||
| const measuredItemHeight = useRef<number>(0); |
There was a problem hiding this comment.
Let us initialize measuredItemHeight with getAverageItemLength() value and keep them in sync.
There was a problem hiding this comment.
Done — initialized measuredItemHeight ref with getAverageItemLength(route) ?? 0 so it has the saved value immediately on mount.
Initialize measuredItemHeight ref with the saved getAverageItemLength
value so it's immediately available on component mount. Move the ref
assignment inside the sync conditional so both the local ref and context
value are always updated together.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
Awesome. Looks good now. 86158-web-chrome-002.mp4 |
|
@MelvinBot Please mark the PR as "Ready for review". |
|
Done — PR has been marked as Ready for review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc18547cd8
ℹ️ 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 viewportItems = Math.ceil(windowHeight / itemHeight); | ||
| return Math.min(Math.ceil(savedScrollOffset / itemHeight) + viewportItems, data.length); |
There was a problem hiding this comment.
Cap initial render count for deep saved offsets
The initialNumToRender calculation still grows with savedScrollOffset and reaches data.length for users returning from very deep positions (for example, the domains section on high-traffic accounts with thousands of workspaces). In that case this mounts the entire list again, which reintroduces the same JS-thread stall this change is trying to avoid. Because the formula is ceil(offset / itemHeight) + viewportItems and then clamped with Math.min(..., data.length), sufficiently large offsets deterministically force a full render.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agree. Let's fix this.
@MelvinBot Would using getItemLayout help here?
There was a problem hiding this comment.
Yes, getItemLayout would be the right solution here. Here's why:
The problem: The current initialNumToRender = ceil(savedOffset / itemHeight) + viewportItems grows linearly with scroll depth. For a user scrolled to the domains section at the bottom of a 5K-workspace list, this approaches data.length and reintroduces the same JS-thread stall.
How getItemLayout + initialScrollIndex solves it: Instead of rendering all items from index 0 to N, FlatList uses getItemLayout to compute the exact offset of any item without rendering it, then renders only a small window (~10 items) around the initialScrollIndex. Performance is O(1) regardless of scroll depth — rendering 10 items whether the user scrolled to position 100 or position 5000.
The tradeoff: getItemLayout assumes predictable item heights. This list has mixed types:
- Workspace rows: uniform height (the measured/estimated height we already track)
- Domains header: different height
- Domain rows: different height
- Empty state components: different height
However, workspace rows dominate the list (the vast majority of items for high-traffic accounts), and the non-uniform items are at boundaries. Small inaccuracies from height mismatches are acceptable for scroll restoration — the user just needs to land near where they were, not pixel-perfect.
Implementation sketch:
const savedScrollIndex = savedScrollOffset > 0
? Math.round(savedScrollOffset / itemHeight)
: undefined;
// On the FlatList:
getItemLayout={(_, index) => ({
length: itemHeight,
offset: itemHeight * index,
index,
})}
initialScrollIndex={savedScrollIndex}This eliminates initialNumToRender entirely for scroll restoration and caps rendering at a constant number of items regardless of scroll depth. We'd keep the onScroll save and ScrollOffsetContext integration as-is.
One caveat: ListHeaderComponent adds content above the list items that getItemLayout doesn't account for by default. We'd need to either include the header height in the offset calculation, or convert the header into the first item in the data array so getItemLayout covers it.
Want me to implement this approach?
Explanation of Change
When navigating away from the Workspaces list and returning (e.g., tapping a workspace, then swiping back), the scroll position resets to the top. A previous fix (PR #77313) addressed this by integrating with
ScrollOffsetContextbut was reverted because it setinitialNumToRender={data.length}, which rendered all items on mount and caused a multi-second JS thread block for accounts with 5K+ workspaces.This PR re-applies the scroll position restoration approach using
useLayoutEffect(for flicker-free restoration before paint) while fixing the performance issue. Instead of rendering all items,initialNumToRenderis now dynamically calculated based on the saved scroll offset divided by an estimated row height (~88px), plus a small viewport buffer. For a user scrolled 3000px deep, this renders ~44 items — far less than the 5,000+ that caused the regression.Changes:
WorkspacesListPage.tsx: AddedScrollOffsetContextintegration withonScrollto save position,useLayoutEffectto restore it, and a smartinitialNumToRendercalculationScrollOffsetContextProvider.tsx: IncludeSCREENS.WORKSPACES_LISTin the route filter so saved scroll offsets survive navigation state cleanupFixed Issues
$ #76271
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Web: Chrome
Tested via automated browser testing. Scroll position was set to 200px, navigated into a workspace, navigated back — scroll position was preserved at 200px (both via in-app back button and browser back navigation).
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari