Skip to content

fix(native-chat): sending brings the latest into view and follows the reply - #24514

Merged
brennanb2025 merged 23 commits into
stablyai:mainfrom
AmoabaKelvin:fix/desktop-chat-scrolling
Oct 6, 2026
Merged

brennanb2025 merged 23 commits into
stablyai:mainfrom
AmoabaKelvin:fix/desktop-chat-scrolling

Conversation

@AmoabaKelvin

@AmoabaKelvin AmoabaKelvin commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 37 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1353 $\color{#cf222e}{\Huge{\mathbf{−}}}$​36 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1317
Prod 29 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​658 $\color{#cf222e}{\Huge{\mathbf{−}}}$​100 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​558

ELI5

In the desktop chat, if you scrolled up to read something and then sent a message, you stayed parked in old history and had to find your way back: your message and the reply arrived off-screen.

Now sending takes you to your message and follows the reply. Only you scrolling away stops that. This PR is the scrolling behaviour only; the solid "Jump to latest" landed in #25759, and "Jump to top" is in #25702.

What Changed

Before

  • Sending while scrolled up left you where you were, so your message and the reply arrived off-screen.
  • The transcript could not be reached with the keyboard.

After

  • Sending brings the latest into view and follows the reply, the moment you press it. This covers the composer, retry (including retrying a failed launch), answering a question or approval card, steering a queued message, conversation commands and goals, and slash commands that submit.
  • Only your own scrolling stops following: wheeling or paging up when there's more above, grabbing the scrollbar, clicking the transcript while already scrolled up, or a touch drag that carries the view off the end. Getting back to the end follows again.
  • Nothing else moves you. Opening or closing a tool run or turn doesn't change following: at the live end the view stays pinned as output streams, and if you've scrolled up you stay put. Content getting shorter, layout changes and the app's own scrolling never stop following.
  • Sends that aren't yours from this pane don't move a reader who scrolled up: a message sent from another device, a queued message the host delivers by itself, a message that waits as a queued card, and Stop.
  • Resuming a paused queue brings the latest into view once the host confirms it resumed, in the pane that pressed Resume.
  • Keyboard. The transcript is a labelled region you can Tab to and scroll with the arrow keys, Page Up/Down, Home/End and Space.

Mechanism

  • The message list exposes one command, "reveal latest". The composer calls it through a new onSubmitted callback when you press send, and each pane wraps its other sends (retry, answers, queue actions) with the same command. Each pane has its own list, so a reveal never moves another chat. A test fails if a future send path forgets to report.
  • Whether the transcript follows its end changes only on reader input (wheel, keys, touch, scrollbar, a click while scrolled up), handled in native-chat-reader-scroll-input.ts. Scroll events alone never stop following, so layout and size changes can't knock you off the end.

Why

The issue asks for one predictable scrolling model, and this is the common pattern for chat transcripts: your own scrolling is the only thing that leaves the end, and sending is the only thing that brings you back. Revealing at the press, rather than after the host accepts, needs no tracking of which send is still in flight, and an answer the host refuses has still shown you the latest, which is harmless.

Alternatives considered:

  • Stop following when you open a row, and resume when you close it. An earlier version of this PR did that. It needed a record of what you'd opened, and every review round found another case it got wrong (output shrinking, the reply moving on while a row was open, hiding the pane mid-way). Removed.
  • Decide "you left the end" from where the scroll landed (any scroll that ends more than a few pixels from the bottom). That's what main did. It can't tell your scroll from the content changing under you, so a tool's output collapsing could knock you off the end.
  • Reveal only after the host accepts the send. Also an earlier version. It needed to remember which pane and which send each pending reveal belonged to, for no visible gain.
  • Put your message at the top of the view on send. Rejected: it needs blank space below the message, which fights the way this list is pinned to its end.

Not in this PR: restoring your position when you reopen a session, screen-reader announcements for new messages, and performance work on long transcripts.

Linked Issue

Part of #23797. Jump to top, also asked for there, is in #25702; the solid Jump to latest landed in #25759.

Visual Proof

Recorded on an earlier version of this PR (before the button changes were split out and before the scrolling rework below), so it shows the old button looks and the old "opening a row stops following" rule. A new recording is to follow.

CleanShot.2026-10-02.at.04.33.13.mp4

Testing

  • All 386 native-chat and structured-session test files (4,146 tests) pass, plus the web typecheck, lint and formatting. Each rule above has a test that was checked to fail with that rule removed (15 rules).

  • Not yet re-tested in the running app after the rework; macOS only before it. Not tested on Windows, Linux or over SSH; the change is renderer-only and touches no paths, shortcuts or host calls.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Renderer-only. No change to what a paired client and host exchange, no new shortcuts, no file paths. Per scroll event the list sets two booleans that skip re-rendering when unchanged; per streamed chunk the path is the same as before.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@AmoabaKelvin
AmoabaKelvin force-pushed the fix/desktop-chat-scrolling branch from 7b51aa8 to 19144c4 Compare October 2, 2026 04:42
@AmoabaKelvin
AmoabaKelvin marked this pull request as ready for review October 2, 2026 04:42
@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Adds scroll-to-latest and scroll-to-top controls to the chat interface.

The PR appears safe to merge; no distinct new blocking finding was established.

Summary

The PR makes local native-chat sends reveal and follow the latest message, preserves position while readers inspect opened rows, and adds accessible controls for jumping to the latest message or the start of paged history.

  • Structured answers now hold their reveal until acceptance and discard it after recorded reader navigation.
  • PTY card sends now avoid revealing for a missing terminal or empty answer.
  • Tests cover navigation, focus, paging, and send paths.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Send[Accepted pane send] --> Reveal[Reveal latest and follow reply]
  Open[Open transcript row] --> Detach[Hold reader position]
  Close[Close last opened row] --> Reveal
  Navigate[Scroll or jump into history] --> Detach
  Top[Jump to top] --> Pages[Load older pages] --> Start[Show conversation start]
Loading

Reviews (2) · Last reviewed commit: "fix(native-chat): an empty answer no lon..."

Comment thread src/renderer/src/components/native-chat/NativeChatResolvedView.tsx Outdated
Comment thread src/renderer/src/components/native-chat/NativeChatSubagentRun.tsx Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one rough edge inline.

Reviewed changes

This run reviewed the full PR (19144c4236, 66 files): a renderer-only rework of the native-chat scrolling model, with no wire, host, path or shortcut changes.

  • Send reveals the latest — NativeChatMessageList exposes revealLatest through an imperative handle, and the composer plus every non-composer submit (retry, prompt answers, queue steer/resume, host commands) report through a new optional onSubmitted. Mobile/host-delivered messages and Stop deliberately do not move the reader.
  • Follow-state machine — nextFollowState replaces the single following boolean with following | detached(scroll|navigation|open), so opening a row detaches, and closing the last opened row resumes following wherever the end then is.
  • Jump controls — new NativeChatJumpControls with a solid "Jump to latest" and an up-arrow "Jump to top" that pages older history via the new HISTORY_START target in useNativeChatRailHistoryJump.
  • Accessibility & motion — transcript is a focusable named region, focus hands back to it when a focused jump button hides, and reduced-motion disables smooth align and the spinner.
  • Tests — new unit/component suites for the state table, reader navigation and jump controls, plus a source-scan ratchet over send sites.

ℹ️ Nitpicks

  • native-chat-send-reveal-ratchet.test.ts only asserts the literal token onSubmitted?.() exists somewhere in a file that emits emitNativeChatMessageSent(, so a future send path could add the token in a comment and still pass. The anti-vacuous senders.length > 0 check is good; matching the call site (or asserting the token appears after the emit within the same function) would make the guard actually pin behavior.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/renderer/src/components/native-chat/NativeChatResolvedView.tsx Outdated
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c633fb50-6d49-4559-88f2-6d9ba9413bed
📥 Commits

Reviewing files that changed from the base of the PR and between b5974c1 and da90399.

📒 Files selected for processing (2)
  • src/renderer/src/components/native-chat/NativeChatMessageList.reader-navigation.test.tsx
  • src/renderer/src/components/native-chat/NativeChatMessageList.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

The changes add an event-based transcript follow model that accounts for reader scrolling, navigation, and opened rows. The message list gains accessible region and focus behavior, and history jumps stop when the pane is hidden. Submission and session-action paths now invoke latest-message reveal callbacks under specified acceptance conditions. Reduced-motion preferences control message alignment scrolling. Tests cover these behaviors, including follow-state transitions, submission callbacks, reveal actions, and accessibility.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to da903

A focused test run may fail, and an unsuccessful queued action may move the reader unexpectedly. These limited issues do not affect persisted data or host behavior; the PR is mergeable with owner awareness.

Security Architecture Review

Security architecture risk: 🔵 Low · up to da903

The inspected changes affect conversation navigation without adding command permissions or data access. No security regression was identified in those paths, but some failure and session-switch behavior remains unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The added handle's effective authority is limited to its owning transcript's navigation and follow state. The inspected handle and production consumers do not use it to execute agent tools, select another target, or acquire host privileges.

Trust Boundaries and Controls

  • observed — The inspected action adapters forward prompt arguments and message identities to existing controllers. Reveal changes navigation separately; it does not synthesize an approval, turn a refused response into acceptance, or replace the controller's execution path.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 54 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: sending reveals the latest message and follows the reply.
Description check ✅ Passed The description explains the user-facing change, implementation, rationale, linked issue, and testing. The visual proof is from an earlier version and does not show the final behavior, but the descrip…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run re-reviewed the two commits since the prior pullfrog review (19144c4236): the held answer reveal and the empty-answer guard. Both address the prior inline concern cleanly.

  • Held answer reveal — useStructuredNativeChatSubmitReveal.revealingRespond now captures a reveal at click time and only fires it if respond returns non-null and no reader act intervened. Backed by a new readerActsRef counter and untilReaderActs in use-native-chat-transcript-scroll, which count every non-programmatic-scroll follow event.
  • Empty-answer guard — NativeChatResolvedView gates sendAnswer on targetPtyId && hasAskAnswer(prompt, selections) and sendRaw on targetPtyId, exactly matching the delegate's early-return conditions, so a no-op answer or a vanished PTY no longer moves the reader.
  • Ratchet tightened — line comments are stripped before matching onSubmitted?.(), closing the comment-only loophole from the prior nitpick. Tests cover the empty answer and the missing terminal.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

AmoabaKelvin and others added 4 commits October 4, 2026 22:46
…umps to latest and top

Sending while scrolled up left the reader parked in old history, and the way back was a faint button. A send now resumes following, opening a row to read it stops following until it is closed, and Jump to latest and Jump to top sit above the composer. Refs stablyai#23797.
…o scrolled away

An answer's reveal is now held from the click and dropped if the reader acts before the host accepts. In the terminal lane, an answer with no terminal to write to reveals nothing.
The terminal lane's reveal now uses the same check the send does, and the send-site guard test ignores comments.
@nwparker
nwparker force-pushed the fix/desktop-chat-scrolling branch from cbedfa9 to f351fc5 Compare October 5, 2026 05:55

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d67decd3-7ec4-4f35-8365-afb19a458eb5
📥 Commits

Reviewing files that changed from the base of the PR and between cbedfa91c75f0e83b0e7bfecdac00fc9e2dcc57d and f351fc5.

📒 Files selected for processing (11)
  • src/renderer/src/components/native-chat/NativeChatResolvedView.reveal-latest.test.tsx
  • src/renderer/src/components/native-chat/NativeChatResolvedView.tsx
  • src/renderer/src/components/native-chat/NativeChatStructuredSession.test-harness.tsx
  • src/renderer/src/components/native-chat/NativeChatStructuredSession.test.tsx
  • src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx
  • src/renderer/src/i18n/locales/en.json
  • src/renderer/src/i18n/locales/es.json
  • src/renderer/src/i18n/locales/fr.json
  • src/renderer/src/i18n/locales/ja.json
  • src/renderer/src/i18n/locales/ko.json
  • src/renderer/src/i18n/locales/zh.json
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/renderer/src/i18n/locales/es.json
  • src/renderer/src/i18n/locales/ko.json
  • src/renderer/src/i18n/locales/en.json
  • src/renderer/src/i18n/locales/ja.json
  • src/renderer/src/i18n/locales/zh.json
  • src/renderer/src/i18n/locales/fr.json

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

expect(stubs.revealLatest).toHaveBeenCalledTimes(3)
})

it('leaves the reader where they are when the terminal is gone and nothing is written', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Give this test its own session fixture.

When this test runs alone, stubs.session is still null. NativeChatResolvedView then reads session.messages and throws before the assertions run. Initialize the session for each test, and reset it during cleanup.

{/* Host-held drafts, never transcript rows. Above the status area, so running shells and agents sit next to the composer. */}
<NativeChatQueuedMessageList
controller={controller.queuedMessages}
controller={submits.queuedMessages}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

git diff --unified=50 f873aaac5b2544c02f17d98a188b75dc843a3de7 f351fc5e0ce9ce77007295b05bd3ae9369dd0ac0 -- src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx
printf '\n--- queue references ---\n'
rg -n --glob '*.{ts,tsx}' 'queuedMessages|revealLatest|useStructuredNativeChatSubmitReveal' src/renderer/src
printf '\n--- candidate controller declarations ---\n'
rg -n --glob '*.{ts,tsx}' '(^|[[:space:]])(async[[:space:]]+)?(steer|resume)[[:space:]]*[:=(]' src/renderer/src

Repository: stablyai/orca

Length of output: 45117


🏁 Script executed:

printf '%s\n' '--- reveal wrapper ---'
nl -ba src/renderer/src/components/native-chat/use-structured-native-chat-submit-reveal.ts | sed -n '1,115p'
printf '%s\n' '--- queue controller types and methods ---'
nl -ba src/renderer/src/components/native-chat/use-structured-agent-session-queued-messages.ts | sed -n '1,205p'
printf '%s\n' '--- direct queue transport mutation ---'
rg -n --glob '*.ts' --glob '*.tsx' 'const mutate|mutate\s*=|queuedMessagesSteer|queuedMessagesResume|agentSession\.queuedMessages' src/renderer/src/components/native-chat src/renderer/src | head -100

Repository: stablyai/orca

Length of output: 14894


🏁 Script executed:

printf '%s\n' '--- mutation implementation ---'
nl -ba src/renderer/src/components/native-chat/use-structured-agent-session-mutate.ts | sed -n '1,280p'
printf '%s\n' '--- queue RPC handlers and contracts ---'
rg -n --glob '*.{ts,tsx}' 'agentSession\.queuedMessageSend|agentSession\.queuedMessagesResume|queuedMessageSend|queuedMessagesResume' src | head -160

Repository: stablyai/orca

Length of output: 17082


🏁 Script executed:

printf '%s\n' '--- queued result contracts ---'
rg -n -C 8 'AgentSessionSendResult|AgentSessionQueuedMessagesResumeResult|QueuedMessagesResumeResult|QueuedMessageAction' src/shared/agent-session-wire.ts src/shared/agent-session-queued-message-wire.ts src/shared/agent-session-wire*
printf '%s\n' '--- host queue action methods ---'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-queued-mutations.ts | sed -n '155,225p'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-queued-mutations.ts | sed -n '300,365p'
printf '%s\n' '--- service dispatch wrappers ---'
nl -ba src/main/runtime/rpc/methods/structured-agent-session-queued-methods.ts | sed -n '1,45p'

Repository: stablyai/orca

Length of output: 13326


🏁 Script executed:

nl -ba src/renderer/src/components/native-chat/use-structured-agent-session.ts | sed -n '175,260p'
printf '%s\n' '--- reveal hold behavior ---'
nl -ba src/renderer/src/components/native-chat/use-native-chat-reveal-latest.ts | sed -n '1,70p'

Repository: stablyai/orca

Length of output: 6341


🏁 Script executed:

nl -ba src/renderer/src/components/native-chat/use-structured-agent-session.ts | sed -n '155,183p'

Repository: stablyai/orca

Length of output: 1405


Reveal only after queued actions succeed.

submits.queuedMessages reveals before host acceptance. A stale or withdrawn card can make steer() fail, and resume() can return resumed: false when no pause remains. Both methods discard their results. steerNewest() also reveals as soon as a local card is available, before its send completes. Propagate each action’s outcome and use a held reveal only after Send-now is accepted or Resume lifts the pause. This keeps the reader in place if the action fails or they scroll first.

@nwparker nwparker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed local-send intent, follow/detach state, virtualization and stale-window guards. Repaired rebase omissions in Retry reveal and cancelAsk forwarding; retry regression verified red/green. 2633 native-chat tests plus focused retry/cancellation tests, web typecheck and quality gate pass. Explicit jump-to-top loads older pages and can take time/memory on very long or remote sessions.

Required PR Checks verify passed on this exact commit: f351fc5.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run re-reviewed the one commit since the prior pullfrog review (cbedfa91c7): a rebase-repair commit that restores two pieces of upstream behavior the branch had dropped while it was based on an older main.

  • Question cancellation restored — NativeChatResolvedView destructures and forwards cancelAsk from useNativeChatInteractiveSend into its NativeChatInteractiveSend object and its dependency list, so rejecting an active question regains the upstream cancel path (the type gained cancelAsk on main, so the omission also broke typecheck).
  • Retry reveal restored — NativeChatStructuredSession passes retryDelivery (wrapped by useStructuredNativeChatSubmitReveal) instead of the raw controller.retry, so retrying a delivery notice brings the latest into view again.
  • Coverage — a new NativeChatStructuredSession test pins reveal-on-retry end-to-end (it fails against the raw controller.retry), and the resolved-view mock gains the cancelAsk stub.

I traced the sibling submits wrappers (messageListRef, retryLaunch, queuedMessages, respond, revealLatest) and confirmed they were all still threaded through; retry was the only stale one. Both changed sites are correct and the added test can fail.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Jump to top moves to its own PR so this one lands the send reveal and
follow behaviour on their own.
@brennanb2025 brennanb2025 changed the title fix(native-chat): sending brings the latest into view, with visible jumps to latest and top fix(native-chat): sending brings the latest into view, with a visible jump to latest Oct 5, 2026
@brennanb2025

Copy link
Copy Markdown
Contributor

I split "Jump to top" out of this PR so the send and follow behaviour can land on its own. The up-arrow button and its "start of conversation" history jump now live in #25702 (credited to Kelvin), stacked on this one.

Pushed two commits here, no force:

The description now says "Part of #23797"; #25702 closes it. Web typecheck and the 67 native-chat test files touched pass.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run re-reviewed the commits since the prior pullfrog review (f351fc5e0c, #5410923145): a merge of main and a follow-up commit that removes the "Jump to top" feature from this PR.

  • Jump to top removed — NativeChatJumpControls now renders only the "Jump to latest" button (the Tooltip, spinner and ArrowUp/Loader2 are gone); useNativeChatRailHistoryJump drops HISTORY_START, startFromBeginning and startPending; useNativeChatTranscriptScroll drops scrollToTop and awayFromTop; jumpToStart is unthreaded from NativeChatMessageList; and the components.native-chat.jumpToTop keys are removed from all six locales. Jump to top moves to #25702.
  • Tooltip test scaffolding retired — the native-chat-app-root-test-render.tsx render wrapper (added only to supply the TooltipProvider the removed tooltip needed) is deleted, and the affected suites revert to importing render from @testing-library/react directly.
  • Rail-history-jump coverage restored — the useLaneRailHistoryJump tests revert to the id-targeted API, with the hidden-pane abort folded back in as its own case alongside list unmount.
  • main merged in — the branch now carries upstream's answerCommandLocally, onSlashCommand(command, output?), subagent roster disclosure, promptsUnanswerable and agent prop work; the reveal/follow feature is threaded through it unchanged.

I verified no reference to the removed symbols (jumpToTop, startFromBeginning, startPending, HISTORY_START, jumpToStart, native-chat-app-root-test-render) survives in the renderer, and that showOlderHistory and restoreScrollOffset still have live callers. The removal is complete and self-contained, and the merge preserved the feature.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

…ange

The solid, fading Jump to latest button moves to the UI PR (stablyai#25702) so this one carries only the scroll behaviour.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor note inline.

Reviewed changes

This run re-reviewed the one commit since the prior pullfrog review (b5974c11a8, #5421800817): a scope-reduction that returns "Jump to latest" to its pre-PR form.

  • Jump-to-latest control reverted — NativeChatJumpControls and its test are deleted, and the button returns to the inline, conditionally rendered form in NativeChatMessageList with the previous translucent bg-card/90 backdrop-blur styling.
  • Message-list plumbing tidied — NativeChatMessageList now destructures { scrollToBottom, scrollMessageToTop, ...follow } from useNativeChatTranscriptScroll and derives its ref type from the handle hook (Parameters<typeof useNativeChatMessageListHandle>[0]); the reader-navigation suite drops the focus-handback case and its inert check.

Note: 1 inline comment(s) dropped because they did not anchor to lines inside the PR diff:

  • src/renderer/src/components/native-chat/NativeChatMessageList.tsx:425-435 (RIGHT) — line 435 (RIGHT) is not inside a diff hunk

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@pullfrog

pullfrog Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Follow-up to the incremental review, whose inline note was dropped by the anchor pre-flight (the button body is unchanged from main, so only the {showJump ? ( → {follow.showJump ? ( line sits in a diff hunk).

Minor: reverting to the inline button also drops the focus handling NativeChatJumpControls added. There is no onMouseDown preventDefault, so a pointer press moves focus onto the button, and because it unmounts the moment follow.showJump flips false, a keyboard activation leaves focus on document.body. The PR description still advertises "Clicking a jump button leaves focus in the composer; if a focused button disappears, focus moves to the transcript." Either restore the handoff or correct the description.

No critical or blocking issues in the delta.

Pullfrog  | View workflow run | via Pullfrog | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025 brennanb2025 changed the title fix(native-chat): sending brings the latest into view, with a visible jump to latest fix(native-chat): sending brings the latest into view and follows the reply Oct 6, 2026
@brennanb2025

Copy link
Copy Markdown
Contributor

Split this one more step: this PR is now the scrolling behaviour only (send brings the latest into view and follows the reply, opening something stops following, keyboard access to the transcript).

The visual changes moved to #25702: the solid, fading "Jump to latest" (with its focus hand-off when it hides) joins "Jump to top" there. This PR renders "Jump to latest" exactly as main does today; it still uses your visibility and click handling. Nothing in the behaviour or its tests changed. The only test removed here is the focus/inert one, which moved with the button.

To stay under the file's line limit with the button back inline, NativeChatMessageList.tsx destructures the scroll hook as follow (the name use-native-chat-reader-opens already uses) and types the ref prop from the handle hook.

# Conflicts:
#	src/renderer/src/components/native-chat/NativeChatStructuredSession.test.tsx
#	src/renderer/src/components/native-chat/use-native-chat-composer-submit.test.tsx
@brennanb2025

Copy link
Copy Markdown
Contributor

Split the button work one more step, so there are three PRs, each reviewable on its own:

  1. fix(native-chat): sending brings the latest into view and follows the reply #24514 (this PR): the scrolling behaviour only.
  2. feat(native-chat): a solid, fading Jump to latest #25759: the solid, fading "Jump to latest" (stacked on this PR).
  3. feat(native-chat): a Jump to top button beside Jump to latest #25702: the "Jump to top" button (stacked on feat(native-chat): a solid, fading Jump to latest #25759).

I also merged current main into this branch. CI here was failing the 400-line limit on NativeChatComposer.tsx: the PR's onSubmitted lines plus a then-longer main version went over. main has since trimmed that file, so the merge alone fixes it. Two test-file conflicts with main's draft-store changes were resolved by keeping both sides.

brennanb2025 and others added 8 commits October 5, 2026 20:50
…layout

Replace open-row lifetime tracking with bounded position preservation, and tie delayed reveals to the originating reader and session. Keep queued drafts and picker actions from navigating the transcript.

Co-authored-by: Kelvin Amoaba <97001695+AmoabaKelvin@users.noreply.github.com>
A refused queue Resume moved readers before the host answered, and hiding during disclosure settlement could leave a reopened view at the tail without following new output. Resume now reveals its originating reader only after success, and a restored pane derives following from its actual distance to the end.

Merge current main while preserving its chat layout and the existing reader ownership and submission mechanisms.

Co-authored-by: Kelvin Amoaba <97001695+AmoabaKelvin@users.noreply.github.com>
The rail test rejected the retained-position takeover before a message jump. Check its offset and the selected row target, then verify the jump stays spent and a later diff can reveal.

Merge main to resolve the transcript styling conflict, preserve incoming typography and remove obsolete test props.

Co-authored-by: Kelvin Amoaba <97001695+AmoabaKelvin@users.noreply.github.com>
Pending reader takeover could keep an earlier end target once a duplicate 250 ms gate expired. Let the existing pending-frame check keep the reader's actual offset until that frame ends.

Move unchanged interactive reveal and approval projection into their existing modules so both view files meet the line limit.

Co-authored-by: Kelvin Amoaba <97001695+AmoabaKelvin@users.noreply.github.com>
Integrate the current main branch so the corrected protocol lint can let
required checks run. Preserve the chat scrolling change and both locale
updates, and format two existing test calls.

Co-authored-by: Kelvin Amoaba <97001695+AmoabaKelvin@users.noreply.github.com>
@brennanb2025

Copy link
Copy Markdown
Contributor

Sending in a long native chat could leave the new message below the viewport. The original patch could also move the reader after they had scrolled away or changed conversations, and could leave following paused after an opened tool row disappeared.

Reviewed and pushed head: 47a1fd9859d54e99d08c84912e3ae4197ee9ceba. Visible local submissions now reveal latest in their originating conversation. Passive output preserves history reading; newer navigation, hiding the pane or changing conversations cancels obsolete reveals. Queued drafts and picker-only actions preserve position. Explicit Resume reveals only after the host confirms it resumed.

The opened-row decision follows the common pattern: preserve the clicked position while expansion or collapse settles, then derive following from the reader's position. Leaving a row open does not pause following for its lifetime. The existing virtualizer owns cancellation and the pending reader-scroll frame, so there is no second deadline that can stop reconciling a still-pending frame.

The review covered the whole PR adversarially, including an independent architecture proposal drawn from the issue, separate submission and scroll-geometry reviews, focused fixes, fresh reviewers after changes, and a final readiness review without counsel. All 14 scored findings are fixed: nine P2 and five P3. No P0/P1 candidate or open proven finding remains. The table gives source lines at discovery, with evidence and the correction; filenames are under src/renderer/src/components/native-chat/.

Finding Cause Source at discovery Evidence and correction
P2 R1-01 PR use-native-chat-structured-composer-send.ts:76, use-native-chat-composer-submit.ts:94, use-native-chat-session-option-command.ts:116 Three deferred-result reproductions overrode newer reader movement. Capture reader ownership at submission and validate it at completion.
P2 R1-02 PR native-chat-autoscroll.ts:98, NativeChatToolRun.tsx:317 Source construction showed hidden or removed details leaving the open-row pause set. Replace lifetime tracking with bounded position preservation and following derived from geometry.
P2 R1-03 PR use-native-chat-session-option-command.ts:116, use-native-chat-reveal-latest.ts:18 A hook rerender reproduction navigated a replacement conversation. Bind delayed results to their originating transcript.
P2 R1-04 PR, incomplete fix use-native-chat-pty-composer-send.ts:68, use-native-chat-picker-command-dispatch.ts:69 Two reproductions showed locally answered /context adding content without revealing it. Include visible local responses in the submission result. This also existed on main; it was an omission from the requested fix.
P3 R1-05 PR use-native-chat-structured-composer-send.ts:75 Two real-dispatcher assertions showed picker-only /model and /effort revealing latest. Distinguish opening a picker from adding visible content.
P2 R1-06 PR NativeChatMessageList.tsx:278, use-native-chat-transcript-scroll.ts:184 Source/controller construction showed held responses surviving newer history navigation or pane hiding. Invalidate reader ownership when navigation begins and when the pane hides or changes.
P2 R1-07 PR use-native-chat-transcript-scroll.ts:161 Production and virtualizer source showed an old end target surviving expansion and resolving to an inserted row. Replace the pending target with the retained offset before layout changes. The specific rendered frame ordering remains unverified.
P3 R1-08 PR native-chat-reader-scroll-input.ts:34, native-chat-root-key-routing.ts:37, native-chat-typing-redirect.ts:31 Source tracing showed Space/Shift+Space in the newly focusable transcript being intercepted by terminal typing capture. Preserve transcript keyboard routing through the existing ignore selector.
P2 R2-01 PR use-structured-native-chat-submit-reveal.ts:70 Source tracing showed Resume revealing before host success, including refusal or no-op. Use the existing resumed result and originating reader ownership.
P2 R2-02 Review loop use-native-chat-transcript-scroll.ts:198 The actual list/virtualizer test harness hid before expansion settled, reopened at the end, then ended 8,248 px away after 200 passive rows; the no-hide control passed. Re-derive following after restoring the viewport.
P3 R3-01 PR NativeChatMessageList.message-rail-windowing.test.tsx:98 CI and a fresh rerun disproved the one-scroll assertion: takeover and selected-row alignment are separate operations. Assert both positions and no repeated navigation. No user-facing rail defect was demonstrated.
P3 R4-01 Review loop NativeChatResolvedView.tsx:458 Executed CI source and scoped lint proved the main integration exceeded the 400-line limit. Extract existing logic without changing its expression or adding a suppression.
P3 R4-02 Review loop NativeChatStructuredSession.tsx:424 The same executed-source/lint controls proved this file also exceeded its limit. Extract existing logic while preserving submission, queue and cancellation behavior.
P2 R4-03 Review loop use-native-chat-transcript-scroll.ts:146 A deterministic hook reproduction held the pending frame past 250 ms: the viewport moved to 2,000 px while its target stayed at 8,204 px. Immediate, 250 ms and released-frame controls passed. Remove the duplicate caller deadline; the existing pending frame owns reconciliation. This proves the omitted update, not a rendered reversal.

Fixes landed in 5441267 (R1), ac137ff (R2), efefff8 (R3), and 04a1286 (R4). Final head 47a1fd9 normally merges pinned main 4de9f9f85a858c711a1513852407354337e1bd36. That main change fixes the earlier incoming-main protocol line-limit failure. This integration changes no PR production design; its authored corrections are formatting only. Transport remains unchanged, with no new wire or persisted state.

Deferred P3 R1-D1: the solid Jump to latest control is #25759, and Jump to top is #25702. This PR does not complete all of #23797; semantic history restoration after an actual transcript unmount remains a wider limitation.

Final-head local verification passes: 409 targeted tests across 29 explicit files, one queued renderer typecheck, one serial queued Node typecheck, and scoped lint/format/quality checks across all 59 changed source files. Only two unchanged React effect warnings remain. The promoted pending-frame regression fails before its correction and passes afterward. Counts are from the final committed run, not added across rounds. Hook/DOM geometry tests do not certify Chromium rendering or live-provider delivery.

Final CI is complete: 32 checks, 16 successful, 16 skipped, zero failures or cancellations; all four workflows succeeded. The successful PR Checks run includes static analysis/typecheck, all five Node unit shards, relay integration, both package jobs (including Windows), and aggregate verification. Headless persistence, Mobile verification and test line-count checks also passed. The static/typecheck and unit shard 2 checkout logs confirm merge commit 739c2ee4db365b0ea131313cb0bdb4e21c371abd, with pinned main and this PR head as parents. Its tree and all 66 changed path blobs match the final head. There is no final red to classify as PR-caused, review-loop-caused, also on main, or a flake. Earlier failed heads are not final-head evidence.

The 16 skipped checks remain unverified: end-to-end, cross-version wire, Git compatibility, shell contracts, managed hooks on Node 18, browser provider, Codex index repair, xterm synchronization, real IME, real WSL terminal, mobile bundle/tests-on-main, desktop template, and three Linux runtime compatibility jobs. A successful Windows package is not Windows runtime QA.

Rendered/Chromium, SSH and Windows runtime QA were not completed, and there are no final rendered screenshots. M4Air was overloaded and serialized behind another live rig; earlier builds did not reach a usable renderer. Windows Codex startup was blocked by a confirmed update prompt and launcher-reported sign-in requirements, with sign-in disallowed; the low-spec attachment attempt also failed before execution. Openclaw was unreachable. No final current-main/PR rendered comparison ran. Brennan explicitly instructed us to finish without waiting for an M4Air slot. Actual provider/SSH delivery, physical input, reduced-motion rendering and mobile emulator behavior remain unverified. Earlier M4Air drivers were released and their owned processes and disposable profiles cleaned up. The launcher closed the high-spec Windows prompt terminal; the low-spec attachment was abandoned before an observed app launch. Remote evidence checkouts are retained. The PR has not been merged, and Brennan retains that decision.

# Conflicts:
#	src/renderer/src/components/native-chat/NativeChatJumpToLatest.tsx
#	src/renderer/src/components/native-chat/NativeChatMessageList.tsx
#	src/renderer/src/components/native-chat/NativeChatStructuredSession.test-harness.tsx
#	src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx
#	src/renderer/src/components/native-chat/use-structured-agent-session.ts
… at the press

Opening or closing a row used to stop the transcript from following the
newest output. A reader at the live end who expanded a tool run then lost
the stream. Sends that wait on the host (answers, commands, goals, option
changes) only brought the latest into view after the host replied, so the
reader watched nothing happen at the press.

Now following stops only on a reader gesture: an upward wheel or scroll key
when there is content above, a scrollbar press, a touch drag once it has
carried the view off the end, or a press on content while already away from
the end. Opening, closing, layout changes and the app's own scrolls never
stop it, and returning to the end resumes it. Every local send reveals the
latest at the press. A message that waits as a queued card, a message from
another device, and Stop leave a reader who scrolled up where they are.
Queue Resume still reveals only after the host lifts the pause, and only in
the pane that pressed it while that pane is shown.

Removes the disclosure position hold, the reader-opens wiring, the held
reveal and its reader generation tracking. The queued-card decision moves
into the outbox send so the session hook stays under the line limit. Also
restores the transcript label import and the approval card's verified send
that the merge with main dropped.
# Conflicts:
#	src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx
#	src/renderer/src/components/native-chat/use-structured-agent-session-outbox.ts
#	src/renderer/src/components/native-chat/use-structured-agent-session.ts
@brennanb2025

brennanb2025 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Reworked the scrolling rules to the common pattern for chat transcripts, and merged current main (head a699e539863).

What's different from the previous head:

  • Opening or closing a row no longer changes following. Before, opening a tool run held it in place and stopped following, and closing it could resume. Now at the live end the view stays pinned as output streams, and a reader who scrolled up stays put.
  • Only your own scrolling stops following (wheel or keys up, scrollbar, a click while already scrolled up, a touch drag off the end). Scroll events alone never stop it, so content shrinking or layout changes can't knock you off the end.
  • Every send from this pane reveals the latest when you press it, not after the host accepts. That removes the held-reveal tracking. Resuming a paused queue still reveals only once the host confirms it resumed.

The production diff shrank from about +927 to +608 lines, and two files are gone (use-native-chat-disclosure-position.ts, use-native-chat-reader-opens.ts). The review loop's fixes that still apply are kept, e.g. a reveal never moves another pane, a hidden or reopened pane works out whether it's at the end, and the rail scrolls once per pick. The ones about held reveals and opened rows no longer apply, because that code is gone.

Also from the main merge: the "Jump to latest" button now uses the solid version from #25759, and NativeChatComposer.tsx shares the two terminal send paths' common options to stay under its line limit.

All 386 native-chat and structured-session test files pass, plus the web typecheck and lint. For each of the 15 rules, removing it makes its test fail. The running app hasn't been re-tested since the rework; a new recording will follow. The PR description is updated.

This supersedes the earlier review summary (#24514 (comment)). That one describes the pre-rework design: the opened-row hold and held reveals are gone now.

@brennanb2025
brennanb2025 merged commit 86d03ff into stablyai:main Oct 6, 2026
53 of 55 checks passed
brennanb2025 added a commit that referenced this pull request Oct 7, 2026
…r-stop

Main's #24514 reveals the transcript on a send and on a Resume that lifts the
pause. This branch's composer Resume goes through the same reveal; the queue's
Resume answers whether it lifted the pause on both surfaces. The composer's
awaitable structured send keeps main's command reveal at the press, and main's
new navigation test fires that send without awaiting it.

Line limits after the merge: the composer's working/Stop rule moves into the
structured stop controls; the queue controller takes the transport's fields by
spread; the option picker request type is named; test harness resets combined.
brennanb2025 added a commit that referenced this pull request Oct 7, 2026
…brennanb2025/nc-host-send-queue

- Queue UI (#24586) taken as main has it: paused header, Resume, Steer, clear-queue question,
  `queueSendsNext` working state, composer Resume and queue hold.
- The chat's send answers 'queued' when the transcript draws it as a card (main's rule, read through
  the in-memory sends' own projection), so the composer moves the reader only for a bubble.
- The failed-start send wrapper passes that answer through; the submit reveal hook drops the
  per-message Retry this branch removed (launch Retry still reveals).
- The pending-send projection takes any send shape, so the node project's queued-card test does not
  reach the renderer store.
- Deleted outbox files stay deleted; #24514's queued-card admission test is ported to the sends hook.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants