From 3f51fc5fbe2164531a42b61d461f95934fede0a6 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 21 Jul 2026 19:56:57 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=AD=20fix:=20Keep=20Message=20Nav=20Ch?= =?UTF-8?q?evrons=20Working=20on=20In-Thread=20Steers=20(#14377)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An applied steer renders nested inside the response, whose `relative` content column becomes the steer's offsetParent, so its `offsetTop` is local to that column rather than measured against the scroll content like top-level rows. The rail compared that value against `scrollTop`, so once the viewport reached a steer every jump/current-row decision was computed in the wrong coordinate space — the "previous" chevron kept re-targeting the steer and got stuck. - Add `entryTop(el, container)` that sums `offsetTop` up the offsetParent chain until it leaves the scroll container, folding nested steers back into one content-space origin (top-level rows collapse to a single hop) - Use it for the four message-entry measurements (current row, offset cache, jump previous/next); leave the column-rib offsets untouched - Guard `getCurrentVisibleId` on a null scroll container - Test a nested steer (local offset inside a positioned column) lands its rib at the true thread position; fails with the old single-hop offsetTop --- .../components/Chat/Messages/MessageNav.tsx | 53 ++++++++++++++++--- .../Messages/__tests__/MessageNav.spec.tsx | 50 +++++++++++++++++ 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/client/src/components/Chat/Messages/MessageNav.tsx b/client/src/components/Chat/Messages/MessageNav.tsx index e7a4cd2c86..afb3a9aad9 100644 --- a/client/src/components/Chat/Messages/MessageNav.tsx +++ b/client/src/components/Chat/Messages/MessageNav.tsx @@ -146,6 +146,31 @@ function computeTargetScroll( return Math.max(0, Math.min(target, max)); } +/** + * An entry's top edge in the scroll container's content space — the same space + * as `container.scrollTop`. A single `offsetTop` is measured from the nearest + * positioned ancestor, which for an in-thread steer is the response's `relative` + * content column, not the scroll content. Mixing that local value with the + * content-space `offsetTop` of top-level rows compares different origins and + * breaks every rail decision (jump targets, current row, fisheye centering) the + * moment the viewport reaches a steer. Summing `offsetTop` up the offsetParent + * chain until it leaves the container folds any nesting back into one origin; + * top-level rows collapse to a single hop. + */ +function entryTop(el: HTMLElement, container: HTMLElement): number { + let top = 0; + let node: Element | null = el; + while (node instanceof HTMLElement) { + top += node.offsetTop; + const parent = node.offsetParent; + if (!(parent instanceof HTMLElement) || parent === container || !container.contains(parent)) { + break; + } + node = parent; + } + return top; +} + type RibDims = { baseW: number; baseH: number; peakW: number; peakH: number }; const RIB_END: RibDims = { baseW: 3, baseH: 3, peakW: 4.5, peakH: 4.5 }; @@ -289,18 +314,26 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject { + const container = scrollableRef.current; + if (!container) { + return null; + } let nextId: string | null = null; let nextTop = Number.POSITIVE_INFINITY; for (const id of visibleSetRef.current) { const el = observedRef.current.get(id); - if (!el || el.offsetTop >= nextTop) { + if (!el) { + continue; + } + const top = entryTop(el, container); + if (top >= nextTop) { continue; } nextId = id; - nextTop = el.offsetTop; + nextTop = top; } return nextId; - }, []); + }, [scrollableRef]); useEffect(() => { messagesByIdRef.current = messagesById; @@ -862,8 +895,14 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject { for (let i = 0; i < entries.length; i++) { const el = resolveEntryEl(entries[i].id); - offsetsTop[i] = el ? el.offsetTop : Number.POSITIVE_INFINITY; - offsetsBottom[i] = el ? el.offsetTop + el.offsetHeight : Number.POSITIVE_INFINITY; + if (!el) { + offsetsTop[i] = Number.POSITIVE_INFINITY; + offsetsBottom[i] = Number.POSITIVE_INFINITY; + continue; + } + const top = entryTop(el, container); + offsetsTop[i] = top; + offsetsBottom[i] = top + el.offsetHeight; } }; recomputeOffsets(); @@ -1128,7 +1167,7 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject scrollTop + JUMP_EPS) { + if (entryTop(el, container) - scrollMargin > scrollTop + JUMP_EPS) { scrollToStart(entries[i].id); return; } diff --git a/client/src/components/Chat/Messages/__tests__/MessageNav.spec.tsx b/client/src/components/Chat/Messages/__tests__/MessageNav.spec.tsx index 921e5776c8..5cd92eb8a0 100644 --- a/client/src/components/Chat/Messages/__tests__/MessageNav.spec.tsx +++ b/client/src/components/Chat/Messages/__tests__/MessageNav.spec.tsx @@ -476,6 +476,56 @@ describe('MessageNav', () => { expect(steerRib?.getAttribute('aria-label')).not.toContain('Danny'); }); + it('places a nested steer at its content-space position, not its offset-parent-local offset', () => { + // A steer renders inside the response's `relative` content column, so its + // raw offsetTop is local to that column, not its true position in the + // thread. The rail must sum the offsetParent chain — otherwise the steer's + // small local offset reads as the topmost row and hijacks the current + // indicator (and, with it, the up/down chevrons) whenever it is on screen. + const messages = [ + buildMessage({ messageId: 'u1', text: 'first ask', isCreatedByUser: true }), + buildMessage({ messageId: 'a1', text: 'long tool run' }), + buildMessage({ messageId: 'u2', text: 'follow-up', isCreatedByUser: true }), + buildMessage({ messageId: 'a2', text: 'second reply' }), + ]; + mockUseGetMessagesByConvoId.mockReturnValue({ data: messages }); + const { scrollable } = buildDom(messages); + const response = scrollable.querySelector('#a1') as HTMLElement; + + // Nest the steer in a positioned column of its own; the steer's offsetTop + // (40) is local to that column (380), so its content-space top is 420 — + // below a1 (300) and above u2 (500). jsdom leaves offsetParent null, so + // the nesting has to be declared for the chain-walk to have anything to + // sum. + const column = document.createElement('div'); + column.className = 'relative'; + Object.defineProperty(column, 'offsetTop', { value: 380, configurable: true }); + response.appendChild(column); + const steer = appendSteerNode(column, 's1', 'steer mid-run words', 40); + Object.defineProperty(steer, 'offsetParent', { value: column, configurable: true }); + + const scrollableRef = { current: scrollable } as RefObject; + const { container } = render(); + act(() => { + jest.advanceTimersByTime(250); + }); + + const io = MockIntersectionObserver.last(); + act(() => { + io!.trigger([ + { target: document.getElementById('a1')!, isIntersecting: true }, + { target: document.getElementById('steer-s1')!, isIntersecting: true }, + ]); + jest.advanceTimersByTime(32); + }); + + // Topmost-by-content-space is the response, not the steer with the smaller + // local offset. Before the chain-walk this landed on 'steer-s1'. + const current = container.querySelectorAll('[aria-current="true"]'); + expect(current).toHaveLength(1); + expect(current[0]).toHaveAttribute('data-msg-id', 'a1'); + }); + it('un-lights a steer rib when its DOM node is replaced (pending → applied swap)', async () => { const messages = [ buildMessage({ messageId: 'u1', text: 'first ask', isCreatedByUser: true }),