diff --git a/STAGING-v0.0.5.md b/STAGING-v0.0.5.md index ea5431c..6a44fae 100644 --- a/STAGING-v0.0.5.md +++ b/STAGING-v0.0.5.md @@ -48,6 +48,7 @@ branch → run relevant tests → record below. | 28 | #164 | fix/104-training-control | #104 | both findings (P1 start-training constraints bypass-able by omission; P2 user-controlled strings injected verbatim into agent system prompts) already fixed by author's fixup `4e5a0ed` (postdates the comments): omission now rejected — agent must declare `optimization_metric`/`max_trials` when configured; `TrainingConfig.optimization_metric` restricted to a plain-identifier charset; +3 tests | hand-rolled boot-time `ALTER TABLE projects ADD COLUMN training_config` (dead `_run_migrations` path) converted to Alembic revision `3f7a1c9e2d64_projects_training_config` (chained on `0bae8e0f765d`); `_run_alembic_sync` stamp semantics fixed — legacy DBs now stamp at the INITIAL revision then `upgrade head` (stamping straight at head would silently skip post-cutover DDL like this column); alembic tests updated (+2: stamp-then-upgrade applies post-initial DDL, fresh-upgrade has the column) | full backend suite: **453 passed, 8 skipped** (alembic 8/8, training_config 18/18); ruff check + format --check clean; frontend vitest 4/4, tsc clean, prettier --check clean; CLI `alembic upgrade head` on fresh SQLite verified | single conflict: `backend/schemas.py` import block (kept both `re` + `Any`); head `4e5a0ed` verified as merge parent | | 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 | ## Deferred / not merged diff --git a/docs/manual-tests/chat-memoization.md b/docs/manual-tests/chat-memoization.md new file mode 100644 index 0000000..4de8fbc --- /dev/null +++ b/docs/manual-tests/chat-memoization.md @@ -0,0 +1,61 @@ +# Manual test: chat item memoization & streaming (PR #150) + +PR #150 memoizes `ChatItemView` (frontend/src/app/page.tsx) and hoists the +`remarkGfm` plugin array so markdown bubbles stop re-parsing on every streamed +token. Each item also receives a per-item `isStreaming` boolean (not the shared +streaming-item id), so when a stream starts or ends only the affected bubble +re-renders. There is no frontend unit-test infrastructure in this repo, so +verify the behavior manually as follows. + +## Setup + +1. Start the stack (backend + frontend) as usual, e.g. `docker compose up` + or `cd frontend && npm ci && npm run dev` against a running backend. +2. Open the app in Chrome and open a session with an existing chat history + (or create one and send a few messages first, including at least one + assistant reply containing markdown: headings, a table, a code block). +3. Open Chrome DevTools → install/enable **React Developer Tools** → + Profiler tab → gear icon → check **"Record why each component rendered"** + and enable **"Highlight updates when components render"** in the settings. + +## Test 1 — only the streaming bubble re-renders per token + +1. Start Profiler recording. +2. Send a new message and let the assistant stream a long reply. +3. Stop recording after the stream ends. + +**Expected:** +- During token updates, render highlights flash only around the streaming + assistant bubble, not around earlier chat bubbles. +- In the Profiler flamegraph, prior `ChatItemView` instances show + "Did not render" / memo bailout for the token-update commits. +- At the stream **start** and **end** commits, only the streaming item + re-renders (its `isStreaming` prop flips); other items still bail out — + this is the per-item boolean fix from the Greptile review. + +## Test 2 — markdown is not re-parsed in old bubbles + +1. With a history containing a large markdown reply (table + code block), + start streaming a new reply. +2. In the Profiler, select the old markdown bubble's `ChatItemView`. + +**Expected:** it reports no renders during the stream. If it re-rendered +every token (the pre-PR behavior), `ReactMarkdown` would re-parse its content +each time — visible as jank/CPU in the Performance tab on long histories. + +## Test 3 — streaming caret behaves correctly + +1. Watch the streaming assistant bubble while tokens arrive. + +**Expected:** +- A blinking caret is shown at the end of the streaming bubble only. +- When the stream finishes, the caret disappears from that bubble. +- No other bubble ever shows a caret. + +## Test 4 — no visual/behavioral regressions + +Quickly sanity-check every chat item type still renders: +user messages (incl. file pills and @-mentions), assistant markdown +(GFM tables/strikethrough/task lists must still render — this exercises the +hoisted `CHAT_MARKDOWN_PLUGINS`), tool cards, sub-agent cards, clarification +cards, and error items. Scroll through an existing long session to confirm. diff --git a/frontend/src/app/page.tsx b/frontend/src/app/page.tsx index a6c1dec..f785c0b 100644 --- a/frontend/src/app/page.tsx +++ b/frontend/src/app/page.tsx @@ -3937,7 +3937,17 @@ function renderGroupedChatItems( } result.push(); } else { - result.push(renderChatItem(cur, streamingItemId, sessionId)); + // Pass a per-item boolean instead of the shared streamingItemId string: + // when streaming starts/ends only the affected item sees a prop change, + // so `memo` still bails out for every other bubble. + result.push( + , + ); i++; } } @@ -4091,14 +4101,31 @@ function AttachedFilesPreview({ } // --------------------------------------------------------------------------- -// renderChatItem +// ChatItemView — memoized per-item renderer (formerly the plain +// `renderChatItem` function). renderGroupedChatItems re-runs on every +// `agent_token`/`agent_message` SSE event (each one calls `setChatItems`), +// which previously re-invoked this as a plain function for every prior +// message and re-parsed markdown for all of them — O(messages) ReactMarkdown +// parses per streamed chunk. Wrapping it in `memo`, keyed by `item.id` at +// the call site, means React bails out and skips re-render (and re-parse) +// for every bubble except the one whose `item` object reference actually +// changed (the currently-streaming assistant bubble). // --------------------------------------------------------------------------- -function renderChatItem( - item: ChatItem, - streamingItemId?: string | null, - sessionId?: string | null, -) { +// Stable remark-plugins array for chat bubbles — a fresh `[remarkGfm]` +// literal on every render would give ReactMarkdown a "new" plugin list each +// time, undermining the memoization above even when `item` didn't change. +const CHAT_MARKDOWN_PLUGINS = [remarkGfm]; + +const ChatItemView = memo(function ChatItemView({ + item, + isStreaming, + sessionId, +}: { + item: ChatItem; + isStreaming?: boolean; + sessionId?: string | null; +}) { switch (item.type) { case 'user': { const files: string[] = item.meta?.files || []; @@ -4107,7 +4134,7 @@ function renderChatItem( const hasFiles = files.length > 0; const tokens = mentions && mentions.length > 0 ? wireToDraft(item.content, mentions) : null; return ( -
+
{hasFiles && } {hasText && ( @@ -4135,10 +4162,9 @@ function renderChatItem( const agentColor = agentMeta ? AGENT_COLORS[agentMeta.color] : null; const avatarBg = agentColor ? agentColor.bg : 'bg-emerald-500/20'; const avatarText = agentColor ? agentColor.text : 'text-emerald-400'; - const isStreaming = item.id === streamingItemId; return ( -
+
@@ -4148,7 +4174,7 @@ function renderChatItem( {agentMeta && (
{agentMeta.label}
)} - {item.content} + {item.content} {isStreaming && ( )} @@ -4158,29 +4184,26 @@ function renderChatItem( } case 'tool_start': case 'tool_end': - return ; + return ; case 'code_output': return null; // folded into the tool card above case 'subagent_start': case 'subagent_end': - return ; + return ; case 'clarification': - return ; + return ; case 'agent_tool': - return ; + return ; case 'error': return ( -
+
{item.content}
); case 'status': return ( -
+
+
{item.content} complete @@ -4210,4 +4233,4 @@ function renderChatItem( default: return null; } -} +});