From ead14123ca121d00d153eba7d8d1dbbc06f97dad Mon Sep 17 00:00:00 2001 From: "ellipsis-dev[bot]" <65095814+ellipsis-dev[bot]@users.noreply.github.com> Date: Wed, 29 Jul 2026 17:36:10 +0000 Subject: [PATCH] =?UTF-8?q?fix(connect):=20land=20the=20=E2=86=91/?= =?UTF-8?q?=E2=86=93=20snap=20on=20the=20edge=20you=20are=20travelling=20t?= =?UTF-8?q?owards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Walking into an entry taller than the window put the highlight in the wrong place, so a long message was entered from the end you were leaving rather than the end you were heading for. Two bugs compounded: 1. snapToEntry inferred direction from the entry's POSITION rather than the direction of travel, so it top-aligned any too-tall entry. Backing UP into one from below jumped to its far-away first line instead of the last line just above you. It now takes a dir argument: down lands on the entry's first row, up on its last, and revealMore walks through the rest — the two read the entry continuously either way. 2. ensureVisible discarded the snap target entirely when the entry was the NEWEST one, pinning to the bottom to keep following streamed content. For a trailing entry taller than the window that meant walking down onto it showed its end. The bottom-follow is now keyed on the snap landing on the last screenful, which is what it always meant. So with two messages (short)(long), ↓ off (short) lands on the top of (long); with (short)(long)(short), ↑ off the trailing (short) lands on the bottom of (long). The bottom-follow decision moves into a pure snapAnchorForEntry so both halves are covered by tests. Co-Authored-By: Claude --- src/ui/ConnectApp.tsx | 22 ++++++----------- src/ui/transcriptRows.ts | 53 +++++++++++++++++++++++++++++++--------- test/connect-app.test.ts | 50 +++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 25 deletions(-) diff --git a/src/ui/ConnectApp.tsx b/src/ui/ConnectApp.tsx index a1a1bf0..3b1d22e 100644 --- a/src/ui/ConnectApp.tsx +++ b/src/ui/ConnectApp.tsx @@ -53,7 +53,7 @@ import { padPanelBlocks, pendingMessageRows, rowViewport, - snapToEntry, + snapAnchorForEntry, spacerRow, type RowSpan, type ScrollAnchor, @@ -988,18 +988,12 @@ export function ConnectApp(props: ConnectAppProps): React.ReactElement { [allRows, view], ) - // Snap the window so a highlighted entry is readable: entering from above, - // its first line goes to the top of the frame; from below, it aligns to the - // bottom edge. See snapToEntry. + // Snap the window so a highlighted entry is readable, given which way the + // highlight is travelling (`dir`: 1 for ↓, -1 for ↑). See snapToEntry. const ensureVisible = useCallback( - (key: string): void => { - const target = snapToEntry(allRows, key, view, view.capacity) - if (target === null) return - const range = entryRange(allRows, key) - // Snapping to the newest entry means following the bottom again, so new - // content keeps arriving in view. - if (range && range.last >= allRows.length - 1) setScrollAnchor(null) - else setScrollAnchor(anchorAt(allRows, target)) + (key: string, dir: 1 | -1 = 1): void => { + const move = snapAnchorForEntry(allRows, key, view, view.capacity, dir) + if (move) setScrollAnchor(move.anchor) }, [allRows, view], ) @@ -1107,7 +1101,7 @@ export function ConnectApp(props: ConnectAppProps): React.ReactElement { const target = idx === -1 ? navKeys.length - 1 : Math.max(0, idx - 1) if (navKeys.length > 0) { setNavKey(navKeys[target]) - ensureVisible(navKeys[target]) + ensureVisible(navKeys[target], -1) } return } @@ -1118,7 +1112,7 @@ export function ConnectApp(props: ConnectAppProps): React.ReactElement { setScrollAnchor(null) } else { setNavKey(navKeys[idx + 1]) - ensureVisible(navKeys[idx + 1]) + ensureVisible(navKeys[idx + 1], 1) } return } diff --git a/src/ui/transcriptRows.ts b/src/ui/transcriptRows.ts index 37877c3..4a3cfd4 100644 --- a/src/ui/transcriptRows.ts +++ b/src/ui/transcriptRows.ts @@ -519,27 +519,58 @@ export function entryRange( } // Where the window must sit for `entryKey` to be readable, given where it sits -// now — the ↑/↓ snap. +// now and which way the highlight is travelling (`dir`: 1 for ↓, -1 for ↑) — +// the ↑/↓ snap. // -// An entry coming into frame from ABOVE puts its first line at the TOP of the -// window: you read a message from its beginning, with as much of it in front -// of you as fits. So does one too tall to fit whole. An entry arriving from -// BELOW aligns to the bottom edge, the direction it was already travelling. -// One already fully in frame doesn't move the window at all. Returns the flat -// row index to pin to the top, or null to leave the window alone. Pure, for -// tests. +// An entry already fully in frame doesn't move the window at all. One that +// FITS but sits off-frame comes in from the side it is on: from above to the +// top of the window, from below to the bottom edge. One TOO TALL to fit lands +// on the edge you are heading towards, so the walk keeps its direction of +// travel: ↓ lands on its FIRST line (you read a long message from its +// beginning) and ↑ on its LAST (you back into the end of it). From there +// ↑/↓ scroll THROUGH the rest of it a row at a time — see revealMore — so the +// two together read the entry continuously in whichever direction you started. +// +// Returns the flat row index to pin to the top, or null to leave the window +// alone. Pure, for tests. export function snapToEntry( rows: readonly TranscriptRow[], entryKey: string, view: { start: number; end: number }, capacity: number, + dir: 1 | -1 = 1, ): number | null { const range = entryRange(rows, entryKey) if (!range) return null + // Already readable whole: don't jostle the window. + if (range.first >= view.start && range.last < view.end) return null + const bottomAligned = Math.max(0, range.last - capacity + 1) const height = range.last - range.first + 1 - if (range.first < view.start || height >= capacity) return range.first - if (range.last >= view.end) return Math.max(0, range.last - capacity + 1) - return null + if (height >= capacity) return dir < 0 ? bottomAligned : range.first + return range.first < view.start ? range.first : bottomAligned +} + +// snapToEntry's row index as the scroll move to apply: null to leave the window +// where it is, or the anchor to park on — itself null when the snap lands on the +// last screenful, which means following the bottom again so streamed content +// keeps arriving in view. +// +// That bottom-follow is keyed on WHERE THE SNAP LANDS, not on the entry being +// the newest one: an entry taller than the window is snapped to an interior row +// (its first line, when walking down into it), and pinning to the bottom there +// would throw the snap away and show the entry's end instead of its beginning. +// Pure, for tests. +export function snapAnchorForEntry( + rows: readonly TranscriptRow[], + entryKey: string, + view: { start: number; end: number }, + capacity: number, + dir: 1 | -1 = 1, +): { anchor: ScrollAnchor | null } | null { + const target = snapToEntry(rows, entryKey, view, capacity, dir) + if (target === null) return null + if (target >= rows.length - capacity) return { anchor: null } + return { anchor: anchorAt(rows, target) } } // Agent and user prose rendered as markdown (bold, headings, bullets, tables, diff --git a/test/connect-app.test.ts b/test/connect-app.test.ts index 916a8cb..a58708e 100644 --- a/test/connect-app.test.ts +++ b/test/connect-app.test.ts @@ -20,6 +20,7 @@ import { itemRows, layOutItems, rowViewport, + snapAnchorForEntry, snapToEntry, withRenderedMarkdown, type TranscriptRow, @@ -902,6 +903,55 @@ describe('snapToEntry', () => { ] expect(snapToEntry(short, 'b', { start: 0, end: 3 }, 3)).toBeNull() }) + + it('backs ↑ into a too-tall entry at its LAST line, keeping the walk going up', () => { + // 'b' spans rows 1..10. Walking UP out of 'c' lands on 'b's bottom edge + // (rows 7..10 on a 4-row window), not its far-away first line. + expect(snapToEntry(rows, 'b', { start: 8, end: 12 }, 4, -1)).toBe(7) + }) + + it('walks ↓ into a too-tall entry at its FIRST line', () => { + expect(snapToEntry(rows, 'b', { start: 0, end: 4 }, 4, 1)).toBe(1) + }) +}) + +describe('snapAnchorForEntry', () => { + // A short entry, then one 10 rows tall — taller than the 4-row window. + const shortThenLong: TranscriptRow[] = [ + { id: 'a', entryKey: 'a', spans: [] }, + ...Array.from({ length: 10 }, (_, i) => ({ id: `b${i}`, entryKey: 'b', spans: [] })), + ] + + it('parks on the TOP of a trailing too-tall entry walked into from above', () => { + // ↓ off the short entry onto the long last one: its first line, NOT the + // bottom-follow that being the newest entry used to force. + expect(snapAnchorForEntry(shortThenLong, 'b', { start: 0, end: 4 }, 4, 1)).toEqual({ + anchor: { entryKey: 'b', rowOffset: 0 }, + }) + }) + + it('follows the bottom when the snap really does land on the last screenful', () => { + // The same trailing entry, but short enough to fit: bottom-aligning it IS + // the bottom of the log, so keep streaming content in view. + const shortTail: TranscriptRow[] = [ + ...Array.from({ length: 6 }, (_, i) => ({ id: `a${i}`, entryKey: 'a', spans: [] })), + { id: 'b0', entryKey: 'b', spans: [] }, + ] + expect(snapAnchorForEntry(shortTail, 'b', { start: 0, end: 4 }, 4, 1)).toEqual({ anchor: null }) + }) + + it('parks on the BOTTOM of a too-tall entry walked into from below', () => { + // (short) (long) (short) with the highlight on the trailing short one: ↑ + // lands on the long entry's END — rows 7..10 of an 11-row block. + const withTail: TranscriptRow[] = [...shortThenLong, { id: 'c', entryKey: 'c', spans: [] }] + expect(snapAnchorForEntry(withTail, 'b', { start: 8, end: 12 }, 4, -1)).toEqual({ + anchor: { entryKey: 'b', rowOffset: 6 }, + }) + }) + + it('reports no move for an entry already fully in frame', () => { + expect(snapAnchorForEntry(shortThenLong, 'a', { start: 0, end: 4 }, 4, -1)).toBeNull() + }) }) describe('withRenderedMarkdown', () => {