Conversation
WalkthroughAdds a Spinner component and useSpinner hook, introduces animated status indicators and a status legend in CLI dashboard/panels, adds responsive width/padding calculations using Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Panels/Dashboard
participant Effect as useEffect
participant SpinnerHook as useSpinner
participant Renderer as InkRenderer
UI->>Effect: mount or agents state change
alt any agent RUNNING
Effect->>SpinnerHook: start interval (e.g. 80ms)
loop every interval
SpinnerHook->>Renderer: emit next frame
Renderer->>Renderer: render spinner (green) + status legend
end
else no running agents
Effect->>SpinnerHook: clear interval
Renderer->>Renderer: render static emoji (idle/error/stopped)
end
Renderer->>Renderer: compute layout using string-width and apply padding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/cli/src/commands/agent.tsx (2)
434-434: Fix TypeScript any type for workspace state.The
workspacestate is typed asany, which bypasses type safety. This should be properly typed asWorkspace | null.Apply this diff to fix the type:
- const [workspace, setWorkspace] = React.useState<any>(null); + const [workspace, setWorkspace] = React.useState<Workspace | null>(null);
479-479: Fix TypeScript any type for startAgents parameter.The
wsparameter instartAgentsis implicitly typed asany. This should be explicitly typed asWorkspace.Apply this diff to fix the type:
- const startAgents = async (ws: any, agents: AgentType[]) => { + const startAgents = async (ws: Workspace, agents: AgentType[]) => {
🧹 Nitpick comments (1)
apps/cli/src/commands/panels.tsx (1)
347-354: Consider edge case: empty agent list.The width calculation logic at lines 347-354 assumes
filteredAgentshas items. While the parent conditional at line 316 handles the empty case, the complex padding calculation could benefit from a defensive check to ensureagentsPanelInnerWidthis always positive.Consider adding a guard:
const targetWidth = Math.max(agentsPanelInnerWidth, contentWidth); +const safeTargetWidth = Math.max(0, targetWidth); -const paddingNeeded = Math.max(0, targetWidth - contentWidth); +const paddingNeeded = Math.max(0, safeTargetWidth - contentWidth);This is defensive coding to handle potential edge cases where panel width calculations might produce unexpected values on extremely narrow terminals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/cli/package.json(1 hunks)apps/cli/src/commands/agent.tsx(1 hunks)apps/cli/src/commands/dashboard.tsx(6 hunks)apps/cli/src/commands/init.tsx(1 hunks)apps/cli/src/commands/panels.tsx(10 hunks)apps/cli/src/lib/launch/run.ts(6 hunks)apps/cli/src/lib/orchestrators/__tests__/process-orchestrator.test.ts(1 hunks)apps/cli/src/lib/orchestrators/process-orchestrator.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cli/src/commands/dashboard.tsx (1)
apps/cli/src/types/process.ts (1)
Process(15-29)
🪛 GitHub Actions: CI
apps/cli/src/commands/agent.tsx
[error] 434-434: lint/suspicious/noExplicitAny: Unexpected any. Specify a different type for 'workspace'.
[error] 479-479: lint/suspicious/noExplicitAny: Unexpected any. Type of 'ws' parameter for startAgents should be specified.
🔇 Additional comments (11)
apps/cli/src/lib/orchestrators/__tests__/process-orchestrator.test.ts (1)
250-253: LGTM - Formatting improvement.The multi-line formatting improves readability and aligns with the broader formatting refactors in this PR.
apps/cli/src/commands/agent.tsx (1)
646-647: LGTM - Minor formatting improvement.The added space after "Success:" improves readability.
apps/cli/src/lib/launch/run.ts (1)
109-111: LGTM - Formatting improvements throughout.All changes are formatting-only refactors that improve readability by breaking complex expressions across multiple lines. No functional changes.
Also applies to: 156-160, 218-220, 244-247, 311-315, 323-325, 380-384
apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
39-41: LGTM - Formatting improvements throughout.All changes are formatting-only refactors that improve readability by breaking complex expressions across multiple lines. No functional changes.
Also applies to: 86-89, 256-258, 297-299
apps/cli/src/commands/init.tsx (1)
187-192: LGTM - Formatting improvement.The multi-line formatting improves readability and aligns with the broader formatting refactors in this PR.
apps/cli/src/commands/dashboard.tsx (3)
37-37: Well-implemented spinner animation.The spinner animation is properly implemented with good performance considerations:
- Only runs when there are running agents
- 80ms interval provides smooth animation
- Proper cleanup in useEffect return
- Dependency on
spinnerFrames.lengthis safe since it's a constant arrayAlso applies to: 44-110
275-290: Excellent status indicator improvements.The replacement of text badges with emoji/color indicators plus the status legend significantly improves the UI:
- More visual and intuitive status display
- Animated spinner for running agents provides dynamic feedback
- Legend helps users understand the indicators
- Color coding (green/yellow/red/gray) follows common UX patterns
Also applies to: 357-374
496-513: LGTM - Improved agent row rendering.The updated rendering logic properly handles visual emphasis for both selected and running agents, with appropriate color coding. The sessionName truncation prevents layout issues on narrow terminals.
Also applies to: 520-520
apps/cli/src/commands/panels.tsx (3)
3-3: LGTM - Proper dependencies and spinner implementation.The
string-widthimport enables precise width calculations for terminal rendering. The spinner animation follows the same well-implemented pattern as indashboard.tsx- only runs when agents are running, with proper cleanup.Also applies to: 45-56, 96-110
319-384: Complex but correct width calculation logic.The agent entry rendering with dynamic width calculations is well-implemented:
stringWidthproperly measures content width including Unicode characterstargetWidthusesMath.maxto prevent truncation by taking the larger of panel inner width or actual content width- Padding is only applied when needed
- Background color for selected items improves UX
The logic correctly handles responsive layouts and prevents text overflow.
436-481: LGTM - Improved controls footer.The refactored controls section is more compact and includes a helpful status legend that aligns with the dashboard UI. The responsive behavior for small terminals (hiding the Details panel shortcut) is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/cli/src/commands/panels.tsx (5)
45-56: Responsive widths and shared spinner frame are reasonable; consider extreme‑small terminal behaviorThe responsive calculations for
workspacePanelWidth/agentsPanelWidthandagentsPanelInnerWidth, combined with a singlespinnerFrameviauseSpinner(), are a nice improvement.One edge case worth noting: on very narrow terminals (e.g. widths in the low 30s), the hard minimums (
Math.max(20, …)for both panels) can exceed the availablecontentWidth, causing horizontal overflow. This will mostly just truncate/wrap, but if you care about extremely small environments, you might want to enforce:
workspacePanelWidth + agentsPanelWidth + gap <= contentWidthand clamp one or both widths accordingly.Otherwise this looks solid.
238-256: Workspace status spinner logic is consistent; check semantics of “running”Using
hasRunningto drive a green spinner per workspace and falling back to a gray circle is clear and visually consistent with the agents panel.Just double‑check that treating
!a.endedAtas “running” for workspaces (even whenstatusisn’tRUNNING) matches your domain semantics; if anIDLE-but-not-yet-ended process should count as non‑running, you may want to tighten the predicate tostatus === RUNNINGonly.
343-368: Selected-row highlighting and padding behavior look correctWrapping the entire row in a
<Text backgroundColor={backgroundColor}>and nesting the individual<Text>spans (arrow, spinner, agent type, time, padding) is a good way to get a contiguous highlight for the selected row. The computedpaddingNeededis applied as literal spaces, so the rendered width matches the earliertargetWidthcomputation.Only minor stylistic note:
color={textColor ?? "yellow"}for the arrow means the (invisible) unselected arrow still usesyellow, which is harmless but slightly surprising. If you want to be precise, you could usecolor={textColor}and rely on default color when not selected.Functionally this is fine as‑is.
424-467: Controls footer and status legend are clearer; consider matching legend icon to spinnerThe multi‑row footer with explicit key hints and a status legend improves discoverability a lot, and the responsiveness around showing the
[3] Detailshint only on wider terminals is thoughtful.One micro‑consistency point: running agents/workspaces show a spinner glyph, but the legend uses a solid green
●for “Running”. That’s still understandable, but if you want perfect visual alignment you could either:
- Show the spinner glyph in the legend as well, or
- Switch the running indicator in the lists to a green
●(or prepend●+spinner) so legend and rows match.Not a correctness issue, just a UX polish opportunity.
301-341: Consider renamingcontentWidthtorowContentWidthfor clarity, and test spinner glyph alignment across target terminalsThe naming suggestion is valid: renaming the per-row
contentWidthvariable (e.g., torowContentWidth) will eliminate confusion with the module-levelcontentWidthand improve readability.On
string-widthcaveats: combining marks have zero advance width, certain character sequences may render differently than measured, and Braille spinner glyphs can misalign due to font/terminal variation despite being single-cell Unicode. Your code usesspinnerFrameand other Unicode glyphs (circles, arrows, X marks) that could be affected. Recommended: normalize text (NFC) before measuring and test on target terminals to verify glyph alignment. If users report misalignment across different terminal environments, consider adding guards for combining/enclosing sequences or measuring directly with the actual terminal renderer.apps/cli/src/components/Spinner.tsx (1)
4-36: Strengthen hook robustness by including fullframesarray in dependency listThe hook works fine with static frames, but React's exhaustive-deps rule recommends depending on the actual values your effect uses. If you only depend on
frames.lengthbut the frame content changes, the effect will not rerun and will use the old frames.The suggested refactor is sound:
- Depend on the full
framesarray (or a stable memoized version ifframesis recreated frequently)- Reset
indexto 0 whenframeschanges to avoid temporary out-of-range access- React.useEffect(() => { + React.useEffect(() => { + setIndex(0); if (frames.length === 0) { return; } const timer = setInterval(() => { setIndex((i) => (i + 1) % frames.length); }, interval); return () => clearInterval(timer); - }, [frames.length, interval]); + }, [frames, interval]);This keeps behavior identical for current usage while making the hook robust to dynamic
frameschanges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/cli/src/commands/dashboard.tsx(5 hunks)apps/cli/src/commands/panels.tsx(9 hunks)apps/cli/src/components/Spinner.tsx(1 hunks)apps/desktop/src/lib/trpc/routers/projects/projects.ts(1 hunks)apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/lib/trpc/routers/projects/projects.ts
- apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cli/src/commands/dashboard.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cli/src/commands/panels.tsx (1)
apps/cli/src/components/Spinner.tsx (1)
useSpinner(17-36)
🔇 Additional comments (5)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
2-2: LGTM! Clean import deduplication.Removing the duplicate import improves code clarity without any functional changes.
apps/cli/src/components/Spinner.tsx (1)
44-56: Spinner component usage looks good and is Ink-friendlyThe
Spinnerwrapper arounduseSpinneris straightforward and integrates nicely with Ink’s<Text>(color pass-through, null-render on emptyframes, sensible defaults). No issues from a correctness standpoint.apps/cli/src/commands/panels.tsx (3)
292-294: Agents panel width and flex behavior make sense for small vs large terminalsBinding
width={agentsPanelWidth}andflexGrow={isSmallTerminal ? 1 : 0}gives agents more room on narrow terminals while keeping a fixed-ish layout when there’s space. That’s a good trade‑off for readability. No changes needed.
395-409: Details panel formatting is clear and compactInlining the “Agent:” label with a bolded agent type and formatting
endedAtviatoLocaleString()reads well and uses space efficiently. This also plays nicely with hiding the whole panel on small terminals.No issues; behavior matches the rest of the UI.
3-14: Imports and type usage are clean and tree-shake friendlySwitching to
string-widthanduseSpinner, plus usingtype-only imports forAgent/Process, keeps module boundaries clear and aids tree shaking. Thestring-widthpackage correctly handles full-width characters, emoji sequences, combining marks, and ANSI codes—all essential for robust terminal column width calculations in CLI applications. No issues.
Resolved conflict in workspaces.ts by keeping the simpleGit import needed for the canDelete procedure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.