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 <noreply@anthropic.com>

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<string, { view: ViewLike; inited: boolean }>;
+  setPreviewMode: ReturnType<typeof vi.fn>;
+};
+// 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<typeof import('@/services/environment')>();
+  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 }));
+  });
+});

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=<hash>)
-  // 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<typeof useReaderStore.getState>) => {
+      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=<hash>) 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);

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);
     },