fix(shell): scroll restore on Back + reset on new filter (bugs 1, 3)
The shell owns the scroller, so it owns the scroll policy. PageScroller now takes a scrollKey identifying the logical view and keeps a per-view offset in a module-level map, recorded live on scroll: - same key returning (feed -> channel -> Back, filters unchanged) restores where you were (bug 1) -- the feed's scroller unmounts, but the saved offset survives and is re-applied, retried over a few frames so the virtualized list has time to reach its full height; - a new key (feed filters changed) starts at the top of the fresh result set (bug 3). q is normalised out of the feed key so typing doesn't reset. Recording on scroll rather than at unmount matters: by cleanup time the scroller's scrollTop is already zeroed, so a cleanup-read saved 0. Also fixes a Maximum-update-depth loop this introduced: the combined element/fade ref was an inline callback, so React re-attached it every commit and re-ran useScrollFade's state setter. Bound it with useCallback. Adds E2E locks for bugs 1, 3 and 4 (restore on Back, reset on filter, toolbar stays pinned). Full suite 14/14 green on :5173; tsc + knip clean.
This commit is contained in:
@@ -29,4 +29,30 @@ test.describe("channel view", () => {
|
|||||||
await expect(page.getByTestId("channel-title")).toHaveCount(0);
|
await expect(page.getByTestId("channel-title")).toHaveCount(0);
|
||||||
await expect(page.getByTestId("video-card").first()).toBeVisible();
|
await expect(page.getByTestId("video-card").first()).toBeVisible();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Bug 1 (Platform S4): opening a channel and coming Back restores the feed's scroll position
|
||||||
|
// rather than dumping you at the top. The shell owns the scroller and remembers each view's offset.
|
||||||
|
test("Back restores the feed scroll position", async ({ page }) => {
|
||||||
|
await openApp(page);
|
||||||
|
await expect(page.getByTestId("video-card").first()).toBeVisible();
|
||||||
|
|
||||||
|
const main = page.getByRole("main");
|
||||||
|
await main.evaluate((el) => el.scrollBy(0, 3000));
|
||||||
|
// Let any page-fetch triggered by the scroll settle, so the same content is cached for the
|
||||||
|
// return trip (a fetch still in flight at navigation would leave a shorter list to restore into).
|
||||||
|
await page.waitForTimeout(600);
|
||||||
|
const saved = await main.evaluate((el) => el.scrollTop);
|
||||||
|
expect(saved).toBeGreaterThan(1500);
|
||||||
|
|
||||||
|
await page.getByTestId("video-card-channel").first().click();
|
||||||
|
await expect(page.getByTestId("channel-title")).toBeVisible();
|
||||||
|
await page.getByTestId("channel-back").click();
|
||||||
|
await expect(page.getByTestId("video-card").first()).toBeVisible();
|
||||||
|
|
||||||
|
// Restored to roughly where we were, not reset to the top. Tolerance absorbs the virtualizer's
|
||||||
|
// estimate-vs-measured row heights differing slightly between the two mounts.
|
||||||
|
await expect
|
||||||
|
.poll(() => page.getByRole("main").evaluate((el) => el.scrollTop), { timeout: 5000 })
|
||||||
|
.toBeGreaterThan(saved - 500);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -47,4 +47,35 @@ test.describe("feed", () => {
|
|||||||
.poll(async () => (await renderedIds()).some((id) => id && !before.includes(id)))
|
.poll(async () => (await renderedIds()).some((id) => id && !before.includes(id)))
|
||||||
.toBe(true);
|
.toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Bug 4 (Platform S4): the toolbar (show/content chips, count, sort, view) is fixed chrome — it
|
||||||
|
// pins in the shell's toolbar band above the scroller, so it stays put while the cards scroll.
|
||||||
|
test("the feed toolbar stays pinned while cards scroll", async ({ page }) => {
|
||||||
|
await openApp(page);
|
||||||
|
const count = page.getByTestId("feed-result-count");
|
||||||
|
await expect(count).toBeVisible();
|
||||||
|
const topBefore = await count.evaluate((el) => el.getBoundingClientRect().top);
|
||||||
|
|
||||||
|
await page.getByRole("main").evaluate((el) => el.scrollBy(0, 4000));
|
||||||
|
|
||||||
|
await expect(count).toBeInViewport();
|
||||||
|
const topAfter = await count.evaluate((el) => el.getBoundingClientRect().top);
|
||||||
|
expect(Math.abs(topAfter - topBefore)).toBeLessThan(4);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Bug 3 (Platform S4): changing a filter is a fresh result set, so the scroll jumps back to the
|
||||||
|
// top rather than stranding you deep in the old list's offset.
|
||||||
|
test("a new filter resets the scroll to the top", async ({ page }) => {
|
||||||
|
await openApp(page);
|
||||||
|
await expect(page.getByTestId("video-card").first()).toBeVisible();
|
||||||
|
|
||||||
|
await page.getByRole("main").evaluate((el) => el.scrollBy(0, 4000));
|
||||||
|
expect(await page.getByRole("main").evaluate((el) => el.scrollTop)).toBeGreaterThan(500);
|
||||||
|
|
||||||
|
// Switch the show filter → a different result set → back to the top.
|
||||||
|
await page.getByTestId("feed-show-all").click();
|
||||||
|
await expect
|
||||||
|
.poll(() => page.getByRole("main").evaluate((el) => el.scrollTop))
|
||||||
|
.toBeLessThan(50);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
+13
-1
@@ -632,6 +632,17 @@ export default function App() {
|
|||||||
<div className="flex-1 grid place-items-center text-muted">{t("common.loading")}</div>
|
<div className="flex-1 grid place-items-center text-muted">{t("common.loading")}</div>
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// Scroll restore/reset key for the shared normal-page scroller (see PageScroller): the same
|
||||||
|
// page/feed-filters returns you to where you were (bug 1); a changed feed filter is a new key, so
|
||||||
|
// the fresh result set starts at the top (bug 3). `q` is normalised out so typing in the search
|
||||||
|
// box doesn't reset the scroll on every keystroke; the live-YouTube sub-view gets its own key.
|
||||||
|
const normalScrollKey =
|
||||||
|
page !== "feed"
|
||||||
|
? `page:${page}`
|
||||||
|
: ytSearch != null
|
||||||
|
? `yt:${ytSearch}`
|
||||||
|
: `feed:${JSON.stringify({ ...filters, q: "" })}`;
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div className="h-screen flex text-fg">
|
<div className="h-screen flex text-fg">
|
||||||
<NavSidebar
|
<NavSidebar
|
||||||
@@ -689,7 +700,7 @@ export default function App() {
|
|||||||
)}
|
)}
|
||||||
<div className="relative flex-1 min-w-0 flex flex-col">
|
<div className="relative flex-1 min-w-0 flex flex-col">
|
||||||
{channelView ? (
|
{channelView ? (
|
||||||
<PageShell>
|
<PageShell scrollKey={`channel:${channelView.id}`}>
|
||||||
<Suspense fallback={pageFallback}>
|
<Suspense fallback={pageFallback}>
|
||||||
<ChannelPage
|
<ChannelPage
|
||||||
key={channelView.id}
|
key={channelView.id}
|
||||||
@@ -718,6 +729,7 @@ export default function App() {
|
|||||||
</PageShell>
|
</PageShell>
|
||||||
) : (
|
) : (
|
||||||
<PageShell
|
<PageShell
|
||||||
|
scrollKey={normalScrollKey}
|
||||||
header={
|
header={
|
||||||
<Header
|
<Header
|
||||||
me={meQuery.data!}
|
me={meQuery.data!}
|
||||||
|
|||||||
@@ -1,10 +1,39 @@
|
|||||||
import { type ReactNode } from "react";
|
import { useCallback, useLayoutEffect, useRef, type ReactNode } from "react";
|
||||||
import { useScrollFade } from "../lib/useScrollFade";
|
import { useScrollFade } from "../lib/useScrollFade";
|
||||||
|
|
||||||
// The page's scroll container. Two of these exist and are never both mounted: App renders one for
|
// Per-view saved scroll offsets. Module-level so a view's position survives its scroller
|
||||||
// the normal pages, ChannelPage renders its own (it's a separate branch of App's tree, not a page
|
// unmounting — opening a channel page swaps in a different shell/scroller — and is restored when
|
||||||
// inside the usual one). The feed, the channel page and Plex browse all scroll inside it rather
|
// the same view returns. Keyed by the shell's `scrollKey`.
|
||||||
// than owning a scroller — which is why the edge-fade lives here once instead of three times.
|
const savedScroll = new Map<string, number>();
|
||||||
|
|
||||||
|
// Put the scroller back to `top`. The feed is virtualized, so on a remount the list needs a frame
|
||||||
|
// or two to render enough rows to reach its full height; an immediate `scrollTop` would otherwise
|
||||||
|
// clamp short. Re-apply over a few frames until it sticks (or the content is simply shorter now).
|
||||||
|
function restoreScroll(el: HTMLElement, top: number): () => void {
|
||||||
|
if (top <= 0) {
|
||||||
|
el.scrollTop = 0;
|
||||||
|
return () => {};
|
||||||
|
}
|
||||||
|
let raf = 0;
|
||||||
|
let frames = 0;
|
||||||
|
let cancelled = false;
|
||||||
|
const tick = () => {
|
||||||
|
if (cancelled) return;
|
||||||
|
if (el.scrollHeight - el.clientHeight >= top) el.scrollTop = top;
|
||||||
|
if (el.scrollTop >= top - 1 || frames++ > 90) return; // reached, or gave up after ~1.5s
|
||||||
|
raf = requestAnimationFrame(tick);
|
||||||
|
};
|
||||||
|
raf = requestAnimationFrame(tick);
|
||||||
|
return () => {
|
||||||
|
cancelled = true;
|
||||||
|
cancelAnimationFrame(raf);
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
// The page's scroll container — the ONE scroller in the app, owned by PageShell. The feed, the
|
||||||
|
// channel page and Plex browse all scroll inside it rather than owning a scroller of their own,
|
||||||
|
// which is why the edge-fade and the scroll restore/reset policy live here once instead of three
|
||||||
|
// times.
|
||||||
//
|
//
|
||||||
// Two invariants this element carries, both learned the hard way:
|
// Two invariants this element carries, both learned the hard way:
|
||||||
//
|
//
|
||||||
@@ -19,13 +48,61 @@ import { useScrollFade } from "../lib/useScrollFade";
|
|||||||
// the gutter and re-open that loop. Unlike the rail and the side panels, this scroller wears the
|
// the gutter and re-open that loop. Unlike the rail and the side panels, this scroller wears the
|
||||||
// fade next to its scrollbar, not instead of it.
|
// fade next to its scrollbar, not instead of it.
|
||||||
//
|
//
|
||||||
|
// Scroll restore/reset policy (`scrollKey` identifies the logical view):
|
||||||
|
// - returning to a view with the SAME key — feed → open a channel → Back, feed filters unchanged —
|
||||||
|
// restores where you were (bug 1). The channel page is a different scroller, so the feed's
|
||||||
|
// scroller unmounts and remounts; the saved offset survives in `savedScroll` and is re-applied.
|
||||||
|
// - a NEW key — the feed's filters changed, i.e. a fresh result set — starts at the top (bug 3).
|
||||||
|
// The offset is recorded on every scroll rather than at unmount: by the time an unmount cleanup
|
||||||
|
// runs the scroller's scrollTop has already been zeroed by the teardown, so a cleanup-time read
|
||||||
|
// saves 0 and the restore is lost. Recording live sidesteps that (and StrictMode's synthetic
|
||||||
|
// unmount, which would otherwise persist a 0 too).
|
||||||
|
//
|
||||||
// The fade state lives in here rather than in App so that a flip re-renders this component alone —
|
// The fade state lives in here rather than in App so that a flip re-renders this component alone —
|
||||||
// `children` arrives as an already-built element, so React skips the page subtree underneath.
|
// `children` arrives as an already-built element, so React skips the page subtree underneath.
|
||||||
export default function PageScroller({ children }: { children: ReactNode }) {
|
export default function PageScroller({
|
||||||
|
children,
|
||||||
|
scrollKey,
|
||||||
|
}: {
|
||||||
|
children: ReactNode;
|
||||||
|
scrollKey?: string;
|
||||||
|
}) {
|
||||||
const fade = useScrollFade();
|
const fade = useScrollFade();
|
||||||
|
const fadeRef = fade.ref;
|
||||||
|
const elRef = useRef<HTMLElement | null>(null);
|
||||||
|
// Stable identity: an inline ref callback changes every render, which makes React detach and
|
||||||
|
// re-attach the ref each commit — and since fade.ref is a state setter, that re-attach loops it
|
||||||
|
// into "Maximum update depth exceeded". useScrollFade's ref setter is itself stable, so binding
|
||||||
|
// once here is safe.
|
||||||
|
const setRef = useCallback(
|
||||||
|
(node: HTMLElement | null) => {
|
||||||
|
elRef.current = node;
|
||||||
|
fadeRef(node);
|
||||||
|
},
|
||||||
|
[fadeRef],
|
||||||
|
);
|
||||||
|
|
||||||
|
useLayoutEffect(() => {
|
||||||
|
const el = elRef.current;
|
||||||
|
if (!el || scrollKey == null) return;
|
||||||
|
const key = scrollKey;
|
||||||
|
const want = savedScroll.get(key) ?? 0;
|
||||||
|
const cancelRestore = restoreScroll(el, want);
|
||||||
|
// Record position continuously (not at unmount): the scroller's scrollTop is unreliable by the
|
||||||
|
// time an unmount cleanup runs, and a save-on-scroll survives whatever tears down last.
|
||||||
|
const onScroll = () => {
|
||||||
|
savedScroll.set(key, el.scrollTop);
|
||||||
|
};
|
||||||
|
el.addEventListener("scroll", onScroll, { passive: true });
|
||||||
|
return () => {
|
||||||
|
cancelRestore();
|
||||||
|
el.removeEventListener("scroll", onScroll);
|
||||||
|
};
|
||||||
|
}, [scrollKey]);
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<main
|
<main
|
||||||
ref={fade.ref}
|
ref={setRef}
|
||||||
style={fade.style}
|
style={fade.style}
|
||||||
className="flex-1 min-w-0 overflow-y-auto [scrollbar-gutter:stable]"
|
className="flex-1 min-w-0 overflow-y-auto [scrollbar-gutter:stable]"
|
||||||
>
|
>
|
||||||
|
|||||||
@@ -38,10 +38,13 @@ export default function PageShell({
|
|||||||
header,
|
header,
|
||||||
banners,
|
banners,
|
||||||
children,
|
children,
|
||||||
|
scrollKey,
|
||||||
}: {
|
}: {
|
||||||
header?: ReactNode;
|
header?: ReactNode;
|
||||||
banners?: ReactNode;
|
banners?: ReactNode;
|
||||||
children: ReactNode;
|
children: ReactNode;
|
||||||
|
// Identifies the logical view for the scroll restore/reset policy — see PageScroller.
|
||||||
|
scrollKey?: string;
|
||||||
}) {
|
}) {
|
||||||
const [toolbarSlot, setToolbarSlot] = useState<HTMLDivElement | null>(null);
|
const [toolbarSlot, setToolbarSlot] = useState<HTMLDivElement | null>(null);
|
||||||
return (
|
return (
|
||||||
@@ -50,7 +53,7 @@ export default function PageShell({
|
|||||||
<div className={`flex-1 min-w-0 min-h-0 flex flex-col ${header ? "pt-[var(--hdr-h)]" : ""}`}>
|
<div className={`flex-1 min-w-0 min-h-0 flex flex-col ${header ? "pt-[var(--hdr-h)]" : ""}`}>
|
||||||
{banners}
|
{banners}
|
||||||
<div ref={setToolbarSlot} className="shrink-0" />
|
<div ref={setToolbarSlot} className="shrink-0" />
|
||||||
<PageScroller>{children}</PageScroller>
|
<PageScroller scrollKey={scrollKey}>{children}</PageScroller>
|
||||||
</div>
|
</div>
|
||||||
</ToolbarSlot.Provider>
|
</ToolbarSlot.Provider>
|
||||||
);
|
);
|
||||||
|
|||||||
Reference in New Issue
Block a user