fix(post): stop thread navigation from forcing OP alignment (#1052)

* Update README.md

* fix(post): stop thread navigation from forcing OP alignment

Route-driven thread opens now use the normal top-of-page behavior, while explicit OP permalink intents carry a scrollThreadContainerCid state and resolve against the visible thread container only. The old spacer injection path is removed.

* fix(post): address PR review follow-ups

src/lib/utils/__tests__/thread-scroll-utils.test.ts

@@ -1,9 +1,12 @@
-import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
-import { clearThreadScrollSpacer, openThreadAtTop } from '../thread-scroll-utils';
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { getRequestedThreadTopCid, getThreadTopNavigationState, scrollThreadContainerToTop } from '../thread-scroll-utils';
 
-const appendThreadContainer = (cid: string, top = 240) => {
+const appendThreadContainer = ({ cid, top = 240, hidden = false, parent = document.body }: { cid: string; top?: number; hidden?: boolean; parent?: HTMLElement }) => {
   const element = document.createElement('div');
   element.dataset.threadContainerCid = cid;
+  if (hidden) {
+    element.style.display = 'none';
+  }
   element.getBoundingClientRect = () =>
     ({
       bottom: top + 100,
@@ -13,110 +16,87 @@ const appendThreadContainer = (cid: string, top = 240) => {
       top,
       width: 100,
     }) as DOMRect;
-  document.body.appendChild(element);
+  parent.appendChild(element);
   return element;
 };
 
 describe('thread-scroll-utils', () => {
   let scrollToMock: ReturnType<typeof vi.fn>;
-  let requestAnimationFrameMock: ReturnType<typeof vi.fn>;
 
   beforeEach(() => {
     vi.clearAllMocks();
 
     scrollToMock = vi.fn();
-    requestAnimationFrameMock = vi.fn((callback: FrameRequestCallback) => {
-      callback(0);
-      return 1;
-    });
 
     Object.defineProperty(window, 'scrollTo', {
       configurable: true,
       value: scrollToMock,
       writable: true,
     });
-    Object.defineProperty(window, 'requestAnimationFrame', {
-      configurable: true,
-      value: requestAnimationFrameMock,
-      writable: true,
-    });
     Object.defineProperty(window, 'scrollY', {
       configurable: true,
-      value: 0,
+      value: 32,
       writable: true,
     });
-    Object.defineProperty(window, 'innerHeight', {
-      configurable: true,
-      value: 100,
-      writable: true,
-    });
-    Object.defineProperty(document.documentElement, 'scrollHeight', {
-      configurable: true,
-      value: 200,
-      writable: true,
-    });
-  });
 
-  afterEach(() => {
-    clearThreadScrollSpacer();
     document.body.innerHTML = '';
   });
 
-  it('scrolls the current thread without pushing duplicate history entries', () => {
-    appendThreadContainer('thread-cid');
-    const navigateMock = vi.fn();
+  it('scrolls the matching thread container with a plain window scroll', () => {
+    appendThreadContainer({ cid: 'thread-cid', top: 240 });
 
-    expect(
-      openThreadAtTop({
-        cid: 'thread-cid',
-        currentPathname: '/mu/thread/thread-cid',
-        navigate: navigateMock,
-        threadRoute: '/mu/thread/thread-cid',
-      }),
-    ).toBe(true);
+    expect(scrollThreadContainerToTop('thread-cid')).toBe(true);
 
-    expect(navigateMock).not.toHaveBeenCalled();
-    expect(requestAnimationFrameMock).toHaveBeenCalledOnce();
-    expect(scrollToMock).toHaveBeenCalledTimes(2);
-    expect(scrollToMock).toHaveBeenNthCalledWith(1, {
+    expect(scrollToMock).toHaveBeenCalledOnce();
+    expect(scrollToMock).toHaveBeenCalledWith({
       behavior: 'auto',
       left: 0,
-      top: 240,
+      top: 272,
     });
   });
 
-  it('navigates to the OP thread before scrolling when the route differs', () => {
-    appendThreadContainer('thread-cid');
-    const navigateMock = vi.fn();
+  it('ignores preview copies when resolving the scroll target', () => {
+    const previewWrapper = document.createElement('div');
+    previewWrapper.dataset.threadScrollPreview = 'true';
+    document.body.appendChild(previewWrapper);
+    appendThreadContainer({ cid: 'thread-cid', top: 12, parent: previewWrapper });
+    appendThreadContainer({ cid: 'thread-cid', top: 180 });
 
-    expect(
-      openThreadAtTop({
-        cid: 'thread-cid',
-        currentPathname: '/mu/thread/reply-cid',
-        navigate: navigateMock,
-        threadRoute: '/mu/thread/thread-cid',
-      }),
-    ).toBe(true);
+    expect(scrollThreadContainerToTop('thread-cid')).toBe(true);
 
-    expect(navigateMock).toHaveBeenCalledWith('/mu/thread/thread-cid');
-    expect(requestAnimationFrameMock).toHaveBeenCalledOnce();
-    expect(scrollToMock).toHaveBeenCalledTimes(2);
+    expect(scrollToMock).toHaveBeenCalledWith({
+      behavior: 'auto',
+      left: 0,
+      top: 212,
+    });
   });
 
-  it('returns false when the permalink cannot resolve a thread target', () => {
-    const navigateMock = vi.fn();
+  it('prefers the visible thread container when cached duplicates are hidden', () => {
+    appendThreadContainer({ cid: 'thread-cid', top: 12, hidden: true });
+    appendThreadContainer({ cid: 'thread-cid', top: 180 });
 
-    expect(
-      openThreadAtTop({
-        cid: undefined,
-        currentPathname: '/mu/thread/thread-cid',
-        navigate: navigateMock,
-        threadRoute: '/mu/thread/thread-cid',
-      }),
-    ).toBe(false);
+    expect(scrollThreadContainerToTop('thread-cid')).toBe(true);
 
-    expect(navigateMock).not.toHaveBeenCalled();
-    expect(requestAnimationFrameMock).not.toHaveBeenCalled();
+    expect(scrollToMock).toHaveBeenCalledWith({
+      behavior: 'auto',
+      left: 0,
+      top: 212,
+    });
+  });
+
+  it('returns false when no thread container exists', () => {
+    expect(scrollThreadContainerToTop('missing-cid')).toBe(false);
     expect(scrollToMock).not.toHaveBeenCalled();
   });
+
+  it('serializes and reads thread-top navigation state', () => {
+    expect(getThreadTopNavigationState()).toBeUndefined();
+    expect(getThreadTopNavigationState('thread-cid')).toEqual({
+      scrollThreadContainerCid: 'thread-cid',
+    });
+    expect(getRequestedThreadTopCid(getThreadTopNavigationState())).toBeUndefined();
+    expect(getRequestedThreadTopCid({ scrollThreadContainerCid: 'thread-cid' })).toBe('thread-cid');
+    expect(getRequestedThreadTopCid({ scrollThreadContainerCid: 42 })).toBeUndefined();
+    expect(getRequestedThreadTopCid(null)).toBeUndefined();
+  });
 });

src/lib/utils/thread-scroll-utils.ts

@@ -1,43 +1,42 @@
-const THREAD_SCROLL_SPACER_ID = 'thread-scroll-spacer';
+const THREAD_SCROLL_PREVIEW_SELECTOR = '[data-thread-scroll-preview="true"]';
 
-const setThreadScrollSpacerHeight = (height: number) => {
-  const existingSpacer = document.getElementById(THREAD_SCROLL_SPACER_ID);
-  if (height <= 0) {
-    existingSpacer?.remove();
-    return;
-  }
-
-  const spacer =
-    existingSpacer ||
-    Object.assign(document.createElement('div'), {
-      id: THREAD_SCROLL_SPACER_ID,
-    });
-
-  spacer.setAttribute('aria-hidden', 'true');
-  spacer.style.height = `${Math.ceil(height)}px`;
-  spacer.style.pointerEvents = 'none';
-  spacer.style.opacity = '0';
-
-  if (!existingSpacer) {
-    document.body.appendChild(spacer);
-  }
+type ThreadTopNavigationState = {
+  scrollThreadContainerCid?: string;
 };
 
-export const clearThreadScrollSpacer = () => {
-  document.getElementById(THREAD_SCROLL_SPACER_ID)?.remove();
+export const getThreadTopNavigationState = (cid?: string): ThreadTopNavigationState | undefined =>
+  cid
+    ? {
+        scrollThreadContainerCid: cid,
+      }
+    : undefined;
+
+export const getRequestedThreadTopCid = (state: unknown) => {
+  if (!state || typeof state !== 'object') return undefined;
+
+  const cid = (state as ThreadTopNavigationState).scrollThreadContainerCid;
+  return typeof cid === 'string' ? cid : undefined;
+};
+
+const isVisibleScrollTarget = (element: HTMLElement) => {
+  const style = window.getComputedStyle(element);
+  const rect = element.getBoundingClientRect();
+
+  return style.display !== 'none' && style.visibility !== 'hidden' && style.opacity !== '0' && rect.width > 0 && rect.height > 0;
+};
+
+export const findPreferredScrollTarget = (selector: string, excludedAncestorSelector = THREAD_SCROLL_PREVIEW_SELECTOR) => {
+  const candidates = Array.from(document.querySelectorAll<HTMLElement>(selector)).filter((element) => !element.closest(excludedAncestorSelector));
+  return candidates.find(isVisibleScrollTarget) ?? candidates[0];
 };
 
 export const scrollThreadContainerToTop = (cid?: string) => {
   if (!cid) return false;
 
-  const threadContainer = document.querySelector<HTMLElement>(`[data-thread-container-cid="${cid}"]`);
+  const threadContainer = findPreferredScrollTarget(`[data-thread-container-cid="${cid}"]`);
   if (!threadContainer) return false;
 
   const desiredTop = window.scrollY + threadContainer.getBoundingClientRect().top;
-  const maxScrollTop = document.documentElement.scrollHeight - window.innerHeight;
-  const extraSpace = Math.max(0, desiredTop - maxScrollTop);
-
-  setThreadScrollSpacerHeight(extraSpace);
   window.scrollTo({
     top: desiredTop,
     left: 0,
@@ -46,28 +45,3 @@ export const scrollThreadContainerToTop = (cid?: string) => {
 
   return true;
 };
-
-export const openThreadAtTop = ({
-  cid,
-  currentPathname,
-  navigate,
-  threadRoute,
-}: {
-  cid?: string;
-  currentPathname?: string;
-  navigate: (route: string) => void;
-  threadRoute?: string;
-}) => {
-  if (!cid || !threadRoute) return false;
-
-  if (currentPathname !== threadRoute) {
-    navigate(threadRoute);
-  }
-
-  scrollThreadContainerToTop(cid);
-  window.requestAnimationFrame(() => {
-    scrollThreadContainerToTop(cid);
-  });
-
-  return true;
-};