cue-hardening: leak fix, panel layout, themed scrollbars + GeneralTab cleanup#742
Merged
reachrazamair merged 6 commits intorcfrom Apr 6, 2026
Merged
cue-hardening: leak fix, panel layout, themed scrollbars + GeneralTab cleanup#742reachrazamair merged 6 commits intorcfrom
reachrazamair merged 6 commits intorcfrom
Conversation
Hardens the boundary between the group-chat domain and the Cue automation
engine to stop group-chat transcripts from appearing in chained Cue
pipeline output.
- Tighten group-chat session-id regexes in main/constants.ts to anchor
groupChatId on the canonical UUID format. Eliminates greedy-backtrack
ambiguity when participant names contain sentinel substrings like
"-participant-" — the previous greedy capture would mis-parse and
flush buffered output against the wrong owner.
- session-parser: add strict "group-chat-" prefix guard and strip
"-recovery" from UUID-suffixed IDs.
- data-listener / exit-listener: drop any sessionId that starts with
GROUP_CHAT_PREFIX but matches neither the moderator nor participant
shape. Without this guard a malformed group-chat ID would fall through
to safeSend('process:data'), the web broadcast path, and
cueEngine.notifyAgentCompleted, leaking transcript bytes and firing
spurious agent.completed subscriptions.
- cue-engine.notifyAgentCompleted: document the load-bearing invariant
that sourceOutput is built EXCLUSIVELY from completionData.stdout
with no fallback to any session-store or group-chat buffer. Adding
such a fallback would re-introduce the leak.
- cue-fan-in-tracker: dedupe sources by resolving them to canonical
session IDs upfront so a yaml that lists the same session by both
name and id no longer hangs waiting for a phantom second completion.
Tests cover adversarial regex inputs, the empty-stdout invariant from
the exit-listener path, fan-in dedupe, and the cross-domain containment
guards in both listeners.
Drops eslint warnings introduced when commit 5a79180 moved the Usage & Stats and WakaTime sections to the Encore Features tab but left their destructured props, state, helpers, and effect behind in GeneralTab.tsx. Removed: - Trash2 icon import (unused) - 8 destructured useSettings fields (statsCollectionEnabled + setStatsCollectionEnabled, defaultStatsTimeRange + setter, setWakatimeEnabled, wakatimeApiKey, wakatimeDetailedTracking + setter) - 7 useState declarations (statsDbSize, statsEarliestDate, statsClearing, statsClearResult, wakatimeCliStatus, wakatimeKeyValid, wakatimeKeyValidating) - handleWakatimeApiKeyChange callback - WakaTime CLI availability useEffect (~50 lines, no consumer) - Stale "Load sync settings and stats data" comment `npm run lint:eslint` now exits clean. The 84 GeneralTab tests still pass — none referenced the removed members.
Fixes three layout bugs in the agent config panel's collapsed view: 1. Output prompt box was hard-coded to flex: hasMultipleTriggers ? 0 : 1 so in multi-trigger mode the column collapsed to its content min and the textarea sat with empty space below it. Output now always uses flex: 1 — fills the column whether single- or multi-trigger. 2. Multi-trigger left rail had no scroll. Each EdgePromptRow used flex: 1 plus a textarea minHeight, so 3+ triggers exceeded the parent's intrinsic min and rows visually overlapped — bottom row's title rendered over the textarea above. Each row now uses flexShrink: 0 (intrinsic content height) and the parent column has its own overflowY: auto with a 6px scrollbar gutter so additional rows scroll instead of squeezing each other. 3. Single-trigger collapsed wasted vertical space. Input/output textareas were gated `expanded ? flex:1 : minHeight:80` so collapsed mode never filled the column even when the panel was tall. Both now always use flex: 1, minHeight: 88 (or 72 with fan-in). Also: - collapsedHeight in NodeConfigPanel replaced with a formula that accounts for triggerEdgeCount: single trigger 280, +60/extra trigger capped at +120, +130 if fan-in. Tighter for the common case, larger when needed. - Removed the redundant outer overflow: auto on AgentConfigPanel and the input/output split row. NodeConfigPanel content wrapper is now overflow: hidden for agent nodes — single source of scroll per axis. - Title spans get explicit flexShrink: 0 + marginBottom so flex layout can never collapse them under the textarea. Load-bearing.
Replaces the hard-coded rgba(255,255,255,0.15) scrollbar thumb (which
was invisible on light themes like GitHub, One Light, Catppuccin Latte,
Ayu Light) with a unified, theme-driven scrollbar system that picks up
the active theme automatically — no per-component changes needed.
Three layers:
1. useThemeStyles now injects six CSS variables on :root:
--accent-color = theme.accent
--highlight-color = theme.accent (legacy alias)
--scrollbar-thumb = theme.border
--scrollbar-thumb-hover = theme.textDim
--scrollbar-thumb-active = theme.accent
--scrollbar-track = theme.bgActivity
Hook updates them in a useEffect so theme switches are instant. The
ThemeColors interface gained `border`, `textDim`, `bgActivity` —
App.tsx already passes the full theme.colors so callers are
unaffected.
2. index.css gets three layers of scrollbar rules:
- Global *::-webkit-scrollbar — themes EVERY scrollable element in
the app (~200 inline `overflow: auto` containers that don't carry
a class). 10px width, soft inset border for a "floated" thumb,
transparent track.
- .scrollbar-thin — slim 6px variant for tight UI areas (~50
components), now uses the same CSS variables instead of hardcoded
white rgba.
- .scrollbar-thin.scrolling/.fading — stateful brighten-on-scroll
then fade-after-1s animation, kept intact and managed by the
existing JS in useThemeStyles.
Firefox `scrollbar-color` is also set on `*` for cross-browser
support. Every var() has a sensible fallback so initial paint
(before useThemeStyles runs) renders correctly.
3. New ScrollArea component (src/renderer/components/ScrollArea.tsx) —
thin wrapper for new call sites. Two variants (default 10px / thin
6px), four axis modes (both/x/y/none), hideScrollbar opt-out, full
ref forwarding and HTML attribute passthrough. Existing
.scrollbar-thin usages keep working unchanged — this is purely
additive.
Tests pin the contract:
- useThemeStyles.test.ts (6 tests) — verifies all six CSS variables are
set, mapping is correct, vars update on theme change, light theme
produces visible thumb (regression).
- ScrollArea.test.tsx (13 tests) — variant class composition, axis
modes, ref forwarding, attribute passthrough, hideScrollbar override.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTightens group-chat session parsing to require UUIDs, drops unrecognized group-chat sessions early in process listeners, deduplicates fan-in sources and centralizes stdout handling in Cue, adds a ScrollArea component, expands theme scrollbar tokens/styles, and adds/updates tests across parsing, cue, listeners, and renderer. Changes
Sequence Diagram(s)sequenceDiagram
participant ProcessManager as ProcessManager
participant DataListener as DataListener
participant Parser as SessionParser
participant SafeSend as safeSend / Renderer
participant WebServer as WebServer
participant OutputBuf as OutputBuffer
ProcessManager->>DataListener: emit 'data' (sessionId, payload)
DataListener->>Parser: parseModerator(sessionId) / parseParticipant(sessionId)
Parser-->>DataListener: no match (unrecognized group-chat shape)
DataListener->>DataListener: log warning "unrecognized group-chat sessionId shape"
Note over DataListener,SafeSend: Early return — no safeSend, no broadcast, no append
sequenceDiagram
participant ProcessManager as ProcessManager
participant ExitListener as ExitListener
participant Parser as SessionParser
participant CueEngine as CueEngine
participant SafeSend as safeSend / Renderer
ProcessManager->>ExitListener: emit 'exit' (sessionId, {status, exitCode, completionData?})
ExitListener->>Parser: parseModerator / parseParticipant
alt recognized group-chat
ExitListener->>CueEngine: notifyAgentCompleted({...completionData...})
else unrecognized group-chat
ExitListener->>ExitListener: log warning and return
end
ExitListener->>SafeSend: (only if non-group-chat or recognized) safeSend('process:exit', ...)
sequenceDiagram
participant CueEngine as CueEngine
participant Subscriber as CompletionSubscriber
participant FanIn as FanInTracker
CueEngine->>CueEngine: rawStdout = completionData?.stdout ?? ''
CueEngine->>Subscriber: for each subscriber (single source_session)
Note over CueEngine,FanIn: Fan-in dedup uses resolveSourcesToIds(sources) -> Set(ids)
CueEngine->>FanIn: send event.payload { sourceOutput: rawStdout.slice(-N), outputTruncated: rawStdout.length > N }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis is a four-commit polish pass that closes a group-chat→Cue output leak via UUID-anchored regexes and defense-in-depth domain-containment guards in both Confidence Score: 5/5Safe to merge — all remaining findings are P2 style suggestions with no behavioral impact. The high-stakes leak fix is sound: UUID-anchored regexes remove the greedy-backtrack root cause, domain-containment guards in both listeners provide defense-in-depth, the source-output invariant in cue-engine.ts is clearly documented with a regression test, and adversarial test cases cover the key edge cases. The UI layout and CSS changes are additive and well-isolated. The only finding is a cosmetic log-message inconsistency (sources.length vs resolvedSourceIds.size in two timeout log messages) with zero behavioral impact. src/main/cue/cue-fan-in-tracker.ts — two timeout log messages reference the raw sources.length rather than resolvedSourceIds.size; no functional impact. Important Files Changed
Sequence DiagramsequenceDiagram
participant P as Process Manager
participant DL as data-listener
participant EL as exit-listener
participant GC as Group Chat Domain
participant CE as Cue Engine
participant R as Renderer
P->>DL: data(sessionId, chunk)
alt sessionId starts with group-chat-
DL->>DL: moderatorMatch?
alt moderator match
DL->>GC: appendToGroupChatBuffer()
DL-->>P: return (contained)
else participant match
DL->>GC: appendToGroupChatBuffer()
DL-->>P: return (contained)
else unrecognized group-chat shape (NEW GUARD)
DL-->>P: return (DROP — prevents cross-domain leak)
end
else regular session
DL->>R: safeSend(process:data)
end
P->>EL: exit(sessionId, code)
alt sessionId starts with group-chat-
EL->>EL: moderatorMatch?
alt moderator match
EL->>GC: routeModeratorResponse()
EL-->>P: return (contained)
else participant match
EL->>GC: routeAgentResponse()
EL-->>P: return (contained)
else unrecognized group-chat shape (NEW GUARD)
EL-->>P: return (DROP — blocks spurious Cue triggers)
end
else regular session
EL->>R: safeSend(process:exit)
EL->>CE: notifyAgentCompleted(status, exitCode only)
end
|
Collaborator
|
Thanks for this contribution, @reachrazamair! Really solid work across all four commits. The leak fix is particularly well-done — UUID-anchored regexes to eliminate the greedy-backtrack ambiguity, defense-in-depth containment guards in both listeners, the documented source-output invariant with a regression test, and the fan-in dedupe fix. The adversarial test cases are a nice touch. The themed scrollbar system, pipeline editor layout fixes, and GeneralTab cleanup are all clean and well-isolated. One minor note flagged by Greptile (P2, no behavioral impact): in LGTM — approving. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/components/Settings/tabs/GeneralTab.tsx (1)
162-165:⚠️ Potential issue | 🟠 MajorReport sync-load failures to Sentry (not console only).
This catch handles UI fallback, but it currently only logs to console, so production failures won’t be captured with context.
Proposed fix
import { useState, useEffect, useCallback } from 'react'; +import { captureException } from '../../../utils/sentry'; ... .catch((err) => { - console.error('Failed to load sync settings:', err); + captureException(err instanceof Error ? err : new Error(String(err)), { + extra: { context: 'GeneralTab: load sync settings' }, + }); setSyncError('Failed to load storage settings'); });As per coding guidelines:
src/**/*.{ts,tsx}— “Do not silently swallow errors… Use Sentry utilities (captureException,captureMessage) fromsrc/utils/sentry.tsfor explicit error reporting with context.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Settings/tabs/GeneralTab.tsx` around lines 162 - 165, The catch block that currently console.errors in the sync settings load (the .catch(...) near setSyncError in GeneralTab.tsx) must report the error to Sentry: import the Sentry helper (captureException) from src/utils/sentry.ts and call captureException(err, { extra: { context: 'Failed to load sync/settings in GeneralTab' } }) (and optionally captureMessage with a short string) before or after calling setSyncError('Failed to load storage settings'), so production failures are sent to Sentry with context instead of only being logged to console.src/renderer/hooks/ui/useThemeStyles.ts (1)
136-139:⚠️ Potential issue | 🟠 MajorGuard non-Element scroll targets before
classListaccess.On Line 137,
e.targetis force-cast toElement; if the target is not anElement,classListaccess can throw and break scrollbar-state updates.Proposed fix
- const handleScroll = (e: Event) => { - const target = e.target as Element; - if (!target.classList.contains('scrollbar-thin')) return; + const handleScroll = (e: Event) => { + const target = e.target; + if (!(target instanceof Element) || !target.classList.contains('scrollbar-thin')) { + return; + } // Batch updates via requestAnimationFrame to avoid blocking scroll pendingUpdates.add(target);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/ui/useThemeStyles.ts` around lines 136 - 139, The handler handleScroll currently force-casts e.target to Element and then accesses classList, which can throw if the target isn't an Element; update handleScroll to first check the runtime type (e.g., if (!(e.target instanceof Element)) return) before accessing classList, then proceed with the existing classname check and logic so non-Element events are safely ignored.
🧹 Nitpick comments (2)
src/main/process-listeners/exit-listener.ts (1)
463-477: Log this containment drop before returning.This guard is load-bearing, but right now a malformed
group-chat-*exit just disappears. If the parser or regexes regress again, production users will have no breadcrumb for why those exits vanished even thoughloggeris already available in this file.Suggested change
if (isGroupChatSession) { + logger.warn( + '[GroupChat] Unrecognized group-chat sessionId shape on exit; dropping to prevent cross-domain leak', + 'ProcessListener', + { sessionId, exitCode: code } + ); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-listeners/exit-listener.ts` around lines 463 - 477, The guard using isGroupChatSession currently returns silently; add a log statement using the existing logger immediately before the return so malformed "group-chat-*" exits are recorded (e.g., logger.warn or logger.info with context like the sessionId and a message about containment drop) in the function in exit-listener.ts where isGroupChatSession is checked; ensure the log is concise and preserves the early-return behavior.src/main/cue/cue-fan-in-tracker.ts (1)
94-103: Use the deduped source count in timeout logs too.The tracker now waits on
resolvedSourceIds, but these timeout messages still interpolatesources.length. With a config like['Agent A', 'agent-a', 'Agent B'], a timeout will log1/3even though only two unique sources exist, which makes the new dedupe behavior look broken while debugging.Suggested change
const resolvedSourceIds = resolveSourcesToIds(sources); + const expectedSourceCount = resolvedSourceIds.size; const timedOutSources: string[] = []; for (const resolvedId of resolvedSourceIds) { if (!completedIds.has(resolvedId)) { timedOutSources.push(resolvedId); } @@ deps.onLog( 'cue', - `[CUE] Fan-in "${sub.name}" timed out (continue mode) — firing with ${completedNames.length}/${sources.length} sources` + `[CUE] Fan-in "${sub.name}" timed out (continue mode) — firing with ${completedNames.length}/${expectedSourceCount} sources` ); @@ deps.onLog( 'cue', - `[CUE] Fan-in "${sub.name}" timed out (break mode) — ${completedNames.length}/${sources.length} completed, waiting for: ${timedOutSources.join(', ')}` + `[CUE] Fan-in "${sub.name}" timed out (break mode) — ${completedNames.length}/${expectedSourceCount} completed, waiting for: ${timedOutSources.join(', ')}` );Also applies to: 120-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-fan-in-tracker.ts` around lines 94 - 103, Timeout logs still use sources.length instead of the deduplicated resolvedSourceIds count, causing misleading "x/y" messages; update any logging or interpolations that currently reference sources.length (notably where resolvedSourceIds is computed in resolveSourcesToIds and where timedOutSources is built) to use resolvedSourceIds.length (and if needed resolvedSourceIds.size logic) so the reported total reflects the deduped source count—apply the same replacement for the later block around the 120-136 region that also logs timeouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/cue/cue-completion-chains.test.ts`:
- Around line 211-234: The test creates a CueEngine, calls engine.start() but
never stops it which leaves background state; update the test (in
cue-completion-chains.test.ts) to call engine.stop() after notifyAgentCompleted
so the engine heartbeat/watcher is cleaned up, and add an assertion mirroring
the preceding regression check to verify event.payload.outputTruncated === false
alongside the existing expect(event.payload.sourceOutput).toBe(''); locate the
test by the use of createMockConfig, new CueEngine(deps), engine.start(),
engine.notifyAgentCompleted('agent-a') and modify it to call engine.stop() and
assert outputTruncated is false on the event payload.
In `@src/main/group-chat/session-parser.ts`:
- Around line 39-42: The code is incorrectly stripping "-recovery" from
UUID-suffixed participant IDs; in session-parser.ts where you use
sessionId.match(REGEX_PARTICIPANT_UUID) and set participantName from
uuidMatch[2], stop removing the suffix — remove the .replace(/-recovery$/, '')
so participantName is assigned directly from uuidMatch[2]; if you need to
normalize timestamp-form IDs, handle that separately in the timestamp-parsing
branch instead of altering the UUID branch.
In `@src/renderer/index.css`:
- Around line 74-76: The scrollbar track color is hardcoded to "transparent" in
the WebKit and fallback rules, so the --scrollbar-track token set by
useThemeStyles is never used; update the track rules (e.g., the
*::-webkit-scrollbar-track selector and the corresponding non-WebKit
scrollbar-track rules around the other track blocks) to use
var(--scrollbar-track) instead of "transparent" so the theme token is applied
consistently across WebKit and Firefox fallbacks.
---
Outside diff comments:
In `@src/renderer/components/Settings/tabs/GeneralTab.tsx`:
- Around line 162-165: The catch block that currently console.errors in the sync
settings load (the .catch(...) near setSyncError in GeneralTab.tsx) must report
the error to Sentry: import the Sentry helper (captureException) from
src/utils/sentry.ts and call captureException(err, { extra: { context: 'Failed
to load sync/settings in GeneralTab' } }) (and optionally captureMessage with a
short string) before or after calling setSyncError('Failed to load storage
settings'), so production failures are sent to Sentry with context instead of
only being logged to console.
In `@src/renderer/hooks/ui/useThemeStyles.ts`:
- Around line 136-139: The handler handleScroll currently force-casts e.target
to Element and then accesses classList, which can throw if the target isn't an
Element; update handleScroll to first check the runtime type (e.g., if
(!(e.target instanceof Element)) return) before accessing classList, then
proceed with the existing classname check and logic so non-Element events are
safely ignored.
---
Nitpick comments:
In `@src/main/cue/cue-fan-in-tracker.ts`:
- Around line 94-103: Timeout logs still use sources.length instead of the
deduplicated resolvedSourceIds count, causing misleading "x/y" messages; update
any logging or interpolations that currently reference sources.length (notably
where resolvedSourceIds is computed in resolveSourcesToIds and where
timedOutSources is built) to use resolvedSourceIds.length (and if needed
resolvedSourceIds.size logic) so the reported total reflects the deduped source
count—apply the same replacement for the later block around the 120-136 region
that also logs timeouts.
In `@src/main/process-listeners/exit-listener.ts`:
- Around line 463-477: The guard using isGroupChatSession currently returns
silently; add a log statement using the existing logger immediately before the
return so malformed "group-chat-*" exits are recorded (e.g., logger.warn or
logger.info with context like the sessionId and a message about containment
drop) in the function in exit-listener.ts where isGroupChatSession is checked;
ensure the log is concise and preserves the early-return behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a61945ae-16d2-45a4-b067-4f90e228be6f
📒 Files selected for processing (21)
src/__tests__/main/constants.test.tssrc/__tests__/main/cue/cue-completion-chains.test.tssrc/__tests__/main/group-chat/session-parser.test.tssrc/__tests__/main/performance-optimizations.test.tssrc/__tests__/renderer/components/ScrollArea.test.tsxsrc/__tests__/renderer/hooks/useThemeStyles.test.tssrc/main/constants.tssrc/main/cue/cue-engine.tssrc/main/cue/cue-fan-in-tracker.tssrc/main/group-chat/session-parser.tssrc/main/process-listeners/__tests__/data-listener.test.tssrc/main/process-listeners/__tests__/exit-listener.test.tssrc/main/process-listeners/data-listener.tssrc/main/process-listeners/exit-listener.tssrc/renderer/components/CuePipelineEditor/panels/AgentConfigPanel.tsxsrc/renderer/components/CuePipelineEditor/panels/EdgePromptRow.tsxsrc/renderer/components/CuePipelineEditor/panels/NodeConfigPanel.tsxsrc/renderer/components/ScrollArea.tsxsrc/renderer/components/Settings/tabs/GeneralTab.tsxsrc/renderer/hooks/ui/useThemeStyles.tssrc/renderer/index.css
Seven independent review findings across the four prior commits, all verified against current code before fixing. Inline: - cue-completion-chains.test: the "produces empty sourceOutput when completionData is omitted" regression test was leaking engine state by never calling engine.stop(), and only checked sourceOutput without also pinning outputTruncated. Both fixed. - session-parser: stop stripping "-recovery" in the UUID branch. Production never combines UUID suffix with -recovery (recovery sessions are always minted with timestamp suffix in group-chat-router.respawnParticipantWithRecovery), so the strip was dead code that would silently truncate a legitimate participant name ending in "-recovery". The timestamp branch still handles recovery correctly. Test fixture updated to assert the corrected behavior. - index.css: --scrollbar-track was injected by useThemeStyles but never consumed (track was hardcoded to transparent in three places). Wired it through *::-webkit-scrollbar-track, *::-webkit-scrollbar-corner, the .scrollbar-thin track, and the Firefox scrollbar-color fallback, with a `transparent` default so initial paint is preserved. Outside diff: - GeneralTab: the sync-settings load .catch() only console.error'd. Now also reports to Sentry via captureException with a context tag per the project's error-handling policy in CLAUDE.md. - useThemeStyles handleScroll: e.target was force-cast to Element, which would crash on Document/Window scroll events. Added an `instanceof Element` guard before classList access. Nits: - cue-fan-in-tracker: timeout logs (continue + break modes) referenced raw sources.length instead of the deduped resolvedSourceIds.size. With duplicate references in YAML this would print misleading "1/2 completed" when fan-in is actually waiting for 0 more. Hoisted totalSources once and used it in both log strings. - exit-listener: the group-chat containment guard returned silently; added a logger.warn so production drops surface in logs with sessionId + exitCode context. Validated: - npm run lint (3x tsc) clean - npm run lint:eslint clean - 204 tests across the 8 impacted suites pass
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/group-chat/session-parser.test.ts`:
- Around line 15-17: The comment saying "Real UUID v4s for test fixtures" is
inaccurate because GC_ID_2 is a v1 UUID; update the test to either replace
GC_ID_2 with a true v4 value or change the comment to say "Real UUIDs for test
fixtures" so it no longer asserts v4; edit the constants GC_ID and GC_ID_2 (or
the surrounding comment) in the test file to keep tests correct and the comment
truthful.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8fa12a22-fb12-43dc-b975-037147d77299
📒 Files selected for processing (8)
src/__tests__/main/cue/cue-completion-chains.test.tssrc/__tests__/main/group-chat/session-parser.test.tssrc/main/cue/cue-fan-in-tracker.tssrc/main/group-chat/session-parser.tssrc/main/process-listeners/exit-listener.tssrc/renderer/components/Settings/tabs/GeneralTab.tsxsrc/renderer/hooks/ui/useThemeStyles.tssrc/renderer/index.css
✅ Files skipped from review due to trivial changes (1)
- src/main/process-listeners/exit-listener.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/tests/main/cue/cue-completion-chains.test.ts
- src/renderer/components/Settings/tabs/GeneralTab.tsx
- src/renderer/index.css
- src/main/group-chat/session-parser.ts
GC_ID_2 (6ba7b810-9dad-11d1-80b4-00c04fd430c8) is the RFC 4122 namespace UUID — a v1, not v4. The regex in constants.ts accepts the canonical 8-4-4-4-12 hex shape regardless of version, so the test still works correctly, but the comment was inaccurate. Updated to "Real UUIDs" and documented that the regex is version-agnostic. No fixture or behavior changes.
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
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
Four-commit polish pass on the Cue feature and the surrounding theme/UI surface:
rgba(255,255,255,0.15)thumb (invisible on light themes) with a unified, theme-driven scrollbar system that automatically picks up the active theme. Includes a new<ScrollArea>wrapper component for new call sites.npm run lint,npm run lint:eslint, and the relevant test suites are all clean — see the test plan below.1.
fix(cue): prevent group-chat output from leaking into Cue pipeline runs—2664050dInvestigation started from a user report that group-chat output was appearing inside chained Cue pipeline runs. The audit found three independent paths that could either cause or amplify the leak; this commit closes all of them.
Root cause: greedy regex on group-chat session IDs
REGEX_PARTICIPANT_TIMESTAMPwas/^group-chat-(.+)-participant-(.+)-(\d{13,})$/— both captures greedy.groupChatIdis always auuidv4()(seegroup-chat-storage.ts:createGroupChat), but participant names are user-supplied. A participant whose name contains the literal substring-participant-would cause the regex to backtrack to the last occurrence and parse out the wrong(groupChatId, participantName)pair. Output chunks buffered inoutput-buffer.tswould then get flushed against the wrong owner on process exit.Fix: anchor
groupChatIdon the canonical UUID format, make the participant-name capture lazy, add a strict^group-chat-prefix guard in the parser, and strip-recoveryfrom UUID-suffixed IDs (previously only handled in the timestamp branch).Containment guards in data-listener and exit-listener
The bigger problem: if a
group-chat--prefixedsessionIdfailed to match the moderator branch and failed to parse as a participant, bothdata-listenerandexit-listenerwould fall through to the regularsafeSend('process:data' | 'process:exit', ...)path, the web broadcast path, andcueEngine.notifyAgentCompleted. Tightening the regex would have exposed this for any legacy ID that no longer parses.Fix: added explicit "if it starts with
GROUP_CHAT_PREFIX, drop it" guards in both listeners, after the moderator/participant branches but before any forwarding. Group-chat bytes are now structurally contained to the group-chat domain.Source-output invariant in cue-engine
notifyAgentCompletedbuilds thesourceOutputpayload field that becomes{{CUE_SOURCE_OUTPUT}}in downstream prompts. Audit confirmed it's built exclusively fromcompletionData.stdoutwith no fallback to any session-store or buffer. The exit-listener path passes{ status, exitCode }only — nostdout— so the resultingsourceOutputis''.This invariant is load-bearing: any future fallback to a session output store, group-chat buffer, or live process buffer would re-introduce the leak. The commit adds an explicit comment block in
cue-engine.tsdocumenting this and a regression test (produces empty sourceOutput when completionData has no stdout) that will fail loudly if anyone adds such a fallback.Fan-in source dedupe
While auditing
cue-fan-in-tracker, found a latent bug where YAML listing the same session by both name and ID (e.g.source_session: ['Agent A', 'agent-a']) would causesources.lengthto overcount and the fan-in to hang forever waiting for a phantom second completion. The trackerMapis keyed by sessionId so the count would never reach the target.Fix: added a
resolveSourcesToIds()helper that resolves user-authored source names/IDs to a dedupedSet<string>of canonical session IDs. BothhandleCompletionandhandleFanInTimeoutnow use this set as the source of truth instead of the rawsourcesarray.Files
src/main/constants.ts— UUID-anchored regexessrc/main/group-chat/session-parser.ts— strict prefix guard, recovery suffix handlingsrc/main/process-listeners/data-listener.ts— domain containment guardsrc/main/process-listeners/exit-listener.ts— domain containment guardsrc/main/cue/cue-engine.ts— source-output invariant comment + raw stdout extractionsrc/main/cue/cue-fan-in-tracker.ts—resolveSourcesToIds, dedupe in completion + timeout pathssrc/__tests__/main/constants.test.ts— fixtures updated to UUIDs + adversarial casessrc/__tests__/main/group-chat/session-parser.test.ts— adversarial inputs (-participant-in name, UUID-shaped names, 13-digit numbers in name, recovery suffix)src/__tests__/main/cue/cue-completion-chains.test.ts— empty-stdout regression + fan-in dedupe regressionsrc/__tests__/main/performance-optimizations.test.ts— fixtures updated to UUIDssrc/main/process-listeners/__tests__/data-listener.test.ts— drops unrecognized group-chat sessionsrc/main/process-listeners/__tests__/exit-listener.test.ts— drops unrecognized group-chat session, asserts only{status, exitCode}passed to Cue2.
chore(settings): remove dead Stats/WakaTime code from GeneralTab—946c6e00Pre-existing eslint warnings (22 in total) reported by
npm run lint:eslint. Commit5a79180f("refactor: move Usage & Stats settings from General tab to Encore Features tab") moved the UI but left the destructured props, state, callbacks, and auseEffectbehind inGeneralTab.tsx.Removed
Trash2icon import (unused)useSettings()fields:statsCollectionEnabled+ setter,defaultStatsTimeRange+ setter,setWakatimeEnabled,wakatimeApiKey,wakatimeDetailedTracking+ setteruseStatedeclarations:statsDbSize,statsEarliestDate,statsClearing,statsClearResult,wakatimeCliStatus,wakatimeKeyValid,wakatimeKeyValidatinghandleWakatimeApiKeyChangecallbackuseEffect(~50 lines, no consumer)// Load sync settings and stats data when modal openscommentFiles
src/renderer/components/Settings/tabs/GeneralTab.tsx(−90 lines)npm run lint:eslintnow exits clean. All 84 GeneralTab component tests still pass.3.
fix(cue-pipeline): output box fills column and multi-trigger rows scroll—a833af66Three layout bugs in the agent config panel's collapsed view, all reported in the same iteration:
Bug 3.1 — Output prompt box collapses to its content min in multi-trigger mode
In multi-trigger mode the right column's
flex: 0made the output textarea sit at itsminHeight: 80with empty space below it, while the input column on the left took all the remaining width. Fix: output column always usesflex: 1and the textarea always usesflex: 1, minHeight: 88(or 72 with fan-in). Output now fills its column whether single- or multi-trigger.Bug 3.2 — Multi-trigger left rail had no scroll, rows visually overlapped
Each
EdgePromptRowusedflex: 1plustextarea minHeight: 68. With 3+ triggers in a ~280px collapsed content area, the parent column couldn't satisfy each row's intrinsic min, so flex layout shrank rows below their content size — the result was the bottom row's title rendering on top of the textarea above it. The user's wording ("no two input prompt box titles must overlap to the stuff on their y axis") matches exactly.Fix: each row now uses
flexShrink: 0(intrinsic content height) instead offlex: 1in collapsed mode. Title spans get explicitflexShrink: 0+marginBottom: 4so flex layout can never collapse them. The parent column gets its ownoverflowY: autowithpaddingRight: 6reserving the scrollbar gutter, so additional rows scroll instead of squeezing each other.Bug 3.3 — Single-trigger collapsed wasted vertical space
Input/output textareas were gated
expanded ? flex:1 : minHeight:80, so collapsed mode never filled the column even when the panel was tall. Fix: both now always useflex: 1, minHeight: 88regardless of expanded state.Bonus —
collapsedHeightformulaReplaced the hardcoded ladder (320 / 360 / 420) with a formula that accounts for
triggerEdgeCount:Tighter for the common single-trigger case, larger when multi-trigger or fan-in needs the room. Capped so the panel can never eat the entire canvas.
Bonus — single source of scroll per axis
Removed the redundant outer
overflowY: autoonAgentConfigPaneland theoverflow: autoon the input/output split row.NodeConfigPanel's content wrapper is nowoverflow: hiddenfor agent nodes — the inner panels manage their own scroll regions (left rail in multi-trigger mode, nothing for the rest). Eliminates triple-nested scroll regions.Files
src/renderer/components/CuePipelineEditor/panels/NodeConfigPanel.tsxsrc/renderer/components/CuePipelineEditor/panels/AgentConfigPanel.tsxsrc/renderer/components/CuePipelineEditor/panels/EdgePromptRow.tsx4.
feat(theme): app-wide themed scrollbars driven by CSS variables—14cbb62bThe previous
.scrollbar-thinrule usedrgba(255, 255, 255, 0.15)for the thumb — works on dark themes, invisible on light themes (GitHub, One Light, Catppuccin Latte, Ayu Light). This commit replaces the hardcoded color with a unified, theme-driven system that automatically picks up the active theme on every scrollable element in the app.Architecture (3 layers)
Layer 1 — Theme bridge (
useThemeStyles.ts)The hook is the single bridge between the React theme system and CSS. It now injects six CSS variables on
:root:--accent-colortheme.accent--highlight-colortheme.accent--scrollbar-thumbtheme.border--scrollbar-thumb-hovertheme.textDim--scrollbar-thumb-activetheme.accent--scrollbar-tracktheme.bgActivityThe
ThemeColorsinterface gainedborder,textDim,bgActivity.App.tsxalready passes the fulltheme.colorspalette so callers are unaffected — the structural type just sees more fields now.Layer 2 — Global CSS (
index.css)*::-webkit-scrollbar— themes EVERY scrollable element in the app, including the ~200 inlineoverflow: autocontainers that don't carry a class. 10px width, 6px border-radius, 2px transparent inset border viabackground-clip: padding-boxfor a "floated" thumb look.scrollbar-coloron*— cross-browser fallback for non-WebKit Electron variants..scrollbar-thin— slim 6px variant for tight UI areas (~50 components: sidebars, autocomplete dropdowns, command palettes, the agent config panel's left rail). Rewritten to use the same CSS variables..scrollbar-thin.scrolling/.fading— stateful brighten-on-scroll → fade-after-1s animation, kept intact and managed by the existing JS inuseThemeStyles. Only fires for elements with.scrollbar-thinto avoid wiring scroll listeners to every element in the app.var()has a sensible fallback so initial paint (beforeuseThemeStylesruns) renders correctly.Layer 3 —
<ScrollArea>wrapper component (new)Thin React wrapper at
src/renderer/components/ScrollArea.tsxfor documenting intent at new call sites. Existing.scrollbar-thinusages keep working unchanged — this is purely additive, not a mass migration.default(10px, matches global app default) |thin(6px, with fade-on-idle)both(default) |x|y|nonehideScrollbaropt-out (applies.no-scrollbar)forwardRefid,role,aria-*,onScroll, etc.)Why hybrid (CSS variables + global rules + opt-in component)
Pure-component approach would require touching ~50 components and ~200 inline
overflow: autocontainers. CSS-variable + global-rule approach themes them all with zero per-component changes, while the<ScrollArea>component gives new code a clean call-site primitive when intent matters.Files
src/renderer/hooks/ui/useThemeStyles.ts— expanded CSS variable injection + interfacesrc/renderer/index.css— global themed scrollbar rules (3 layers)src/renderer/components/ScrollArea.tsx— new wrapper componentsrc/__tests__/renderer/hooks/useThemeStyles.test.ts— new (6 tests)src/__tests__/renderer/components/ScrollArea.test.tsx— new (13 tests)Test plan
npm run lint(3× tsc) — clean, exit 0npm run lint:eslint— clean, 0 warnings, exit 0npm run testfiltered to relevant files — all passing:src/__tests__/main/cue,src/__tests__/main/group-chat, andsrc/main/process-listeners/__tests__src/__tests__/renderer/components/CuePipelineEditorsrc/__tests__/renderer/components/Settings/tabs/GeneralTab.test.tsx(cleanup)src/__tests__/renderer/hooks/useThemeStyles.test.tsandsrc/__tests__/renderer/components/ScrollArea.test.tsx(themed scrollbars)-participant-no longer causes group-chat output to leak.Notes for reviewers
group-chat-<weird>would have leaked through to the regular renderer channel. The defense-in-depth guards indata-listenerandexit-listenerare intentional belt-and-suspenders.ScrollAreacomponent is opt-in for new code only — please don't mass-migrate existing.scrollbar-thinusages in this PR. Reach for it when adding new scrollable regions or when refactoring areas touched for other reasons.Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests