Chat transcript scroll ownership: Solid port of use-stick-to-bottom, reference-exact card contract - #437
Open
omridevk wants to merge 22 commits into
Open
Chat transcript scroll ownership: Solid port of use-stick-to-bottom, reference-exact card contract#437omridevk wants to merge 22 commits into
omridevk wants to merge 22 commits into
Conversation
…ment instead of the stale published flag (#397) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nter/selection escape (#432) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…latch, holds only on user toggles, preview pin follows viewport (#430) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… trackpad momentum (#432) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… the bottom (#430) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-to-bottom resize handler (#397) observeContent wired the MutationObserver to both re-subscribe children AND call onResize() directly, on top of the ResizeObserver's own async firing for the same DOM change. Upstream use-stick-to-bottom has exactly one resize-difference source (ResizeObserver on a stable content node); the doubled, independently-timed trigger let the 1ms resizeDifference reset from one call race the scroll settle of the other, so a native scrollTop clamp during a resize could get misread as a user up-scroll and silently latch escapedFromLock/isAtBottom false with no wheel or scroll input. The next turn's growth then never re-engaged the follow chase (ring buffer of scroll writes stayed empty). Fix: MutationObserver now only keeps ResizeObserver child subscriptions current (persistent WeakSet, no disconnect/reobserve churn); onResize fires solely from ResizeObserver, matching upstream's single-source contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…capped/grow presentation prop (#430) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t-ui exactly (#430) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#397) Move createStickToBottom out of ui-kit-chat/src/behaviors into its own published package: use-stick-to-bottom (StackBlitz, MIT) has no Solid port on npm, so we claim the ecosystem gap under @Conciv scope. ui-kit-chat now depends on it as a workspace dep; its public API is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n, ported from assistant-ui useScrollLock (#430) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ault on, matching assistant-ui (#430) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… keys on reasoning only (#430) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tract (#397 #430 #432) Extends behaviors/scroll.stories.tsx with real-input (vitest/browser userEvent.wheel) coverage of wheel-escape, escape-then-collapse, and send-message re-pin scenarios, replacing a direct scrollTop assignment in the pre-existing ScrollToEndNoLayoutShift story with a real wheel gesture (direct scrollTop writes proved unreliable for triggering the escape path in headless Chromium). Adds tool-card.stories.tsx covering the approval force-open-once contract (userOpen wins over a re- triggered approval). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…input behavior stories (#397 #432) Adds stick-to-bottom.stories.tsx: pinned-follow-through-growth, fresh-turn re-engagement (the observeContent single-source regression story, replacing test/stick-to-bottom-resume.browser.test.tsx), real wheel escape/re-lock, silent re-lock on shrink (pinned and parked- above), and a real CDP mouse-drag selection guard. Real input is bridged via vitest/browser's userEvent.wheel and cdp() Input.dispatch- MouseEvent — storybook/test's userEvent dispatches synthetic events that do not trigger native scroll/wheel handling. Registers the package's stories with the storybook app (apps/storybook had no glob entry for it yet) and adds a .fallowrc.json threshold override: stick-to-bottom.ts is exercised only by *.stories.tsx now, which fallow's ignorePatterns excludes from coverage estimation, so deleting the last *.test.tsx here reads as a coverage drop rather than the coverage move it actually is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… storybook stories Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… real-pointer hover state (#430) Previous Turn Actions Reveal On Hover flaked only when other story files ran first in the same shared vitest-browser page (isolate:false, maxWorkers:2): a real leftover Chromium pointer position from an earlier story landed over the "older" message on mount, and Chromium's native hover recompute fired a genuine mouseenter that set chat.view.hovering before the play function ran. A single userEvent.hover/unhover/pointer call before the baseline assertion raced against that async native recompute and sometimes lost. Re-issuing unhover(older) inside the retrying waitFor gives it repeated real-pointer chances to win the race, converging the baseline deterministically. No production code changed - action-bar.tsx's hover-reveal logic is correct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ot pixels (#430 #432) Thread.Viewport now surfaces data-at-bottom/data-escaped attributes (mirroring the existing zag/ark data-state convention), backed by escapedFromLock now exposed through ThreadAutoScroll. Story assertions for solid-stick-to-bottom and ui-kit-chat's scroll behaviors read those attributes and text/role queries instead of scrollTop/±px math, per Omri's ruling that pixel-delta assertions are flaky and unreadable. Added one typed, cast-free locator helper per stories file, replacing all `as HTMLElement` querySelector casts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdded ChangesChat scrolling and card state ownership
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/solid-stick-to-bottom/src/stick-to-bottom.stories.tsx (1)
87-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the imperative API.
GrowthHarnessexposes only row mutation. No story callsstick.scrollToBottom()orstick.stopScroll(), so two of the five public members ofStickToBottomstay unexercised. The.fallowrc.jsonCRAP override states that the story suite drives this module directly, so the gap also weakens that justification.Expose both methods through
GrowthHarnessApiand add a story that escapes with a wheel-up, then re-pins withscrollToBottom(), and a story that callsstopScroll()during growth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solid-stick-to-bottom/src/stick-to-bottom.stories.tsx` around lines 87 - 115, Extend GrowthHarnessApi and GrowthHarness to expose imperative wrappers for stick.scrollToBottom() and stick.stopScroll(). Add stories covering wheel-up escape followed by scrollToBottom() re-pinning, and invoking stopScroll() while rows grow, ensuring both public methods are exercised.packages/solid-stick-to-bottom/src/stick-to-bottom.ts (1)
338-346: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider sharing the pointer-state listeners across instances.
Each
createStickToBottomcall adds threedocumentlisteners.packages/ui-kit-chat/src/styled/chain-of-thought.tsxcallscreateStickToBottomper chain-of-thought shell, so a long transcript registers three document listeners per card. The listeners only maintain one boolean.A module-level shared pointer tracker with reference counting keeps behavior identical and bounds the listener count.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solid-stick-to-bottom/src/stick-to-bottom.ts` around lines 338 - 346, Update the pointer-state handling in createStickToBottom to use a module-level shared tracker with reference counting instead of registering three document listeners per instance. Attach the mousedown, mouseup, and click listeners when the first instance is created, share the boolean state across instances, and remove the listeners when the final instance is disposed while preserving existing pointer behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/solid-stick-to-bottom/LICENSE`:
- Line 4: Update the copyright holder on the second line of the LICENSE from
“aidx contributors” to “conciv” so the shipped attribution matches the
repository and published package.
In `@packages/ui-kit-chat/src/styled/thread.tsx`:
- Line 83: Update the Reasoning component invocation in the thinking-part
rendering path to pass streaming only when the current part is the final active
thinking part, so its CollapsibleCard opens during streaming. Keep non-final or
inactive reasoning cards unchanged, and add coverage verifying the active
reasoning content is visible while streaming.
---
Nitpick comments:
In `@packages/solid-stick-to-bottom/src/stick-to-bottom.stories.tsx`:
- Around line 87-115: Extend GrowthHarnessApi and GrowthHarness to expose
imperative wrappers for stick.scrollToBottom() and stick.stopScroll(). Add
stories covering wheel-up escape followed by scrollToBottom() re-pinning, and
invoking stopScroll() while rows grow, ensuring both public methods are
exercised.
In `@packages/solid-stick-to-bottom/src/stick-to-bottom.ts`:
- Around line 338-346: Update the pointer-state handling in createStickToBottom
to use a module-level shared tracker with reference counting instead of
registering three document listeners per instance. Attach the mousedown,
mouseup, and click listeners when the first instance is created, share the
boolean state across instances, and remove the listeners when the final instance
is disposed while preserving existing pointer behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e93db45-b319-4ca1-881f-472c0d343607
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
.changeset/chat-scroll-ownership.md.fallowrc.jsonapps/storybook/.storybook/main.tspackages/publish/src/guards.tspackages/solid-stick-to-bottom/LICENSEpackages/solid-stick-to-bottom/README.mdpackages/solid-stick-to-bottom/package.jsonpackages/solid-stick-to-bottom/src/index.tspackages/solid-stick-to-bottom/src/stick-to-bottom.stories.tsxpackages/solid-stick-to-bottom/src/stick-to-bottom.tspackages/solid-stick-to-bottom/tsconfig.build.jsonpackages/solid-stick-to-bottom/tsconfig.jsonpackages/solid-stick-to-bottom/vite.config.tspackages/solid-stick-to-bottom/vitest.shims.d.tspackages/ui-kit-chat/package.jsonpackages/ui-kit-chat/src/behaviors/scroll.stories.tsxpackages/ui-kit-chat/src/behaviors/thread-follow-machine.tspackages/ui-kit-chat/src/behaviors/use-follow-pause.tspackages/ui-kit-chat/src/behaviors/use-scroll-lock.tspackages/ui-kit-chat/src/behaviors/use-thread-auto-scroll.tspackages/ui-kit-chat/src/behaviors/use-thread-scroll.tspackages/ui-kit-chat/src/primitives/chain-of-thought/chain-of-thought.tsxpackages/ui-kit-chat/src/primitives/thread/thread.tsxpackages/ui-kit-chat/src/primitives/thread/viewport-context.tsxpackages/ui-kit-chat/src/primitives/util/create-auto-collapse.tsxpackages/ui-kit-chat/src/primitives/util/create-force-open-once.tsxpackages/ui-kit-chat/src/styled/chain-of-thought.stories.tsxpackages/ui-kit-chat/src/styled/chain-of-thought.tsxpackages/ui-kit-chat/src/styled/reasoning.stories.tsxpackages/ui-kit-chat/src/styled/reasoning.tsxpackages/ui-kit-chat/src/styled/thread.stories.tsxpackages/ui-kit-chat/src/styled/thread.tsxpackages/ui-kit-chat/src/tools/styled/collapsible-card.tsxpackages/ui-kit-chat/src/tools/styled/tool-card.stories.tsxpackages/ui-kit-chat/src/tools/styled/tool-card.tsxpackages/ui-kit-chat/test/thread-auto-scroll.browser.test.tsxpackages/ui-kit-chat/test/thread-follow-machine.test.tspackages/ui-kit-chat/test/thread-hold.browser.test.tsx
💤 Files with no reviewable changes (4)
- packages/ui-kit-chat/test/thread-hold.browser.test.tsx
- packages/ui-kit-chat/test/thread-auto-scroll.browser.test.tsx
- packages/ui-kit-chat/src/behaviors/thread-follow-machine.ts
- packages/ui-kit-chat/test/thread-follow-machine.test.ts
| MIT License | ||
|
|
||
| Copyright (c) 2024 - present StackBlitz | ||
| Copyright (c) 2026 aidx contributors |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the copyright holder name.
The second copyright line names "aidx contributors". This repository and the published package use conciv. The LICENSE ships with the published package, so the attribution should match the project.
📄 Proposed fix
-Copyright (c) 2026 aidx contributors
+Copyright (c) 2026 conciv contributors📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Copyright (c) 2026 aidx contributors | |
| Copyright (c) 2026 conciv contributors |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/solid-stick-to-bottom/LICENSE` at line 4, Update the copyright
holder on the second line of the LICENSE from “aidx contributors” to “conciv” so
the shipped attribution matches the repository and published package.
| {(part) => ( | ||
| <ChainOfThought.Step icon={<Brain size={13} />} last={props.last}> | ||
| <Reasoning text={part().content} /> | ||
| <Reasoning text={part().content} grow={CHAIN_OF_THOUGHT_GROW} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the active reasoning state to Reasoning.
Line 83 does not pass streaming to Reasoning. Reasoning uses this prop to auto-open its CollapsibleCard. The outer ChainOfThought state at line 134 does not control the nested card.
When the final thinking part streams, the outer chain opens but the reasoning card stays closed. Pass streaming only to the final active thinking part. Add coverage that verifies its content is visible during streaming.
Proposed fix
function ChainPart(props: {
part: MessagePart | undefined
entries: ToolCardEntry[]
fallback: ToolUIComponent
last?: boolean
+ streaming?: boolean
}): JSX.Element {
- <Reasoning text={part().content} grow={CHAIN_OF_THOUGHT_GROW} />
+ <Reasoning text={part().content} streaming={props.streaming} grow={CHAIN_OF_THOUGHT_GROW} />
<ChainPart
part={parts()[partIndex()]}
entries={props.entries}
fallback={props.fallback}
last={partPosition === chain().indices.length - 1}
+ streaming={chainReasoningStreaming(chain().indices) && partIndex() === lastPartIndex()}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Reasoning text={part().content} grow={CHAIN_OF_THOUGHT_GROW} /> | |
| <Reasoning text={part().content} streaming={props.streaming} grow={CHAIN_OF_THOUGHT_GROW} /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui-kit-chat/src/styled/thread.tsx` at line 83, Update the Reasoning
component invocation in the thinking-part rendering path to pass streaming only
when the current part is the final active thinking part, so its CollapsibleCard
opens during streaming. Keep non-final or inactive reasoning cards unchanged,
and add coverage verifying the active reasoning content is visible while
streaming.
…ve shrink clamp (#430) useScrollLock's resetPosition unconditionally rewrote scrollTop back to the pre-collapse snapshot on every native 'scroll' event during the 200ms lock window. When a tall card collapses with the viewport parked near (but not at) the bottom, the outer stick-to-bottom instance's own ResizeObserver-driven clamp (stick-to-bottom.ts:handleContentResize) rides scrollTop down in step with the shrinking scrollHeight — but resetPosition kept yanking it back up toward the stale snapshot on every one of those clamp-triggered scroll events, producing a slow tug-of-war "crawl" for ~150ms followed by a late catch-up jump once the lock expired, instead of a smooth ride-down. Live-instrumented repro (write-level stack traces against the real dev widget) confirmed the fight: resetPosition and the outer clamp alternated writes to the same element throughout the animation. The fix makes the restore a one-directional guard — only correct scrollTop if something moved it ABOVE the snapshot (the unwanted-reflow case useScrollLock exists for). It never fights a legitimate downward clamp, so once the shrink pushes the max below the snapshot, the outer machine's own ride-down proceeds unopposed and settles exactly at the new bottom with nothing moving after animationend. The genuinely-detached (far-park) case is unaffected: no shrink ever crosses that headroom, so the guard never engages. Co-Authored-By: Claude Fable 5 <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
1 participant
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.
Fixes #397. Fixes #430. Fixes #432.
What this is
The chat transcript's scroll and card behavior, rebuilt on reference implementations instead of bespoke judgment. Governing rule: the viewport moves only when (a) pinned at the bottom while content streams, (b) the scroll-to-bottom button is clicked, (c) a message is sent. Nothing else ever scrolls the reader — including card open/close, tool activity, content shrink, or approval prompts.
The journey (full evidence trail on the issues)
Three incremental patches to the old thread-follow machine each fixed one symptom and exposed another (instrumented RCAs on #397). The pivot: delete the bespoke machine and port
use-stick-to-bottom(the library both Vercel AI Elements and CopilotKit v2 ship) to Solid, faithfully.@conciv/solid-stick-to-bottom(new published package): line-faithful Solid port — 70px near-bottom band, up-scroll escapes, shrink silently re-locks and never scrolls, spring follow, shadow-DOM-aware selection guard. MIT with upstream attribution. npm has no other Solid port.#430): auto-open only while their own reasoning streams; auto-close once, latched; user toggles permanent (assistant-ui's contract); approval force-opens the individual tool card one-shot (assistant-uiprevRequiresActionpattern), reactive to late approval flips; pure tool chains never auto-open — this killed the per-tool open/close "breathing" (instrumented: 3 open-close cycles per turn → 0 mutations).useScrollLockported + follow paused until the collapsible's realanimationend(597px jump → 0px, revert-checked).= true), verified from their source.grow(uncapped, AI-Elements style) with agrow={false}capped pane (assistant-ui style, stick-with-escape on its own pane) — pure choice between the two reference poles.Tests
Storybook play-stories with real input (CDP wheel/drag/selection — synthetic events don't scroll): follow, escape, re-lock, shrink, selection, button, send-pin, card contract, approval, toggle-lock. Assertions are state-based (
data-at-bottom/data-escapedviewport attributes + role queries), not pixel deltas, with two named positional exceptions. Full storybook suite forced: 93 files / 293 tests green; package suites, lint, fallow green.format:checkfails only on a pre-existing file on main.Not in this PR
#431 (virtualization + incremental grouping) is designed, spike-proven, and staged separately — perf baseline recorded; stage gates decide how far it goes.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation