Fix external scrRef change-related issues (UNFINISHED)

Author: alex-rawlings-yyc

cspell.json

@@ -49,6 +49,7 @@
     "recentering",
     "recenters",
     "relayout",
+    "resnap",
     "sandboxed",
     "scriptio",
     "scrollers",

src/__tests__/components/ContinuousView.test.tsx

@@ -524,6 +524,71 @@ describe('ContinuousView initial render', () => {
     const focusedBox = screen.getByText('beginning').closest('[data-phrase-box="true"]');
     expect(focusedBox).toHaveAttribute('data-focus-state', 'focused');
   });
+
+  it('falls back to focusedTokenRef when the lagging displayed ref is from another book', () => {
+    // During a book change displayFocusedTokenRef lags by one fade, so it briefly names a token from
+    // the previous book that no longer exists in the new book. The focus must follow the live
+    // focusedTokenRef (the new book's active verse) rather than collapsing to the book's first phrase.
+    const book = makeBook();
+    const { rerender } = render(
+      <ContinuousView {...requiredProps(book, { focusedTokenRef: 'tok-2' })} />,
+      withAnalysisStore,
+    );
+
+    // Swap to a different book whose token refs share none of the previous book's. The displayed ref
+    // ('tok-2') is now absent; focusedTokenRef points at the new book's *second* phrase.
+    const otherBook: Book = {
+      id: 'MAT',
+      bookRef: 'MAT',
+      textVersion: '1',
+      segments: [
+        {
+          id: 'MAT 1:1',
+          startRef: { book: 'MAT', chapter: 1, verse: 1 },
+          endRef: { book: 'MAT', chapter: 1, verse: 1 },
+          baselineText: 'Alpha',
+          tokens: [
+            {
+              ref: 'mat-tok-0',
+              surfaceText: 'Alpha',
+              writingSystem: 'en',
+              type: 'word',
+              charStart: 0,
+              charEnd: 5,
+            },
+          ],
+        },
+        {
+          id: 'MAT 1:2',
+          startRef: { book: 'MAT', chapter: 1, verse: 2 },
+          endRef: { book: 'MAT', chapter: 1, verse: 2 },
+          baselineText: 'Beta',
+          tokens: [
+            {
+              ref: 'mat-tok-1',
+              surfaceText: 'Beta',
+              writingSystem: 'en',
+              type: 'word',
+              charStart: 0,
+              charEnd: 4,
+            },
+          ],
+        },
+      ],
+    };
+
+    scrollIntoViewMock.mockClear();
+    rerender(<ContinuousView {...requiredProps(otherBook, { focusedTokenRef: 'mat-tok-1' })} />);
+
+    // The scroll target is resolved through focusPhraseIndex, which falls back to focusedTokenRef
+    // ('mat-tok-1', the second phrase) rather than collapsing to phrase 0. So the element scrolled
+    // into view is the one containing "Beta", never "Alpha".
+    const scrolledTexts = scrollIntoViewMock.mock.contexts.map((el) =>
+      el instanceof HTMLElement ? el.textContent : undefined,
+    );
+    expect(scrolledTexts.some((t) => t?.includes('Beta'))).toBe(true);
+    expect(scrolledTexts.some((t) => t?.includes('Alpha'))).toBe(false);
+  });
 });
 
 // ---------------------------------------------------------------------------

src/__tests__/components/Interlinearizer.test.tsx

@@ -654,17 +654,25 @@ describe('Interlinearizer', () => {
     ).not.toBeInTheDocument();
   });
 
