fix: stabilize hold-mode assistant autoscroll#533
Open
pascalandr wants to merge 7 commits into
Open
Conversation
Hold mode now latches after manual escape or when the Assistant answer text reaches the viewport top, and it remains held until the user manually reaches the true bottom. Content growth, hold-target changes, and stream completion no longer re-enable follow or snap the viewport while held. Hold target eligibility is restricted to user-readable Assistant answer text. Tool/status/output blocks and reflection/reasoning-only content are excluded from hold targeting, and held Assistant text anchors are restored across subsequent content renders. Verification: node --test packages/ui/src/components/virtual-follow-behavior.test.ts; npm run typecheck --workspace @codenomad/ui; npm run build --workspace @codenomad/ui.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27159386614 Artifacts expire in 7 days.
|
Prompt submission now performs an explicit bottom jump that suppresses stale Hold latches before and after sending, so the optimistic user prompt and the new exchange return to the true bottom after manual scrolling in Hold mode. The virtual follow controller also exposes a focused hold-clear transition for disabling Hold while held, and optimistic user rendering now keeps only the primary submitted text visible while preserving synthetic helper-part hiding. Verified with the virtual follow behavior node test, UI typecheck, UI build, and diff whitespace checks.
Adds a durable submit/new-stream bottom-follow intent so Hold mode keeps the newly submitted prompt and assistant stream at the bottom until the new exchange is mounted and settled. Suppresses stale previous Assistant hold anchors and snapshot restores while that intent is active, then preserves suppression if the old target is still current so it cannot reclaim the viewport at stream start. Validated with node --test packages/ui/src/components/virtual-follow-behavior.test.ts, npm run typecheck --workspace @codenomad/ui, npm run build --workspace @codenomad/ui, and git diff --check.
Contributor
Author
|
I want you to perform the gatekeeper checks for latest PR state check for regressions |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27163704081 Artifacts expire in 7 days. |
Contributor
|
Gatekeeper review complete for the latest PR state. Request understood: perform a ruthless review for regressions and better implementations. I treated this as review-only and did not modify code. Findings:
Validation:
Gatekeeper verdict: do not merge as-is. The active-hold latching behavior is directionally correct, but the broad -- |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27163817328 Artifacts expire in 7 days. |
Limit the virtual follow bottom-pin suspension gate to external suspension or an actively latched Hold target, so escaped Assistant streaming can rejoin near the bottom while active Hold remains latched until true bottom. Adds behavior coverage for future eligible hold targets versus active hold latches.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27164365361 Artifacts expire in 7 days.
|
Contributor
Author
|
Gatekeeper blocker from #533 (comment) addressed in c236b7f. The bottom-pin suspension now applies only to external suspension or an actively latched Hold target, so ordinary escaped streaming can rejoin near the bottom while active Hold still remains latched until true bottom. Added regression coverage; local validation passed 37/37 virtual-follow tests, UI typecheck, UI build, and diff checks. The medium note about measuring a dedicated inner Assistant text element remains a documented non-blocking follow-up. |
Contributor
Author
|
@CodeNomadBot |
Contributor
|
Gatekeeper re-review complete for the latest PR state after Request understood: “again” means re-run the same ruthless PR gatekeeper check against the current branch. I treated this as review-only and did not modify code. Findings:
Validation from this bot checkout:
Gatekeeper verdict: mergeable from code-review perspective. I do not see a remaining blocking regression in the latest diff. The CI/local dependency-backed typecheck and build results should be trusted over this bot checkout for those two commands because this environment is missing required packages. -- |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27164936545 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27478538808 Artifacts expire in 7 days.
|
Collaborator
Hold-mode scroll state follow-upI checked the current Hold + prompt-submit behavior and found a few state-machine issues that can still cause stale Hold state or unwanted bottom-follow jumps. 1. Issues found
2. How we fixed it locally
Validation run locally: 3. Actual diff from local changesdiff --git a/packages/ui/src/components/virtual-follow-behavior.test.ts b/packages/ui/src/components/virtual-follow-behavior.test.ts
index daaa0d7a..c144e4ca 100644
--- a/packages/ui/src/components/virtual-follow-behavior.test.ts
+++ b/packages/ui/src/components/virtual-follow-behavior.test.ts
@@ -12,8 +12,8 @@ import {
type ScrollControllerMetrics,
} from "./virtual-follow-behavior.ts"
-const userScroll = (direction: "up" | "down" | null, atBottom: boolean, canPinToBottom = false) =>
- ({ type: "user-scroll", direction, atBottom, canPinToBottom }) as const
+const userScroll = (direction: "up" | "down" | null, atBottom: boolean, canPinToBottom = false, suppressRejoin = false) =>
+ ({ type: "user-scroll", direction, atBottom, canPinToBottom, suppressRejoin }) as const
function metrics(offset: number, scrollHeight = 3000, clientHeight = 600): ScrollControllerMetrics {
return {
@@ -39,6 +39,20 @@ describe("virtual follow behavior", () => {
assert.deepEqual(next.effect, { type: "none" })
})
+ it("does not magnet to bottom after releasing hold while the stale target is current", () => {
+ const next = transitionFollowMode({ type: "escaped" }, userScroll("down", false, true, true))
+
+ assert.deepEqual(next.mode, { type: "escaped" })
+ assert.deepEqual(next.effect, { type: "none" })
+ })
+
+ it("rejoins follow at true bottom after releasing hold", () => {
+ const next = transitionFollowMode({ type: "escaped" }, userScroll("down", true, true, true))
+
+ assert.deepEqual(next.mode, { type: "following" })
+ assert.deepEqual(next.effect, { type: "none" })
+ })
+
it("rejoins follow and pins bottom when escaped user scrolls down with pin permission", () => {
const next = transitionFollowMode({ type: "escaped" }, userScroll("down", false, true))
@@ -53,10 +67,17 @@ describe("virtual follow behavior", () => {
assert.deepEqual(next.effect, { type: "none" })
})
- it("keeps hold latched when the user scrolls down above bottom", () => {
+ it("exits hold when the user scrolls down above bottom", () => {
const next = transitionFollowMode({ type: "holding", key: "message-1" }, userScroll("down", false, true))
- assert.deepEqual(next.mode, { type: "holding", key: "message-1" })
+ assert.deepEqual(next.mode, { type: "escaped" })
+ assert.deepEqual(next.effect, { type: "none" })
+ })
+
+ it("does not rejoin bottom follow from a held manual down-scroll reported at bottom", () => {
+ const next = transitionFollowMode({ type: "holding", key: "message-1" }, userScroll("down", true, true))
+
+ assert.deepEqual(next.mode, { type: "escaped" })
assert.deepEqual(next.effect, { type: "none" })
})
@@ -74,10 +95,10 @@ describe("virtual follow behavior", () => {
assert.deepEqual(next.effect, { type: "none" })
})
- it("keeps hold latched for directionless user scroll away from bottom", () => {
+ it("exits hold for directionless user scroll away from bottom", () => {
const next = transitionFollowMode({ type: "holding", key: "message-1" }, userScroll(null, false, true))
- assert.deepEqual(next.mode, { type: "holding", key: "message-1" })
+ assert.deepEqual(next.mode, { type: "escaped" })
assert.deepEqual(next.effect, { type: "none" })
})
@@ -103,17 +124,17 @@ describe("virtual follow behavior", () => {
assert.deepEqual(next.effect, { type: "align-hold", key: "message-1" })
})
- it("keeps hold latched when the hold target disappears", () => {
+ it("exits hold without jumping when the hold target disappears", () => {
const next = transitionFollowMode({ type: "holding", key: "message-1" }, { type: "hold-target-changed", key: null, canPinToBottom: true })
- assert.deepEqual(next.mode, { type: "holding", key: "message-1" })
+ assert.deepEqual(next.mode, { type: "escaped" })
assert.deepEqual(next.effect, { type: "none" })
})
- it("keeps hold latched when a later hold target is reported", () => {
+ it("exits hold without jumping when a later hold target is reported", () => {
const next = transitionFollowMode({ type: "holding", key: "message-1" }, { type: "hold-target-changed", key: "message-2", canPinToBottom: true })
- assert.deepEqual(next.mode, { type: "holding", key: "message-1" })
+ assert.deepEqual(next.mode, { type: "escaped" })
assert.deepEqual(next.effect, { type: "none" })
})
@@ -124,6 +145,17 @@ describe("virtual follow behavior", () => {
assert.deepEqual(next.effect, { type: "scroll-bottom", immediate: true, suppressHold: true })
})
+ it("explicit top jumps leave hold and forget the held id", () => {
+ const controller = new VirtualScrollController(true)
+ controller.holdCandidate("message-1", true)
+
+ const result = controller.jumpTop(true)
+
+ assert.deepEqual(result.state.mode, { type: "escaped" })
+ assert.deepEqual(result.effect, { type: "scroll-top", immediate: true })
+ assert.equal(controller.heldKey(), null)
+ })
+
it("prompt submission overrides a stale hold latch and returns to bottom follow", () => {
const controller = new VirtualScrollController(true)
controller.holdCandidate("old-assistant-answer", true)
@@ -184,7 +216,7 @@ describe("virtual follow behavior", () => {
assert.deepEqual(next.effect, { type: "scroll-bottom", immediate: true, suppressHold: false })
})
- it("keeps auto-pin suspended while a hold target is actively latched", () => {
+ it("suspends auto-pin while active but exits hold on manual scroll", () => {
const suspend = shouldSuspendAutoPinToBottomForHold({
externalSuspend: false,
activeHoldTargetKey: "streaming-assistant-answer",
@@ -194,7 +226,7 @@ describe("virtual follow behavior", () => {
const next = transitionFollowMode({ type: "holding", key: "streaming-assistant-answer" }, userScroll("down", false, !suspend))
assert.equal(suspend, true)
- assert.deepEqual(next.mode, { type: "holding", key: "streaming-assistant-answer" })
+ assert.deepEqual(next.mode, { type: "escaped" })
assert.deepEqual(next.effect, { type: "none" })
})
@@ -238,14 +270,15 @@ describe("virtual follow behavior", () => {
assert.deepEqual(result.effect, { type: "none" })
})
- it("does not resume or snap when a held target disappears", () => {
+ it("forgets the held id without scrolling when a held target disappears", () => {
const controller = new VirtualScrollController(true)
controller.holdCandidate("message-1", true)
const result = controller.holdTargetChanged(null, true)
- assert.deepEqual(result.state.mode, { type: "holding", key: "message-1" })
+ assert.deepEqual(result.state.mode, { type: "escaped" })
assert.deepEqual(result.effect, { type: "none" })
+ assert.equal(controller.heldKey(), null)
})
it("lets fresh user upward movement escape even during a programmatic window", () => {
@@ -300,7 +333,7 @@ describe("virtual follow behavior", () => {
assert.deepEqual(result.effect, { type: "none" })
})
- it("keeps hold latched until downward movement reaches actual bottom", () => {
+ it("exits hold immediately on downward movement before actual bottom", () => {
const controller = new VirtualScrollController(true)
controller.holdCandidate("message-1", true)
controller.recordProgrammaticOffset(2100, false)
@@ -308,14 +341,9 @@ describe("virtual follow behavior", () => {
const nearBottom = controller.observeViewport(metrics(2220), 100, false, true)
- assert.deepEqual(nearBottom.state.mode, { type: "holding", key: "message-1" })
+ assert.deepEqual(nearBottom.state.mode, { type: "escaped" })
assert.deepEqual(nearBottom.effect, { type: "none" })
-
- controller.setUserIntent("down", 800)
- const atBottom = controller.observeViewport(metrics(2400), 200, false, true)
-
- assert.deepEqual(atBottom.state.mode, { type: "following" })
- assert.deepEqual(atBottom.effect, { type: "none" })
+ assert.equal(controller.heldKey(), null)
})
it("still escapes follow on upward movement at bottom", () => {
diff --git a/packages/ui/src/components/virtual-follow-behavior.ts b/packages/ui/src/components/virtual-follow-behavior.ts
index 84e111e3..fd5cd57d 100644
--- a/packages/ui/src/components/virtual-follow-behavior.ts
+++ b/packages/ui/src/components/virtual-follow-behavior.ts
@@ -11,7 +11,7 @@ export type FollowEffect =
| { type: "align-hold"; key: string }
export type FollowEvent =
- | { type: "user-scroll"; direction: "up" | "down" | null; atBottom: boolean; canPinToBottom: boolean }
+ | { type: "user-scroll"; direction: "up" | "down" | null; atBottom: boolean; canPinToBottom: boolean; suppressRejoin?: boolean }
| { type: "jump-top"; immediate: boolean }
| { type: "jump-bottom"; immediate: boolean; explicit: boolean }
| { type: "jump-key"; key: string; block: ScrollLogicalPosition; smooth: boolean; followAfter: boolean }
@@ -95,15 +95,12 @@ export function transitionFollowMode(mode: FollowMode, event: FollowEvent): Foll
switch (event.type) {
case "user-scroll": {
if (mode.type === "holding") {
- if (event.atBottom && event.direction !== "up") {
- return { mode: { type: "following" }, effect: noFollowEffect }
- }
- return { mode, effect: noFollowEffect }
+ return { mode: { type: "escaped" }, effect: noFollowEffect }
}
if (event.direction === "up") {
return { mode: { type: "escaped" }, effect: noFollowEffect }
}
- if (mode.type === "escaped" && event.direction === "down" && event.canPinToBottom) {
+ if (!event.suppressRejoin && mode.type === "escaped" && event.direction === "down" && event.canPinToBottom) {
return {
mode: { type: "following" },
effect: { type: "scroll-bottom", immediate: true, suppressHold: false },
@@ -143,7 +140,13 @@ export function transitionFollowMode(mode: FollowMode, event: FollowEvent): Foll
return { mode, effect: noFollowEffect }
case "hold-target-changed":
- return { mode, effect: noFollowEffect }
+ if (mode.type !== "holding" || event.key === mode.key) {
+ return { mode, effect: noFollowEffect }
+ }
+ return {
+ mode: { type: "escaped" },
+ effect: noFollowEffect,
+ }
case "clear-hold":
if (mode.type !== "holding") {
@@ -267,7 +270,7 @@ export class VirtualScrollController {
return this.result(next.effect)
}
- observeViewport(metrics: ScrollControllerMetrics, now: number, programmatic: boolean, canPinToBottom = false): ScrollControllerResult {
+ observeViewport(metrics: ScrollControllerMetrics, now: number, programmatic: boolean, canPinToBottom = false, suppressRejoin = false): ScrollControllerResult {
const previousOffset = this.state.lastObservedOffset
const offset = metrics.offset
const scrolledUp = offset < previousOffset - 1
@@ -295,6 +298,7 @@ export class VirtualScrollController {
direction,
atBottom,
canPinToBottom: canMagnetToBottom,
+ suppressRejoin,
})
this.state.mode = next.mode
this.state.lastObservedAtBottom = this.isAutoFollowing() && atBottom
diff --git a/packages/ui/src/components/virtual-follow-list.tsx b/packages/ui/src/components/virtual-follow-list.tsx
index f1a0a257..33ee36b0 100644
--- a/packages/ui/src/components/virtual-follow-list.tsx
+++ b/packages/ui/src/components/virtual-follow-list.tsx
@@ -210,6 +210,7 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
let lastResetKey: string | number | undefined
let suppressAutoScrollOnce = false
let suppressHoldUntilTargetChanges = false
+ let suppressedHoldTargetKey: string | null = null
let lastItemsReference = props.items()
let restoreToken = 0
@@ -265,6 +266,7 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
}
function resetStaleHoldForBottomIntent() {
+ suppressedHoldTargetKey = activeHoldTargetKey() ?? holdTargetKey() ?? suppressedHoldTargetKey
restoreToken += 1
scrollController.setRestoring(false)
setDidTriggerHoldForCurrentTarget(false)
@@ -332,8 +334,12 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
}
function syncControllerResult(result: ScrollControllerResult) {
+ const previousHeldKey = activeHoldTargetKey()
setFollowMode(result.state.mode)
if (result.state.mode.type !== "holding") {
+ if (previousHeldKey !== null) {
+ suppressedHoldTargetKey = previousHeldKey
+ }
clearHeldAnchor()
} else if (heldAnchorKey !== null && heldAnchorKey !== result.state.mode.key) {
clearHeldAnchor()
@@ -363,6 +369,7 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
performance.now(),
hasProgrammaticScrollIntent(),
canRejoinFollowFromDownScroll(metrics),
+ shouldSuppressBottomRejoinForReleasedHold(),
)
break
}
@@ -517,6 +524,7 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
if (!streamingActive()) return false
if (effectiveSuspendAutoPinToBottom()) return false
if (activeHoldTargetKey() !== null) return false
+ if (shouldSuppressBottomRejoinForReleasedHold()) return false
const items = props.items()
if (items.length === 0) return false
if (isAtBottom(metrics)) return true
@@ -527,6 +535,10 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
return viewportEndIndex >= Math.max(items.length - DEFAULT_REJOIN_LAST_ITEM_COUNT, 0)
}
+ function shouldSuppressBottomRejoinForReleasedHold() {
+ return suppressedHoldTargetKey !== null && holdTargetKey() === suppressedHoldTargetKey
+ }
+
function getSnapshotMetrics(element: HTMLDivElement, handle?: VirtualizerHandle) {
const scrollTop = handle?.scrollOffset ?? element.scrollTop
const scrollHeight = handle ? getCurrentScrollSize(element, handle) : element.scrollHeight
@@ -728,7 +740,13 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
setShowScrollBottomButton(hasItems && !atBottom)
setShowScrollTopButton(hasItems && !atTop)
- const result = scrollController.observeViewport(metrics, now, programmatic, canRejoinFollowFromDownScroll(metrics))
+ const result = scrollController.observeViewport(
+ metrics,
+ now,
+ programmatic,
+ canRejoinFollowFromDownScroll(metrics),
+ shouldSuppressBottomRejoinForReleasedHold(),
+ )
if (result.state.mode.type !== followMode().type || result.effect.type !== "none") {
suppressHoldUntilTargetChanges = false
}
@@ -881,6 +899,7 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
}
function alignHoldTarget(key: string) {
+ if (activeHoldTargetKey() !== key) return
const element = scrollElement()
if (!element) return
const target = resolveHoldTargetElement(key)
@@ -920,6 +939,7 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
if (!autoScroll()) return
if (externalSuspendAutoPinToBottom()) return
if (!targetKey) return
+ if (targetKey === suppressedHoldTargetKey) return
if (didTriggerHoldForCurrentTarget()) return
if (suppressHoldUntilTargetChanges) return
@@ -1003,6 +1023,7 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
itemElements.clear()
setDidTriggerHoldForCurrentTarget(false)
suppressHoldUntilTargetChanges = false
+ suppressedHoldTargetKey = null
pendingBottomRepinAfterHold = false
clearHeldAnchor()
}))
@@ -1014,6 +1035,9 @@ export default function VirtualFollowList<T>(props: VirtualFollowListProps<T>) {
if (nextTargetKey !== prevTargetKey && !hasActiveBottomFollowIntent()) {
suppressHoldUntilTargetChanges = false
}
+ if (nextTargetKey !== suppressedHoldTargetKey && nextTargetKey !== null) {
+ suppressedHoldTargetKey = null
+ }
if (activeHoldTargetKey() === null) return
if (nextTargetKey === activeHoldTargetKey()) return
dispatchFollowEvent({ type: "hold-target-changed", key: nextTargetKey, canPinToBottom: !externalSuspendAutoPinToBottom() }) |
Collaborator
|
@pascalandr - Please apply the changes suggested and test. Thanks |
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
2 participants
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.
Summary
Validation