perf: UI smoothness pass — terminal + chat + styles#158
Merged
Conversation
- lineHeight 1.2 and letterSpacing 0.5 for better readability - cursorStyle 'bar' instead of block for modern feel - scrollback increased to 10,000 lines - alt fast-scroll and macOptionIsMeta enabled - tab switching changed from instant display:none to 150ms opacity fade https://claude.ai/code/session_01KXU1uAUwx3L82TMLnAmU4z
opacity:0 + pointerEvents:none does not remove hidden tabs from the browser tab order — the xterm.js textarea remains focusable. Adding the inert attribute (via ref callback to avoid TypeScript attribute gaps) makes the entire inactive container and all its descendants unfocusable, matching the behaviour of the previous display:none while preserving the 150ms opacity fade animation. https://claude.ai/code/session_01KXU1uAUwx3L82TMLnAmU4z
Streaming agent output was firing the per-key listener set synchronously for every PTY chunk, forcing a fresh allocation + synchronous term.write() per byte. Stage incoming chunks in a pending array per agent key and flush once per animation frame, so subscribers see at most one notification per frame with the cumulative history. clearPtyBuffer now cancels any pending flush for the key. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
xterm measures cell width/height from a glyph at init time; if the JetBrains Mono webfont hasn't finished loading, the measurement falls back to system monospace and rows/cols are mis-sized until the next resize. awaitFontSettle races document.fonts.load() against a 1.5s timeout so the runtime can defer its post-open refit until the real font is available. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move the Terminal instance, addons, PTY subscription, predictive echo, and parked DOM host into a module-level registry keyed by agent. React mount/detach just reparents the parked host into the visible container — xterm never tears down on tab switches, so the snapshot attach + chunk replay can no longer overlap and produce the duplicate text the user reported. Folded into this change because they all flow through the new runtime: - Defer WebglAddon construction to the next rAF after open(), so the terminal boots with the DOM renderer and upgrades on the next frame. A module-level suggestedRenderer flag demotes the rest of the session to DOM on context loss or construction throw. - Wait for awaitFontSettle() after open() before locking cell metrics in, then refit + refresh(0, rows-1). Removes the SIGWINCH "bounce" hack the old init() used to force a redraw. - Trailing-debounce the ResizeObserver to 75 ms, ignore zero-size entries (avoids bad fits during allotment drags), and re-pin the viewport to bottom after fit if it was pinned before. - useTerminal becomes a thin delegate: acquireTerminalRuntime -> mount(container) on visible -> detach() on unmount; dispose only when the agent is no longer in the store. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The split-mode page container wrapped each SplitTerminalPage in an absolute-positioned div animated via transform: translateX(). Sliding the WebGL canvas this way produces visible ghosting and "scroll trail" trails during streaming output. Swap the transform for plain display: visible ? 'block' : 'none' — page nav buttons and indicator pills still work without the animation. PR #142's tab-mode opacity fade is unaffected (different code path, doesn't animate the canvas). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Drop the 10s content-window heuristic (`dedupeHumanMessages` / `areDuplicateHumanMessages`). Each chat message already arrives with a unique broker event_id and the agent store reconciles by that id, so the ChatView memo just needs to scope by channel/DM. The heuristic could let duplicates slip through outside the window with mismatched routing and could collapse legitimately distinct human messages with identical bodies. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the manual scrollTop = scrollHeight effect that ran on every
messages.length change with useStickToBottom's ResizeObserver-driven
sticky behaviour. The old effect fought the browser's scroll anchoring
and yanked the viewport mid-scroll during streaming. The new wiring
keeps the user at the bottom only while they were already at the bottom
and instantly jumps on channel/DM switch via scrollToBottom('instant').
Adds [overflow-anchor:none] to the inner content container per the lib's
recommended setup, and stabilises the onReply/onReact callbacks with
useCallback so memoised ChatMessage children won't re-render on every
parent tick (paired with item #7).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ChatMessage and ThreadParticipantAvatar used `useAgentStore((state) => state.agents.find(...))` inside every message component. Every PTY tick that touched the agents array re-ran the find and re-rendered every message even when the matched agent was unchanged. Add a cached `agents` -> Map<agent-key, Agent> lookup that only rebuilds when the agents array reference changes, exposed via a new `useAgentByName(projectId, name)` hook. The hook returns the Agent object reference, so Zustand's default Object.is comparison short- circuits the re-render when the specific agent hasn't changed. Migrate ChatMessage and ThreadParticipantAvatar to the new hook and wrap ChatMessage in React.memo. Combined with the useCallback callbacks in ChatView (item #6), memoised messages skip parent re-renders entirely during streaming. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
backdrop-filter: blur(18-22px) is one of the heaviest compositor effects in Electron, especially with three surfaces stacked over the workspace. The trigger is always-visible and contributes per-frame cost; the dropdown and panel are transient but still composite while the sidebar scrolls underneath them. Bump background opacity to 96-98% to preserve the surface character without paying the blur.
|
Warning Review limit reached
More reviews will be available in 44 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughPersistent per-agent xterm runtimes were added and used by a refactored useTerminal; PTY chunks are rAF-batched with tail-only delivery; ChatMessage was memoized and ChatView now uses use-stick-to-bottom; agent lookup caching, font-settle helper, UI visibility tweaks, tests, and minor dependency/style updates were included. ChangesTerminal and Chat Performance Refactor
sequenceDiagram
participant App
participant Hook as useTerminal
participant Runtime as TerminalRuntime
participant PTY as PTY Buffer Store
participant Broker as Broker IPC
App->>Hook: mount(container, agent key)
Hook->>Runtime: acquireTerminalRuntime(agent key)
Runtime->>Runtime: create/park host + xterm Terminal
Hook->>Runtime: mount(container)
Runtime->>PTY: subscribePtyBuffer(key)
Runtime->>Broker: attachTerminal (snapshot seed/write)
PTY->>Runtime: pending chunks (rAF batched)
Runtime->>Runtime: write/route through predictive echo
App->>Hook: unmount
Hook->>Runtime: detach() (park host)
Runtime->>Runtime: retain term/state for reuse
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- relay_inbound: skip append when message id already exists - useAgentByName: when projectId provided, only return exact match
- Wrap each listener call in try/catch with console.error. - Iterate a snapshot ([...keyListeners]) so unsubscribes during dispatch don't corrupt iteration.
…lush Fix #1/#13: initIfReady schedules a requestAnimationFrame retry when the container has no layout (hidden tab, split page mounted at display:none). mount() now always invokes initIfReady regardless of prior parentage so the retry actually fires once the container becomes visible. Fix #6: dispose() cancels the init rAF and calls clearPtyBuffer(key) before flipping the disposed flag, so a queued rAF flush can't write into a torn-down xterm. writeFromBuffer also early-returns when disposed as belt-and-braces.
Fix #2: chunks that arrived mid-IPC stay in pending until the next rAF. If we capture writtenChunks before that flush, the subsequent subscribe replays them on top of the snapshot we just wrote — duplicate text. Expose flushPtyChunksNow(key) that cancels the pending rAF and runs flushPending synchronously, then call it in attachAndSeed right before reading getPtyChunks(key).length.
Fix #8: previously listeners received the full (post-trim) buffer and sliced from their captured writtenChunks. At the 10k cap the trim shifts the window, so writtenChunks > buffer.length and the slice drops the freshly-added chunks. Listeners now receive only the newly-queued chunks ("tail"). The terminal runtime does the snapshot-aware initial replay manually against getPtyChunks before subscribing, and AgentNode re-pulls the canonical buffer on each notification so its preview honours the trim cap.
Fix #9: track last-sent rows/cols and skip duplicate resizePty IPC. The ResizeObserver fires on every dragged pixel; the cell grid only changes at discrete steps. Fix #12: refuse a second concurrent mount of the same runtime into a different container. Currently chunkAgents doesn't trigger this, but silently reparenting would tear xterm out from under the original owner. Fix #15: post-font-settle metrics may differ from the pre-settle ones the predictor was built with. Call predictiveEcho.onResize after the refit so column wraps line up with the real grid. Also adds refreshOnShow() and setInputSrttGetter() to the runtime interface in preparation for use-terminal wiring.
Fix #11: WebGL doesn't repaint while the host is display:none. When the visible effect runs after a hidden→visible transition, call the new runtime.refreshOnShow() so the canvas redraws. Fix #10: the runtime captures opts.getInputSrtt once at first acquire. Rebind on each effect run via setInputSrttGetter so a remount with a fresh inputSrttRef can't leave the predictor reading a stale ref. Fix #14 (detach guard) was implemented inside the runtime in the prior commit by tracking lastMountedContainer — no use-terminal change needed.
Per gemini-code-assist review on PR #158. The window-focus handler scheduled a 50ms setTimeout but only cleaned up the event listener, leaving the timeout to fire on a possibly-disposed runtime. Capture the timer and clearTimeout on cleanup; also defensively null-check term before calling focus().
# Conflicts: # src/renderer/src/hooks/use-terminal.ts
miyaontherelay
changed the base branch from
claude/terminal-visual-tweaks
to
main
…-event redraws User reported stacked duplicate TUI cards (Claude Code tool calls) after tab-switching back to the same terminal — ~1 card per switch. None of the PTY chunk paths, predictive-echo passthroughs, snapshot-replay races, or listener leaks could account for it under code-tracing. The smoking gun: TUIs that enable DECSET ?1004 (focus event reporting) receive '\x1b[I' on every focusin of the xterm textarea. Claude Code redraws its TUI on focus-in. On a card layout that re-emits the same content via cursor-positioning, the new card appends below the cursor instead of overwriting in place — stacking one card per tab switch. Pear's visibility effect fired term.focus() 50ms after every visible flip. Removed. User-initiated clicks on the terminal still focus via the pointerdown handler in the main effect; window-focus auto-focus is preserved since it fires far less often. UX cost: one extra click to focus the textarea after a tab switch, in exchange for eliminating duplicate TUI redraws.
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
DO NOT MERGE. Logs every appendPtyChunk arrival and every writeChunks invocation with a preview. Will be reverted after the duplication root cause is identified. User reports duplication on single `/mcp` invocation with no tab switch, which rules out the focus-event theory and the prior 15 fixes. Need to disambiguate: is the chunk arriving twice from the broker (two [diag:pty-append] logs with same preview), is writeChunks being called twice (two [diag:runtime:writeChunks] with same preview), or is the same content arriving in distinct chunks (TUI re-emit).
Six fixes from coderabbit/devin/cubic review threads:
1. terminal-runtime: don't latch attachSeeded before broker.attachTerminal
resolves. Split attachInFlight vs attachSeeded — only set seeded on
success path, and reset attachInFlight in finally. A failed IPC no
longer permanently locks the runtime out of re-attach.
(coderabbit, cubic confidence-9 — same root cause)
2. terminal-runtime: SIGWINCH bounce re-reads liveTerm.rows/cols inside
the .then callback. The captured (rows, cols) before the async
boundary could be stale if the user resized between the first and
second resizePty IPC.
(cubic confidence-8)
3. agent-store: ChatMessage gains a flag set by
addHumanMessage; findOptimisticHumanMatch only replaces records
carrying that flag. Without the scope, two distinct human messages
sharing body/target/time-window would collapse into one. Reconcile
clears the flag after replacement so a subsequent optimistic can
still match its own future canonical echo.
(cubic confidence-6)
4. terminal-runtime + use-terminal: cleanup uses runtime.clearOnDataIf(
handler) instead of setOnData(null). When cross-tree React commit
ordering fires an old hook's cleanup after a new hook installed its
own onData, the identity check prevents wiping the new hook's input
forwarding. Same bug class as the token-based detach guard, applied
to onData.
(cubic confidence-7)
5. TerminalPane: tab-mode inactive terminals reverted to
from the brief opacity+visibility transition. The
opacity path kept every inactive WebGL canvas live in the compositor
even though invisible, costing GPU memory and per-frame composite
work. The visible-effect's refreshOnShow() handles the WebGL stale-
frame concern on return.
(devin)
6. terminal-runtime: removed releaseTerminalRuntime and the refCount
bookkeeping. Both were dead code — useTerminal never called release,
and refCount was never read. Disposal already goes through
disposeTerminalRuntime on store-driven agent release.
(devin)
Plus: new src/renderer/src/stores/pty-buffer-store.test.ts with 7
regression tests covering rAF coalescing, tail-only listener
semantics, clear-cancels-pending-flush, flushPtyChunksNow synchronous
drain, listener-throw isolation, unsubscribe, and the AGENTS.md-
mandated duplicate/replay case (renderer-side guarantee: each append
delivers once; dedup is the broker's responsibility).
(devin: AGENTS.md violation on missing PTY-buffering tests)
Gates: tsc clean on touched files; vitest 7/7 pass; build clean.
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/lib/terminal-runtime-registry.ts (1)
582-589:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't mark a resize as delivered before
resizePty()succeeds.
lastSentRows/Colsare updated before the IPC resolves. IfresizePty()rejects, the runtime now believes that size was already delivered, so laterfitAndSync()calls at the same cell grid become no-ops and the PTY can stay stuck at the old geometry until the user hits a different size.🐛 Proposed fix
if (size.rows !== lastSentRows || size.cols !== lastSentCols) { + const prevRows = lastSentRows + const prevCols = lastSentCols lastSentRows = size.rows lastSentCols = size.cols pear.broker .resizePty(opts.projectId, opts.agentName, size.rows, size.cols) - .catch(() => {}) + .catch(() => { + if (lastSentRows === size.rows && lastSentCols === size.cols) { + lastSentRows = prevRows + lastSentCols = prevCols + } + }) }🤖 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 `@src/renderer/src/lib/terminal-runtime-registry.ts` around lines 582 - 589, The code updates lastSentRows/lastSentCols before the IPC call resolves, so if pear.broker.resizePty rejects the runtime will think the resize was delivered; move the assignment of lastSentRows and lastSentCols to after resizePty() succeeds (i.e., in the .then/await success path) and only swallow errors in the .catch without mutating those variables; update the block around pear.broker.resizePty (the resize dispatch path used by fitAndSync/resize handling) so failures do not mark the size as delivered and allow subsequent fitAndSync() calls to retry.
♻️ Duplicate comments (1)
src/renderer/src/lib/terminal-runtime-registry.ts (1)
436-448:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop polling hidden terminals every frame.
initIfReady()requeues itself on every frame while the container is0×0.mount()always kicks that off, and the hook still mounts runtimes before they have layout, so inactivedisplay:nonepanes keep a live rAF loop until they are shown again. Retry from a visibility/layout signal instead of recursively scheduling frames off the hidden container. Based on learnings fromsrc/renderer/src/hooks/use-terminal.ts:134-191, the hook mounts zero-layout containers and only detaches on cleanup.Also applies to: 511-513
🤖 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 `@src/renderer/src/lib/terminal-runtime-registry.ts` around lines 436 - 448, initIfReady currently requeues itself every rAF while the container has no layout, causing hidden terminals to spin indefinitely; replace the recursive rAF retry with a one-time visibility/layout listener (e.g., ResizeObserver, IntersectionObserver or MutationObserver) that watches the container and calls initIfReady when it gets non-zero layout, and remove the continuous pendingInitFrame rAF loop/logic in initIfReady and the analogous retry block (also present around the mount flow). Keep existing guards (term, disposed, hasLayout) and ensure the observer is disconnected on dispose or after the first successful init.
🧹 Nitpick comments (2)
src/renderer/src/components/terminal/TerminalPane.tsx (1)
1032-1032: 💤 Low valueConsider documenting the display toggle approach here too.
Split mode includes a clear comment (lines 954-958) explaining why
display: block/nonereplacedtranslateXto avoid WebGL canvas ghosting. Tabbed mode uses the same approach but lacks a similar inline comment. Adding a brief comment here would help maintainers understand the consistency and rationale.📝 Suggested documentation addition
return ( <div key={agentKey} + // Use display:none (not visibility:hidden or translateX) to avoid + // WebGL canvas ghosting during streaming. Persistent runtime handles remount. className="absolute inset-0" style={{ display: active ? 'block' : 'none' }}🤖 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 `@src/renderer/src/components/terminal/TerminalPane.tsx` at line 1032, Add a short inline comment next to the style toggling in TerminalPane (the element using style={{ display: active ? 'block' : 'none' }}) explaining that we use display:block/none instead of translateX to avoid WebGL canvas ghosting and repaint/scroll issues (same rationale as the split-mode comment near the split rendering logic); this keeps the tabbed-mode behavior consistent and documents why we avoid CSS transforms for hiding panes..agentworkforce/workforce/personas/terminal-renderer.json (1)
28-28: ⚡ Quick winConsider externalizing the prompt for maintainability.
The
claudeMdContentembeds ~8KB of markdown as a JSON string. While the content itself is well-structured and appropriate for a specialist persona, inline embedding creates friction:
- No syntax highlighting or markdown tooling when editing
- Difficult to review prose changes in diffs (single massive line)
- Escaping and formatting challenges
- Hard to compose or reuse prompt sections across personas
♻️ Recommended approach
Store the prompt as
.agentworkforce/workforce/personas/terminal-renderer.prompt.mdand reference it:- "claudeMdContent": "# Terminal Renderer\n\n..." + "claudeMdContentPath": "./terminal-renderer.prompt.md"If the harness doesn't yet support
claudeMdContentPath, this remains a future refactor target.🤖 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 @.agentworkforce/workforce/personas/terminal-renderer.json at line 28, claudeMdContent is embedding a huge markdown blob inline; extract that markdown into a separate file and reference it from the persona JSON by adding a new key claudeMdContentPath that points to the new markdown file, leaving the existing claudeMdContent as a backwards-compatible fallback for harnesses that don't yet support external paths (remove the inline field in a follow-up once consumers support claudeMdContentPath); update any loader/consumer to prefer claudeMdContentPath over claudeMdContent if present.
🤖 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 @.agentworkforce/workforce/personas/terminal-renderer.json:
- Around line 25-27: The persona file sets "permissions" with "mode":
"bypassPermissions", which is a security risk; either replace that mode with a
scoped permission object granting only the minimal read/analysis capabilities
the terminal-renderer persona needs (e.g., read access to logs/configs and no
write/execute privileges) or add an adjacent comment explaining why
bypassPermissions is strictly required and limited; locate the "permissions"
block in .agentworkforce/workforce/personas/terminal-renderer.json and change
the "mode": "bypassPermissions" entry to a scoped permissions spec or add the
justification comment next to that symbol.
- Line 28: The persona's anti-goals conflict with project policy by forbidding
tests ("Don't add tests for things you can't reliably automate against an
Electron renderer") while the project mandates regression tests for broker/PTY
changes; update the persona JSON's anti-goals section (the string containing
"Don't add tests for things you can't reliably automate against an Electron
renderer") to add an explicit exception: when touching broker start, event
streaming, PTY buffering, spawned personas, or integration notifications,
include regression tests (duplicate/replay cases) per project guidelines, or
alternatively add a documented carve-out explaining test scope and justification
so reviewers can accept PTY-related changes without violating policy.
In `@src/renderer/src/components/terminal/TerminalPane.tsx`:
- Around line 1033-1034: Split mode currently sets aria-hidden but not inert for
hidden terminals, causing inconsistent interaction blocking with tabbed mode; in
the split-mode render path (the JSX element that uses the visible boolean in
TerminalPane/RenderSplitTerminal — the element around where
aria-hidden={!visible} is set) add inert={!visible} so hidden panes are truly
non-interactive, and if TypeScript complains about the inert prop update the
element typing (or cast the element/props) so inert is accepted consistently
with the tabbed-mode usage of aria-hidden={!active} inert={!active}.
In `@src/renderer/src/stores/agent-store.ts`:
- Around line 377-404: The current duplicate-check (isDuplicateHumanEcho)
prevents inserting a second optimistic human message with the same body/target
within the dedupe window, causing the real second message to be dropped later;
to fix, stop gating optimistic/local inserts with isDuplicateHumanEcho — either
change isDuplicateHumanEcho to ignore messages where local === true or skip
running it inside addHumanMessage for local/optimistic messages, and ensure the
relay_inbound handling uses findOptimisticHumanMatch (which matches only
existing.local records) to reconcile canonical echoes instead of relying on
isDuplicateHumanEcho; update references in addHumanMessage,
isDuplicateHumanEcho, and the relay_inbound fallback so local optimistic echoes
are allowed and then correctly replaced by their canonical event_id.
---
Outside diff comments:
In `@src/renderer/src/lib/terminal-runtime-registry.ts`:
- Around line 582-589: The code updates lastSentRows/lastSentCols before the IPC
call resolves, so if pear.broker.resizePty rejects the runtime will think the
resize was delivered; move the assignment of lastSentRows and lastSentCols to
after resizePty() succeeds (i.e., in the .then/await success path) and only
swallow errors in the .catch without mutating those variables; update the block
around pear.broker.resizePty (the resize dispatch path used by fitAndSync/resize
handling) so failures do not mark the size as delivered and allow subsequent
fitAndSync() calls to retry.
---
Duplicate comments:
In `@src/renderer/src/lib/terminal-runtime-registry.ts`:
- Around line 436-448: initIfReady currently requeues itself every rAF while the
container has no layout, causing hidden terminals to spin indefinitely; replace
the recursive rAF retry with a one-time visibility/layout listener (e.g.,
ResizeObserver, IntersectionObserver or MutationObserver) that watches the
container and calls initIfReady when it gets non-zero layout, and remove the
continuous pendingInitFrame rAF loop/logic in initIfReady and the analogous
retry block (also present around the mount flow). Keep existing guards (term,
disposed, hasLayout) and ensure the observer is disconnected on dispose or after
the first successful init.
---
Nitpick comments:
In @.agentworkforce/workforce/personas/terminal-renderer.json:
- Line 28: claudeMdContent is embedding a huge markdown blob inline; extract
that markdown into a separate file and reference it from the persona JSON by
adding a new key claudeMdContentPath that points to the new markdown file,
leaving the existing claudeMdContent as a backwards-compatible fallback for
harnesses that don't yet support external paths (remove the inline field in a
follow-up once consumers support claudeMdContentPath); update any
loader/consumer to prefer claudeMdContentPath over claudeMdContent if present.
In `@src/renderer/src/components/terminal/TerminalPane.tsx`:
- Line 1032: Add a short inline comment next to the style toggling in
TerminalPane (the element using style={{ display: active ? 'block' : 'none' }})
explaining that we use display:block/none instead of translateX to avoid WebGL
canvas ghosting and repaint/scroll issues (same rationale as the split-mode
comment near the split rendering logic); this keeps the tabbed-mode behavior
consistent and documents why we avoid CSS transforms for hiding panes.
🪄 Autofix (Beta)
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: 75de6b1e-ec6a-4ea6-b8bd-0c6dc09ccfdf
📒 Files selected for processing (7)
.agentworkforce/workforce/personas/terminal-renderer.jsonsrc/renderer/src/components/terminal/TerminalPane.tsxsrc/renderer/src/hooks/use-terminal.tssrc/renderer/src/lib/terminal-runtime-registry.tssrc/renderer/src/stores/agent-store.tssrc/renderer/src/stores/pty-buffer-store.test.tssrc/renderer/src/stores/pty-buffer-store.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/stores/pty-buffer-store.ts
- src/renderer/src/hooks/use-terminal.ts
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
User reports chat duplication 'especially when a lot of messages flow in,' mostly on agent messages. The prior fixes only guarded human messages via isDuplicateHumanEcho; agent messages relied purely on alreadySeenById (event_id equality). When the broker delivers the same logical agent message via two streams with different event_ids (relay_inbound vs reconcileMessages snapshot, or worker restart re-emit), both records survive id-based dedup and the user sees the message twice in chat. Per AGENTS.md: 'Prefer stable event identity over content matching ... PTY and broker events should carry event_id, id, or seq; dedupe by that identity first and use short content-based windows only as a fallback.' This is the fallback. - New isDuplicateAgentEcho() — same shape as the human guard, scoped to non-human senders, matches on (from, body, project, target), with a tighter 2s window so legitimately distinct fast agent replies don't collapse. - relay_inbound handler: appends only when neither human nor agent dedupe guards fire. - reconcileChatMessages: after the id-miss + optimistic-match check, the agent guardrail prevents adding a second-id copy of an already- reconciled agent message. Gates: tsc clean; existing pty-buffer-store tests still pass; build clean. Terminal duplication under heavy load is a separate issue still under investigation; the prior diag capture was clean on /mcp but heavy-load conditions may expose a snapshot/buffer race not yet pinpointed.
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
Four threads:
1. agent-store: stop collapsing legitimately distinct repeated human
sends. addHumanMessage no longer gates on isDuplicateHumanEcho —
it always appends the optimistic record. Renamed
isDuplicateHumanEcho to isCanonicalEchoOfLocalHuman and scoped it
to local: true records so the relay_inbound human-dedup only
catches the optimistic-canonical pair, never two distinct "ok"
sends within the 10s window. (coderabbit major)
2. TerminalPane: added inert={!visible} to the split-page wrapper to
match the tabbed-mode treatment. Both use display: none and both
should block interaction with hidden subtrees. (coderabbit minor)
3. terminal-renderer persona: added an explicit _justification field
alongside bypassPermissions, documenting the legitimate need
(reading xterm dist, writing temp instrumentation + regression
tests across the renderer/main boundary). Matches the pattern in
@agentworkforce/persona-autonomous-actor. (coderabbit major)
4. terminal-renderer persona: updated the conflicting anti-goal to
acknowledge AGENTS.md's regression-test requirement for PTY
buffering / broker work. The original phrasing ("manual test
plans are the contract") would have authorized the persona to skip
tests AGENTS.md explicitly requires. Now the anti-goal scopes to
visual/render behavior that can't be unit-tested while making
clear the project requirement supersedes for the auto-testable
surface. (coderabbit major)
Gates: tsc clean; vitest 7/7 pass; build clean.
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ge copy Two cubic findings on the agent-dedupe guardrail: 1. Perf — reconcileChatMessages was calling Array.from(byId.values()) on every incoming message, producing O(n²) array copies on the existing message buffer. Under the user's stated target of 1000+ communicating agents this becomes a real smoothness regression. isDuplicateAgentEcho now accepts an Iterable<ChatMessage> so the reconcile path passes byId.values() directly. No copy per message. 2. Correctness — the previous projectId predicate let undefined wildcard-match either side, which could shadow a real distinct message in another project (e.g. an agent message with projectId could be falsely identified as a duplicate of an unscoped message with the same body/target/time-window). Now requires exact equality of message.projectId === candidate.projectId on both sides. Gates: tsc clean; vitest 7/7; build clean.
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Phase 1 of the test-infrastructure expansion — exercises the renderer
store layer at the "1000s of agents communicating" scale.
agent-store.stress.test.ts (7 tests, ~6.7s):
- 1000 agents × 50 messages + 100 human sends + 5 reconciles burst
- no-duplicate-ids invariant across relay_inbound + reconcile streams
- optimistic human sends never lost to isCanonicalEchoOfLocalHuman
- cross-project agent dedupe does NOT false-positive
- 3s-apart identical agent sends both survive the 2s window
- broker-replay (same agent+body+project within 2s) collapses to one
- MAX_CHAT_MESSAGES cap (5000) holds under 60k-message burst
- reconcileMessages returns stable messages array reference on no-op
pty-buffer-store.stress.test.ts (7 tests, ~50ms):
- 10 agents × 5000 chunks exactly-once delivery to always-on listeners
- rapid subscribe/unsubscribe churn doesn't drop chunks for siblings
- MAX_PTY_BUFFER_CHUNKS (10k) trim cap holds under 30k-chunk burst
- tail-only semantics: mid-stream subscriber sees only post-subscribe
chunks even across trim
- clearPtyBuffer cancels in-flight rAF flushes — no stale data fires
- throwing listener does not block delivery to siblings
- flushPtyChunksNow drains synchronously without duplicate fire
Total wall time ~7s (well under the 30s budget). Files are picked up by
the existing vitest node-env project via the *.test.ts include pattern;
no vitest config changes were needed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
… tests Adds a scoped happy-dom project to `vitest.config.mjs` so renderer component lifecycle can be unit-tested without an Electron runtime. Existing node-env tests are unchanged (split by `*.dom.test.ts`). Mock xterm at `src/renderer/src/__test__/xterm-mock.ts` records raw `write()` byte sequences and lifecycle calls (open/dispose/refresh/ resize/loadAddon) without emulating xterm's VT parser. Coverage: - terminal-runtime-registry: token-based mount/detach ownership, pty-buffer subscribe-on-mount + unsubscribe-on-dispose with no listener leak across remounts, identity-checked `clearOnDataIf`, `refreshOnShow` repaint contract, dispose cancels pending init rAF. - useTerminal: `runtimeRef.current` (via returned term) stays stable across re-renders for the same agent key; switching agentName produces a fresh runtime/terminal. No production code changes — research-and-test deliverable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
This was referenced Jun 8, 2026
miyaontherelay
added a commit
that referenced
this pull request
Jun 8, 2026
Resolves the 817ms longest-frame at 5k visible chat surfaced by PR #158's stress harness phase 3. virt-impl (codex) shipped 3 phases; virt-review (claude) GO with no REWORK. 218/218 tests pass.
miyaontherelay
added a commit
that referenced
this pull request
Jun 8, 2026
User asked for the persona's skills to be LOCAL files (not phantom
remote package references like the autonomous-actor pattern). This
commit:
1. Creates six substantive markdown skill files under
.agentworkforce/workforce/personas/__assets/terminal-renderer/ —
each capturing a discrete domain of the persona's expertise:
- xterm-internals.md (105 lines) — parser pipeline, renderer
trade-offs, addon discipline, focus mode, viewport vs scrollback,
font measurement timing
- pty-broker-pipeline.md (172 lines) — chunk path from broker IPC
to term.write, rAF coalescing, snapshot-vs-replay race, SIGWINCH
bounce, trim cap accounting, predictive echo discipline
- ansi-vt-sequences.md (152 lines) — sequence taxonomy, cursor
movement, DECSET modes (?1004 / ?1049 / ?2026), TUI redraw
patterns and their failure modes
- bug-class-triage.md (104 lines) — symptom-to-bug-class lookup
table built from the PR #158 catalogue; use BEFORE reading source
- lifecycle-decoupling.md (249 lines) — runtime registry pattern,
token-based mount ownership, parked host, clearOnDataIf identity
check, dispose ordering
- fix-discipline.md (152 lines) — one-write invariant, read before
guessing, minimal-diff scope, instrument-don't-guess after two
failed fixes, AGENTS.md test requirement
2. Updates terminal-renderer.json skills array to reference each file
via __assets/terminal-renderer/<id>.md local path. Description for
each skill summarizes the file content for quick orientation.
Matches the existing .agentworkforce local asset pattern (slack-comms
uses __assets/slack-comms/slack-comms.md for its claudeMd).
934 lines of structured operating knowledge extracted from the PR #158
fix cycle so future investigations don't have to re-derive it.
khaliqgant
added a commit
that referenced
this pull request
Jun 8, 2026
* chore(persona): write local skill files for terminal-renderer
User asked for the persona's skills to be LOCAL files (not phantom
remote package references like the autonomous-actor pattern). This
commit:
1. Creates six substantive markdown skill files under
.agentworkforce/workforce/personas/__assets/terminal-renderer/ —
each capturing a discrete domain of the persona's expertise:
- xterm-internals.md (105 lines) — parser pipeline, renderer
trade-offs, addon discipline, focus mode, viewport vs scrollback,
font measurement timing
- pty-broker-pipeline.md (172 lines) — chunk path from broker IPC
to term.write, rAF coalescing, snapshot-vs-replay race, SIGWINCH
bounce, trim cap accounting, predictive echo discipline
- ansi-vt-sequences.md (152 lines) — sequence taxonomy, cursor
movement, DECSET modes (?1004 / ?1049 / ?2026), TUI redraw
patterns and their failure modes
- bug-class-triage.md (104 lines) — symptom-to-bug-class lookup
table built from the PR #158 catalogue; use BEFORE reading source
- lifecycle-decoupling.md (249 lines) — runtime registry pattern,
token-based mount ownership, parked host, clearOnDataIf identity
check, dispose ordering
- fix-discipline.md (152 lines) — one-write invariant, read before
guessing, minimal-diff scope, instrument-don't-guess after two
failed fixes, AGENTS.md test requirement
2. Updates terminal-renderer.json skills array to reference each file
via __assets/terminal-renderer/<id>.md local path. Description for
each skill summarizes the file content for quick orientation.
Matches the existing .agentworkforce local asset pattern (slack-comms
uses __assets/slack-comms/slack-comms.md for its claudeMd).
934 lines of structured operating knowledge extracted from the PR #158
fix cycle so future investigations don't have to re-derive it.
* docs(persona): address review feedback on terminal-renderer skill files
Multiple bot reviewers (coderabbit, cubic, gemini) flagged the same
high-confidence issue: 'Companion reading' sections in each skill file
referenced the original long ID names (xterm-internals-and-renderers.md,
pty-broker-streaming-pipeline.md, etc.) rather than the actual short
filenames (xterm-internals.md, pty-broker-pipeline.md). Cross-doc links
were dead.
Fixed:
- All 6 'Companion reading' sections now reference the actual filenames
- Two inline references (bug-class-triage.md, fix-discipline.md)
- Character set designation sequences: \e(B / \e(0 (no space between
'(' and final byte — gemini correctly noted my readability spaces
were technically inaccurate)
- 'one-pixel bounce' → 'one-row bounce' for the SIGWINCH bounce (it's
a terminal-row resize, not a pixel resize)
- 'Move up 60 lines' clarified to distinguish absolute (CSI <r>;<c> H)
vs relative (CSI <n> A) cursor movement
- Language tags on the untyped fenced blocks the bots flagged
specifically (MD040 in xterm-internals, pty-broker-pipeline,
ansi-vt-sequences, fix-discipline)
H1 titles in each file (e.g. # xterm-internals-and-renderers) preserved
— they're descriptive section names, not file references, and serve as
the canonical skill IDs which the persona JSON also uses.
* fix(persona): repoint terminal-renderer local skills to repo-root-relative paths
The 6 skills[] sources used paths relative to the personas dir
(`__assets/terminal-renderer/*.md`), but the AgentWorkforce local-skill
provider (persona-kit skills.js `localProvider`) resolves a relative
`source` against `repoRoot = process.cwd()` (cli.js:1242/1346/1376) — the
project root. So they resolved to `<root>/__assets/...`, which does not
exist, and the `cp` install silently failed: the skills never loaded.
Fix: use the full repo-root-relative path
`.agentworkforce/workforce/personas/__assets/terminal-renderer/*.md`,
matching the supported local-skill form documented in the provider
(e.g. `.agentworkforce/workforce/skills/<name>.md`). All 6 now resolve to
real files; JSON validates; skill files unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(persona): add SKILL frontmatter to terminal-renderer skill files
A Claude skill is only valid/discoverable with a YAML frontmatter block
(name + description). The 6 terminal-renderer skill .md files started
straight at the H1 with no frontmatter, so they would not register as
skills once installed. Add `name` + a one-line "Use when…" `description`
to each, matching the workforce personas/skills/<name>/SKILL.md shape.
`name` is set to each file's basename stem (ansi-vt-sequences,
bug-class-triage, fix-discipline, lifecycle-decoupling, pty-broker-pipeline,
xterm-internals) — the install dir the local-skill provider derives from
the source basename, which a Claude skill's frontmatter name must match.
Files stay where they are; the earlier path fix is unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Khaliq <khaliqgant@gmail.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 9, 2026
Merged
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
2 participants
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.
Summary
End-to-end UI smoothness pass addressing the choppy terminal + chat behavior (duplicated text, scroll trails, text drag during streaming agent output). Stacks on top of #142 (visual tweaks), preserving all of those polish changes.
Implements 7 fixes from the original survey plus 15 follow-up bug fixes from a multi-agent code review + 2 review-bot fixes.
What changed — original 7 fixes
Terminal pipeline
pty-buffer-store.ts): stage incoming chunks per agent and flush once perrequestAnimationFrameinstead of firing listeners synchronously per byte. Listener semantics are now tail-only (each notification carries just the new chunks) so subscribers can't redraw the whole window per tick.lib/terminal-runtime-registry.ts, rewrittenuse-terminal.ts): xterm instances, addons, predictive echo, and the PTY buffer subscription now live in a module-level registry keyed by agent. React only attaches/detaches a DOM host; tab switches reparent the host instead of disposing the runtime. Kills the duplicate-text class that came from re-attach + snapshot + buffer-replay racing on remount.translateXover WebGL (TerminalPane.tsx): split-page swap is now adisplaytoggle. Sliding a WebGL canvas via CSS transform was a primary source of "text drag" / ghost trails.use-terminal.tsvia registry): 75 ms trailing debounce, skip zero-size entries, preserve viewport-pinned-to-bottom across fits. SIGWINCH bounce dropped.lib/font-settle.ts): awaitdocument.fonts.load()then refit + refresh + reset predictive echo metrics, so xterm doesn't measure cell width before JetBrains Mono is ready.use-terminal.ts): defer addon to next rAF; on construction throw or context loss, set a module-levelsuggestedRenderer = 'dom'so subsequent terminals skip WebGL for the rest of the session.Chat rendering
ChatView.tsx): thededupeHumanMessages10-second(channel, body)heuristic is gone. Plus:relay_inboundhandler inagent-store.tsnow does an id-based no-op before append, so the same brokerevent_idcan't render twice.use-stick-to-bottom+[overflow-anchor:none](ChatView.tsx): replaces the manualscrollTop = scrollHeighteffect that fought the browser's own anchoring during streaming.React.memo(ChatMessage)+useAgentByNameselector (ChatMessage.tsx,agent-store.ts): every message previously didstate.agents.find(...)against the whole agents array — one PTY tick re-rendered every message. New cached map selector + memo'd row component cuts that to O(actually-changed). Selector strictly scopes byprojectId(no cross-project fallback when projectId is provided).Styles
styles.css): threebackdrop-filter: blur(18–22px) saturate(...)surfaces on the project switcher replaced with high-opacity gradient backgrounds.Fix pass — 15/15 verified clean
A multi-agent review of the original 7-fix branch turned up 7 Tier 1 blockers and 8 Tier 2 worth-fixing. All 15 have been verified by the final reviewer:
initIfReadyretries via rAF when container has no layout;mount()always runs init regardless of parentage. Split-page hidden terminals now initialize correctly.flushPtyChunksNow(key)synchronously drains pending chunks before snapshot baseline, eliminating the rAF-after-snapshot duplicate-text race.relay_inboundid-based dedupe before append.useAgentByNameonly consults name-only key whenprojectId === undefined.pty-buffer-store.dispose()callsclearPtyBuffer(key)before nulling state;writeFromBufferearly-returns ondisposed || !term.[...keyListeners]snapshot before iteration in both flush and clear paths.lastSentRows/lastSentColscached;resizePtyIPC skipped on no-op.setInputSrttGetterrebound on eachuseTerminaleffect run.runtime.refreshOnShow()triggered from visible effect to redraw the WebGL canvas afterdisplay: nonereturns.mount(container)returns a symbol token;detach(token)is a no-op for stale tokens. The latest mount silently reparents the host, so React cross-tree handoffs (B.mount → A.cleanup) keep the canvas in B. Replaces the original Fix Reuse existing local broker on startup #12 refusal guard and Fix feat(proactive-agents): vendored runtime types + Monaco + smoke test (spec 03) #14lastMountedContainerguard with a single coherent model.predictiveEcho.onResizecalled after post-font-settle refit.Plus 2 review-bot fixes:
setTimeoutin window-focus handler now captured +clearTimeoutd on cleanup; defensive null-check ontermbeforefocus().Manual test plan
resizePtyIPC only fires on row/col grid changes (feat(integrations): pear-side integrations feature (manager + catalog + Nango logos) #9).relay_inboundfor that agent arrives: confirm no duplicate chat messages (Add release packaging and Pear install site #3) anduseAgentByNameresolves to the correct project (Replace ad-hoc parsing with Zod schemas; dedupe shared helpers #4).Notes & flags
relay_inboundis whole-message (no per-character assistant streaming), so the original "current-${id}" split from the survey wasn't needed. If incremental assistant edits ever land, the split goes on top.ThreadPanel.tsx'sReplyAvatarstill uses inlineagents.find— one-line follow-up using the newuseAgentByNameselector.