feat: ACP health check, diagnostics page and visual polish#41
Open
feat: ACP health check, diagnostics page and visual polish#41
Conversation
- Add `ConnectionStatusEvent` and `ConnectionConfig` to `types.rs` - Implement `heartbeat_task` to detect dead process every 15s - Add `is_alive()` and `emit_status()` helpers to `AcpConnection` - Refactor `AcpState` to hold `connections` and `configs` separately - Add `acp_check_health` command for on-demand connection health check - Emit `acp:connection-status` event on connect, disconnect and process exit Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Define `AcpConnectionStatus` type (idle | connecting | connected | disconnected | reconnecting) - Listen to `acp:connection-status` events emitted by the Rust backend - Call `acp_check_health` when window regains visibility - Expose `connectionStatus` from `useAcpConnection` hook - Add `AcpConnectionStatusEvent` interface to `types/acp.ts` Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add `run_diagnostics` Rust command to check platform, binary, version, GH_TOKEN and ACP connectivity - Test ACP connection in isolation with a 10s timeout and stderr capture - Add `DiagnosticsSettings` component with visual report and copy-to-clipboard button - Integrate 'Diagnostics' tab into the settings window (`SettingsApp`) - Add pt-BR and en translations for the diagnostics section - Update home tagline and chat placeholder copy in pt-BR Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rkdownViewer - Remove dependency on `ResizablePanelGroup` / `ImperativePanelHandle` - Replace lateral panels with absolute overlay drawers controlled by boolean state - Add floating toggle buttons to open outline (left) and review (right) - Simplify collapse/expand logic by removing imperative refs - Preserve auto-open behaviour when entering the 'reviewing' phase Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…onents - Increase padding and minimum height of workspace and session cards (120px) - Use `line-clamp-2` on card titles for improved readability - Reduce grid column count and increase gap between cards - Add drag handle to `TerminalInput` for resizable height - Change send shortcut from Cmd/Ctrl+Enter to Enter (Shift+Enter inserts newline) - Show reconnect button only when connection is inactive (`disabled` prop) - Display `acp_session_id` in the active session header - Widen new session dialog and increase textarea to 20 rows - Add `type="button"` to `MicButton` to prevent accidental form submission - Reduce plan panel `minSize` from 30 to 20 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors ACP state into separate Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend as Tauri
participant Child as ACP Child
User->>Frontend: click "connect" (workspace_id, binary, cwd, gh_token)
Frontend->>Backend: invoke acp_connect(...)
Backend->>Backend: create ConnectionConfig & lock `state.connections`
Backend->>Backend: spawn AcpConnection (store app_handle, workspace_id)
Backend->>Backend: start heartbeat_task(childRef, workspace_id, app_handle)
Backend->>Frontend: emit acp:connection-status ("connecting")
rect rgba(100,150,200,0.5)
loop periodic heartbeat
Backend->>Child: try_wait()
alt child running
Child-->>Backend: still running
else child exited
Child-->>Backend: exited
Backend->>Backend: clear child ref & abort tasks
Backend->>Frontend: emit acp:connection-status ("disconnected")
Frontend->>Frontend: update connectionStatus
end
end
end
User->>Frontend: request health
Frontend->>Backend: invoke acp_check_health(workspace_id)
Backend->>Backend: lookup connection & call is_alive()
alt alive
Backend-->>Frontend: return "connected" and emit status
else not alive
Backend-->>Frontend: return "disconnected"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/tauri/src/components/WorkspaceCard.tsx (1)
86-98:⚠️ Potential issue | 🟡 MinorOverlapping badges when both
unresolvedCommentsandsessionCountare non-zero.Both conditional blocks render badges at the exact same position (
absolute top-1.5 right-1.5). If bothunresolvedComments > 0andsessionCount > 0, the badges will overlap visually.Consider either:
- Mutually exclusive rendering (show only one, prioritizing one over the other)
- Distinct positioning (e.g., stack vertically or offset horizontally)
- Combining into a single badge if both represent related counts
🛠️ Option 1: Mutually exclusive (show sessionCount only if no unresolvedComments)
{unresolvedComments != null && unresolvedComments > 0 && ( <span className="absolute top-1.5 right-1.5 inline-flex items-center gap-0.5 text-muted-foreground group-hover:opacity-0 transition-opacity"> <MessageSquare className="h-3 w-3" /> <span className="font-semibold text-[10px]">{unresolvedComments}</span> </span> )} - {sessionCount != null && sessionCount > 0 && ( + {sessionCount != null && sessionCount > 0 && !(unresolvedComments != null && unresolvedComments > 0) && ( <span className="absolute top-1.5 right-1.5 inline-flex items-center gap-0.5 text-muted-foreground group-hover:opacity-0 transition-opacity"> <MessageSquare className="h-3 w-3" /> <span className="font-semibold text-[10px]">{sessionCount}</span> </span> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/WorkspaceCard.tsx` around lines 86 - 98, The two badge blocks in WorkspaceCard.tsx both render at "absolute top-1.5 right-1.5" and will overlap when unresolvedComments and sessionCount are >0; make them mutually exclusive by only rendering the sessionCount badge when unresolvedComments is null or 0 (i.e., change the sessionCount conditional to something like "sessionCount != null && sessionCount > 0 && (!unresolvedComments || unresolvedComments === 0)"), keeping the existing MessageSquare and styling so the unresolvedComments badge takes priority.
🧹 Nitpick comments (5)
apps/tauri/src/components/MarkdownViewer.tsx (1)
336-336: Consider usingcn()utility for conditional class merging.Per coding guidelines, the
cn()utility fromlib/utils.tsshould be used for merging Tailwind CSS classes. This improves readability and handles edge cases in class concatenation.♻️ Proposed refactor
+import { cn } from "@/lib/utils";Then at line 336:
- <div className={`relative overflow-hidden ${isEmbedded ? "h-full" : "flex-1 min-h-0"}`}> + <div className={cn("relative overflow-hidden", isEmbedded ? "h-full" : "flex-1 min-h-0")}>As per coding guidelines: "Use the
cn()utility fromlib/utils.tsfor merging Tailwind CSS classes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/MarkdownViewer.tsx` at line 336, Replace the template literal className on the div inside the MarkdownViewer component with the cn() utility: import cn from 'lib/utils' (or existing utils import) at the top of the file and use cn('relative overflow-hidden', isEmbedded ? 'h-full' : 'flex-1 min-h-0') for the div's className to handle conditional Tailwind class merging and avoid manual string concatenation.apps/tauri/src/components/ActiveSessionView.tsx (1)
191-194: Handle clipboard write failures explicitly.On Line 192,
navigator.clipboard.writeText(...)can reject and currently fails silently.🔧 Suggested patch
- onClick={() => navigator.clipboard.writeText(session.acp_session_id ?? session.id)} + onClick={async () => { + try { + await navigator.clipboard.writeText(session.acp_session_id ?? session.id); + } catch (err) { + console.error("[ActiveSessionView] failed to copy session id:", err); + } + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/ActiveSessionView.tsx` around lines 191 - 194, The onClick handler in ActiveSessionView.tsx uses navigator.clipboard.writeText(session.acp_session_id ?? session.id) but does not handle rejections; change the onClick to call an async handler (or attach a .catch) that attempts to write (using session.acp_session_id ?? session.id), wraps the await/write in try/catch, and on error logs the error and surfaces a user-facing fallback (e.g., a toast/error message or fallback prompt) so clipboard failures are not silent; update references to session.acp_session_id and session.id accordingly.apps/tauri/src/components/TerminalInput.tsx (1)
97-97: Usecn()for conditional class merging.Line 97 uses template-string class merging; this should use
cn()for consistency.🔧 Suggested patch
+import { cn } from "@/lib/utils"; @@ - <RefreshCw className={`h-3.5 w-3.5 ${loading ? "animate-spin" : ""}`} /> + <RefreshCw className={cn("h-3.5 w-3.5", loading && "animate-spin")} />As per coding guidelines: Use the
cn()utility fromlib/utils.tsfor merging Tailwind CSS classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/TerminalInput.tsx` at line 97, The button/text render in TerminalInput (component TerminalInput in TerminalInput.tsx) currently uses a template-string to merge Tailwind classes around {t("chat.send")} — replace that template-string class merging with the cn() utility from lib/utils.ts: import cn at the top if missing, then convert the className expression to cn('base-classes', condition && 'conditional-class', other && 'other-class') so classes are merged consistently; ensure you remove the backtick/template literal and use cn(...) for all conditional class logic referencing the same element that renders t("chat.send").apps/tauri/src-tauri/src/acp/types.rs (1)
145-145: Prefer an enum over rawStringfor connection status.The codebase only uses four valid statuses: "connecting", "connected", "disconnected", and "reconnecting". Using
Stringon Line 145 allows invalid statuses to compile, creating a mismatch with the frontend union type. A serializable enum will enforce valid states at compile time without adding complexity—serde derives are already in place on the struct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src-tauri/src/acp/types.rs` at line 145, Replace the raw String status field with a serializable enum to enforce the four valid states: define an enum (e.g., ConnectionStatus) with variants Connecting, Connected, Disconnected, Reconnecting and derive serde::{Serialize, Deserialize}, then change the struct field pub status: String to pub status: ConnectionStatus (and update any places constructing or matching the old String to use the enum variants or implement From/Into if needed); ensure serde rename attributes (or rename_all) match the exact string values expected by the frontend so serialization/deserialization remains compatible.apps/tauri/src-tauri/src/acp/commands.rs (1)
262-287: Consider avoiding holding the lock across theis_alive().awaitcall.While this works because
is_alive()locks a different mutex (child), holdingconnectionslock across an await point is generally discouraged as it can increase lock contention and block other commands that need to access connections.♻️ Proposed refactor to reduce lock scope
pub async fn acp_check_health( workspace_id: String, app_handle: AppHandle, state: State<'_, AcpState>, ) -> Result<String, String> { - let connections = state.connections.lock().await; - let status = if let Some(conn) = connections.get(&workspace_id) { - if conn.is_alive().await { - conn.emit_status("connected", None); - "connected" - } else { - conn.emit_status("disconnected", None); - "disconnected" - } - } else { + let conn_opt = { + let connections = state.connections.lock().await; + connections.get(&workspace_id).cloned() + }; + let status = if let Some(conn) = conn_opt { + if conn.is_alive().await { + conn.emit_status("connected", None); + "connected" + } else { + conn.emit_status("disconnected", None); + "disconnected" + } + } else { let event = ConnectionStatusEvent { workspace_id: workspace_id.clone(), status: "disconnected".to_string(), attempt: None, }; let _ = app_handle.emit("acp:connection-status", &event); "disconnected" }; Ok(status.to_string()) }Note: This requires
AcpConnectionto implementClone. If that's not feasible, the current approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src-tauri/src/acp/commands.rs` around lines 262 - 287, The acp_check_health function currently holds the mutex on state.connections across the await call to AcpConnection::is_alive(), which can increase contention; change it to lock, lookup the AcpConnection, clone (or otherwise obtain an owned handle such as an Arc) the connection, then drop the connections lock before calling conn.is_alive().await and conn.emit_status(...), ensuring AcpConnection (or the stored value) implements Clone/Arc so you can call is_alive and emit_status without holding the connections mutex; update acp_check_health, AcpState usage, and the stored connection type accordingly and keep the existing ConnectionStatusEvent/app_handle emit fallback logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/tauri/src/components/MarkdownViewer.tsx`:
- Line 275: The onClick handler currently toggles only the local state via
setReviewOpen and can drift from the hook state review.isPanelOpen; update the
handler in the MarkdownViewer component so it calls both setReviewOpen((v)=>!v)
and review.setIsPanelOpen(!review.isPanelOpen) (or use the same toggle value) so
the local overlay state and the hook's panel state remain in sync with
review.setIsPanelOpen and review.isPanelOpen.
- Around line 343-347: The rendered HTML from comrak::markdown_to_html in the
render_markdown function is returned unsanitized and later injected via
dangerouslySetInnerHTML; update render_markdown to sanitize the HTML before
returning by running the comrak output through an HTML sanitizer (e.g.,
ammonia). Specifically, after calling comrak::markdown_to_html(...) in
render_markdown, pass the result to ammonia::clean (or configure an
ammonia::Builder to strip dangerous tags/attributes like <script>, on* handlers,
and javascript: URIs) and return the sanitized string instead of the raw comrak
output so the frontend Article component no longer receives unsafe HTML.
In `@apps/tauri/src/components/SessionCard.tsx`:
- Line 84: The span uses a template literal to merge Tailwind classes which
violates the repo rule; replace the template string with the cn() utility from
lib/utils.ts so classes are merged consistently. Locate the span in SessionCard
(the element with className={`w-2 h-2 rounded-full flex-shrink-0
${PHASE_COLORS[session.phase]}`}), import cn if not present, and change the
className to cn('w-2 h-2 rounded-full flex-shrink-0',
PHASE_COLORS[session.phase]) to preserve the same classes while using the
canonical helper.
In `@apps/tauri/src/components/settings/DiagnosticsSettings.tsx`:
- Around line 26-27: Replace the hardcoded Tailwind literal classes on the
CheckCircle2 and AlertCircle icons with theme token-based HSL CSS variable
classes; specifically, in the JSX that renders CheckCircle2 and AlertCircle,
remove "text-green-500" and "text-yellow-500" and replace them with HSL
variable-based classes (for example use text-[hsl(var(--green-9))] for the
success icon and text-[hsl(var(--yellow-9))] for the warning icon) while keeping
the existing sizing/shrink classes (h-4 w-4 shrink-0) so the icons follow the
app's theme tokens.
- Around line 89-90: DiagnosticsSettings.tsx currently renders hardcoded
user-facing strings (e.g. "ok — ${result.acp_elapsed_ms}ms" and the labels
around the diagnostics rows) which bypass i18n; update the component to import
and call useTranslation() (or reuse existing t if present) and replace all
visible strings in the DiagnosticsSettings component with t(...) lookups (create
suitable keys like diagnostics.ok_elapsed, diagnostics.failed,
diagnostics.labelX for the rows referenced around lines 147/154/160/165),
interpolating result.acp_elapsed_ms via t('diagnostics.ok_elapsed', { ms:
result.acp_elapsed_ms }) or similar, and ensure default/fallback text is
provided so translations don’t break rendering.
- Around line 65-81: In copyReport, currently setCopied(true) runs immediately
after navigator.clipboard.writeText which can fail; update copyReport to await
the Promise returned by
navigator.clipboard.writeText(lines.filter(Boolean).join("\n")) and only call
setCopied(true) (and the subsequent setTimeout to clear it) inside the
successful resolution path, while handling rejection (catch) to log or handle
the error instead of showing copied feedback; reference
navigator.clipboard.writeText and the copyReport function to locate where to add
async/await and error handling.
- Around line 52-63: The runDiagnostics function currently awaits
invoke("run_diagnostics") without a catch, so errors become unhandled and the UI
never shows an error state; update runDiagnostics to wrap the await in try/catch
where you call invoke<DiagnosticsResult>("run_diagnostics", { binaryPath:
copilotPath, ghToken }), catch any thrown error and set an error state (e.g.,
setError or setResult with an error flag/message) before rethrowing or
returning, and ensure setLoading(false) still runs in finally; if there is no
error state hook yet, add a state variable like [error, setError] and use it to
display a user-facing message from the caught error.
In `@apps/tauri/src/components/TerminalInput.tsx`:
- Around line 52-55: The Enter key handler (in the keydown listener that calls
handleSend) should avoid sending while IME composition is active; update the
handler to check for composition by either inspecting e.nativeEvent.isComposing
or tracking compositionStart/compositionEnd and return early if composing, then
only call handleSend when !e.shiftKey and not composing. Reference the keydown
handler and the handleSend call in TerminalInput (the block that currently reads
if (e.key === "Enter" && !e.shiftKey) { ... }) and add the composition check
before preventing default and invoking handleSend.
- Around line 28-43: The drag listeners added in handleDragStart (onMove/onUp
via document.addEventListener) can leak if the component unmounts before
mouseup; update TerminalInput to ensure those listeners are removed on unmount
by adding a useEffect cleanup that removes the same onMove and onUp handlers (or
track and remove listeners stored on a ref), or convert onMove/onUp to stable
named functions stored on refs so the cleanup can call
document.removeEventListener for both "mousemove" and "mouseup" when the
component unmounts; keep using dragRef and setInputHeight as before.
In `@apps/tauri/src/hooks/useAcpConnection.ts`:
- Line 74: The guard in useAcpConnection that prevents duplicate connects only
checks connectedRef.current and connectionStatus === "connecting", so it still
allows a new acp_connect when connectionStatus === "reconnecting"; update the
conditional in the connection initiation (the line using connectedRef.current
and connectionStatus) to also treat "reconnecting" as a blocking state (e.g.,
check connectionStatus === "connecting" || connectionStatus === "reconnecting"
or use an array/includes check) so acp_connect cannot be invoked while
reconnecting.
- Around line 64-67: The onVisible handler currently triggers acp_check_health
for both hidden and visible transitions; update the onVisible function (the
handler that calls invoke("acp_check_health", { workspaceId })) to first check
the actual visibility state (e.g., window.document.visibilityState === "visible"
or a passed visibility boolean) and only call invoke when the window is visible
and connectedRef.current is true, leaving the connectedRef.current guard intact.
- Around line 2-4: The useAcpConnection hook currently imports invoke and listen
directly; update it to call the global Tauri APIs instead by replacing direct
calls to invoke(...) with window.__TAURI__.core.invoke(...) and listen(...) with
window.__TAURI__.event.listen(...), keeping the existing event name and handler
that uses the AcpConnectionStatusEvent type and preserving the returned
UnlistenFn semantics; ensure any type references to UnlistenFn remain compatible
(or wrap the returned unlisten in the same signature) and guard access if
window.__TAURI__ might be undefined before calling.
---
Outside diff comments:
In `@apps/tauri/src/components/WorkspaceCard.tsx`:
- Around line 86-98: The two badge blocks in WorkspaceCard.tsx both render at
"absolute top-1.5 right-1.5" and will overlap when unresolvedComments and
sessionCount are >0; make them mutually exclusive by only rendering the
sessionCount badge when unresolvedComments is null or 0 (i.e., change the
sessionCount conditional to something like "sessionCount != null && sessionCount
> 0 && (!unresolvedComments || unresolvedComments === 0)"), keeping the existing
MessageSquare and styling so the unresolvedComments badge takes priority.
---
Nitpick comments:
In `@apps/tauri/src-tauri/src/acp/commands.rs`:
- Around line 262-287: The acp_check_health function currently holds the mutex
on state.connections across the await call to AcpConnection::is_alive(), which
can increase contention; change it to lock, lookup the AcpConnection, clone (or
otherwise obtain an owned handle such as an Arc) the connection, then drop the
connections lock before calling conn.is_alive().await and conn.emit_status(...),
ensuring AcpConnection (or the stored value) implements Clone/Arc so you can
call is_alive and emit_status without holding the connections mutex; update
acp_check_health, AcpState usage, and the stored connection type accordingly and
keep the existing ConnectionStatusEvent/app_handle emit fallback logic.
In `@apps/tauri/src-tauri/src/acp/types.rs`:
- Line 145: Replace the raw String status field with a serializable enum to
enforce the four valid states: define an enum (e.g., ConnectionStatus) with
variants Connecting, Connected, Disconnected, Reconnecting and derive
serde::{Serialize, Deserialize}, then change the struct field pub status: String
to pub status: ConnectionStatus (and update any places constructing or matching
the old String to use the enum variants or implement From/Into if needed);
ensure serde rename attributes (or rename_all) match the exact string values
expected by the frontend so serialization/deserialization remains compatible.
In `@apps/tauri/src/components/ActiveSessionView.tsx`:
- Around line 191-194: The onClick handler in ActiveSessionView.tsx uses
navigator.clipboard.writeText(session.acp_session_id ?? session.id) but does not
handle rejections; change the onClick to call an async handler (or attach a
.catch) that attempts to write (using session.acp_session_id ?? session.id),
wraps the await/write in try/catch, and on error logs the error and surfaces a
user-facing fallback (e.g., a toast/error message or fallback prompt) so
clipboard failures are not silent; update references to session.acp_session_id
and session.id accordingly.
In `@apps/tauri/src/components/MarkdownViewer.tsx`:
- Line 336: Replace the template literal className on the div inside the
MarkdownViewer component with the cn() utility: import cn from 'lib/utils' (or
existing utils import) at the top of the file and use cn('relative
overflow-hidden', isEmbedded ? 'h-full' : 'flex-1 min-h-0') for the div's
className to handle conditional Tailwind class merging and avoid manual string
concatenation.
In `@apps/tauri/src/components/TerminalInput.tsx`:
- Line 97: The button/text render in TerminalInput (component TerminalInput in
TerminalInput.tsx) currently uses a template-string to merge Tailwind classes
around {t("chat.send")} — replace that template-string class merging with the
cn() utility from lib/utils.ts: import cn at the top if missing, then convert
the className expression to cn('base-classes', condition && 'conditional-class',
other && 'other-class') so classes are merged consistently; ensure you remove
the backtick/template literal and use cn(...) for all conditional class logic
referencing the same element that renders t("chat.send").
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/tauri/src-tauri/src/acp/commands.rsapps/tauri/src-tauri/src/acp/connection.rsapps/tauri/src-tauri/src/acp/types.rsapps/tauri/src-tauri/src/lib.rsapps/tauri/src/SettingsApp.tsxapps/tauri/src/components/ActiveSessionView.tsxapps/tauri/src/components/DirectoryWorkspace.tsxapps/tauri/src/components/HomeScreen.tsxapps/tauri/src/components/MarkdownViewer.tsxapps/tauri/src/components/MicButton.tsxapps/tauri/src/components/NewSessionForm.tsxapps/tauri/src/components/ReviewPanel.tsxapps/tauri/src/components/SessionCard.tsxapps/tauri/src/components/TerminalChat.tsxapps/tauri/src/components/TerminalInput.tsxapps/tauri/src/components/WorkspaceCard.tsxapps/tauri/src/components/settings/DiagnosticsSettings.tsxapps/tauri/src/hooks/useAcpConnection.tsapps/tauri/src/locales/en.jsonapps/tauri/src/locales/pt-BR.jsonapps/tauri/src/types/acp.ts
The visibilitychange event fires on both hide and show transitions. Guard with `document.hidden` so health checks only run when the window regains focus, not when the user tabs away. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- sync reviewOpen state with review.setIsPanelOpen on toggle - block connect calls when status is 'reconnecting' - enable comrak render.escape to prevent raw HTML pass-through (XSS) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- SessionCard: use cn() utility instead of template-literal class merging - TerminalInput: store drag listeners in ref and clean up on unmount - TerminalInput: guard Enter-to-send with isComposing check for IME support - DiagnosticsSettings: add explicit error state and catch block in runDiagnostics - DiagnosticsSettings: await clipboard write before setting copied feedback - DiagnosticsSettings: localize ACP Command/Stderr/Home/Path labels via i18n - useAcpConnection: switch from @tauri-apps/api package imports to window.__TAURI__ globals Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add --success, --success-foreground, --warning, --warning-foreground CSS variables to index.css for both light and dark themes, following the same HSL pattern as --destructive. Register them in tailwind.config.ts so that text-success, bg-success, text-warning, bg-warning etc. become available as Tailwind utilities. Replace hardcoded text-green-500 / text-yellow-500 / text-green-600 dark:text-green-400 in DiagnosticsSettings and CliInstallSettings with the new theme-aware text-success and text-warning tokens. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/tauri/src/components/MarkdownViewer.tsx (1)
357-379: Consider using shadcn Button component for consistency.The floating toggle buttons for outline and review panels use native
<button>elements. Per coding guidelines, shadcn/ui primitives should be used for UI components to maintain consistency with the rest of the application.♻️ Suggested refactor using shadcn Button
{/* Outline toggle button */} {!outlineOpen && headings.length > 0 && ( - <button + <Button + variant="outline" + size="icon" onClick={() => setOutlineOpen(true)} - className="absolute top-3 left-3 z-20 h-8 w-8 flex items-center justify-center rounded-md bg-card border border-border text-muted-foreground hover:text-foreground hover:bg-accent transition-colors shadow-sm" + className="absolute top-3 left-3 z-20 h-8 w-8 shadow-sm" title={t("outline.title")} > <Hash className="h-4 w-4" /> - </button> + </Button> )} {/* Review toggle button */} {!reviewOpen && ( - <button + <Button + variant="outline" + size="icon" onClick={() => { setReviewOpen(true); review.setIsPanelOpen(true); }} - className="absolute top-3 right-3 z-20 h-8 w-8 flex items-center justify-center rounded-md bg-card border border-border text-muted-foreground hover:text-foreground hover:bg-accent transition-colors shadow-sm" + className="absolute top-3 right-3 z-20 h-8 w-8 shadow-sm relative" title={t("review.togglePanel")} > <MessageSquare className="h-3.5 w-3.5" /> {review.unresolvedCount > 0 && ( <span className="absolute -top-1 -right-1 min-w-[16px] h-[16px] px-1 rounded-full text-[9px] font-semibold bg-foreground text-background flex items-center justify-center"> {review.unresolvedCount} </span> )} - </button> + </Button> )}As per coding guidelines: "Use shadcn/ui for all UI primitives" for components under
apps/tauri/src/components/**/*.{ts,tsx}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/MarkdownViewer.tsx` around lines 357 - 379, The floating toggle buttons use native <button> elements; replace them with the shadcn Button primitive (import the Button from your shadcn/ui exports) to follow UI conventions, keeping the same props and behavior: for the outline toggle replace the button that calls setOutlineOpen(true) with a Button that preserves title={t("outline.title")}, className, Hash icon child and positioning; for the review toggle replace the button that calls setReviewOpen(true); review.setIsPanelOpen(true) with a Button that preserves the onClick handler, title={t("review.togglePanel")}, MessageSquare icon child and the unresolvedCount badge (keep the badge span inside the Button), and ensure any accessibility attributes and transitions/styles are preserved.apps/tauri/src/components/TerminalInput.tsx (1)
72-77: Make the resize handle keyboard-accessible.This interactive handle is currently mouse-only (
divwithonMouseDown), so keyboard users can’t resize.As per coding guidelines `**/*.{tsx,ts}`: Use `useTranslation()` hook for all user-facing text in React components.♿ Suggested patch
- <div - className="h-1.5 -mt-1 cursor-row-resize flex items-center justify-center group" - onMouseDown={handleDragStart} - > + <button + type="button" + aria-label={t("chat.resizeInput")} + className="h-1.5 -mt-1 w-full cursor-row-resize flex items-center justify-center group" + onMouseDown={handleDragStart} + onKeyDown={(e) => { + if (e.key === "ArrowUp") setInputHeight((h) => Math.min(400, h + 16)); + if (e.key === "ArrowDown") setInputHeight((h) => Math.max(80, h - 16)); + }} + > <div className="w-8 h-0.5 rounded-full bg-border group-hover:bg-muted-foreground/40 transition-colors" /> - </div> + </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/TerminalInput.tsx` around lines 72 - 77, The resize handle in TerminalInput is mouse-only; make it keyboard-accessible by using the useTranslation() hook to provide a translated aria-label and converting the handle element into a focusable control that responds to keyboard events: import and call useTranslation(), add an aria-label (e.g., t('resizeHandle') or similar) and tabindex if needed, and wire onKeyDown to the existing handleDragStart or a small wrapper that interprets Enter/Space to begin drag and ArrowUp/ArrowDown (and PageUp/PageDown) to adjust size; update the element referenced in the diff (the div with onMouseDown and handleDragStart) to be a focusable button or a div with role and keyboard handlers so keyboard users can resize.apps/tauri/src/components/settings/CliInstallSettings.tsx (1)
117-117: Usecn()for conditional class merging.Line 117 should use
cn()instead of template interpolation to match project convention.As per coding guidelines: `apps/tauri/src/**/*.{ts,tsx}`: “Use the `cn()` utility from `lib/utils.ts` for merging Tailwind CSS classes”.🔧 Proposed refactor
import { CheckCircle, Circle } from "lucide-react"; +import { cn } from "@/lib/utils"; @@ - <p className={`text-xs ${result.success ? "text-success" : "text-destructive"}`}> + <p className={cn("text-xs", result.success ? "text-success" : "text-destructive")}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/settings/CliInstallSettings.tsx` at line 117, Replace the template string className on the <p> in CliInstallSettings (currently using `className={`text-xs ${result.success ? "text-success" : "text-destructive"}`}`) with the project's class-merging utility: import and call cn(...) from lib/utils.ts and pass "text-xs" plus a conditional object/expression that uses result.success to select "text-success" or "text-destructive", so the element uses cn(...) with result.success instead of template interpolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/tauri/src-tauri/src/lib.rs`:
- Around line 370-373: gh_token_set is currently true for whitespace-only values
and HOME is used without a Windows fallback; make gh_token_set mirror
test_acp_connection by trimming gh_token (use
gh_token.as_deref().map(str::trim).filter(|s| !s.is_empty()).is_some()) so
whitespace-only tokens are ignored, and derive home_env using
std::env::var("HOME").or_else(|_|
std::env::var("USERPROFILE")).unwrap_or_default() so Windows USERPROFILE is used
when HOME is absent; update references around gh_token_set, PATH and home_env
accordingly.
In `@apps/tauri/src/hooks/useAcpConnection.ts`:
- Around line 37-52: The promise returned by window.__TAURI__.event.listen in
useAcpConnection.ts isn't handling rejections, so listener registration failures
become unhandled; update the call chain on the listen(...) promise (the
then(...) that assigns unlisten) to also include a catch(...) that handles
registration errors (e.g., setConnectionError with a descriptive message, set
connectedRef.current = false if appropriate, and ensure any partial cleanup by
calling fn if provided), and ensure unlisten is only assigned on successful
registration; reference the listen invocation and the unlisten
variable/connectedRef.current/setConnectionError to locate where to add the
.catch handler.
- Around line 95-113: The disconnect/cleanup currently skips teardown when
connectedRef.current is false, which can miss disconnecting if acp_connect
succeeded but the status event was lost; update the disconnect callback and the
cleanup effect to rely not solely on connectedRef.current but also attempt a
best-effort acp_disconnect when workspace may be connected: in the disconnect
function (disconnect) and the unmount cleanup inside useEffect, always call
window.__TAURI__.core.invoke("acp_disconnect", { workspaceId }) (wrap in
try/catch or use .catch to swallow errors) instead of returning early based only
on connectedRef.current, and after the invoke ensure connectedRef.current =
false and setConnectionStatus("idle"); keep connectedRef updates from successful
acp_connect/status events but do not gate teardown on them.
---
Nitpick comments:
In `@apps/tauri/src/components/MarkdownViewer.tsx`:
- Around line 357-379: The floating toggle buttons use native <button> elements;
replace them with the shadcn Button primitive (import the Button from your
shadcn/ui exports) to follow UI conventions, keeping the same props and
behavior: for the outline toggle replace the button that calls
setOutlineOpen(true) with a Button that preserves title={t("outline.title")},
className, Hash icon child and positioning; for the review toggle replace the
button that calls setReviewOpen(true); review.setIsPanelOpen(true) with a Button
that preserves the onClick handler, title={t("review.togglePanel")},
MessageSquare icon child and the unresolvedCount badge (keep the badge span
inside the Button), and ensure any accessibility attributes and
transitions/styles are preserved.
In `@apps/tauri/src/components/settings/CliInstallSettings.tsx`:
- Line 117: Replace the template string className on the <p> in
CliInstallSettings (currently using `className={`text-xs ${result.success ?
"text-success" : "text-destructive"}`}`) with the project's class-merging
utility: import and call cn(...) from lib/utils.ts and pass "text-xs" plus a
conditional object/expression that uses result.success to select "text-success"
or "text-destructive", so the element uses cn(...) with result.success instead
of template interpolation.
In `@apps/tauri/src/components/TerminalInput.tsx`:
- Around line 72-77: The resize handle in TerminalInput is mouse-only; make it
keyboard-accessible by using the useTranslation() hook to provide a translated
aria-label and converting the handle element into a focusable control that
responds to keyboard events: import and call useTranslation(), add an aria-label
(e.g., t('resizeHandle') or similar) and tabindex if needed, and wire onKeyDown
to the existing handleDragStart or a small wrapper that interprets Enter/Space
to begin drag and ArrowUp/ArrowDown (and PageUp/PageDown) to adjust size; update
the element referenced in the diff (the div with onMouseDown and
handleDragStart) to be a focusable button or a div with role and keyboard
handlers so keyboard users can resize.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/tauri/src-tauri/src/lib.rsapps/tauri/src/components/MarkdownViewer.tsxapps/tauri/src/components/SessionCard.tsxapps/tauri/src/components/TerminalInput.tsxapps/tauri/src/components/settings/CliInstallSettings.tsxapps/tauri/src/components/settings/DiagnosticsSettings.tsxapps/tauri/src/hooks/useAcpConnection.tsapps/tauri/src/index.cssapps/tauri/src/locales/en.jsonapps/tauri/src/locales/pt-BR.jsonapps/tauri/tailwind.config.ts
✅ Files skipped from review due to trivial changes (1)
- apps/tauri/src/components/settings/DiagnosticsSettings.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/tauri/src/components/SessionCard.tsx
- lib.rs: trim whitespace from gh_token before marking it as set; add USERPROFILE fallback for home_env on Windows - useAcpConnection: add .catch() to the listen() promise chain to surface failed event-listener registration as an error state - useAcpConnection: remove connectedRef guards from disconnect and unmount cleanup so teardown always runs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/tauri/src-tauri/src/lib.rs`:
- Around line 362-366: The code checks binary_path.trim().is_empty() but then
uses the untrimmed binary_path value; change the logic so you obtain and use a
trimmed string (e.g., map or filter and call .trim().to_string() on binary_path)
before falling back to std::env::var("COPILOT_PATH") or "copilot", ensuring the
variable named binary contains the trimmed path so spawning processes won't fail
on paths with leading/trailing whitespace.
In `@apps/tauri/src/hooks/useAcpConnection.ts`:
- Around line 33-64: The effect must reset hook state immediately when
workspaceId changes to avoid stale UI: before calling
window.__TAURI__.event.listen in the useEffect,
setConnectionStatus("disconnected"), setConnectionError(null) and set
connectedRef.current = false so the UI reflects the new workspace until an event
arrives; keep the existing cancelled/unlisten logic and error handling intact.
- lib.rs: apply trim() to binary_path value before using it in spawn, not only for the emptiness check; a path with leading/trailing whitespace would fail process spawn unnecessarily - useAcpConnection: reset connectedRef, connectionStatus and connectionError at the top of the workspaceId effect so the UI never shows stale state from a previous workspace while waiting for the first event from the new one Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/tauri/src-tauri/src/lib.rs`:
- Around line 373-377: The gh_token_set logic currently treats an environment
variable present but empty/whitespace as valid; change the check so GH_TOKEN is
considered set only when its value is non-empty after trimming. Replace the
std::env::var("GH_TOKEN").is_ok() piece with a trimmed-value check (e.g.
std::env::var("GH_TOKEN").map(|v| !v.trim().is_empty()).unwrap_or(false)) and
keep the existing gh_token.as_deref().map(|s|
!s.trim().is_empty()).unwrap_or(false) logic so gh_token_set accurately reflects
a usable token for the gh_token_set variable.
- Around line 383-389: The timeout-wrapped subprocess created when checking
--version can leak because the child isn't configured to be killed if the future
is dropped; modify the Command construction used for version_result (the
tokio::process::Command in the version_result block) to call kill_on_drop(true)
before calling .output(), e.g. set .kill_on_drop(true) on the Command that runs
.arg("--version").output() so the child process is terminated when the timeout
drops the future.
In `@apps/tauri/src/hooks/useAcpConnection.ts`:
- Around line 42-57: The hook's async event listener registration
(window.__TAURI__.event.listen in useAcpConnection) can miss a fast acp_connect
result and leave state stuck at "connecting"; after the listener promise
resolves (the .then where you set unlisten), perform an immediate fallback sync:
call the ACP status getter (e.g., invoke a function like
acp_get_connection_status or equivalent IPC/getter) and then call
setConnectionStatus(status), update connectedRef.current and
setConnectionError(null) if status === "connected" (or set connectedRef.current
= false if "disconnected"), so the UI transitions correctly even if the connect
event fired before the listener was ready. Ensure you perform this only once
after listener registration resolves and before relying solely on future events.
- Around line 46-53: connectedRef.current isn't updated for statuses other than
"connected"/"disconnected", which can leave it stale; update
connectedRef.current whenever setConnectionStatus is called based on
event.payload.status (e.g., set to true only for "connected" and false for
"connecting", "reconnecting", and any non-"connected" values) and preserve
existing logic for clearing setConnectionError when status === "connected" so
state and the ref stay synchronized.
- lib.rs: fix GH_TOKEN env var check to trim whitespace before treating it as set, matching the same treatment applied to the gh_token argument - lib.rs: add kill_on_drop(true) to the --version subprocess so it is terminated when the 5s timeout drops the future, preventing process leak - useAcpConnection: consolidate connectedRef updates to a single assignment (status === 'connected') so all status transitions (reconnecting, connecting, etc.) keep the ref in sync, not just connected/disconnected - useAcpConnection: add post-connect fallback health query after acp_connect succeeds to avoid stuck 'connecting' state when the status event fires before the listener is registered Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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
heartbeat_taskin Rust to detect process death every 15s; addedis_alive()andemit_status()helpers;acp_check_healthcommand for on-demand status query;ConnectionStatusEventnow emitted on connect, disconnect and process exituseAcpConnectionnow listens toacp:connection-statusevents from Rust and exposes a typedconnectionStatusvalue (idle | connecting | connected | disconnected | reconnecting); health is re-checked when the window regains visibilityrun_diagnosticsRust command tests platform, binary presence, version, GH_TOKEN and ACP connectivity (with 10s timeout + stderr capture); newDiagnosticsSettingscomponent with visual report and copy-to-clipboard; integrated as a new tab in the settings windowResizablePanelGroup/ImperativePanelHandlewith absolute overlay drawers controlled by simple boolean state; floating toggle buttons open outline (left) and review (right); auto-open onreviewingphase preservedline-clamp-2titles; grid columns reduced and gaps increased;TerminalInputgains a drag handle for resizable height; send shortcut changed from Cmd/Ctrl+Enter to Enter (Shift+Enter inserts newline); reconnect button only visible when disconnected;acp_session_idshown in session header; new session dialog widened with a taller textarea;MicButtongetstype="button"to prevent accidental form submissionTest plan
idle → connecting → connectedacp_check_healthshould fire and reflect current statusMarkdownViewerreviewingphaseTerminalInputby dragging the handle up and downSummary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements
Localization