feat(render): component library foundation — SubagentStatusBar, StreamProgress, ErrorCard + live overlay integration - #1321
Conversation
…treamProgress, ErrorCard (#sprint0) Three new pure-function rendering components in src/cli/render/: - subagentStatusBar(spec) — single-line status bar for active subagent dispatches, with label, phase, elapsed timer, and batch badge. subagentStatusStack() renders multiple with a 3-line cap + overflow. - streamProgress(spec) — single-line progress indicator with animated spinner, label, token count, cost, and elapsed. Designed for OverlayComposer live region repaint. - errorCard(spec) — structured error card with recovery hint, replacing ad-hoc errorBox() callers. Wraps drawBox with red border + optional dim/italic hint line. All follow the established pattern: stateless (spec) → string functions, semantic palette roles, internal terminal width reads, ANSI-safe truncation. 26 new tests (11 + 9 + 6). Part of the component library initiative to surpass Claude Code's terminal UX through composable rendering primitives rather than a renderer rewrite.
…rrorCard, typography glyphs Task 1 — SubagentStatusBar in OverlayComposer: - Add 'subagent-status' slot above 'tool-lane' in the z-order array (stream-renderer.ts arm()) - Add activeSubagents Map<string,SubagentStatusBarSpec> + subagentStartedAt Map<string,number> fields - Register 'subagent-status' slot in registerOverlaySlots() (lifecycle.ts) with subagentStatusStack() - Track new subagent sources in processEvent() (process.ts): add on first event, remove on done/error - Add 250ms setInterval ticker in arm() to update elapsedMs and flush overlay; clear in dispose() - Import SubagentStatusBarSpec from '../render.js' in stream-renderer.ts and process.ts - Extend ProcessCtx with activeSubagents, subagentStartedAt, overlayComposerForStatus - Extend registerOverlaySlots ctx with optional getActiveSubagents accessor Task 2 — Replace errorBox callers with errorCard: - src/cli/errors/presenter.ts: import errorCard, replace errorBox(msg, hint) call - src/cli/_lib/stream-renderer-orchestrator-emit.ts: replace errorBox in emitErrorBox() Task 3 — Typography fixes: - tool-category.ts: shell glyph '$' → '▸' (run-arrow, consistent visual language) - tool-category.ts: other glyph '●' → '◌' (hollow circle, distinct from read's filled ●) - Update affected tests: turn-handler-format.test.ts, interactive-progress-banner.test.ts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…cing, error card stack, timer unref - Fix formatCost sub-cent path: divide by 100 before formatting ($0.500 → $0.0050) - Collapse dead code branch in formatCost (>=100 cents now renders whole dollars) - Fix subagent status bar double-spacing when phase is absent (4-space gap → 2-space) - Strip duplicate Error: header from err.stack before passing as errorCard hint - .unref() both setInterval timers to prevent event loop hang on abnormal exit - Fix stale JSDoc: "4-frame cycle" → "10-frame cycle", "errorBox" → "errorCard" - Add tests for sub-cent and large-dollar cost formatting
|
Confirmed: Given the Now let me compile the final synthesis. PR #1321 Review: Component Library FoundationTarget: Findings1.
|
The shell glyph change in tool-category.ts was propagated to
turn-handler-format.test.ts and interactive-progress-banner.test.ts
but missed the tool-lane inline snapshots and external snapshot file.
Files fixed:
- tool-lane.test.ts: inline snapshot at line 1247
- tool-lane.overlay.test.ts: indexOf('$ bash') → indexOf('▸ bash')
- __snapshots__/tool-lane.test.ts.snap: 3 snapshot strings
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 483a63d9cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A gap > 5 between consecutive started events means a new round | ||
| // (the model produced a new assistant turn with more tool calls) | ||
| if (seqs[i]! - seqs[i - 1]! > 5) rounds++; |
There was a problem hiding this comment.
This heuristic merges ordinary sequential tool rounds: when an assistant issues one tool call, receives its completion, and then issues another, the started events commonly have sequence numbers 0 and 2, so the > 5 test reports both as one round. The repository's repeated-tool-use fixtures use exactly this started/completed sequence pattern, meaning this script can collapse an entire multi-round session into a single round and corrupt the experiment metric it was introduced to measure.
Useful? React with 👍 / 👎.
| ARM_B_START=$(date +%s) | ||
| node "$AFK_BIN" chat \ | ||
| -m "$MODEL" \ |
There was a problem hiding this comment.
When the caller has exported AFK_WORKSPACE_DISABLED=1, this invocation inherits it, so Arm B also runs with the workspace disabled even though the script labels it as the treatment. In that environment both arms are controls and the generated comparison is invalid; unset or override the variable for this command.
Useful? React with 👍 / 👎.
| const lines = visible.map((e) => subagentStatusBar(e)); | ||
|
|
||
| if (overflow > 0) { | ||
| lines.push(palette.dim(` … +${overflow} more running`)); |
There was a problem hiding this comment.
With more than maxLines active agents, the function first renders maxLines entries and then appends the summary, producing maxLines + 1 rows (for example, four rows under the default three-row cap). This defeats the stated overlay-height safeguard precisely during large parallel waves; reserve one of the capped rows for the overflow summary.
Useful? React with 👍 / 👎.
| const fillLen = Math.max(0, width - fixedWidth); | ||
| const fill = palette.dim('─'.repeat(Math.min(fillLen, 20))); | ||
|
|
||
| const parts = [left, fill, ...(phase ? [phase] : []), palette.dim(elapsed)]; | ||
| if (batch) parts.push(batch); | ||
|
|
||
| return parts.join(' '); |
There was a problem hiding this comment.
When the fixed label, phase, elapsed value, and badge exceed width, fillLen merely becomes zero and the full row is still returned. A narrow terminal or a long agent label therefore soft-wraps this supposedly single-line component, consuming unaccounted overlay rows and defeating the stack height cap; truncate the variable fields or the assembled ANSI-safe row to the available columns.
Useful? React with 👍 / 👎.
Research docs (.afk/research/*), HANDOFF.md, AB test results (scripts/ab-results/*), and measurement scripts were committed to this branch but are not part of the component library feature. Files remain in the worktree, just removed from the branch.
|
The second line has the close codes but no open codes. In a streaming terminal context, ANSI state carries from line 0's opening codes. But when split and re-emitted via Here is the synthesized review: PR #1321 Review: feat(render): component library foundationTarget: Findings1. medium · blocking:true · confidence:high · correctness ·
const fixedWidth =
displayWidth(leftPlain) +
2 + // gap after label
(phasePlain ? displayWidth(phasePlain) + 2 : 0) +
displayWidth(elapsed) + // ← missing +2 for fill-to-next-part gap
(batchPlain ? 2 + displayWidth(batchPlain) : 0);Suggestion: Add 2. medium · blocking:true · confidence:high · correctness · The Suggestion: Update to list all six slots: 3. medium · blocking:true · confidence:high · api-compat ·
const stackTrace = err.stack?.split('\n').slice(1).join('\n');
const box = errorCard({ body: err.message, hint: stackTrace });Suggestion: 4. medium · blocking:false · confidence:high · correctness · Multi-line stack traces passed as Suggestion: Optional follow-up: split hint on 5. medium · blocking:true · confidence:high · test-coverage · No test exercises the Suggestion: Add integration tests that call 6. low · blocking:false · confidence:high · spec-compliance ·
Suggestion: Add 7. nit · blocking:false · confidence:high · api-compat · Glyph changes (shell What was not checked
Decision: DO NOT MERGE -- 3 medium blocking (1 correctness, 1 api-compat, 1 test-coverage); 1 medium waived (correctness, non-data-affecting ANSI edge case); 1 low, 1 nit. Done
🤖 Posted by |
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (scheduled sweep), generated by the /review tool — a maintainer will follow up.
Prior rounds
No prior automated rounds on this PR — first review.
Decision
MERGE (with follow-ups)
Severity arithmetic: 0 critical, 0 high → MERGE (with follow-ups); 2 medium advisory, 3 low advisory, 2 nit.
Per sweep policy, only critical and high findings block. Both medium findings below are advisory; a follow-up issue is recommended.
Findings
🟡 Medium (advisory, non-blocking)
M1 — subagentStatusBar fill calculation overruns by 2 columns
src/cli/render/subagent-status-bar.ts:44–57 · correctness · file-state · ref: 21dc61e
parts.join(' ') produces 2 extra chars that fixedWidth doesn't account for. fixedWidth budgets one ' ' gap (after label) but the join inserts a second gap (fill → elapsed), so the rendered line is always 2 chars wider than width. On narrow terminals this soft-wraps the overlay row and corrupts the layout.
fixedWidth = left + 2 + elapsed ← missing: fill→next gap (+2)
actual = left + 2 + fill + 2 + elapsed ← always overruns by 2
Arithmetic verified: fillLen=19, actual=42, target=40, overflow=2 on a width=40 terminal with a 17-char label.
Suggestion: Add + 2 to fixedWidth for the fill→next element join. Add a column-width assertion in tests.
M2 — formatCost rounds $1.00–$1.99 to nearest dollar: up to $0.50 display error
src/cli/render/stream-progress.ts:71–75 · correctness · file-state · ref: 21dc61e
The third branch fires for any cost ≥ $1.00 and uses .toFixed(0):
return `$${(cents / 100).toFixed(0)}`; // fires at cents >= 100formatCost(150) → $2 (actual $1.50). formatCost(249) → $2 (actual $2.49). No test pins the $1.50 case. Maximum user-visible error: $0.50.
Suggestion: Use .toFixed(2) consistently for the ≥$1 range, or only suppress .00 for exact dollars. Add tests for costCents: 150 and costCents: 250.
🔵 Low (advisory, non-blocking)
L1 — subagentStatusBar label not passed through sanitizeLabel
src/cli/render/subagent-status-bar.ts:22–26 · security (defense-in-depth) · file-state · ref: 21dc61e
spec.label is passed directly to palette.tool() without sanitizeLabel. The two production call paths (subagent-executor.ts via stripEscapeSequences and compose-executor.ts via /^[A-Za-z0-9_-]+$/ schema validation) sanitize agentType before it arrives here, so no active exploit path exists at this ref. However, the drawBox JSDoc explicitly says inputs must be pre-sanitized by callers, and subagentStatusBar is a new public surface without that contract stated. A future call path skipping upstream sanitization would be silently vulnerable.
Suggestion: Apply sanitizeLabel to spec.label and spec.phase inside subagentStatusBar, matching the defense posture of tool-lane-format-sanitize.ts.
L2 — Full stack trace as hint in emitErrorBox (pre-existing, carried forward)
src/cli/_lib/stream-renderer-orchestrator-emit.ts:386–387 · correctness · diff-context · ref: 21dc61e
The original errorBox(err.message, err.stack) also showed the full stack. The migration makes it slightly more prominent (inside the card rather than as a secondary detail). Pre-existing behavior; non-blocking for this PR.
L3 — 'subagent-status' slot not covered by registerOverlaySlots integration tests
src/cli/_lib/stream-renderer-lifecycle.test.ts · test-coverage · absence confirmed · ref: 21dc61e
The six-slot variant isn't tested at integration level: absent getActiveSubagents → renders '', populated map → renders stack. Unit tests for subagentStatusStack exist but don't cover the overlay slot registration path.
⚪ Nit (advisory)
N1 — errorBox still exported without deprecation JSDoc
Both error-box.js and error-card.js are re-exported from render/index.ts. The two production callers are fully migrated, but the old API has no @deprecated tag. Add a JSDoc note pointing to errorCard.
N2 — Stale test descriptions: 'renders error events as errorBox'
src/cli/_lib/stream-renderer.test.ts:140 still says errorBox but exercises errorCard under the hood. Update description.
Withdrawn / not applicable
- StreamProgress unwired — PR body explicitly says "What's next (not in this PR): Wire StreamProgress into the streaming output." Intentional scope exclusion, not a spec-compliance failure.
▸glyph reuse (picker cursor vs. shell glyph) — distinct render contexts, no behavioral conflict. Noted as nit only.
What was not checked
- Citations verified inline against branch HEAD
21dc61e355e13c14e4e1a74361b9df195fe30e2e. - Stated intent: PR #1321 title + body — spec-compliance assessed.
- Did not run the test suite locally; CI shows all green (326 tests passing).
- Did not review the Telegram or daemon surfaces.
StreamProgressintentionally out of scope per PR body.
1 participant
Summary
Component library foundation for surpassing Claude Code's terminal UX — rendering primitives + live integration + typography polish.
Why component library, not renderer rewrite
Devils-advocated three approaches: CellState/React renderer rewrite (scored 7/20), targeted additions (17/20), and component library on existing compositor (18/20). Claude Code's visual advantage is its component library (collapsible tool cards, progress animations, status bars), not its renderer. Building components on the existing 22,500-line battle-tested compositor delivers visible UX improvements weekly without a multi-month rewrite or bus-factor-1 dependency.
What's new
Rendering primitives (
src/cli/render/)subagentStatusBar(spec){ label, phase?, elapsedMs, batchIndex?, batchSize? }◉ research-agent ── thinking… 12s ∥2/4subagentStatusStack(entries, maxLines?)… +N more runningoverflowstreamProgress(spec){ label, spinnerFrame, elapsedMs, tokenCount?, costCents? }⠹ Generating… 1.2k tokens 4.7serrorCard(spec){ title?, body, hint? }All follow the established pattern: stateless
(spec) → string, semanticpalette.<role>colors, internal terminal width reads, ANSI-safe truncation.Live integration
SubagentStatusBar overlay slot — registered as
'subagent-status'in the OverlayComposer z-order, above'tool-lane'. When subagents dispatch during a parent turn, users see live-updating status bars:activeSubagentsmap with label + start timesetIntervalticker → updateselapsedMs, marks slot dirty, flushes overlaydone/error→ entry removed, slot auto-clears… +N more running)ErrorCard replaces errorBox — both callers (error presenter + stream-renderer error path) now render structured error cards with recovery hints instead of plain red boxes.
Typography
$→▸(consistent with Unicode visual language — every other category uses a Unicode glyph)othercategory glyph●→◌(visually distinct fromread's filled dot)Files changed
src/cli/render/subagent-status-bar.tssrc/cli/render/stream-progress.tssrc/cli/render/error-card.tssrc/cli/render/index.tssrc/cli/_lib/stream-renderer.tssrc/cli/_lib/stream-renderer-lifecycle.tssubagent-statusslot inregisterOverlaySlots()src/cli/_lib/stream-renderer-process.tssrc/cli/_lib/stream-renderer-orchestrator-emit.tserrorBox→errorCardsrc/cli/errors/presenter.tserrorBox→errorCardsrc/cli/tool-category.tsTests
Gates
pnpm lint✓pnpm audit:filesize:check✓ (all new files well under 350 LOC)pnpm exec vitest run src/cli/render/ src/cli/errors/✓ (326 tests)What's next (not in this PR)
StreamProgressinto the streaming output (replace/augment existing spinner)ToolCard— collapsible tool-call display with expand/collapseCompactDiffView— inline file-edit diffsInterruptPeek— Ctrl+C → Peek at subagent conversation mid-turn