diff --git a/.gitignore b/.gitignore index 72fa1aa1..658a4d42 100644 --- a/.gitignore +++ b/.gitignore @@ -49,6 +49,7 @@ fastlane/metadata/android/en-US/changelogs result* .playwright-mcp/ +.gstack .claude/worktrees .claude/settings.local.json diff --git a/apps/readest-app/src/__tests__/app/library/use-library-navigation.test.ts b/apps/readest-app/src/__tests__/app/library/use-library-navigation.test.ts deleted file mode 100644 index 6c7cf1a5..00000000 --- a/apps/readest-app/src/__tests__/app/library/use-library-navigation.test.ts +++ /dev/null @@ -1,147 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { renderHook, act } from '@testing-library/react'; - -vi.mock('next/navigation', () => ({ - useRouter: vi.fn(), -})); - -import { useRouter, ReadonlyURLSearchParams } from 'next/navigation'; -import { useLibraryNavigation } from '@/app/library/hooks/useLibraryNavigation'; - -function mockRouter() { - return { - push: vi.fn(), - replace: vi.fn(), - back: vi.fn(), - forward: vi.fn(), - refresh: vi.fn(), - prefetch: vi.fn(), - }; -} - -function makeSearchParams(query: string): ReadonlyURLSearchParams { - // URLSearchParams shares the read-only subset of methods we need. - return new URLSearchParams(query) as unknown as ReadonlyURLSearchParams; -} - -describe('useLibraryNavigation', () => { - beforeEach(() => { - vi.clearAllMocks(); - document.documentElement.removeAttribute('data-nav-direction'); - }); - - it('navigates to /library (no query) when going back to root from a group', () => { - // Regression: clicking the breadcrumb "All" button from /library?group=foo - // must call router.replace('/library'). Previously this navigation was - // wrapped through next-view-transitions' useTransitionRouter which is - // incompatible with Next.js 16.2 RSC navigation when the pathname stays - // the same and all query params are removed, causing the click to do - // nothing on the first attempt. See readest/readest#3782. - const router = mockRouter(); - vi.mocked(useRouter).mockReturnValue(router as never); - - const { result } = renderHook(() => useLibraryNavigation(makeSearchParams('group=foo'))); - - act(() => { - result.current(''); - }); - - expect(router.replace).toHaveBeenCalledTimes(1); - expect(router.replace).toHaveBeenCalledWith('/library', undefined); - }); - - it('preserves other search params when going back to root', () => { - const router = mockRouter(); - vi.mocked(useRouter).mockReturnValue(router as never); - - const { result } = renderHook(() => - useLibraryNavigation(makeSearchParams('groupBy=author&group=cf6342e&sort=title')), - ); - - act(() => { - result.current(''); - }); - - expect(router.replace).toHaveBeenCalledTimes(1); - const calledHref = router.replace.mock.calls[0]![0] as string; - expect(calledHref.startsWith('/library?')).toBe(true); - const calledParams = new URLSearchParams(calledHref.slice('/library?'.length)); - expect(calledParams.get('group')).toBeNull(); - expect(calledParams.get('groupBy')).toBe('author'); - expect(calledParams.get('sort')).toBe('title'); - }); - - it('navigates forward to a group by setting the group param', () => { - const router = mockRouter(); - vi.mocked(useRouter).mockReturnValue(router as never); - - const { result } = renderHook(() => useLibraryNavigation(makeSearchParams(''))); - - act(() => { - result.current('group-id-abc'); - }); - - expect(router.replace).toHaveBeenCalledWith('/library?group=group-id-abc', undefined); - }); - - it('replaces the group param when navigating between sibling groups', () => { - const router = mockRouter(); - vi.mocked(useRouter).mockReturnValue(router as never); - - const { result } = renderHook(() => useLibraryNavigation(makeSearchParams('group=group-a'))); - - act(() => { - result.current('group-b'); - }); - - expect(router.replace).toHaveBeenCalledWith('/library?group=group-b', undefined); - }); - - it('sets data-nav-direction to "back" when returning to root from a group', () => { - const router = mockRouter(); - vi.mocked(useRouter).mockReturnValue(router as never); - - const { result } = renderHook(() => useLibraryNavigation(makeSearchParams('group=foo'))); - - act(() => { - result.current(''); - }); - - expect(document.documentElement.getAttribute('data-nav-direction')).toBe('back'); - }); - - it('sets data-nav-direction to "forward" when entering a group', () => { - const router = mockRouter(); - vi.mocked(useRouter).mockReturnValue(router as never); - - const { result } = renderHook(() => useLibraryNavigation(makeSearchParams(''))); - - act(() => { - result.current('group-id'); - }); - - expect(document.documentElement.getAttribute('data-nav-direction')).toBe('forward'); - }); - - it('invokes onBeforeNavigate with the current group before navigating', () => { - const router = mockRouter(); - vi.mocked(useRouter).mockReturnValue(router as never); - const onBeforeNavigate = vi.fn(); - - const { result } = renderHook(() => - useLibraryNavigation(makeSearchParams('group=foo'), onBeforeNavigate), - ); - - act(() => { - result.current(''); - }); - - expect(onBeforeNavigate).toHaveBeenCalledTimes(1); - expect(onBeforeNavigate).toHaveBeenCalledWith('foo'); - // Order matters: callback should run before router.replace so callers can - // capture state (e.g. scroll position) of the leaving view. - expect(onBeforeNavigate.mock.invocationCallOrder[0]).toBeLessThan( - router.replace.mock.invocationCallOrder[0]!, - ); - }); -}); diff --git a/apps/readest-app/src/app/library/hooks/useLibraryNavigation.ts b/apps/readest-app/src/app/library/hooks/useLibraryNavigation.ts deleted file mode 100644 index c4f8ea18..00000000 --- a/apps/readest-app/src/app/library/hooks/useLibraryNavigation.ts +++ /dev/null @@ -1,51 +0,0 @@ -import { useCallback } from 'react'; -import { ReadonlyURLSearchParams, useRouter } from 'next/navigation'; - -import { navigateToLibrary } from '@/utils/nav'; - -/** - * Hook for navigating between library views (group/subgroup/root) while - * setting a `data-nav-direction` attribute used by the directional view - * transition CSS. - * - * NOTE: This hook intentionally uses the plain Next.js `useRouter` instead of - * `useAppRouter` (which wraps router calls in `next-view-transitions`'s - * `useTransitionRouter`). The wrapped router is incompatible with Next.js - * 16.2's RSC navigation when only the search params change for the same - * pathname (e.g. `/library?group=foo` -> `/library`), which previously caused - * the breadcrumb "All" button to do nothing on the first click after entering - * a group. See https://github.com/readest/readest/issues/3782 and - * https://github.com/shuding/next-view-transitions/issues/65. - */ -export function useLibraryNavigation( - searchParams: ReadonlyURLSearchParams | null, - onBeforeNavigate?: (currentGroup: string) => void, -) { - const router = useRouter(); - - return useCallback( - (targetGroup: string) => { - const currentGroup = searchParams?.get('group') || ''; - - onBeforeNavigate?.(currentGroup); - - // Detect and set navigation direction so the view transition CSS can - // animate forward (entering a group) vs back (returning to a parent or - // the root) using the appropriate slide direction. - const direction = currentGroup && !targetGroup ? 'back' : 'forward'; - if (typeof document !== 'undefined') { - document.documentElement.setAttribute('data-nav-direction', direction); - } - - const params = new URLSearchParams(searchParams?.toString()); - if (targetGroup) { - params.set('group', targetGroup); - } else { - params.delete('group'); - } - - navigateToLibrary(router, `${params.toString()}`); - }, - [searchParams, router, onBeforeNavigate], - ); -} diff --git a/apps/readest-app/src/app/library/page.tsx b/apps/readest-app/src/app/library/page.tsx index 16b4821d..4be5d379 100644 --- a/apps/readest-app/src/app/library/page.tsx +++ b/apps/readest-app/src/app/library/page.tsx @@ -62,7 +62,6 @@ import { BackupWindow } from './components/BackupWindow'; import { useDragDropImport } from './hooks/useDragDropImport'; import { useTransferQueue } from '@/hooks/useTransferQueue'; import { useAppRouter } from '@/hooks/useAppRouter'; -import { useLibraryNavigation } from './hooks/useLibraryNavigation'; import { Toast } from '@/components/Toast'; import { createBookGroups, @@ -152,16 +151,6 @@ const LibraryPageContent = ({ searchParams }: { searchParams: ReadonlyURLSearchP } }, []); - // Unified navigation function that handles scroll position and direction. - // Uses a plain Next.js router (via useLibraryNavigation) instead of the - // view-transition-wrapped useAppRouter, because next-view-transitions@0.3.5 - // is incompatible with Next.js 16.2 RSC navigation when only search params - // change for the same pathname (e.g. /library?group=foo -> /library), which - // previously broke the breadcrumb "All" button on the first click after - // entering a group. See https://github.com/readest/readest/issues/3782 and - // https://github.com/shuding/next-view-transitions/issues/65. - const handleLibraryNavigation = useLibraryNavigation(searchParams, saveScrollPosition); - useTheme({ systemUIVisible: true, appThemeColor: 'base-200' }); useUICSS(); @@ -206,6 +195,55 @@ const LibraryPageContent = ({ searchParams }: { searchParams: ReadonlyURLSearchP sessionStorage.setItem('lastLibraryParams', searchParams?.toString() || ''); }, [searchParams]); + // Strip the empty `group=` param that `handleLibraryNavigation` sets as a + // workaround for a Next.js 16.2 static-export regression (see the NOTE + // above `handleLibraryNavigation` for full context). This effect runs + // after the router.replace() has committed, so React has already + // re-rendered with the new (empty) group state; we're only rewriting the + // URL cosmetically via window.history.replaceState — Next.js' patched + // replaceState will pick up the new canonical URL without triggering + // another navigation. + useEffect(() => { + if (typeof window === 'undefined') return; + if (searchParams?.get('group') !== '') return; + const url = new URL(window.location.href); + url.searchParams.delete('group'); + const cleanHref = `${url.pathname}${url.search}${url.hash}`; + window.history.replaceState(null, '', cleanHref); + }, [searchParams]); + + // Unified navigation function that handles scroll position and direction. + // Workaround for a Next.js 16.2 static-export regression: navigating to a + // same-pathname URL with an empty search string causes `router.replace()` + // to silently no-op (e.g. `/library?group=foo` -> `/library`), which broke + // the breadcrumb "All" button. By always calling `params.set('group', + // targetGroup)` — including when `targetGroup` is an empty string — the + // resulting URL becomes `/library?group=` instead of `/library`, which + // Next.js does commit. The trailing empty `group=` is stripped via a + // cleanup effect below (purely cosmetic URL rewrite). See + // https://github.com/readest/readest/issues/3782. + const handleLibraryNavigation = useCallback( + (targetGroup: string) => { + const currentGroup = searchParams?.get('group') || ''; + + // Save current scroll position BEFORE navigation + saveScrollPosition(currentGroup); + + // Detect and set navigation direction + const direction = currentGroup && !targetGroup ? 'back' : 'forward'; + document.documentElement.setAttribute('data-nav-direction', direction); + + // Build query params — always `set` so the search string is non-empty + // even when targetGroup is '' (the Next.js 16.2 workaround). + const params = new URLSearchParams(searchParams?.toString()); + params.set('group', targetGroup); + + navigateToLibrary(router, `${params.toString()}`); + }, + // eslint-disable-next-line react-hooks/exhaustive-deps + [searchParams, router], + ); + useEffect(() => { const doCheckAppUpdates = async () => { if (appService?.hasUpdater && settings.autoCheckUpdates) {