Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions STAGING-v0.0.5.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
61 changes: 61 additions & 0 deletions docs/manual-tests/chat-memoization.md
Original file line number Diff line number Diff line change
@@ -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.
67 changes: 45 additions & 22 deletions frontend/src/app/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3937,7 +3937,17 @@ function renderGroupedChatItems(
}
result.push(<ToolGroupCard key={`tg-${group[0].id}`} items={group} />);
} 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(
<ChatItemView
key={cur.id}
item={cur}
isStreaming={cur.id === streamingItemId}
sessionId={sessionId}
/>,
);
Comment thread
greptile-apps[bot] marked this conversation as resolved.
i++;
}
}
Expand Down Expand Up @@ -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;
}) {
Comment thread
greptile-apps[bot] marked this conversation as resolved.
switch (item.type) {
case 'user': {
const files: string[] = item.meta?.files || [];
Expand All @@ -4107,7 +4134,7 @@ function renderChatItem(
const hasFiles = files.length > 0;
const tokens = mentions && mentions.length > 0 ? wireToDraft(item.content, mentions) : null;
return (
<div key={item.id} className="flex justify-end animate-fade-in">
<div className="flex justify-end animate-fade-in">
<div className="max-w-[80%] rounded-2xl rounded-br-md bg-primary-600 text-white text-sm overflow-hidden">
{hasFiles && <UserMessageFilePills files={files} hasText={Boolean(hasText)} />}
{hasText && (
Expand Down Expand Up @@ -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 (
<div key={item.id} className="flex gap-3 animate-fade-in">
<div className="flex gap-3 animate-fade-in">
<div
className={`w-7 h-7 rounded-full ${avatarBg} flex items-center justify-center shrink-0 mt-1`}
>
Expand All @@ -4148,7 +4174,7 @@ function renderChatItem(
{agentMeta && (
<div className={`text-[10px] ${avatarText} font-medium mb-1`}>{agentMeta.label}</div>
)}
<ReactMarkdown remarkPlugins={[remarkGfm]}>{item.content}</ReactMarkdown>
<ReactMarkdown remarkPlugins={CHAT_MARKDOWN_PLUGINS}>{item.content}</ReactMarkdown>
{isStreaming && (
<span className="inline-block w-2 h-5 bg-primary-400 rounded-sm ml-0.5 animate-blink align-text-bottom" />
)}
Expand All @@ -4158,29 +4184,26 @@ function renderChatItem(
}
case 'tool_start':
case 'tool_end':
return <CollapsibleToolCard key={item.id} item={item} />;
return <CollapsibleToolCard item={item} />;
case 'code_output':
return null; // folded into the tool card above
case 'subagent_start':
case 'subagent_end':
return <SubAgentCard key={item.id} item={item} />;
return <SubAgentCard item={item} />;
case 'clarification':
return <ClarificationCard key={item.id} item={item} sessionId={sessionId ?? null} />;
return <ClarificationCard item={item} sessionId={sessionId ?? null} />;
case 'agent_tool':
return <AgentToolCard key={item.id} item={item} />;
return <AgentToolCard item={item} />;
case 'error':
return (
<div
key={item.id}
className="animate-fade-in flex items-center gap-2 px-3 py-2 bg-red-900/30 border border-red-800/50 rounded-lg text-sm text-red-400"
>
<div className="animate-fade-in flex items-center gap-2 px-3 py-2 bg-red-900/30 border border-red-800/50 rounded-lg text-sm text-red-400">
<AlertCircle className="w-4 h-4 shrink-0" />
{item.content}
</div>
);
case 'status':
return (
<div key={item.id} className="text-center">
<div className="text-center">
<span
className={`inline-flex items-center gap-1.5 px-3 py-1 rounded-full text-xs font-medium ${
item.content.includes('running')
Expand All @@ -4200,7 +4223,7 @@ function renderChatItem(
);
case 'stage_complete':
return (
<div key={item.id} className="flex items-center justify-center py-2 animate-fade-in">
<div className="flex items-center justify-center py-2 animate-fade-in">
<div className="flex items-center gap-2 px-4 py-2 rounded-full bg-green-500/10 border border-green-500/20">
<CheckCircle2 className="w-4 h-4 text-green-400" />
<span className="text-sm font-medium text-green-300">{item.content} complete</span>
Expand All @@ -4210,4 +4233,4 @@ function renderChatItem(
default:
return null;
}
}
});