From 8c91ad411c5f1a6ce3ce086923458cd7348ec03b Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Fri, 3 Jul 2026 20:26:41 +0900 Subject: [PATCH] fix(reader): open annotation deep link when a different book is open (#4887) (#4910) An annotation deep link (readest://book/{hash}/annotation/{id}?cfi=...) for a book that is not the one currently shown in the reader was ignored: the reader stayed on the open book. It only worked from the library page. Two causes, both in the reader-mounted path: - useOpenAnnotationLink fell through to navigateToReader when the target book had no live view. router.push to the same /reader route does not re-run the reader's one-shot init effect, so it was a no-op and the book never changed. Route it through the in-place switch event (open-book-in-reader) carrying the cfi, mirroring useOpenBookLink. - The "already open, jump in place" check scanned all viewStates, which keep stale entries for books switched away from (their views are detached from the DOM, never cleared on switch). Switching A -> B -> A matched the stale A view and called goTo on a dead view. Scope the check to the currently displayed bookKeys instead. useBooksManager.openBookInReader now accepts an optional cfi and jumps to it once the switched-in view is ready (marking it a preview so the saved position is not overwritten). Co-authored-by: Claude Fable 5 --- .../hooks/useOpenAnnotationLink.test.ts | 137 ++++++++++++++++++ .../src/app/reader/hooks/useBooksManager.ts | 48 ++++-- .../src/hooks/useOpenAnnotationLink.ts | 31 +++- 3 files changed, 199 insertions(+), 17 deletions(-) create mode 100644 apps/readest-app/src/__tests__/hooks/useOpenAnnotationLink.test.ts diff --git a/apps/readest-app/src/__tests__/hooks/useOpenAnnotationLink.test.ts b/apps/readest-app/src/__tests__/hooks/useOpenAnnotationLink.test.ts new file mode 100644 index 00000000..8c7605a9 --- /dev/null +++ b/apps/readest-app/src/__tests__/hooks/useOpenAnnotationLink.test.ts @@ -0,0 +1,137 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { cleanup, renderHook } from '@testing-library/react'; + +const navigateToReaderMock = vi.fn(); +const getCurrentMock = vi.fn(async () => [] as string[]); + +const libraryState = { + libraryLoaded: true, + getBookByHash: (hash: string) => (hash.startsWith('book') ? { hash } : undefined), +}; + +type ViewLike = { goTo: (cfi: string) => void }; +type ReaderState = { + bookKeys: string[]; + viewStates: Record; + setPreviewMode: ReturnType; +}; +// A reader is mounted; bookKeys lists only the currently-displayed book(s). +// viewStates may carry stale entries for books switched away from. +const readerState: ReaderState = { + bookKeys: ['bookA-1'], + viewStates: {}, + setPreviewMode: vi.fn(), +}; + +vi.mock('@tauri-apps/plugin-deep-link', () => ({ + getCurrent: () => getCurrentMock(), +})); +vi.mock('@/services/environment', async (orig) => { + const actual = await orig(); + return { ...actual, isTauriAppPlatform: () => true }; +}); +vi.mock('@/context/EnvContext', () => ({ useEnv: () => ({ appService: {} }) })); +vi.mock('@/hooks/useTranslation', () => ({ useTranslation: () => (k: string) => k })); +vi.mock('next/navigation', () => ({ useRouter: () => ({ push: vi.fn() }) })); +vi.mock('@/utils/nav', () => ({ + navigateToReader: (...a: unknown[]) => navigateToReaderMock(...a), +})); +vi.mock('@/store/libraryStore', () => { + const useLibraryStore = ((selector: (s: typeof libraryState) => unknown) => + selector(libraryState)) as unknown as { + (selector: (s: typeof libraryState) => unknown): unknown; + getState: () => typeof libraryState; + }; + useLibraryStore.getState = () => libraryState; + return { useLibraryStore }; +}); +vi.mock('@/store/readerStore', () => { + const useReaderStore = (() => readerState) as unknown as { + (): ReaderState; + getState: () => ReaderState; + }; + useReaderStore.getState = () => readerState; + return { useReaderStore }; +}); + +import { useOpenAnnotationLink } from '@/hooks/useOpenAnnotationLink'; +import { eventDispatcher } from '@/utils/event'; + +const CFI = 'epubcfi(/6/4!/4/2)'; +const urlFor = (hash: string) => + `readest://book/${hash}/annotation/note1?cfi=${encodeURIComponent(CFI)}`; + +const collectSwitch = () => { + const switched = vi.fn(); + const handler = (e: Event) => switched((e as CustomEvent).detail); + eventDispatcher.on('open-book-in-reader', handler); + return { + switched, + stop: () => eventDispatcher.off('open-book-in-reader', handler), + }; +}; + +describe('useOpenAnnotationLink — reader already mounted', () => { + beforeEach(() => { + navigateToReaderMock.mockReset(); + readerState.setPreviewMode.mockReset(); + readerState.bookKeys = ['bookA-1']; + readerState.viewStates = { 'bookA-1': { view: { goTo: vi.fn() }, inited: true } }; + window.history.replaceState({}, '', '/reader?ids=bookA'); + }); + afterEach(() => { + cleanup(); + window.history.replaceState({}, '', '/'); + }); + + it('switches the book in place instead of calling the no-op navigateToReader', async () => { + const { switched, stop } = collectSwitch(); + renderHook(() => useOpenAnnotationLink()); + await eventDispatcher.dispatch('app-incoming-url', { urls: [urlFor('bookB')] }); + await Promise.resolve(); + stop(); + + // navigateToReader('/reader?ids=bookB&cfi=...') does not re-init an + // already-mounted reader, so the reader would stay on bookA. The fix routes + // through the in-place switch event carrying the target cfi. + expect(navigateToReaderMock).not.toHaveBeenCalled(); + expect(switched).toHaveBeenCalledWith(expect.objectContaining({ bookHash: 'bookB', cfi: CFI })); + }); + + it('jumps in place when the target book is the currently displayed one', async () => { + const goTo = vi.fn(); + readerState.bookKeys = ['bookB-1']; + readerState.viewStates = { 'bookB-1': { view: { goTo }, inited: true } }; + + renderHook(() => useOpenAnnotationLink()); + await eventDispatcher.dispatch('app-incoming-url', { urls: [urlFor('bookB')] }); + await Promise.resolve(); + + expect(goTo).toHaveBeenCalledWith(CFI); + expect(readerState.setPreviewMode).toHaveBeenCalledWith('bookB-1', true); + expect(navigateToReaderMock).not.toHaveBeenCalled(); + }); + + it('switches back to a book that was opened before but is no longer displayed (#4887)', async () => { + // Simulates opening A -> B -> A all by deep link. After A -> B, bookA-1 is + // still in viewStates with a now-detached view (never cleared on switch), + // while bookKeys reflects only bookB-2. A deep link back to book A must + // switch in place, NOT goTo the stale detached bookA view. + const staleGoTo = vi.fn(); + readerState.bookKeys = ['bookB-2']; + readerState.viewStates = { + 'bookA-1': { view: { goTo: staleGoTo }, inited: true }, + 'bookB-2': { view: { goTo: vi.fn() }, inited: true }, + }; + + const { switched, stop } = collectSwitch(); + renderHook(() => useOpenAnnotationLink()); + await eventDispatcher.dispatch('app-incoming-url', { urls: [urlFor('bookA')] }); + await Promise.resolve(); + stop(); + + expect(staleGoTo).not.toHaveBeenCalled(); + expect(navigateToReaderMock).not.toHaveBeenCalled(); + expect(switched).toHaveBeenCalledWith(expect.objectContaining({ bookHash: 'bookA', cfi: CFI })); + }); +}); diff --git a/apps/readest-app/src/app/reader/hooks/useBooksManager.ts b/apps/readest-app/src/app/reader/hooks/useBooksManager.ts index 7211831b..e98ae054 100644 --- a/apps/readest-app/src/app/reader/hooks/useBooksManager.ts +++ b/apps/readest-app/src/app/reader/hooks/useBooksManager.ts @@ -42,16 +42,45 @@ const useBooksManager = () => { setShouldUpdateSearchParams(true); }; - // Open a book in-place when the widget taps a book while a reader is already - // mounted. REPLACE the open book(s) with the tapped one (single ids=) - // rather than appending: appending produced ids=a+b which, with the OS - // re-delivering the launch deep link, looped. The store update renders the - // new book immediately; closing the previous key follows the same path as - // dismissBook. - const openBookInReader = (bookHash: string) => { + // Jump the switched-in book to a deep-link cfi (#4887) once its view has + // finished initing. The freshly-opened FoliateView first lands on the saved + // reading position, so wait for `inited` and then goTo; mark it a preview so + // the saved position is not overwritten. The subscription cleans itself up on + // success or on load failure. + const goToCfiWhenReady = (bookKey: string, cfi: string) => { + const jump = () => { + const { getView, setPreviewMode } = useReaderStore.getState(); + getView(bookKey)?.goTo(cfi); + setPreviewMode(bookKey, true); + }; + const ready = (state: ReturnType) => { + const vs = state.viewStates[bookKey]; + return { done: !!vs?.error || (!!vs?.inited && !!vs?.view), ok: !!vs?.inited && !!vs?.view }; + }; + const initial = ready(useReaderStore.getState()); + if (initial.done) { + if (initial.ok) jump(); + return; + } + const unsub = useReaderStore.subscribe((state) => { + const { done, ok } = ready(state); + if (!done) return; + unsub(); + if (ok) jump(); + }); + }; + + // Open a book in-place when a widget/deep link targets a book while a reader + // is already mounted. REPLACE the open book(s) with the target one (single + // ids=) rather than appending: appending produced ids=a+b which, with + // the OS re-delivering the launch deep link, looped. The store update renders + // the new book immediately; closing the previous key follows the same path as + // dismissBook. An optional cfi (annotation deep link) is applied once ready. + const openBookInReader = (bookHash: string, cfi?: string) => { const existing = bookKeys.find((key) => key.startsWith(bookHash)); if (existing) { setSideBarBookKey(existing); + if (cfi) goToCfiWhenReady(existing, cfi); return; } const newKey = `${bookHash}-${uniqueId()}`; @@ -59,6 +88,7 @@ const useBooksManager = () => { setBookKeys([newKey]); setSideBarBookKey(newKey); setShouldUpdateSearchParams(true); + if (cfi) goToCfiWhenReady(newKey, cfi); }; // Stable ref so the listener calls the latest closure without re-subscribing. @@ -66,8 +96,8 @@ const useBooksManager = () => { openBookRef.current = openBookInReader; useEffect(() => { const handle = (event: CustomEvent) => { - const { bookHash } = event.detail as { bookHash: string }; - openBookRef.current(bookHash); + const { bookHash, cfi } = event.detail as { bookHash: string; cfi?: string }; + openBookRef.current(bookHash, cfi); }; eventDispatcher.on('open-book-in-reader', handle); return () => eventDispatcher.off('open-book-in-reader', handle); diff --git a/apps/readest-app/src/hooks/useOpenAnnotationLink.ts b/apps/readest-app/src/hooks/useOpenAnnotationLink.ts index 0fe511df..059614d4 100644 --- a/apps/readest-app/src/hooks/useOpenAnnotationLink.ts +++ b/apps/readest-app/src/hooks/useOpenAnnotationLink.ts @@ -62,19 +62,34 @@ export function useOpenAnnotationLink() { return; } - const { viewStates, setPreviewMode } = useReaderStore.getState(); - const openEntry = Object.entries(viewStates).find( - ([key, state]) => key.startsWith(bookHash) && state.view, - ); - if (openEntry) { - const [bookKey, state] = openEntry; + // Only books in the current bookKeys are actually displayed. viewStates + // accumulates stale entries for books switched away from in place (their + // views are detached from the DOM, never cleared on switch), so matching + // against all viewStates would goTo a dead view and the reader would stay + // stuck on the current book when switching back to a previously-open one + // (#4887). Scope the "already open" check to the displayed bookKeys. + const { viewStates, bookKeys, setPreviewMode } = useReaderStore.getState(); + const openKey = bookKeys.find((key) => key.startsWith(bookHash) && viewStates[key]?.view); + if (openKey) { if (cfi) { - state.view!.goTo(cfi); - setPreviewMode(bookKey, true); + viewStates[openKey]!.view!.goTo(cfi); + setPreviewMode(openKey, true); } return; } + // A reader is already mounted showing a different book. router.push to the + // same /reader route does NOT re-run the reader's one-shot init effect, so + // navigateToReader is a no-op and the reader stays on the current book + // (#4887). Switch the book in place via useBooksManager, carrying the cfi + // so the freshly-opened view jumps to the annotation once it is ready. + if (window.location.pathname.startsWith('/reader')) { + eventDispatcher.dispatch('open-book-in-reader', { bookHash, cfi }); + return; + } + + // No reader mounted (library / cold start) - navigate fresh; FoliateViewer + // reads ?cfi= at init. const queryParams = cfi ? `cfi=${encodeURIComponent(cfi)}` : undefined; navigateToReader(router, [bookHash], queryParams); },