fix(connect): land the transcript ↑/↓ snap on the edge you are travelling towards - #94
Merged
Merged
Conversation
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Walking the ↑/↓ highlight in
agent session connectonto a message taller than the transcript window landed it on the wrong edge — you entered a long message from the end you were leaving rather than the end you were heading towards.(short)(long), on(short), press ↓(long)(short)(long)(short), on the trailing(short), press ↑(long)Either way the highlight jumped past the whole message to the far edge, so pressing the same arrow again scrolled back over what you had just skipped.
Two compounding causes
snapToEntryinferred direction from the entry's position, not the direction of travel — any entry too tall to fit was top-aligned unconditionally. Backing up into one from below jumped to its far-away first line instead of the last line just above you.ensureVisiblediscarded the snap target whenever the entry was the newest one, pinning to the bottom so streamed content keeps arriving in view. For a trailing entry taller than the window, walking down onto it showed its end, not its beginning.Fixing one leaves the other symptom, so both scenarios are covered separately.
The fix
snapToEntrytakes adirargument (1for ↓,-1for ↑). A too-tall entry lands on the edge you are heading towards: ↓ on its first row, ↑ on its last. From there ↑/↓ walk the rest a row at a time (revealMore), so the two together read a long entry continuously in whichever direction you started. Entries that fit, and entries already fully in frame, behave exactly as before.snapAnchorForEntry, keyed on where the snap lands (the last screenful) rather than on the entry being the newest. That is what it always meant, and it makes both halves unit-testable — the component half was previously unreachable from the pure-function tests.ConnectApp's ↑/↓ handlers thread the direction through.Nothing else calls
snapToEntry, and this navigation behaviour isn't described indocs/orskills/, so no doc changes are needed.Verification
Full CI equivalent run locally on this branch with bun 1.3.14 —
typecheck,test,build, and the--compilesmoke all clean:test/connect-app.test.tscarries 98 tests, including the 6 new ones: two onsnapToEntryfor the directional too-tall snap, and a newsnapAnchorForEntrysuite of four covering both reported scenarios, the genuine bottom-follow case, and the no-move case.Reverting only
src/ui/transcriptRows.tstomainfails 5 of them — notablybacks ↑ into a too-tall entry at its LAST linewithexpected 1 to be 7, i.e. the old code really did return the far top edge. So these are regression tests, not just coverage.Provenance
The change was written and verified in an Ellipsis sandbox whose token was scoped to the
ellipsis-dev/ellipsismonorepo, so it was parked there as a patch doc in ellipsis-dev/ellipsis#6047. This PR is that patch applied tomain@9d479f1viagit am(clean, no fuzz) and re-verified from scratch here; #6047 is being closed in favour of this one.🤖 Generated with Claude Code