-  it('snap button calls scrollIntoView on the active segment', () => {
+  it('snap button fades, recenters, then scrolls the active segment to the top', () => {
+    jest.useFakeTimers();
     renderInterlinearizer({ book: GEN_1_1_BOOK });
 
     act(() => {
       screen.getByRole('button', { name: /scroll to active verse/i }).click();
     });
 
+    // The button always fade-recenters (so a verse outside the window still comes into view), so the
+    // snap only lands after the fade timeout rebuilds the window behind the curtain.
+    act(() => {
+      jest.advanceTimersByTime(RECENTER_FADE_MS);
+    });
+
     expect(Element.prototype.scrollIntoView).toHaveBeenCalledWith({
       behavior: 'auto',
       block: 'start',
     });
+    jest.useRealTimers();
   });
 
   it('leaves focusedTokenRef undefined when switching off continuousScroll with no strip position and no matching segment', () => {

src/__tests__/components/InterlinearizerLoader.test.tsx

@@ -237,14 +237,19 @@ jest.mock('../../components/modals/ProjectModals', () => ({
   },
 }));
 
-/** Returns a `useWebViewScrollGroupScrRef` hook stub fixed to GEN 1:1. */
-function makeScrollGroupHook() {
+/**
+ * Returns a `useWebViewScrollGroupScrRef` hook stub fixed to a scripture reference.
+ *
+ * @param ref - The reference the stub reports; defaults to GEN 1:1.
+ * @returns A hook returning `[ref, noop setScrRef, undefined scrollGroupId, noop setter]`.
+ */
+function makeScrollGroupHook(ref: SerializedVerseRef = defaultScrRef) {
   return (): [
     SerializedVerseRef,
     (r: SerializedVerseRef) => void,
     number | undefined,
     (id: number | undefined) => void,
-  ] => [defaultScrRef, () => {}, undefined, () => {}];
+  ] => [ref, () => {}, undefined, () => {}];
 }
 
 /**
@@ -356,6 +361,46 @@ describe('InterlinearizerLoader', () => {
     expect(screen.getByTestId('interlinearizer')).toBeInTheDocument();
   });
 
+  it('normalizes a chapter-level (verse 0) reference to verse 1 before passing it to Interlinearizer', () => {
+    render(
+      <InterlinearizerLoader
+        projectId={testProjectId}
+        useWebViewScrollGroupScrRef={makeScrollGroupHook({
+          book: 'GEN',
+          chapterNum: 3,
+          verseNum: 0,
+        })}
+        useWebViewState={makeWebViewState()}
+      />,
+    );
+
+    expect(capturedInterlinearizerProps?.scrRef).toEqual({
+      book: 'GEN',
+      chapterNum: 3,
+      verseNum: 1,
+    });
+  });
+
+  it('passes a verse-level reference through to Interlinearizer unchanged', () => {
+    render(
+      <InterlinearizerLoader
+        projectId={testProjectId}
+        useWebViewScrollGroupScrRef={makeScrollGroupHook({
+          book: 'GEN',
+          chapterNum: 3,
+          verseNum: 4,
+        })}
+        useWebViewState={makeWebViewState()}
+      />,
+    );
+
+    expect(capturedInterlinearizerProps?.scrRef).toEqual({
+      book: 'GEN',
+      chapterNum: 3,
+      verseNum: 4,
+    });
+  });
+
   it('shows Loading when book data has not arrived', () => {
     mockBookData({ book: undefined, isLoading: true });
     render(

src/__tests__/hooks/useSegmentWindow.test.ts

@@ -393,14 +393,88 @@ describe('useSegmentWindow', () => {
     act(() => rerender({ b: book, ref: { book: 'GEN', chapterNum: 1, verseNum: 50 } }));
     act(() => jest.advanceTimersByTime(RECENTER_FADE_MS));
 
-    // The synchronous layout-effect snap has fired once; the post-paint re-snap is still pending.
+    // The synchronous layout-effect snap has fired once; the post-paint re-snap loop is still pending.
     expect(scrollIntoView).toHaveBeenCalledTimes(1);
 
-    // Flushing the requestAnimationFrame re-snaps against the now-settled layout.
+    // Flushing the first animation frame re-snaps against the now-painted layout.
     act(() => jest.advanceTimersByTime(16));
     expect(scrollIntoView).toHaveBeenCalledTimes(2);
   });
 
+  it('keeps re-snapping across frames until the scroll position settles', () => {
+    const book = makeBook(60, 0);
+    const scrollIntoView = jest.fn();
+    Element.prototype.scrollIntoView = scrollIntoView;
+    const { container, rerender } = renderSegmentWindow(book, {
+      book: 'GEN',
+      chapterNum: 1,
+      verseNum: 1,
+    });
+
+    const active = document.createElement('div');
+    active.setAttribute('aria-current', 'true');
+    container.appendChild(active);
+
+    act(() => rerender({ b: book, ref: { book: 'GEN', chapterNum: 1, verseNum: 50 } }));
+    act(() => jest.advanceTimersByTime(RECENTER_FADE_MS));
+    // Layout-effect snap: 1.
+    expect(scrollIntoView).toHaveBeenCalledTimes(1);
+
+    // First frame: snaps (2) and records the resulting scrollTop, then schedules another frame
+    // because the position has not yet been observed twice.
+    act(() => jest.advanceTimersByTime(16));
+    expect(scrollIntoView).toHaveBeenCalledTimes(2);
+
+    // Second frame: snaps (3); scrollTop is unchanged from the prior frame (jsdom has no layout), so
+    // the loop sees the position settled and stops scheduling further frames.
+    act(() => jest.advanceTimersByTime(16));
+    expect(scrollIntoView).toHaveBeenCalledTimes(3);
+
+    // Further frames do not re-snap: the loop has terminated.
+    act(() => jest.advanceTimersByTime(16 * 5));
+    expect(scrollIntoView).toHaveBeenCalledTimes(3);
+  });
+
+  it('stops re-snapping at the frame cap when the scroll position never settles', () => {
+    const book = makeBook(60, 0);
+    const scrollIntoView = jest.fn();
+    Element.prototype.scrollIntoView = scrollIntoView;
+    const { container, rerender } = renderSegmentWindow(book, {
+      book: 'GEN',
+      chapterNum: 1,
+      verseNum: 1,
+    });
+
+    const active = document.createElement('div');
+    active.setAttribute('aria-current', 'true');
+    container.appendChild(active);
+
+    // Make scrollTop change on every read so the settle check never short-circuits the loop; only
+    // the frame cap can stop it.
+    let scrollTop = 0;
+    Object.defineProperty(container, 'scrollTop', {
+      configurable: true,
+      get: () => {
+        scrollTop += 1;
+        return scrollTop;
+      },
+      set: () => {
+        // Ignore writes; the getter drives the ever-changing value.
+      },
+    });
+
+    act(() => rerender({ b: book, ref: { book: 'GEN', chapterNum: 1, verseNum: 50 } }));
+    act(() => jest.advanceTimersByTime(RECENTER_FADE_MS));
+    // Layout-effect snap.
+    expect(scrollIntoView).toHaveBeenCalledTimes(1);
+
+    // Drive far more frames than the cap allows; the loop must stop after exactly the capped number
+    // of re-snaps rather than spinning forever.
+    act(() => jest.advanceTimersByTime(16 * 50));
+    // 1 layout-effect snap + the frame-capped re-snaps.
+    expect(scrollIntoView).toHaveBeenCalledTimes(1 + 20);
+  });
+
   it('does not re-snap on the initial mount, only after a recenter', () => {
     const book = makeBook(60, 0);
     const scrollIntoView = jest.fn();

src/components/ContinuousView.tsx

@@ -231,13 +231,22 @@ export default function ContinuousView({
   /**
    * Group index of the displayed focused token, or `0` when nothing is focused. Single source of
    * truth for scroll position, windowing, arrow disabled state, and per-group focus highlighting.
+   *
+   * During a book change `displayFocusedTokenRef` lags the new book by one fade (it only catches up
+   * when the fade timeout fires), so for a few frames it names a token from the previous book that
+   * no longer exists in this book's `groupIndexByTokenRef`. Falling straight back to `0` then parks
+   * the strip on the new book's very first phrase instead of the verse the user navigated to. Fall
+   * back to the live `focusedTokenRef` first — the parent reseeds it to the new book's active verse
+   * on the book change — so the transient lands on the intended verse rather than book start.
    */
   const focusPhraseIndex = useMemo(() => {
-    if (displayFocusedTokenRef === undefined) return 0;
-    const gi = groupIndexByTokenRef.get(displayFocusedTokenRef);
-    /* v8 ignore next -- gi is always defined when displayFocusedTokenRef is set */
-    return gi === undefined ? 0 : clampIndex(gi, phraseGroups.length);
-  }, [displayFocusedTokenRef, groupIndexByTokenRef, phraseGroups.length]);
+    const resolved =
+      (displayFocusedTokenRef !== undefined
+        ? groupIndexByTokenRef.get(displayFocusedTokenRef)
+        : undefined) ??
+      (focusedTokenRef !== undefined ? groupIndexByTokenRef.get(focusedTokenRef) : undefined);
+    return resolved === undefined ? 0 : clampIndex(resolved, phraseGroups.length);
+  }, [displayFocusedTokenRef, focusedTokenRef, groupIndexByTokenRef, phraseGroups.length]);
 
   const [isVisible, setIsVisible] = useState(false);
 

src/components/Interlinearizer.tsx

@@ -169,26 +169,36 @@ function InterlinearizerInner({
   }, []);
 
   /**
-   * Scrolls the element marked `aria-current="true"` inside the scroll container into view at the
-   * top of the list.
+   * Recenters the segment list on the active verse with the same fade-and-rebuild used for external
+   * navigation. Used by the LocateFixed button and the continuous-scroll mode switch. Always fades
+   * (even when the verse is already on screen) so the active verse is guaranteed to land in view —
+   * a plain `scrollIntoView` of an `aria-current` element silently no-ops when the verse is outside
+   * the render window, leaving the list parked wherever it was.
+   *
+   * `recenterOnActive` is captured via ref so this callback's identity stays stable.
    */
+  const recenterOnActiveRef = useRef<() => void>(() => undefined);
   const snapToActive = useCallback(() => {
-    const container = scrollContainerRef.current;
-    const active = container?.querySelector('[aria-current="true"]');
-    /* v8 ignore next -- active is always found when a verse is rendered; guard for empty lists */
-    active?.scrollIntoView({ behavior: 'auto', block: 'start' });
+    recenterOnActiveRef.current();
   }, []);
 
   // Scroll-anchored window into the full book's segment list. Spans chapters, grows/culls at the
   // scrolled edge, and recenters (with a fade) on the active verse when navigation arrives from
   // outside the list.
-  const { windowSegments, isFaded, displayScrRef, topSentinelRef, bottomSentinelRef } =
-    useSegmentWindow({
-      book,
-      scrRef,
-      scrollContainerRef,
-      internalNavRef,
-    });
+  const {
+    windowSegments,
+    isFaded,
+    displayScrRef,
+    topSentinelRef,
+    bottomSentinelRef,
+    recenterOnActive,
+  } = useSegmentWindow({
+    book,
+    scrRef,
+    scrollContainerRef,
+    internalNavRef,
+  });
+  recenterOnActiveRef.current = recenterOnActive;
 
   /** PhraseId currently hovered anywhere in the interlinearizer; shared across all SegmentViews. */
   const [hoveredPhraseId, setHoveredPhraseId] = useState<string | undefined>();
@@ -217,8 +227,15 @@ function InterlinearizerInner({
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [isRevert, updatePhrase, setPhraseMode]);
 
-  // Snap the segment list to the active verse when switching modes.
+  // Recenter the segment list on the active verse when switching between continuous and segment
+  // modes. Skips the initial mount: the window is already built centered on the anchor there, so a
+  // recenter would needlessly fade. Only an actual mode toggle should fade-and-recenter.
+  const didMountModeSwitchRef = useRef(false);
   useEffect(() => {
+    if (!didMountModeSwitchRef.current) {
+      didMountModeSwitchRef.current = true;
+      return;
+    }
     snapToActive();
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [continuousScroll]);
@@ -332,7 +349,7 @@ function InterlinearizerInner({
             <div className="tw:sticky tw:top-0 tw:z-10 tw:flex tw:justify-end tw:pointer-events-none">
               <button
                 aria-label="Scroll to active verse"
-                className="tw:rounded tw:p-1 tw:text-foreground tw:hover:bg-muted/50 tw:pointer-events-auto"
+                className="tw:rounded tw:p-1 tw:text-foreground tw:bg-background tw:hover:bg-muted/50 tw:pointer-events-auto"
                 tabIndex={-1}
                 onClick={snapToActive}
                 type="button"
@@ -358,6 +375,7 @@ function InterlinearizerInner({
                   focusedTokenRef={continuousScroll ? undefined : focusedTokenRef}
                   hoveredPhraseId={hoveredPhraseId}
                   isActive={
+                    seg.startRef.book === displayScrRef.book &&
                     seg.startRef.chapter === displayScrRef.chapterNum &&
                     seg.startRef.verse === displayScrRef.verseNum
                   }