From 2cf13840b0b2ea5f79568740e6a2ce190976a7a2 Mon Sep 17 00:00:00 2001 From: Chris Bongers Date: Wed, 16 Sep 2026 16:01:36 +0200 Subject: [PATCH 1/4] fix(feed): preserve scroll when returning from posts Wait for feed height changes instead of abandoning scroll restoration after two seconds. Preserve each modal history entry's originating feed position so reopening and closing posts through browser history does not reset it to zero. Cover delayed rendering, user cancellation, viewport changes, history traversal, remounts, and cancelled navigation. --- .../src/hooks/usePostModalNavigation.spec.ts | 176 ++++++++++++++++++ .../src/hooks/usePostModalNavigation.ts | 73 ++++++-- .../src/hooks/useScrollRestoration.spec.ts | 56 +++++- .../shared/src/hooks/useScrollRestoration.ts | 51 ++--- 4 files changed, 307 insertions(+), 49 deletions(-) create mode 100644 packages/shared/src/hooks/usePostModalNavigation.spec.ts diff --git a/packages/shared/src/hooks/usePostModalNavigation.spec.ts b/packages/shared/src/hooks/usePostModalNavigation.spec.ts new file mode 100644 index 00000000000..5f74de5a03a --- /dev/null +++ b/packages/shared/src/hooks/usePostModalNavigation.spec.ts @@ -0,0 +1,176 @@ +import { act, renderHook } from '@testing-library/react'; +import type { ParsedUrlQuery } from 'querystring'; +import post from '../../__tests__/fixture/post'; +import { FeedItemType } from '../components/cards/common/common'; +import type { UseRouterMemory } from './useRouterMemory'; +import { usePostModalNavigation } from './usePostModalNavigation'; + +const mockRouter = { + asPath: '/', + pathname: '/', + query: {} as ParsedUrlQuery, + push: jest.fn< + ReturnType, + Parameters + >(), +}; + +jest.mock('next/router', () => ({ useRouter: () => mockRouter })); +jest.mock('../contexts/LogContext', () => ({ + useLogContext: () => ({ logEvent: jest.fn() }), +})); +jest.mock('../lib/feed', () => ({ postLogEvent: jest.fn() })); +jest.mock('./useFeed', () => ({ isBoostedPostAd: () => false })); +jest.mock('./useKeyboardNavigation', () => ({ + useKeyboardNavigation: jest.fn(), +})); + +let historyKey = 0; + +const setScrollY = (value: number): void => { + Object.defineProperty(window, 'scrollY', { configurable: true, value }); +}; + +const getHistoryEntry = () => ({ + key: window.history.state.key as string, + asPath: mockRouter.asPath, + pathname: mockRouter.pathname, + query: mockRouter.query, +}); + +const restoreHistoryEntry = (entry: ReturnType) => { + window.history.replaceState({ key: entry.key }, '', entry.asPath); + mockRouter.asPath = entry.asPath; + mockRouter.pathname = entry.pathname; + mockRouter.query = entry.query; +}; + +const renderNavigation = () => + renderHook(() => + usePostModalNavigation({ + items: [post, { ...post, id: 'second-post' }].map((item, index) => ({ + type: FeedItemType.Post, + post: item, + page: 0, + index, + dataUpdatedAt: 0, + })), + fetchPage: jest.fn(), + updatePost: jest.fn(), + canFetchMore: false, + feedName: 'main', + }), + ); + +beforeEach(() => { + historyKey += 1; + restoreHistoryEntry({ + key: `${historyKey}`, + asPath: '/', + pathname: '/', + query: {}, + }); + mockRouter.push.mockReset(); + mockRouter.push.mockImplementation(async (url, as) => { + const parsed = new URL(url, window.location.origin); + historyKey += 1; + restoreHistoryEntry({ + key: `${historyKey}`, + asPath: as ?? url, + pathname: parsed.pathname, + query: Object.fromEntries(parsed.searchParams.entries()), + }); + return true; + }); + setScrollY(0); + Object.defineProperty(window, 'scrollTo', { + configurable: true, + value: jest.fn((_x: number, y: number) => setScrollY(y)), + }); +}); + +it('restores the feed when browser Back reopens a closed post', async () => { + const { result, rerender } = renderNavigation(); + setScrollY(5000); + await act(async () => result.current.onOpenModal(0)); + rerender(); + const postEntry = getHistoryEntry(); + setScrollY(0); + + await act(async () => result.current.onCloseModal()); + rerender(); + expect(window.scrollY).toBe(5000); + + restoreHistoryEntry(postEntry); + rerender(); + expect(result.current.selectedPost?.id).toBe(post.id); + setScrollY(0); + await act(async () => result.current.onCloseModal()); + + expect(window.scrollY).toBe(5000); +}); + +it('keeps separate positions for earlier modal history entries after a remount', async () => { + const view = renderNavigation(); + setScrollY(5000); + await act(async () => view.result.current.onOpenModal(0)); + view.rerender(); + const firstEntry = getHistoryEntry(); + await act(async () => view.result.current.onCloseModal()); + view.rerender(); + + setScrollY(9000); + await act(async () => view.result.current.onOpenModal(0)); + view.rerender(); + const secondEntry = getHistoryEntry(); + view.unmount(); + + restoreHistoryEntry(firstEntry); + setScrollY(0); + const utils = renderNavigation(); + await act(async () => utils.result.current.onCloseModal()); + expect(window.scrollY).toBe(5000); + + restoreHistoryEntry(secondEntry); + setScrollY(0); + utils.rerender(); + await act(async () => utils.result.current.onCloseModal()); + expect(window.scrollY).toBe(9000); +}); + +it('carries the original feed position through next-post navigation after a remount', async () => { + const view = renderNavigation(); + setScrollY(5000); + await act(async () => view.result.current.onOpenModal(0)); + view.unmount(); + setScrollY(0); + + const utils = renderNavigation(); + await act(async () => utils.result.current.onNext()); + utils.rerender(); + expect(utils.result.current.selectedPost?.id).toBe('second-post'); + await act(async () => utils.result.current.onCloseModal()); + + expect(window.scrollY).toBe(5000); +}); + +it('does not reset the scroll when a modal has no saved position', async () => { + mockRouter.query = { pmid: post.id, pmcid: 'main', pmp: '/', pmap: '/' }; + const { result } = renderNavigation(); + setScrollY(1200); + await act(async () => result.current.onCloseModal()); + + expect(window.scrollTo).not.toHaveBeenCalled(); +}); + +it('does not restore the feed when closing navigation is cancelled', async () => { + const { result, rerender } = renderNavigation(); + setScrollY(5000); + await act(async () => result.current.onOpenModal(0)); + rerender(); + setScrollY(0); + mockRouter.push.mockResolvedValueOnce(false); + await act(async () => result.current.onCloseModal()); + + expect(window.scrollTo).not.toHaveBeenCalled(); +}); diff --git a/packages/shared/src/hooks/usePostModalNavigation.ts b/packages/shared/src/hooks/usePostModalNavigation.ts index 3c96bfee58d..e9b4419eabb 100644 --- a/packages/shared/src/hooks/usePostModalNavigation.ts +++ b/packages/shared/src/hooks/usePostModalNavigation.ts @@ -8,7 +8,7 @@ import type { FeedItem, PostItem, UpdateFeedPost } from './useFeed'; import { isBoostedPostAd } from './useFeed'; import { Origin, LogEvent } from '../lib/log'; import { webappUrl } from '../lib/constants'; -import { getPathnameWithQuery, objectToQueryParams } from '../lib'; +import { getPathnameWithQuery, objectToQueryParams } from '../lib/links'; import { useKeyboardNavigation } from './useKeyboardNavigation'; import { isExtension } from '../lib/func'; import type { UseRouterMemory as UsePostModalRouter } from './useRouterMemory'; @@ -29,7 +29,7 @@ interface UsePostModalNavigation { onCloseModal: (fromPopState?: boolean) => void; isFetchingNextPage?: boolean; selectedPost: Post | null; - selectedPostIndex: number; + selectedPostIndex: number | undefined; selectedPostIsAd: boolean; } @@ -46,6 +46,10 @@ const useRouter: () => UsePostModalRouter = isExtension ? useRouterMemory : useRouterNext; +const feedScrollPositions = new Map(); +const getHistoryKey = (): string | undefined => + isExtension ? undefined : window.history.state?.key; + export const usePostModalNavigation = ({ items, fetchPage, @@ -63,7 +67,7 @@ export const usePostModalNavigation = ({ const pmid = router.query?.pmid as string; const { logEvent } = useLogContext(); const [isFetchingNextPage, setIsFetchingNextPage] = useState(false); - const scrollPositionOnFeed = useRef(0); + const scrollPositionOnFeed = useRef(); // if multiple feeds/hooks are rendered prevent effects from running while other modal is open const isNavigationActive = feedName === activeFeedName; @@ -86,7 +90,9 @@ export const usePostModalNavigation = ({ return item.post.slug === pmid || item.post.id === pmid; } if (isBoostedPostAd(item)) { - return item.ad.data.post.slug === pmid || item.ad.data.post.id === pmid; + return ( + item.ad.data.post?.slug === pmid || item.ad.data.post?.id === pmid + ); } return false; @@ -101,7 +107,7 @@ export const usePostModalNavigation = ({ const getPostItem = useCallback( (index: number) => { - if (index === null || !items[index]) { + if (!items[index]) { return null; } @@ -125,8 +131,8 @@ export const usePostModalNavigation = ({ ); const getPost = useCallback( - (index: number) => { - if (index === null || !items[index]) { + (index: number | undefined): Post | null => { + if (index === undefined || !items[index]) { return null; } @@ -135,7 +141,7 @@ export const usePostModalNavigation = ({ return item.post; } if (isBoostedPostAd(item)) { - return item.ad.data.post; + return item.ad.data.post ?? null; } return null; @@ -149,6 +155,11 @@ export const usePostModalNavigation = ({ if (post) { const postId = post.slug || post.id; + const historyKey = getHistoryKey(); + const feedScrollPosition = + (pmid && historyKey + ? feedScrollPositions.get(historyKey) + : undefined) ?? scrollPositionOnFeed.current; const newPathname = getPathnameWithQuery( basePathname, @@ -164,14 +175,24 @@ export const usePostModalNavigation = ({ // shallow keeps the feed route: the masked `/posts/:id` URL matches the // markdown middleware matcher, and a non-shallow push lets the server // resolve it into a real post-page navigation instead of the modal - await router.push(newPathname, `${webappUrl}posts/${postId}`, { - scroll: false, - shallow: true, - }); + const navigated = await router.push( + newPathname, + `${webappUrl}posts/${postId}`, + { + scroll: false, + shallow: true, + }, + ); + const postHistoryKey = getHistoryKey(); + if (navigated && postHistoryKey && feedScrollPosition !== undefined) { + feedScrollPositions.set(postHistoryKey, feedScrollPosition); + } } if (post?.type === PostType.Share) { const item = getPostItem(index); - updatePost(item.page, item.index, { ...post, read: true }); + if (item) { + updatePost(item.page, item.index, { ...post, read: true }); + } } }, [ @@ -182,6 +203,7 @@ export const usePostModalNavigation = ({ router, updatePost, feedName, + pmid, ], ); @@ -228,7 +250,9 @@ export const usePostModalNavigation = ({ return item.post.slug === pmid || item.post.id === pmid; } if (isBoostedPostAd(item)) { - return item.ad.data.post.slug === pmid || item.ad.data.post.id === pmid; + return ( + item.ad.data.post?.slug === pmid || item.ad.data.post?.id === pmid + ); } return false; @@ -239,17 +263,22 @@ export const usePostModalNavigation = ({ } }, [openedPostIndex, pmid, items, onChangeSelected, isNavigationActive]); - const selectedPostIsAd = isBoostedPostAd(items[openedPostIndex]); + const selectedPostIsAd = + openedPostIndex !== undefined && isBoostedPostAd(items[openedPostIndex]); const result = { postPosition: getPostPosition(), isFetchingNextPage: false, selectedPostIsAd, onCloseModal: async () => { + const historyKey = getHistoryKey(); + const feedScrollPosition = + (historyKey ? feedScrollPositions.get(historyKey) : undefined) ?? + scrollPositionOnFeed.current; // Extract query params from baseAsPath to preserve original params like 'id' const baseUrl = new URL(baseAsPath, window.location.origin); const searchParams = new URLSearchParams(baseUrl.search); - await router.push( + const navigated = await router.push( getPathnameWithQuery(basePathname, searchParams), baseAsPath, { @@ -257,12 +286,15 @@ export const usePostModalNavigation = ({ }, ); - window.scrollTo(0, scrollPositionOnFeed.current); - - scrollPositionOnFeed.current = 0; + if (navigated && feedScrollPosition !== undefined) { + window.scrollTo(0, feedScrollPosition); + } }, onOpenModal, onPrevious: () => { + if (openedPostIndex === undefined) { + return; + } let index = openedPostIndex - 1; // look for the first post before the current one while (index > 0 && !isPostItem(items[index])) { @@ -284,6 +316,9 @@ export const usePostModalNavigation = ({ onChangeSelected(index); }, onNext: async () => { + if (openedPostIndex === undefined) { + return; + } let index = openedPostIndex + 1; // eslint-disable-next-line no-empty for (; index < items.length && !isPostItem(items[index]); index += 1) {} diff --git a/packages/shared/src/hooks/useScrollRestoration.spec.ts b/packages/shared/src/hooks/useScrollRestoration.spec.ts index 90c80983081..2d22703ffa9 100644 --- a/packages/shared/src/hooks/useScrollRestoration.spec.ts +++ b/packages/shared/src/hooks/useScrollRestoration.spec.ts @@ -14,6 +14,7 @@ const FEED_HEIGHT = 20000; let scrollTo: jest.Mock; let pageHeight: number; +let notifyResize: (() => void) | undefined; // The hook keys positions by history entry, so a fresh key per test keeps its // module-level map from leaking between them. let historyKey: string; @@ -21,6 +22,7 @@ let historyKeyCount = 0; const setPageHeight = (height: number) => { pageHeight = height; + notifyResize?.(); }; const setScrollY = (position: number) => { @@ -45,6 +47,18 @@ const renderScrollRestoration = () => renderHook(() => useScrollRestoration()); beforeEach(() => { jest.useFakeTimers(); + notifyResize = undefined; + jest.mocked(ResizeObserver).mockImplementation((callback) => { + const observer = { + observe: jest.fn(), + unobserve: jest.fn(), + disconnect: jest.fn(() => { + notifyResize = undefined; + }), + }; + notifyResize = () => callback([], observer); + return observer; + }); historyKeyCount += 1; historyKey = `feed-entry-${historyKeyCount}`; @@ -93,10 +107,8 @@ describe('useScrollRestoration', () => { renderScrollRestoration(); - // The previous 1s budget expired here and dropped the user at the bottom of - // the partially rendered feed. act(() => { - jest.advanceTimersByTime(1500); + jest.advanceTimersByTime(2500); }); expect(scrollTo).not.toHaveBeenCalled(); @@ -106,7 +118,7 @@ describe('useScrollRestoration', () => { expect(scrollTo).toHaveBeenCalledWith(0, SAVED_POSITION); }); - it('leaves the user at the top when the page never grows tall enough', () => { + it('waits without polling when the page is too short', () => { saveFeedPosition(); renderScrollRestoration(); @@ -117,6 +129,11 @@ describe('useScrollRestoration', () => { expect(scrollTo).not.toHaveBeenCalled(); expect(window.scrollY).toBe(0); + expect(jest.getTimerCount()).toBe(0); + + setPageHeight(FEED_HEIGHT); + advanceFrames(); + expect(scrollTo).toHaveBeenCalledWith(0, SAVED_POSITION); }); it('keeps the saved position when the router resets the scroll to the top', () => { @@ -163,7 +180,7 @@ describe('useScrollRestoration', () => { it('records the position again once the user takes over', () => { saveFeedPosition(); - renderScrollRestoration(); + const { unmount } = renderScrollRestoration(); act(() => { window.dispatchEvent(new Event('touchmove')); @@ -172,6 +189,7 @@ describe('useScrollRestoration', () => { scrollUserTo(1200); // Remounting is the next back navigation to the same history entry. + unmount(); setPageHeight(VIEWPORT_HEIGHT); setScrollY(0); renderScrollRestoration(); @@ -189,4 +207,32 @@ describe('useScrollRestoration', () => { expect(scrollTo).not.toHaveBeenCalled(); }); + + it('cancels a pending restoration when leaving the feed', () => { + saveFeedPosition(); + const { unmount } = renderScrollRestoration(); + advanceFrames(); + setPageHeight(FEED_HEIGHT); + unmount(); + advanceFrames(); + + expect(scrollTo).not.toHaveBeenCalled(); + expect(notifyResize).toBeUndefined(); + }); + + it('restores when the viewport shrinks enough to reach the saved position', () => { + saveFeedPosition(); + setPageHeight(SAVED_POSITION + VIEWPORT_HEIGHT - 100); + renderScrollRestoration(); + advanceFrames(); + expect(scrollTo).not.toHaveBeenCalled(); + + Object.defineProperty(window, 'innerHeight', { + configurable: true, + value: VIEWPORT_HEIGHT - 100, + }); + act(() => window.dispatchEvent(new Event('resize'))); + + expect(scrollTo).toHaveBeenCalledWith(0, SAVED_POSITION); + }); }); diff --git a/packages/shared/src/hooks/useScrollRestoration.ts b/packages/shared/src/hooks/useScrollRestoration.ts index 459de25a709..1d6c971fd3f 100644 --- a/packages/shared/src/hooks/useScrollRestoration.ts +++ b/packages/shared/src/hooks/useScrollRestoration.ts @@ -3,10 +3,6 @@ import { useEffect, useRef } from 'react'; import { useRouter } from 'next/router'; const scrollPositions: Record = {}; -// A feed restored from cache needs longer than a second to reconcile on a -// mid-range phone. A shorter budget expires mid-render, which is exactly when -// the page is still too short to hold the saved position. -const RESTORE_TIMEOUT_MS = 2000; const getScrollKey = (asPath: string): string => { if (typeof window === 'undefined') { @@ -46,19 +42,24 @@ export const useScrollRestoration = (): void => { } isRestoringRef.current = true; - const deadline = performance.now() + RESTORE_TIMEOUT_MS; let frame = 0; + let stopped = false; + let observer: ResizeObserver; + const controller = new AbortController(); const stop = () => { + stopped = true; isRestoringRef.current = false; cancelAnimationFrame(frame); - window.removeEventListener('wheel', stop); - window.removeEventListener('touchmove', stop); - window.removeEventListener('keydown', stop); - window.removeEventListener('mousedown', stop); + observer.disconnect(); + controller.abort(); }; - const tick = () => { + const restore = () => { + if (stopped) { + return; + } + const maxScroll = document.documentElement.scrollHeight - window.innerHeight; @@ -67,26 +68,26 @@ export const useScrollRestoration = (): void => { if (maxScroll >= target) { window.scrollTo(0, target); stop(); - return; - } - - // Out of budget: the feed never got there, so leave the user at the top. - if (performance.now() >= deadline) { - stop(); - return; } - - frame = requestAnimationFrame(tick); }; + observer = new ResizeObserver(() => { + cancelAnimationFrame(frame); + frame = requestAnimationFrame(restore); + }); + // Restoring must never fight the user, any real input ends the attempt. // `mousedown` covers scrollbar drags, which emit no wheel event. - window.addEventListener('wheel', stop, { passive: true }); - window.addEventListener('touchmove', stop, { passive: true }); - window.addEventListener('keydown', stop); - window.addEventListener('mousedown', stop); - - frame = requestAnimationFrame(tick); + const { signal } = controller; + window.addEventListener('wheel', stop, { passive: true, signal }); + window.addEventListener('touchmove', stop, { passive: true, signal }); + window.addEventListener('keydown', stop, { signal }); + window.addEventListener('mousedown', stop, { signal }); + + observer.observe(document.body); + observer.observe(document.documentElement); + window.addEventListener('resize', restore, { signal }); + frame = requestAnimationFrame(restore); return stop; }, [asPath]); From 163387cde4df3418a5f2b0f95194ec5432b39e8f Mon Sep 17 00:00:00 2001 From: Chris Bongers Date: Wed, 16 Sep 2026 16:22:44 +0200 Subject: [PATCH 2/4] fix(feed): share bounded restoration across navigation paths Route modal close through the same height-aware restoration helper and save its new feed history entry. Bound attempts to ten seconds, cancel on native scrolling, and fall back to bounded polling without ResizeObserver. Consolidate history lookup and cap saved positions. Remove unrelated typing changes requested in review. Add clamping-aware hook regressions and native Chromium checks with Pixel 5 emulation and CPU throttling. --- packages/playwright/package.json | 1 + .../regressions/scrollRestoration.spec.ts | 151 ++++++++++++++++++ .../playwright/scroll-restoration.config.ts | 14 ++ .../src/hooks/usePostModalNavigation.spec.ts | 59 ++++++- .../src/hooks/usePostModalNavigation.ts | 73 ++++----- .../src/hooks/useScrollRestoration.spec.ts | 54 ++++++- .../shared/src/hooks/useScrollRestoration.ts | 92 ++--------- .../shared/src/lib/scrollRestoration.spec.ts | 20 +++ packages/shared/src/lib/scrollRestoration.ts | 119 ++++++++++++++ 9 files changed, 465 insertions(+), 118 deletions(-) create mode 100644 packages/playwright/regressions/scrollRestoration.spec.ts create mode 100644 packages/playwright/scroll-restoration.config.ts create mode 100644 packages/shared/src/lib/scrollRestoration.spec.ts create mode 100644 packages/shared/src/lib/scrollRestoration.ts diff --git a/packages/playwright/package.json b/packages/playwright/package.json index d04fe491e55..4c03ea68237 100644 --- a/packages/playwright/package.json +++ b/packages/playwright/package.json @@ -5,6 +5,7 @@ "description": "E2E tests for daily.dev using Playwright", "scripts": { "test": "playwright test", + "test:scroll-restoration": "playwright test --config scroll-restoration.config.ts", "test:headed": "playwright test --headed", "test:ui": "playwright test --ui", "test:debug": "playwright test --debug", diff --git a/packages/playwright/regressions/scrollRestoration.spec.ts b/packages/playwright/regressions/scrollRestoration.spec.ts new file mode 100644 index 00000000000..2e4151bb0d0 --- /dev/null +++ b/packages/playwright/regressions/scrollRestoration.spec.ts @@ -0,0 +1,151 @@ +import { test, expect } from '@playwright/test'; +import { readFileSync } from 'fs'; +import path from 'path'; +import ts from 'typescript'; + +declare global { + interface Window { + scrollRestorationHarness: typeof import('../../shared/src/lib/scrollRestoration'); + } +} + +const source = readFileSync( + path.resolve(__dirname, '../../shared/src/lib/scrollRestoration.ts'), + 'utf8' +); +const { outputText } = ts.transpileModule(source, { + compilerOptions: { + module: ts.ModuleKind.CommonJS, + target: ts.ScriptTarget.ES2020, + }, +}); + +test.beforeEach(async ({ page, context, browserName }) => { + if (browserName === 'chromium') { + const session = await context.newCDPSession(page); + await session.send('Emulation.setCPUThrottlingRate', { rate: 4 }); + } + await page.route('http://scroll-restoration.test/**', (route) => + route.fulfill({ + contentType: 'text/html', + body: '
Cached feed fixture
', + }) + ); + await page.goto('http://scroll-restoration.test/'); + await page.addScriptTag({ + content: `window.scrollRestorationHarness = {}; (function(exports) { ${outputText}\n })(window.scrollRestorationHarness);`, + }); +}); + +test('native scrolling clamps on a short feed; shared restoration waits for delayed growth', async ({ + page, +}) => { + const clampedPosition = await page.evaluate(() => { + window.scrollTo(0, 5000); + return window.scrollY; + }); + expect(clampedPosition).toBeGreaterThan(0); + expect(clampedPosition).toBeLessThan(5000); + + await page.evaluate(() => { + window.scrollTo(0, 0); + window.scrollRestorationHarness.restoreScrollPosition(5000); + window.setTimeout(() => { + document.getElementById('feed')!.style.height = '12000px'; + }, 2500); + }); + await expect + .poll(() => page.evaluate(() => window.scrollY), { timeout: 6000 }) + .toBe(5000); +}); + +test('late feed growth cannot scroll the reader after the deadline', async ({ + page, +}) => { + await page.evaluate(() => + window.scrollRestorationHarness.restoreScrollPosition(5000) + ); + await expect + .poll( + () => + page.evaluate(() => + window.scrollRestorationHarness.isScrollRestoring() + ), + { + timeout: 12000, + } + ) + .toBe(false); + await page.evaluate(async () => { + document.getElementById('feed')!.style.height = '12000px'; + await new Promise((resolve) => + requestAnimationFrame(() => requestAnimationFrame(resolve)) + ); + }); + expect(await page.evaluate(() => window.scrollY)).toBe(0); +}); + +test('native scroll events cancel a pending restoration', async ({ page }) => { + await page.evaluate(() => { + window.scrollRestorationHarness.restoreScrollPosition(5000); + window.scrollTo(0, 200); + }); + await expect + .poll(() => + page.evaluate(() => window.scrollRestorationHarness.isScrollRestoring()) + ) + .toBe(false); + await page.evaluate(async () => { + document.getElementById('feed')!.style.height = '12000px'; + await new Promise((resolve) => + requestAnimationFrame(() => requestAnimationFrame(resolve)) + ); + }); + expect(await page.evaluate(() => window.scrollY)).toBe(200); +}); + +test('restores delayed content without ResizeObserver', async ({ page }) => { + await page.evaluate(() => { + Reflect.deleteProperty(window, 'ResizeObserver'); + window.scrollRestorationHarness.restoreScrollPosition(5000); + window.setTimeout(() => { + document.getElementById('feed')!.style.height = '12000px'; + }, 2500); + }); + await expect + .poll(() => page.evaluate(() => window.scrollY), { timeout: 6000 }) + .toBe(5000); +}); + +test('restores a modal origin after native browser Back creates a new feed entry on close', async ({ + page, +}) => { + await page.evaluate(() => { + window.history.scrollRestoration = 'manual'; + window.history.replaceState({ key: 'feed' }, '', '/'); + window.history.pushState({ key: 'modal' }, '', '/posts/example'); + window.scrollRestorationHarness.saveScrollPosition( + '/posts/example', + 5000, + 'post-modal' + ); + window.history.pushState({ key: 'closed-feed' }, '', '/'); + }); + await page.goBack(); + await expect(page).toHaveURL('http://scroll-restoration.test/posts/example'); + await page.evaluate(() => { + const target = window.scrollRestorationHarness.getScrollPosition( + '/posts/example', + 'post-modal' + )!; + window.history.pushState({ key: 'closed-again' }, '', '/'); + window.scrollRestorationHarness.saveScrollPosition('/', target); + window.scrollRestorationHarness.restoreScrollPosition(target); + window.setTimeout(() => { + document.getElementById('feed')!.style.height = '12000px'; + }, 2500); + }); + await expect + .poll(() => page.evaluate(() => window.scrollY), { timeout: 6000 }) + .toBe(5000); +}); diff --git a/packages/playwright/scroll-restoration.config.ts b/packages/playwright/scroll-restoration.config.ts new file mode 100644 index 00000000000..32a82498033 --- /dev/null +++ b/packages/playwright/scroll-restoration.config.ts @@ -0,0 +1,14 @@ +import { defineConfig, devices } from '@playwright/test'; + +export default defineConfig({ + testDir: './regressions', + testMatch: 'scrollRestoration.spec.ts', + fullyParallel: true, + workers: 2, + reporter: 'list', + use: { screenshot: 'only-on-failure' }, + projects: [ + { name: 'desktop-chromium', use: { ...devices['Desktop Chrome'] } }, + { name: 'pixel-5-chromium', use: { ...devices['Pixel 5'] } }, + ], +}); diff --git a/packages/shared/src/hooks/usePostModalNavigation.spec.ts b/packages/shared/src/hooks/usePostModalNavigation.spec.ts index 5f74de5a03a..ab554fed114 100644 --- a/packages/shared/src/hooks/usePostModalNavigation.spec.ts +++ b/packages/shared/src/hooks/usePostModalNavigation.spec.ts @@ -4,6 +4,7 @@ import post from '../../__tests__/fixture/post'; import { FeedItemType } from '../components/cards/common/common'; import type { UseRouterMemory } from './useRouterMemory'; import { usePostModalNavigation } from './usePostModalNavigation'; +import { useScrollRestoration } from './useScrollRestoration'; const mockRouter = { asPath: '/', @@ -26,6 +27,9 @@ jest.mock('./useKeyboardNavigation', () => ({ })); let historyKey = 0; +let pageHeight = 20000; +let notifyResize: (() => void) | undefined; +const advanceFrame = () => act(() => jest.advanceTimersByTime(16)); const setScrollY = (value: number): void => { Object.defineProperty(window, 'scrollY', { configurable: true, value }); @@ -63,6 +67,22 @@ const renderNavigation = () => ); beforeEach(() => { + jest.useFakeTimers(); + pageHeight = 20000; + notifyResize = undefined; + Object.defineProperty(document.documentElement, 'scrollHeight', { + configurable: true, + get: () => pageHeight, + }); + jest.mocked(ResizeObserver).mockImplementation((callback) => { + const observer = { + observe: jest.fn(), + unobserve: jest.fn(), + disconnect: jest.fn(), + }; + notifyResize = () => callback([], observer); + return observer; + }); historyKey += 1; restoreHistoryEntry({ key: `${historyKey}`, @@ -85,10 +105,14 @@ beforeEach(() => { setScrollY(0); Object.defineProperty(window, 'scrollTo', { configurable: true, - value: jest.fn((_x: number, y: number) => setScrollY(y)), + value: jest.fn((_x: number, y: number) => + setScrollY(Math.min(y, Math.max(0, pageHeight - window.innerHeight))), + ), }); }); +afterEach(() => jest.useRealTimers()); + it('restores the feed when browser Back reopens a closed post', async () => { const { result, rerender } = renderNavigation(); setScrollY(5000); @@ -99,6 +123,7 @@ it('restores the feed when browser Back reopens a closed post', async () => { await act(async () => result.current.onCloseModal()); rerender(); + advanceFrame(); expect(window.scrollY).toBe(5000); restoreHistoryEntry(postEntry); @@ -107,6 +132,7 @@ it('restores the feed when browser Back reopens a closed post', async () => { setScrollY(0); await act(async () => result.current.onCloseModal()); + advanceFrame(); expect(window.scrollY).toBe(5000); }); @@ -129,12 +155,14 @@ it('keeps separate positions for earlier modal history entries after a remount', setScrollY(0); const utils = renderNavigation(); await act(async () => utils.result.current.onCloseModal()); + advanceFrame(); expect(window.scrollY).toBe(5000); restoreHistoryEntry(secondEntry); setScrollY(0); utils.rerender(); await act(async () => utils.result.current.onCloseModal()); + advanceFrame(); expect(window.scrollY).toBe(9000); }); @@ -151,6 +179,7 @@ it('carries the original feed position through next-post navigation after a remo expect(utils.result.current.selectedPost?.id).toBe('second-post'); await act(async () => utils.result.current.onCloseModal()); + advanceFrame(); expect(window.scrollY).toBe(5000); }); @@ -174,3 +203,31 @@ it('does not restore the feed when closing navigation is cancelled', async () => expect(window.scrollTo).not.toHaveBeenCalled(); }); + +it('waits for a remounted feed to grow after closing a post', async () => { + const view = renderNavigation(); + setScrollY(5000); + await act(async () => view.result.current.onOpenModal(0)); + view.unmount(); + setScrollY(0); + pageHeight = window.innerHeight; + + const { result, rerender } = renderNavigation(); + await act(async () => result.current.onCloseModal()); + rerender(); + const destination = getHistoryEntry(); + act(() => jest.advanceTimersByTime(2500)); + expect(window.scrollTo).not.toHaveBeenCalled(); + expect(window.scrollY).toBe(0); + + pageHeight = 20000; + notifyResize?.(); + advanceFrame(); + expect(window.scrollY).toBe(5000); + + restoreHistoryEntry(destination); + setScrollY(0); + renderHook(() => useScrollRestoration()); + advanceFrame(); + expect(window.scrollY).toBe(5000); +}); diff --git a/packages/shared/src/hooks/usePostModalNavigation.ts b/packages/shared/src/hooks/usePostModalNavigation.ts index e9b4419eabb..d215892b7ac 100644 --- a/packages/shared/src/hooks/usePostModalNavigation.ts +++ b/packages/shared/src/hooks/usePostModalNavigation.ts @@ -8,11 +8,16 @@ import type { FeedItem, PostItem, UpdateFeedPost } from './useFeed'; import { isBoostedPostAd } from './useFeed'; import { Origin, LogEvent } from '../lib/log'; import { webappUrl } from '../lib/constants'; -import { getPathnameWithQuery, objectToQueryParams } from '../lib/links'; +import { getPathnameWithQuery, objectToQueryParams } from '../lib'; import { useKeyboardNavigation } from './useKeyboardNavigation'; import { isExtension } from '../lib/func'; import type { UseRouterMemory as UsePostModalRouter } from './useRouterMemory'; import { useRouterMemory } from './useRouterMemory'; +import { + getScrollPosition, + saveScrollPosition, + restoreScrollPosition, +} from '../lib/scrollRestoration'; export enum PostPosition { First = 'first', @@ -29,7 +34,7 @@ interface UsePostModalNavigation { onCloseModal: (fromPopState?: boolean) => void; isFetchingNextPage?: boolean; selectedPost: Post | null; - selectedPostIndex: number | undefined; + selectedPostIndex: number; selectedPostIsAd: boolean; } @@ -46,10 +51,6 @@ const useRouter: () => UsePostModalRouter = isExtension ? useRouterMemory : useRouterNext; -const feedScrollPositions = new Map(); -const getHistoryKey = (): string | undefined => - isExtension ? undefined : window.history.state?.key; - export const usePostModalNavigation = ({ items, fetchPage, @@ -68,6 +69,9 @@ export const usePostModalNavigation = ({ const { logEvent } = useLogContext(); const [isFetchingNextPage, setIsFetchingNextPage] = useState(false); const scrollPositionOnFeed = useRef(); + const cancelRestore = useRef<() => void>(); + + useEffect(() => () => cancelRestore.current?.(), []); // if multiple feeds/hooks are rendered prevent effects from running while other modal is open const isNavigationActive = feedName === activeFeedName; @@ -90,9 +94,7 @@ export const usePostModalNavigation = ({ return item.post.slug === pmid || item.post.id === pmid; } if (isBoostedPostAd(item)) { - return ( - item.ad.data.post?.slug === pmid || item.ad.data.post?.id === pmid - ); + return item.ad.data.post.slug === pmid || item.ad.data.post.id === pmid; } return false; @@ -107,7 +109,7 @@ export const usePostModalNavigation = ({ const getPostItem = useCallback( (index: number) => { - if (!items[index]) { + if (index === null || !items[index]) { return null; } @@ -131,8 +133,8 @@ export const usePostModalNavigation = ({ ); const getPost = useCallback( - (index: number | undefined): Post | null => { - if (index === undefined || !items[index]) { + (index: number) => { + if (index === null || !items[index]) { return null; } @@ -141,7 +143,7 @@ export const usePostModalNavigation = ({ return item.post; } if (isBoostedPostAd(item)) { - return item.ad.data.post ?? null; + return item.ad.data.post; } return null; @@ -155,11 +157,11 @@ export const usePostModalNavigation = ({ if (post) { const postId = post.slug || post.id; - const historyKey = getHistoryKey(); const feedScrollPosition = - (pmid && historyKey - ? feedScrollPositions.get(historyKey) - : undefined) ?? scrollPositionOnFeed.current; + pmid && !isExtension + ? getScrollPosition(router.asPath, 'post-modal') + : scrollPositionOnFeed.current; + cancelRestore.current?.(); const newPathname = getPathnameWithQuery( basePathname, @@ -183,16 +185,17 @@ export const usePostModalNavigation = ({ shallow: true, }, ); - const postHistoryKey = getHistoryKey(); - if (navigated && postHistoryKey && feedScrollPosition !== undefined) { - feedScrollPositions.set(postHistoryKey, feedScrollPosition); + if (navigated && !isExtension && feedScrollPosition !== undefined) { + saveScrollPosition( + `${webappUrl}posts/${postId}`, + feedScrollPosition, + 'post-modal', + ); } } if (post?.type === PostType.Share) { const item = getPostItem(index); - if (item) { - updatePost(item.page, item.index, { ...post, read: true }); - } + updatePost(item.page, item.index, { ...post, read: true }); } }, [ @@ -208,6 +211,7 @@ export const usePostModalNavigation = ({ ); const onOpenModal = (index: number) => { + cancelRestore.current?.(); if (!pmid) { scrollPositionOnFeed.current = window.scrollY; } @@ -250,9 +254,7 @@ export const usePostModalNavigation = ({ return item.post.slug === pmid || item.post.id === pmid; } if (isBoostedPostAd(item)) { - return ( - item.ad.data.post?.slug === pmid || item.ad.data.post?.id === pmid - ); + return item.ad.data.post.slug === pmid || item.ad.data.post.id === pmid; } return false; @@ -263,17 +265,15 @@ export const usePostModalNavigation = ({ } }, [openedPostIndex, pmid, items, onChangeSelected, isNavigationActive]); - const selectedPostIsAd = - openedPostIndex !== undefined && isBoostedPostAd(items[openedPostIndex]); + const selectedPostIsAd = isBoostedPostAd(items[openedPostIndex]); const result = { postPosition: getPostPosition(), isFetchingNextPage: false, selectedPostIsAd, onCloseModal: async () => { - const historyKey = getHistoryKey(); - const feedScrollPosition = - (historyKey ? feedScrollPositions.get(historyKey) : undefined) ?? - scrollPositionOnFeed.current; + const feedScrollPosition = isExtension + ? scrollPositionOnFeed.current + : getScrollPosition(router.asPath, 'post-modal'); // Extract query params from baseAsPath to preserve original params like 'id' const baseUrl = new URL(baseAsPath, window.location.origin); const searchParams = new URLSearchParams(baseUrl.search); @@ -287,14 +287,12 @@ export const usePostModalNavigation = ({ ); if (navigated && feedScrollPosition !== undefined) { - window.scrollTo(0, feedScrollPosition); + saveScrollPosition(baseAsPath, feedScrollPosition); + cancelRestore.current = restoreScrollPosition(feedScrollPosition); } }, onOpenModal, onPrevious: () => { - if (openedPostIndex === undefined) { - return; - } let index = openedPostIndex - 1; // look for the first post before the current one while (index > 0 && !isPostItem(items[index])) { @@ -316,9 +314,6 @@ export const usePostModalNavigation = ({ onChangeSelected(index); }, onNext: async () => { - if (openedPostIndex === undefined) { - return; - } let index = openedPostIndex + 1; // eslint-disable-next-line no-empty for (; index < items.length && !isPostItem(items[index]); index += 1) {} diff --git a/packages/shared/src/hooks/useScrollRestoration.spec.ts b/packages/shared/src/hooks/useScrollRestoration.spec.ts index 2d22703ffa9..63c2f80a093 100644 --- a/packages/shared/src/hooks/useScrollRestoration.spec.ts +++ b/packages/shared/src/hooks/useScrollRestoration.spec.ts @@ -118,7 +118,7 @@ describe('useScrollRestoration', () => { expect(scrollTo).toHaveBeenCalledWith(0, SAVED_POSITION); }); - it('waits without polling when the page is too short', () => { + it('abandons restoration before unrelated late page growth', () => { saveFeedPosition(); renderScrollRestoration(); @@ -133,7 +133,7 @@ describe('useScrollRestoration', () => { setPageHeight(FEED_HEIGHT); advanceFrames(); - expect(scrollTo).toHaveBeenCalledWith(0, SAVED_POSITION); + expect(scrollTo).not.toHaveBeenCalled(); }); it('keeps the saved position when the router resets the scroll to the top', () => { @@ -235,4 +235,54 @@ describe('useScrollRestoration', () => { expect(scrollTo).toHaveBeenCalledWith(0, SAVED_POSITION); }); + + it('stops on a changed scroll position even without a wheel or touch event', () => { + saveFeedPosition(); + const { unmount } = renderScrollRestoration(); + setPageHeight(3000); + scrollUserTo(1200); + setPageHeight(FEED_HEIGHT); + advanceFrames(); + expect(scrollTo).not.toHaveBeenCalled(); + + unmount(); + setScrollY(0); + renderScrollRestoration(); + advanceFrames(); + expect(scrollTo).toHaveBeenCalledWith(0, 1200); + }); + + it('records scrolling again after the restoration deadline', () => { + saveFeedPosition(); + const { unmount } = renderScrollRestoration(); + act(() => jest.advanceTimersByTime(10000)); + setPageHeight(FEED_HEIGHT); + scrollUserTo(1200); + unmount(); + setScrollY(0); + renderScrollRestoration(); + advanceFrames(); + expect(scrollTo).toHaveBeenCalledWith(0, 1200); + }); + + it('restores with bounded polling when ResizeObserver is unavailable', () => { + saveFeedPosition(); + const observerConstructor = global.ResizeObserver; + Object.defineProperty(global, 'ResizeObserver', { + value: undefined, + }); + try { + const { unmount } = renderScrollRestoration(); + act(() => jest.advanceTimersByTime(2500)); + setPageHeight(FEED_HEIGHT); + act(() => jest.advanceTimersByTime(120)); + expect(scrollTo).toHaveBeenCalledWith(0, SAVED_POSITION); + expect(jest.getTimerCount()).toBe(0); + unmount(); + } finally { + Object.defineProperty(global, 'ResizeObserver', { + value: observerConstructor, + }); + } + }); }); diff --git a/packages/shared/src/hooks/useScrollRestoration.ts b/packages/shared/src/hooks/useScrollRestoration.ts index 1d6c971fd3f..3a7ea02ba13 100644 --- a/packages/shared/src/hooks/useScrollRestoration.ts +++ b/packages/shared/src/hooks/useScrollRestoration.ts @@ -1,95 +1,35 @@ -import { useEffect, useRef } from 'react'; - +import { useEffect } from 'react'; import { useRouter } from 'next/router'; - -const scrollPositions: Record = {}; - -const getScrollKey = (asPath: string): string => { - if (typeof window === 'undefined') { - return asPath; - } - const historyKey = (window.history.state as { key?: string } | null)?.key; - return historyKey ? `${asPath}:${historyKey}` : asPath; -}; +import { + cancelScrollRestoration, + getScrollPosition, + isScrollRestoring, + restoreScrollPosition, + saveScrollPosition, +} from '../lib/scrollRestoration'; export const useScrollRestoration = (): void => { - const { asPath } = useRouter(); - const isRestoringRef = useRef(false); + const { asPath, events } = useRouter(); useEffect(() => { const handleScroll = () => { - // Our own restore pass and Next's reset-to-top on navigation both emit - // scroll events, and neither is where the user actually was. - if (isRestoringRef.current) { - return; + if (!isScrollRestoring()) { + saveScrollPosition(asPath, window.scrollY); } - - scrollPositions[getScrollKey(asPath)] = window.scrollY; }; window.addEventListener('scroll', handleScroll, { passive: true }); + events?.on('routeChangeStart', cancelScrollRestoration); return () => { window.removeEventListener('scroll', handleScroll); + events?.off('routeChangeStart', cancelScrollRestoration); }; - }, [asPath]); + }, [asPath, events]); useEffect(() => { - const target = scrollPositions[getScrollKey(asPath)] ?? 0; - - if (!target) { - return undefined; - } - - isRestoringRef.current = true; - let frame = 0; - let stopped = false; - let observer: ResizeObserver; - const controller = new AbortController(); - - const stop = () => { - stopped = true; - isRestoringRef.current = false; - cancelAnimationFrame(frame); - observer.disconnect(); - controller.abort(); - }; - - const restore = () => { - if (stopped) { - return; - } - - const maxScroll = - document.documentElement.scrollHeight - window.innerHeight; - - // Scrolling before the page is tall enough clamps to the bottom of what - // has rendered so far, so wait rather than settle for a wrong position. - if (maxScroll >= target) { - window.scrollTo(0, target); - stop(); - } - }; - - observer = new ResizeObserver(() => { - cancelAnimationFrame(frame); - frame = requestAnimationFrame(restore); - }); - - // Restoring must never fight the user, any real input ends the attempt. - // `mousedown` covers scrollbar drags, which emit no wheel event. - const { signal } = controller; - window.addEventListener('wheel', stop, { passive: true, signal }); - window.addEventListener('touchmove', stop, { passive: true, signal }); - window.addEventListener('keydown', stop, { signal }); - window.addEventListener('mousedown', stop, { signal }); - - observer.observe(document.body); - observer.observe(document.documentElement); - window.addEventListener('resize', restore, { signal }); - frame = requestAnimationFrame(restore); - - return stop; + const target = getScrollPosition(asPath); + return target ? restoreScrollPosition(target) : undefined; }, [asPath]); }; diff --git a/packages/shared/src/lib/scrollRestoration.spec.ts b/packages/shared/src/lib/scrollRestoration.spec.ts new file mode 100644 index 00000000000..6a6c0b8ce92 --- /dev/null +++ b/packages/shared/src/lib/scrollRestoration.spec.ts @@ -0,0 +1,20 @@ +import { getScrollPosition, saveScrollPosition } from './scrollRestoration'; + +it('evicts old history entries while retaining recent positions', () => { + for (let index = 0; index < 1000; index += 1) { + window.history.replaceState({ key: `bounded-${index}` }, '', '/'); + saveScrollPosition('/', index); + } + expect(getScrollPosition('/')).toBe(999); + window.history.replaceState({ key: 'bounded-0' }, '', '/'); + expect(getScrollPosition('/')).toBeUndefined(); +}); + +it('keeps modal origins separate from scrolling the post entry', () => { + window.history.replaceState({ key: 'modal-position' }, '', '/posts/test'); + saveScrollPosition('/posts/test', 5000, 'post-modal'); + saveScrollPosition('/posts/test', 0); + + expect(getScrollPosition('/posts/test', 'post-modal')).toBe(5000); + expect(getScrollPosition('/posts/test')).toBe(0); +}); diff --git a/packages/shared/src/lib/scrollRestoration.ts b/packages/shared/src/lib/scrollRestoration.ts new file mode 100644 index 00000000000..417ee98d5d0 --- /dev/null +++ b/packages/shared/src/lib/scrollRestoration.ts @@ -0,0 +1,119 @@ +const RESTORE_TIMEOUT_MS = 10000; +const RESTORE_POLL_INTERVAL_MS = 100; +const MAX_SCROLL_POSITIONS = 200; + +type ScrollPositionKind = 'page' | 'post-modal'; + +const scrollPositions = new Map(); +let activeRestoration: (() => void) | undefined; + +const getScrollKey = (asPath: string, kind: ScrollPositionKind): string => { + if (typeof window === 'undefined') { + return `${kind}:${asPath}`; + } + + const url = new URL(asPath, window.location.origin); + const historyKey = (window.history.state as { key?: string } | null)?.key; + return `${kind}:${url.pathname}${url.search}${url.hash}:${historyKey ?? ''}`; +}; + +export const getScrollPosition = ( + asPath: string, + kind: ScrollPositionKind = 'page', +): number | undefined => scrollPositions.get(getScrollKey(asPath, kind)); + +export const saveScrollPosition = ( + asPath: string, + position: number, + kind: ScrollPositionKind = 'page', +): void => { + const key = getScrollKey(asPath, kind); + scrollPositions.delete(key); + scrollPositions.set(key, position); + if (scrollPositions.size > MAX_SCROLL_POSITIONS) { + const oldestKey = scrollPositions.keys().next().value; + if (oldestKey !== undefined) { + scrollPositions.delete(oldestKey); + } + } +}; + +export const isScrollRestoring = (): boolean => !!activeRestoration; + +export const cancelScrollRestoration = (): void => activeRestoration?.(); + +export const restoreScrollPosition = (target: number): (() => void) => { + cancelScrollRestoration(); + const initialPosition = window.scrollY; + const scrollKey = getScrollKey(window.location.href, 'page'); + const controller = new AbortController(); + let observer: ResizeObserver | undefined; + let frame = 0; + let timeout = 0; + let poll = 0; + let stopped = false; + + const stop = () => { + stopped = true; + cancelAnimationFrame(frame); + window.clearTimeout(timeout); + window.clearInterval(poll); + observer?.disconnect(); + controller.abort(); + if (activeRestoration === stop) { + activeRestoration = undefined; + } + }; + + const restore = () => { + if (stopped) { + return; + } + if (getScrollKey(window.location.href, 'page') !== scrollKey) { + stop(); + return; + } + + const maxScroll = + document.documentElement.scrollHeight - window.innerHeight; + if (maxScroll >= target) { + window.scrollTo(0, target); + stop(); + } + }; + + const scheduleRestore = () => { + cancelAnimationFrame(frame); + frame = requestAnimationFrame(restore); + }; + + activeRestoration = stop; + timeout = window.setTimeout(stop, RESTORE_TIMEOUT_MS); + if (typeof ResizeObserver === 'undefined') { + poll = window.setInterval(scheduleRestore, RESTORE_POLL_INTERVAL_MS); + } else { + observer = new ResizeObserver(scheduleRestore); + observer.observe(document.body); + } + + const { signal } = controller; + window.addEventListener('resize', restore, { signal }); + window.addEventListener('popstate', stop, { signal }); + window.addEventListener('wheel', stop, { passive: true, signal }); + window.addEventListener('touchmove', stop, { passive: true, signal }); + window.addEventListener('keydown', stop, { signal }); + window.addEventListener('mousedown', stop, { signal }); + window.addEventListener( + 'scroll', + () => { + if (window.scrollY !== initialPosition) { + stop(); + saveScrollPosition(window.location.href, window.scrollY); + } + }, + { passive: true, signal }, + ); + scheduleRestore(); + + return stop; +}; From 040bdb4203946e3e80721f0a2ceb68eca93acddf Mon Sep 17 00:00:00 2001 From: Chris Bongers Date: Wed, 16 Sep 2026 16:32:22 +0200 Subject: [PATCH 3/4] test(feed): remove standalone Playwright regression suite --- packages/playwright/package.json | 1 - .../regressions/scrollRestoration.spec.ts | 151 ------------------ .../playwright/scroll-restoration.config.ts | 14 -- 3 files changed, 166 deletions(-) delete mode 100644 packages/playwright/regressions/scrollRestoration.spec.ts delete mode 100644 packages/playwright/scroll-restoration.config.ts diff --git a/packages/playwright/package.json b/packages/playwright/package.json index 4c03ea68237..d04fe491e55 100644 --- a/packages/playwright/package.json +++ b/packages/playwright/package.json @@ -5,7 +5,6 @@ "description": "E2E tests for daily.dev using Playwright", "scripts": { "test": "playwright test", - "test:scroll-restoration": "playwright test --config scroll-restoration.config.ts", "test:headed": "playwright test --headed", "test:ui": "playwright test --ui", "test:debug": "playwright test --debug", diff --git a/packages/playwright/regressions/scrollRestoration.spec.ts b/packages/playwright/regressions/scrollRestoration.spec.ts deleted file mode 100644 index 2e4151bb0d0..00000000000 --- a/packages/playwright/regressions/scrollRestoration.spec.ts +++ /dev/null @@ -1,151 +0,0 @@ -import { test, expect } from '@playwright/test'; -import { readFileSync } from 'fs'; -import path from 'path'; -import ts from 'typescript'; - -declare global { - interface Window { - scrollRestorationHarness: typeof import('../../shared/src/lib/scrollRestoration'); - } -} - -const source = readFileSync( - path.resolve(__dirname, '../../shared/src/lib/scrollRestoration.ts'), - 'utf8' -); -const { outputText } = ts.transpileModule(source, { - compilerOptions: { - module: ts.ModuleKind.CommonJS, - target: ts.ScriptTarget.ES2020, - }, -}); - -test.beforeEach(async ({ page, context, browserName }) => { - if (browserName === 'chromium') { - const session = await context.newCDPSession(page); - await session.send('Emulation.setCPUThrottlingRate', { rate: 4 }); - } - await page.route('http://scroll-restoration.test/**', (route) => - route.fulfill({ - contentType: 'text/html', - body: '
Cached feed fixture
', - }) - ); - await page.goto('http://scroll-restoration.test/'); - await page.addScriptTag({ - content: `window.scrollRestorationHarness = {}; (function(exports) { ${outputText}\n })(window.scrollRestorationHarness);`, - }); -}); - -test('native scrolling clamps on a short feed; shared restoration waits for delayed growth', async ({ - page, -}) => { - const clampedPosition = await page.evaluate(() => { - window.scrollTo(0, 5000); - return window.scrollY; - }); - expect(clampedPosition).toBeGreaterThan(0); - expect(clampedPosition).toBeLessThan(5000); - - await page.evaluate(() => { - window.scrollTo(0, 0); - window.scrollRestorationHarness.restoreScrollPosition(5000); - window.setTimeout(() => { - document.getElementById('feed')!.style.height = '12000px'; - }, 2500); - }); - await expect - .poll(() => page.evaluate(() => window.scrollY), { timeout: 6000 }) - .toBe(5000); -}); - -test('late feed growth cannot scroll the reader after the deadline', async ({ - page, -}) => { - await page.evaluate(() => - window.scrollRestorationHarness.restoreScrollPosition(5000) - ); - await expect - .poll( - () => - page.evaluate(() => - window.scrollRestorationHarness.isScrollRestoring() - ), - { - timeout: 12000, - } - ) - .toBe(false); - await page.evaluate(async () => { - document.getElementById('feed')!.style.height = '12000px'; - await new Promise((resolve) => - requestAnimationFrame(() => requestAnimationFrame(resolve)) - ); - }); - expect(await page.evaluate(() => window.scrollY)).toBe(0); -}); - -test('native scroll events cancel a pending restoration', async ({ page }) => { - await page.evaluate(() => { - window.scrollRestorationHarness.restoreScrollPosition(5000); - window.scrollTo(0, 200); - }); - await expect - .poll(() => - page.evaluate(() => window.scrollRestorationHarness.isScrollRestoring()) - ) - .toBe(false); - await page.evaluate(async () => { - document.getElementById('feed')!.style.height = '12000px'; - await new Promise((resolve) => - requestAnimationFrame(() => requestAnimationFrame(resolve)) - ); - }); - expect(await page.evaluate(() => window.scrollY)).toBe(200); -}); - -test('restores delayed content without ResizeObserver', async ({ page }) => { - await page.evaluate(() => { - Reflect.deleteProperty(window, 'ResizeObserver'); - window.scrollRestorationHarness.restoreScrollPosition(5000); - window.setTimeout(() => { - document.getElementById('feed')!.style.height = '12000px'; - }, 2500); - }); - await expect - .poll(() => page.evaluate(() => window.scrollY), { timeout: 6000 }) - .toBe(5000); -}); - -test('restores a modal origin after native browser Back creates a new feed entry on close', async ({ - page, -}) => { - await page.evaluate(() => { - window.history.scrollRestoration = 'manual'; - window.history.replaceState({ key: 'feed' }, '', '/'); - window.history.pushState({ key: 'modal' }, '', '/posts/example'); - window.scrollRestorationHarness.saveScrollPosition( - '/posts/example', - 5000, - 'post-modal' - ); - window.history.pushState({ key: 'closed-feed' }, '', '/'); - }); - await page.goBack(); - await expect(page).toHaveURL('http://scroll-restoration.test/posts/example'); - await page.evaluate(() => { - const target = window.scrollRestorationHarness.getScrollPosition( - '/posts/example', - 'post-modal' - )!; - window.history.pushState({ key: 'closed-again' }, '', '/'); - window.scrollRestorationHarness.saveScrollPosition('/', target); - window.scrollRestorationHarness.restoreScrollPosition(target); - window.setTimeout(() => { - document.getElementById('feed')!.style.height = '12000px'; - }, 2500); - }); - await expect - .poll(() => page.evaluate(() => window.scrollY), { timeout: 6000 }) - .toBe(5000); -}); diff --git a/packages/playwright/scroll-restoration.config.ts b/packages/playwright/scroll-restoration.config.ts deleted file mode 100644 index 32a82498033..00000000000 --- a/packages/playwright/scroll-restoration.config.ts +++ /dev/null @@ -1,14 +0,0 @@ -import { defineConfig, devices } from '@playwright/test'; - -export default defineConfig({ - testDir: './regressions', - testMatch: 'scrollRestoration.spec.ts', - fullyParallel: true, - workers: 2, - reporter: 'list', - use: { screenshot: 'only-on-failure' }, - projects: [ - { name: 'desktop-chromium', use: { ...devices['Desktop Chrome'] } }, - { name: 'pixel-5-chromium', use: { ...devices['Pixel 5'] } }, - ], -}); From d6acc3fb20885f02a458884d0322db250c3d8719 Mon Sep 17 00:00:00 2001 From: Chris Bongers Date: Thu, 17 Sep 2026 11:39:02 +0200 Subject: [PATCH 4/4] fix(feed): make post modal navigation strict-safe --- .../src/hooks/usePostModalNavigation.ts | 54 ++++++++++--------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/packages/shared/src/hooks/usePostModalNavigation.ts b/packages/shared/src/hooks/usePostModalNavigation.ts index d215892b7ac..d38a74bdd55 100644 --- a/packages/shared/src/hooks/usePostModalNavigation.ts +++ b/packages/shared/src/hooks/usePostModalNavigation.ts @@ -34,7 +34,7 @@ interface UsePostModalNavigation { onCloseModal: (fromPopState?: boolean) => void; isFetchingNextPage?: boolean; selectedPost: Post | null; - selectedPostIndex: number; + selectedPostIndex: number | undefined; selectedPostIsAd: boolean; } @@ -94,7 +94,9 @@ export const usePostModalNavigation = ({ return item.post.slug === pmid || item.post.id === pmid; } if (isBoostedPostAd(item)) { - return item.ad.data.post.slug === pmid || item.ad.data.post.id === pmid; + return ( + item.ad.data.post?.slug === pmid || item.ad.data.post?.id === pmid + ); } return false; @@ -108,23 +110,25 @@ export const usePostModalNavigation = ({ }, [items, pmid, isNavigationActive]); const getPostItem = useCallback( - (index: number) => { - if (index === null || !items[index]) { + ( + index: number | undefined, + ): Pick | null => { + if (index === undefined || !items[index]) { return null; } const item = items[index]; if (item.type === 'post') { - return item as PostItem; + return item; } - if (isBoostedPostAd(item)) { + if (isBoostedPostAd(item) && item.ad.data.post) { // For Post Ads, we need to create a PostItem-like structure // Note: AdItem doesn't have a page property, so we'll use -1 as default return { post: item.ad.data.post, page: -1, index: item.index, - } as PostItem; + }; } return null; @@ -133,22 +137,9 @@ export const usePostModalNavigation = ({ ); const getPost = useCallback( - (index: number) => { - if (index === null || !items[index]) { - return null; - } - - const item = items[index]; - if (item.type === 'post') { - return item.post; - } - if (isBoostedPostAd(item)) { - return item.ad.data.post; - } - - return null; - }, - [items], + (index: number | undefined): Post | null => + getPostItem(index)?.post ?? null, + [getPostItem], ); const onChangeSelected = useCallback( @@ -195,7 +186,9 @@ export const usePostModalNavigation = ({ } if (post?.type === PostType.Share) { const item = getPostItem(index); - updatePost(item.page, item.index, { ...post, read: true }); + if (item) { + updatePost(item.page, item.index, { ...post, read: true }); + } } }, [ @@ -254,7 +247,9 @@ export const usePostModalNavigation = ({ return item.post.slug === pmid || item.post.id === pmid; } if (isBoostedPostAd(item)) { - return item.ad.data.post.slug === pmid || item.ad.data.post.id === pmid; + return ( + item.ad.data.post?.slug === pmid || item.ad.data.post?.id === pmid + ); } return false; @@ -265,7 +260,8 @@ export const usePostModalNavigation = ({ } }, [openedPostIndex, pmid, items, onChangeSelected, isNavigationActive]); - const selectedPostIsAd = isBoostedPostAd(items[openedPostIndex]); + const selectedPostIsAd = + openedPostIndex !== undefined && isBoostedPostAd(items[openedPostIndex]); const result = { postPosition: getPostPosition(), isFetchingNextPage: false, @@ -293,6 +289,9 @@ export const usePostModalNavigation = ({ }, onOpenModal, onPrevious: () => { + if (openedPostIndex === undefined) { + return; + } let index = openedPostIndex - 1; // look for the first post before the current one while (index > 0 && !isPostItem(items[index])) { @@ -314,6 +313,9 @@ export const usePostModalNavigation = ({ onChangeSelected(index); }, onNext: async () => { + if (openedPostIndex === undefined) { + return; + } let index = openedPostIndex + 1; // eslint-disable-next-line no-empty for (; index < items.length && !isPostItem(items[index]); index += 1) {}