From 4ec55aa05bddaa935b56b3b01879d17999e52fc8 Mon Sep 17 00:00:00 2001 From: 0xPratik Date: Thu, 29 Oct 2026 18:14:15 +0545 Subject: [PATCH 1/2] fix(conversation): keep map and marker focus, skip empty thread input Addresses findings 5, 6, 7 and 19 of #142. - 5: ConversationMap's Arrow/Home/End scroll only the rail to the next tick and focus it with preventScroll, so a clipped tick no longer scrolls the page or another ancestor. - 6: when the map hides with the focused tick's turn still present, focus now moves to the conversation log instead of falling to the body. The map also drops its preview when it hides or the previewed turn leaves, so the tooltip cannot come back on its own with a later turn. - 7: ConversationEarlier clears its retry focus flag when the marker unmounts, so a later stall or load cannot pull focus to it. - 19: ConversationThread leaves out the input slot for boolean children, as `{show && }` passes. --- e2e/conversation-map.spec.ts | 38 ++++++++++++++++++ src/ui/conversation-earlier.test.tsx | 39 +++++++++++++++++++ src/ui/conversation-earlier.tsx | 10 +++-- src/ui/conversation-map.test.tsx | 58 ++++++++++++++++++++++++++++ src/ui/conversation-map.tsx | 27 ++++++++++--- src/ui/conversation-thread.test.tsx | 20 ++++++++++ src/ui/conversation-thread.tsx | 4 +- 7 files changed, 186 insertions(+), 10 deletions(-) diff --git a/e2e/conversation-map.spec.ts b/e2e/conversation-map.spec.ts index 09c18e04..45c6154f 100644 --- a/e2e/conversation-map.spec.ts +++ b/e2e/conversation-map.spec.ts @@ -156,6 +156,44 @@ test.describe("conversation map", () => { expect(await page.evaluate(() => window.scrollY)).toBe(0); }); + test("moving along the rail by keyboard scrolls only the rail", async ({ + page, + }) => { + await page.goto("/conversation-map-tall-page"); + await expect(current(page)).toHaveAccessibleName("Question 79"); + await expect + .poll(() => railState(page)) + .toEqual({ overflows: true, scrolled: true, showsCurrent: true }); + // Focus the tab stop without scrolling, so the page is still at the top + // and the map below the fold. + await tick(page, "Question 79").evaluate((element) => + (element as HTMLElement).focus({ preventScroll: true }), + ); + /** Whether the rail shows the focused tick. */ + const showsFocused = () => + page + .locator("[data-slot='conversation-map-rail']") + .evaluate((element) => { + const view = element.getBoundingClientRect(); + const box = document.activeElement?.getBoundingClientRect(); + return ( + box !== undefined && + box.top >= view.top - 1 && + box.bottom <= view.bottom + 1 + ); + }); + + await page.keyboard.press("Home"); + await expect(tick(page, "Question 0")).toBeFocused(); + await expect.poll(showsFocused).toBe(true); + expect(await page.evaluate(() => window.scrollY)).toBe(0); + + await page.keyboard.press("End"); + await expect(tick(page, "Question 79")).toBeFocused(); + await expect.poll(showsFocused).toBe(true); + expect(await page.evaluate(() => window.scrollY)).toBe(0); + }); + test("a rail that shrinks keeps the current tick in view", async ({ page, }) => { diff --git a/src/ui/conversation-earlier.test.tsx b/src/ui/conversation-earlier.test.tsx index 6a8ee333..9bf03395 100644 --- a/src/ui/conversation-earlier.test.tsx +++ b/src/ui/conversation-earlier.test.tsx @@ -213,4 +213,43 @@ describe("ConversationEarlier", () => { container.querySelector("[role='log']"), ); }); + + test("a retry that ends the history leaves no focus to restore later", () => { + const { container, rerender } = render( + {}} />, + ); + const button = container.querySelector("button"); + if (button === null) throw new Error("expected the retry button"); + button.focus(); + // The retry brings the last page in the same commit: the marker goes. + act(() => button.click()); + rerender( {}} />); + expect(marker(container)).toBeNull(); + act(() => (document.activeElement as HTMLElement | null)?.blur()); + expect(document.activeElement).toBe(document.body); + + // Older items turn up again, and a later load stalls and recovers. + rerender( {}} />); + rerender( {}} />); + expect(document.activeElement).toBe(document.body); + }); + + test("a retry that ends the history while stalled leaves no focus to restore later", () => { + const { container, rerender } = render( + {}} />, + ); + const button = container.querySelector("button"); + if (button === null) throw new Error("expected the retry button"); + button.focus(); + // Stalled throughout: only the marker's coming and going can reset it. + act(() => button.click()); + rerender( {}} />); + expect(marker(container)).toBeNull(); + act(() => (document.activeElement as HTMLElement | null)?.blur()); + expect(document.activeElement).toBe(document.body); + + rerender( {}} />); + rerender( {}} />); + expect(document.activeElement).toBe(document.body); + }); }); diff --git a/src/ui/conversation-earlier.tsx b/src/ui/conversation-earlier.tsx index c2774bb5..b74a3dab 100644 --- a/src/ui/conversation-earlier.tsx +++ b/src/ui/conversation-earlier.tsx @@ -70,10 +70,14 @@ export const ConversationEarlier = forwardRef< // A retry swaps the button for the loading text: hold focus on the marker // rather than let it fall to the body, and hand it back to the button if - // the retry stalls again. + // the retry stalls again. A retry that ends the history takes the marker + // with it, leaving nothing to restore. useLayoutEffect(() => { const element = markerRef.current; - if (element === null) return; + if (element === null) { + retried.current = false; + return; + } if (stalled) { if (document.activeElement === element) { element.querySelector("button")?.focus(); @@ -86,7 +90,7 @@ export const ConversationEarlier = forwardRef< if (active === null || active === document.body) { element.focus({ preventScroll: true }); } - }, [stalled]); + }, [stalled, hasEarlier]); // Once the last page has landed the marker goes away; if it held focus, // hand focus to the log so it doesn't fall to the body. diff --git a/src/ui/conversation-map.test.tsx b/src/ui/conversation-map.test.tsx index 4b2fcc6f..48920ce9 100644 --- a/src/ui/conversation-map.test.tsx +++ b/src/ui/conversation-map.test.tsx @@ -510,6 +510,53 @@ describe("ConversationMap", () => { mounted.unmount(); }); + test("a preview the map hid with does not come back with a second turn", () => { + const mounted = render(); + act(() => { + tick(mounted.container, "Second question").dispatchEvent( + new PointerEvent("pointerover", { + bubbles: true, + pointerType: "mouse", + }), + ); + }); + expect(mounted.container.querySelector("[role='tooltip']")).not.toBeNull(); + mounted.rerender( + , + ); + expect( + mounted.container.querySelector("[data-slot='conversation-map']"), + ).toBeNull(); + mounted.rerender( + , + ); + expect( + mounted.container.querySelector("[data-slot='conversation-map']"), + ).not.toBeNull(); + expect(mounted.container.querySelector("[role='tooltip']")).toBeNull(); + mounted.unmount(); + }); + + test("a preview whose turn goes does not come back with it", () => { + const mounted = render(); + act(() => { + tick(mounted.container, "Second question").dispatchEvent( + new PointerEvent("pointerover", { + bubbles: true, + pointerType: "mouse", + }), + ); + }); + expect(mounted.container.querySelector("[role='tooltip']")).not.toBeNull(); + const rest = TURNS.filter((turn) => turn.id !== "q2"); + mounted.rerender( + !id.startsWith("q2"))} />, + ); + mounted.rerender(); + expect(mounted.container.querySelector("[role='tooltip']")).toBeNull(); + mounted.unmount(); + }); + test("a focused tick whose turn goes keeps focus on the rail", () => { const mounted = render(); act(() => tick(mounted.container, "First question").focus()); @@ -534,6 +581,17 @@ describe("ConversationMap", () => { mounted.unmount(); }); + test("a focused tick whose rail goes with its turn kept hands focus to the log", () => { + const mounted = render(); + act(() => tick(mounted.container, "Fourth question").focus()); + mounted.rerender(); + expect( + mounted.container.querySelector("[data-slot='conversation-map']"), + ).toBeNull(); + expect(document.activeElement).toBe(scroller(mounted.container)); + mounted.unmount(); + }); + test("scrolling the rail measures the previewed tick in the handler, not in render", () => { const mounted = render( diff --git a/src/ui/conversation-map.tsx b/src/ui/conversation-map.tsx index 258b3171..8094c578 100644 --- a/src/ui/conversation-map.tsx +++ b/src/ui/conversation-map.tsx @@ -120,6 +120,14 @@ export const ConversationMap = forwardRef( const [focusedId, setFocusedId] = useState(null); const previewTextId = useId(); + // The map hid, or the previewed turn left `turns`: the tick unmounted + // with no blur or pointer leave to close its preview, which would come + // back with the turn. Set while rendering (react.dev, "Storing + // information from previous renders"). + if (preview !== null && (hidden || !turnIds.includes(preview.id))) { + setPreview(null); + } + // Clamped once the card has a height, so it stays beside the rail. useLayoutEffect(() => { const card = cardRef.current; @@ -157,11 +165,13 @@ export const ConversationMap = forwardRef( }, [currentId, hidden, reducedMotion]); // The focused tick's turn left `turns` (a history window trimmed it, the - // host deleted it): its button is gone, focus fell to the body, and React - // reports no blur for it. Hand focus to the rail's tab stop, or to the log - // once the rail itself is gone. + // host deleted it), or the rail hid with it: its button is gone, focus fell + // to the body, and React reports no blur for it. Hand focus to the rail's + // tab stop, or to the log once the rail itself is gone. useLayoutEffect(() => { - if (focusedId === null || turnIds.includes(focusedId)) return; + if (focusedId === null || (!hidden && turnIds.includes(focusedId))) { + return; + } setFocusedId(null); const active = document.activeElement; if (active !== null && active !== document.body) return; @@ -169,7 +179,7 @@ export const ConversationMap = forwardRef( ...(railRef.current?.querySelectorAll(TICK) ?? []), ].find((tick) => tick.tabIndex === 0); (stop ?? scrollContainer())?.focus({ preventScroll: true }); - }, [focusedId, turnIds, scrollContainer]); + }, [focusedId, hidden, turnIds, scrollContainer]); // Escape dismisses a preview wherever focus is: one the mouse opened // covers the content while focus stays in the composer or the log. @@ -260,7 +270,12 @@ export const ConversationMap = forwardRef( : index + (event.key === "ArrowDown" ? 1 : -1); // Also at either end, so the keys never scroll the rail or the page. event.preventDefault(); - ticks[next]?.focus(); + const rail = railRef.current; + const target = ticks[next]; + if (rail === null || target === undefined) return; + // Focus alone would scroll every ancestor to reveal a clipped tick. + keepInView(rail, target, "auto"); + target.focus({ preventScroll: true }); }; return ( diff --git a/src/ui/conversation-thread.test.tsx b/src/ui/conversation-thread.test.tsx index 9bc44cd6..ee87ee19 100644 --- a/src/ui/conversation-thread.test.tsx +++ b/src/ui/conversation-thread.test.tsx @@ -172,6 +172,26 @@ describe("ConversationThread", () => { mounted.unmount(); }); + test("leaves out the input slot for boolean children", () => { + for (const show of [false, true]) { + const mounted = render( + + {show} + , + ); + expect( + mounted.container.querySelector( + "[data-slot='conversation-thread-input']", + ), + ).toBeNull(); + mounted.unmount(); + } + }); + test("gives the root its ref, props and classes, and the policy to the conversation", () => { const ref = createRef(); const mounted = render( diff --git a/src/ui/conversation-thread.tsx b/src/ui/conversation-thread.tsx index e09df17e..32ab9cb6 100644 --- a/src/ui/conversation-thread.tsx +++ b/src/ui/conversation-thread.tsx @@ -117,7 +117,9 @@ export function ConversationThread({ - {children === undefined || children === null ? null : ( + {children === undefined || + children === null || + typeof children === "boolean" ? null : (
{children}
From 64ee244e82791da2dee398f7058e45e1ee34f4f1 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 3 Nov 2026 23:10:24 -0800 Subject: [PATCH 2/2] fix(conversation): clear a hidden map preview Hiding the rail unmounts a previewed tick without pointer leave or blur, so the tooltip and its Escape listener stayed. Clear that preview before focus returns to the log, and when the previewed turn is no longer in the map. --- src/ui/conversation-map.test.tsx | 83 +++++++++++++++++++++++--------- src/ui/conversation-map.tsx | 16 +++--- 2 files changed, 66 insertions(+), 33 deletions(-) diff --git a/src/ui/conversation-map.test.tsx b/src/ui/conversation-map.test.tsx index 48920ce9..cc1ab070 100644 --- a/src/ui/conversation-map.test.tsx +++ b/src/ui/conversation-map.test.tsx @@ -511,30 +511,67 @@ describe("ConversationMap", () => { }); test("a preview the map hid with does not come back with a second turn", () => { - const mounted = render(); - act(() => { - tick(mounted.container, "Second question").dispatchEvent( - new PointerEvent("pointerover", { - bubbles: true, - pointerType: "mouse", - }), + const add = document.addEventListener.bind(document); + const remove = document.removeEventListener.bind(document); + let keydowns = 0; + const watch = + (delta: number, call: typeof add) => + ( + type: string, + listener: EventListenerOrEventListenerObject, + options?: boolean | AddEventListenerOptions, + ) => { + if (type === "keydown") keydowns += delta; + call(type, listener, options); + }; + document.addEventListener = watch( + 1, + add, + ) as typeof document.addEventListener; + document.removeEventListener = watch( + -1, + remove, + ) as typeof document.removeEventListener; + try { + const mounted = render(); + const before = keydowns; + act(() => { + tick(mounted.container, "Second question").dispatchEvent( + new PointerEvent("pointerover", { + bubbles: true, + pointerType: "mouse", + }), + ); + }); + expect( + mounted.container.querySelector("[role='tooltip']"), + ).not.toBeNull(); + expect(keydowns).toBe(before + 1); + mounted.rerender( + , ); - }); - expect(mounted.container.querySelector("[role='tooltip']")).not.toBeNull(); - mounted.rerender( - , - ); - expect( - mounted.container.querySelector("[data-slot='conversation-map']"), - ).toBeNull(); - mounted.rerender( - , - ); - expect( - mounted.container.querySelector("[data-slot='conversation-map']"), - ).not.toBeNull(); - expect(mounted.container.querySelector("[role='tooltip']")).toBeNull(); - mounted.unmount(); + expect( + mounted.container.querySelector("[data-slot='conversation-map']"), + ).toBeNull(); + expect(keydowns).toBe(before); + act(() => { + document.dispatchEvent( + new KeyboardEvent("keydown", { key: "Escape", bubbles: true }), + ); + }); + mounted.rerender( + , + ); + expect( + mounted.container.querySelector("[data-slot='conversation-map']"), + ).not.toBeNull(); + expect(mounted.container.querySelector("[role='tooltip']")).toBeNull(); + expect(keydowns).toBe(before); + mounted.unmount(); + } finally { + document.addEventListener = add; + document.removeEventListener = remove; + } }); test("a preview whose turn goes does not come back with it", () => { diff --git a/src/ui/conversation-map.tsx b/src/ui/conversation-map.tsx index 8094c578..8d4292ff 100644 --- a/src/ui/conversation-map.tsx +++ b/src/ui/conversation-map.tsx @@ -120,14 +120,6 @@ export const ConversationMap = forwardRef( const [focusedId, setFocusedId] = useState(null); const previewTextId = useId(); - // The map hid, or the previewed turn left `turns`: the tick unmounted - // with no blur or pointer leave to close its preview, which would come - // back with the turn. Set while rendering (react.dev, "Storing - // information from previous renders"). - if (preview !== null && (hidden || !turnIds.includes(preview.id))) { - setPreview(null); - } - // Clamped once the card has a height, so it stays beside the rail. useLayoutEffect(() => { const card = cardRef.current; @@ -167,8 +159,12 @@ export const ConversationMap = forwardRef( // The focused tick's turn left `turns` (a history window trimmed it, the // host deleted it), or the rail hid with it: its button is gone, focus fell // to the body, and React reports no blur for it. Hand focus to the rail's - // tab stop, or to the log once the rail itself is gone. + // tab stop, or to the log once the rail itself is gone. Clear a preview + // whose tick left the same way before moving focus. useLayoutEffect(() => { + if (preview !== null && (hidden || !turnIds.includes(preview.id))) { + setPreview(null); + } if (focusedId === null || (!hidden && turnIds.includes(focusedId))) { return; } @@ -179,7 +175,7 @@ export const ConversationMap = forwardRef( ...(railRef.current?.querySelectorAll(TICK) ?? []), ].find((tick) => tick.tabIndex === 0); (stop ?? scrollContainer())?.focus({ preventScroll: true }); - }, [focusedId, hidden, turnIds, scrollContainer]); + }, [focusedId, hidden, preview, turnIds, scrollContainer]); // Escape dismisses a preview wherever focus is: one the mouse opened // covers the content while focus stays in the composer or the log.