🧭 fix: Keep Message Nav Chevrons Working on In-Thread Steers (#14377)

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
This commit is contained in:
Danny Avila 2026-07-21 19:56:57 -04:00 committed by GitHub
parent 3e9f07976a
commit 3f51fc5fbe
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 96 additions and 7 deletions

View file

@ -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<HTMLDivE
}, [entries]);
const getCurrentVisibleId = useCallback((): string | null => {
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<HTMLDivE
const recomputeOffsets = () => {
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<HTMLDivE
if (!el) {
continue;
}
if (el.offsetTop - scrollMargin < scrollTop - JUMP_EPS) {
if (entryTop(el, container) - scrollMargin < scrollTop - JUMP_EPS) {
scrollToStart(entries[i].id);
return;
}
@ -1151,7 +1190,7 @@ function MessageNav({ scrollableRef }: { scrollableRef: React.RefObject<HTMLDivE
if (!el) {
continue;
}
if (el.offsetTop - scrollMargin > scrollTop + JUMP_EPS) {
if (entryTop(el, container) - scrollMargin > scrollTop + JUMP_EPS) {
scrollToStart(entries[i].id);
return;
}

View file

@ -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<HTMLDivElement>;
const { container } = render(<MessageNav scrollableRef={scrollableRef} />);
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 }),