diff --git a/STAGING-v0.0.5.md b/STAGING-v0.0.5.md index 6a44fae..f1e810d 100644 --- a/STAGING-v0.0.5.md +++ b/STAGING-v0.0.5.md @@ -49,6 +49,7 @@ branch → run relevant tests → record below. | 29 | #165 | fix/107-cost-budget | #107 | P1 (non-BudgetExceededError from `check_budget` landing session in `failed`) already fixed by author's fixup `1bfed27` (`_check_budget_failopen` + 2 tests); P2 (two-SELECT atomicity in `get_budget_status`) dismissed by author with reasoning (AsyncSession autobegin shares one transaction; race direction is conservative; re-checked after every usage event) | branch repaired in triage worktree `f165`: staging merged in, 8 conflict files resolved preserving BOTH features (training controls AND budget enforcement) — `ProjectSettingsModal.onSave` signature combined to `(config, budgetUsd, training)`; hand-rolled `ALTER TABLE projects ADD COLUMN budget_usd` converted to Alembic revision `8e4b2d6f1a35_projects_budget_usd` (chained on `3f7a1c9e2d64`); alembic tests extended to assert `budget_usd` | branch: full suite **463 passed, 8 skipped**, ruff check + format --check clean (168 files), tsc/prettier/vitest 4/4 clean; post-merge staging: **463 passed, 8 skipped**; alembic chain verified linear, single head `8e4b2d6f1a35`; CLI `alembic upgrade head` fresh SQLite → both new columns present | conflicts kept both: `models.py`, `routers/projects.py`, `schemas.py`, `runner.py` (budget pre-check + wall-clock clamp), `ProjectSettingsModal.tsx` (Budget + Training Controls sections), `Sidebar.tsx`, `api.ts`, `types.ts`; pushed `712e37f` verified as merge parent | | 30 | #87 | feat/workspace-download | #79 | rounds 1–2 already fixed by author (`7dc0d1b`, `8cfc952`): log_table rows iterator, cap-exceeded full file reads, yield-in-finally async generator, unused `READ_CHUNK_BYTES`, 404-on-empty-sessions convention, partial-read zip entries marked in `__truncated.txt`; round-3 P1 (`log_confusion_matrix` exhausts `y_true`/`y_pred` iterators → silent all-zero matrix) outstanding → fixed on PR branch in `1f6ad6d` (materialize both to lists once up front; +2 regression tests in new `backend/tests/test_trainable_runtime.py`) | ruff UP031 (branch predates pinned config): 2 `%`-format raises → f-strings (`f80d7f4`); `frontend/node_modules` worktree symlink accidentally committed via `git add -A` — dropped in `f313653` (branch) and merged to staging as a follow-up merge; main checkout's real `node_modules` had been replaced by that symlink during the merge → reinstalled via `npm ci` | branch: full suite **477 passed, 8 skipped**, ruff check + format --check clean (174 files), tsc/prettier/vitest 4/4 clean; post-merge staging: **477 passed, 8 skipped** | no `db.py`/`models.py` changes → no Alembic revision needed; conflicts: `backend/main.py` router includes (kept both `download` + `samples`); `sandbox.py` auto-merged — #87's refactor (inline SDK preamble → shared `services/trainable_runtime.py`) intact, staging's pre-sandbox volume bootstrap preserved; `conftest.py` auto-merged (MockVolume chunked-read + error injection); `page.tsx`/`Sidebar.tsx` auto-merged; pushed `f313653` (contains merge `f80d7f4` + P1 fix `1f6ad6d`) verified as merge parent | | 31 | #148 | fix/100-dedupe-eventsource | #100 | round-1 findings (P2 second-context vs lib/AGENTS.md; P2 cross-session event bleed) already fixed by author's fixup `3f73570` (session-scoped `publish` + documented stateless bus context + 5 vitest tests); round-2 P1 (throwing bus listener silently kills `connectSSE`'s UI switch — `publish` ran inside the try/catch before the switch) outstanding → fixed on PR branch in `cfbfb40` (per-listener try/catch in `SSEStreamContext.publish`, logs and continues) | branch repaired against staging in `8f74aab`: conflict only in `frontend/package-lock.json` (lockfile churn) — took staging's via `--theirs`, regenerated with `npm install` against merged package.json; `frontend/node_modules` verified real directory (not symlink) | branch: vitest **9/9** (4 api + 5 bus/notebook), tsc clean, prettier clean; post-merge staging: vitest **9/9**, tsc clean, prettier clean | `page.tsx` auto-merged — verified #148's `SSEStreamProvider`/`publish(sid, event)` AND #169's `takeSuggestedPrompt`/`Sparkles` wiring AND #87's `/api/sessions/{id}/download` UI all present; pushed `8f74aab` (contains P1 fix `cfbfb40`) verified as merge parent | +| 32 | #150 | fix/98-memoize-chat | #98 | both P2 findings (shared `streamingItemId` string prop breaks `memo` bailout for ALL items on stream start/end) already fixed by author's fixup `b37907e` (per-item `isStreaming` boolean + manual-test tutorial `docs/manual-tests/chat-memoization.md`) | branch repaired against staging in `3e48748` — ort auto-merged CLEAN (no conflicts; branch was stacked on #148's original commit, staging already carried #148's fixup + P1 repair) | branch: vitest 9/9, tsc clean, prettier clean; post-merge staging: vitest 9/9, tsc clean, prettier clean | `page.tsx` merge kept memoized `ChatItemView` (`isStreaming={cur.id === streamingItemId}` verified) + #148 bus + #169/#87 wiring; pushed `3e48748` verified as merge parent | ## Deferred / not merged diff --git a/docs/manual-tests/chat-autoscroll-pinning.md b/docs/manual-tests/chat-autoscroll-pinning.md new file mode 100644 index 0000000..b862ecf --- /dev/null +++ b/docs/manual-tests/chat-autoscroll-pinning.md @@ -0,0 +1,60 @@ +# Manual test tutorial — chat auto-scroll pinning (PR #152 / issue #99) + +The frontend has no unit-test harness, and the behavior under test (browser +smooth-scroll animation, `scroll` events, streaming SSE tokens) cannot be +reproduced in jsdom anyway. Verify the scroll-pinning behavior manually with +this checklist in a real browser. + +## Setup + +1. Start the app (`docker compose up` or backend + `cd frontend && npm run dev`). +2. Open the chat page and start a session. +3. Send a few prompts that produce **long streaming replies** (e.g. + "Explain gradient descent in 2000 words") until the chat pane has enough + history to scroll (scrollbar visible). + +## Test 1 — pinned follow (baseline) + +1. Scroll to the very bottom of the chat pane. +2. Send a prompt with a long streaming reply. +3. **Expected:** the view stays glued to the bottom for the whole stream; + every new token is visible without touching the scrollbar. + +## Test 2 — scrollback is not hijacked (the original #99 bug) + +1. Send a prompt with a long streaming reply. +2. While tokens are still streaming, scroll **up** several screens and stop. +3. **Expected:** the view stays exactly where you put it; it does NOT jump + back to the bottom on each token. You can read old messages undisturbed + until the stream finishes. + +## Test 3 — re-pin by scrolling back down + +1. Continue from Test 2 (still streaming, scrolled up). +2. Scroll back down to the bottom (within ~96 px of it). +3. **Expected:** auto-follow resumes; the view sticks to the bottom for the + rest of the stream. + +## Test 4 — sending a message re-pins instantly (the Greptile P1 fix) + +1. With plenty of history, scroll far **up** in the pane (several screens). +2. Type a new prompt and hit send. +3. **Expected:** + - The view jumps **instantly** to the bottom (no smooth animation + gliding down — a smooth animation is exactly what used to un-pin the + view mid-flight). + - Your new user bubble is visible at the bottom. + - When the assistant's first tokens arrive (even if they arrive + immediately), the view **follows the stream to the end**. It must not + get stuck at an intermediate scroll position. + +Repeat Test 4 a few times with a fast-responding model; the failure mode it +guards against was a race between the smooth-scroll animation and +first-token latency, so it was timing-dependent. + +## Test 5 — threshold sanity + +1. During a stream, scroll up just a tiny nudge (well under ~96 px from the + bottom). **Expected:** still pinned; the view keeps following. +2. Scroll up more than ~96 px. **Expected:** un-pinned; the view stops + following (Test 2 behavior). diff --git a/frontend/src/app/page.tsx b/frontend/src/app/page.tsx index f785c0b..15e1106 100644 --- a/frontend/src/app/page.tsx +++ b/frontend/src/app/page.tsx @@ -161,6 +161,11 @@ const SUGGESTIONS = [ }, ]; +// How close (px) to the bottom of the chat pane the user must be for +// auto-scroll to stay "pinned". Module-level so the binding is created once +// and is unambiguously stable for the scroll-handler closure. +const AUTO_SCROLL_PIN_THRESHOLD_PX = 96; + // --------------------------------------------------------------------------- // Main page component // --------------------------------------------------------------------------- @@ -237,6 +242,17 @@ function HomePageContent() { const [htmlArtifacts, setHtmlArtifacts] = useState>(() => new Map()); const bottomRef = useRef(null); + // The actual scrollable chat pane (the `overflow-y-auto` div `bottomRef` + // sits at the bottom of). Used to measure scroll position for the + // pinned-to-bottom tracking below. + const chatScrollRef = useRef(null); + // Whether the user is scrolled near the bottom of the chat pane. The + // auto-scroll effect below only fires while this stays true — otherwise a + // user who scrolls up to read earlier output during a long streaming + // response gets yanked back to the bottom on every token. A ref (not + // state) because the scroll handler runs on every native scroll event and + // we don't want that to trigger a re-render. + const pinnedToBottomRef = useRef(true); const sseRef = useRef(null); const inputRef = useRef(null); const prevExperimentIdRef = useRef(null); @@ -295,11 +311,33 @@ function HomePageContent() { fileNames: string[]; } | null>(null); - // Auto-scroll on new chat items + // Auto-scroll on new chat items — but only while the user is pinned near + // the bottom of the pane. `chatItems` changes many times per second while + // an assistant reply streams token-by-token; without the pin gate, + // `scrollIntoView` fired on every single one of those changes and + // hijacked the scroll position, making it impossible to scroll up and + // read earlier output. `behavior: 'auto'` (instant, no animation) is used + // unconditionally: a smooth scroll animates through intermediate positions, + // and each intermediate `scroll` event would make `handleChatScroll` see + // `distanceFromBottom > threshold` and un-pin mid-animation — so if the + // first streaming tokens arrived before the animation landed, auto-scroll + // silently stopped. An instant jump fires a single scroll event already at + // the bottom, which keeps the pin state consistent. useEffect(() => { - bottomRef.current?.scrollIntoView({ behavior: 'smooth' }); + if (!pinnedToBottomRef.current) return; + bottomRef.current?.scrollIntoView({ behavior: 'auto' }); }, [chatItems]); + // Track whether the user is pinned near the bottom of the chat pane via a + // scroll listener + threshold, rather than assuming every render should + // snap back down. + const handleChatScroll = useCallback(() => { + const el = chatScrollRef.current; + if (!el) return; + const distanceFromBottom = el.scrollHeight - el.scrollTop - el.clientHeight; + pinnedToBottomRef.current = distanceFromBottom <= AUTO_SCROLL_PIN_THRESHOLD_PX; + }, []); + // Seed the chat input with the suggested prompt handed off by the // sample-dataset gallery (first-run flow). Consumed exactly once, and // never clobbers something the user already typed. @@ -316,6 +354,9 @@ function HomePageContent() { // --------------------------------------------------------------------------- const addItem = useCallback((item: Omit) => { + // The user just sent something — they're at the input, not mid-read of + // scrollback, so re-pin to bottom even if they'd scrolled up earlier. + if (item.type === 'user') pinnedToBottomRef.current = true; setChatItems((prev) => [ ...prev, { ...item, id: `${Date.now()}-${Math.random()}`, timestamp: Date.now() }, @@ -964,6 +1005,7 @@ function HomePageContent() { setDraft([]); setIsRunning(false); streamingItemIdRef.current = null; + pinnedToBottomRef.current = true; setSessionState('created'); workspacePanelRef.current?.collapse(); setCanvasContent(''); @@ -2064,7 +2106,11 @@ function HomePageContent() { {/* Chat panel */}
-
+