feat(acp): Session management revamp with config controls and plan restore#48
Merged
feat(acp): Session management revamp with config controls and plan restore#48
Conversation
* feat(acp): Add heartbeat, dead connection cleanup and connection logging
- Add Rust-side heartbeat task with 15s ping interval and 3-strike disconnect
- Clean up dead connections in `acp_connect` and `acp_check_health`
- Emit structured connection log events via `acp:log`
- Add `HeartbeatEvent`, `ConnectionLogEntry` and `ConnectionConfig` types
- Add `chat_panel_size` field to session persistence
* feat(acp): Persist session state across minimize/restore and reload history
- Add in-memory session cache with global event listener independent of React lifecycle
- Keep DirectoryWorkspace mounted via CSS hidden when minimized
- Add `persistedWorkspaceId` to AppContext for mount/visibility separation
- Always call `acp_load_session` to replay full conversation from ACP backend
- Restore cached messages on "already loaded" fallback
- Add periodic 30s heartbeat in `useAcpConnection` for proactive disconnect detection
- Remove auto-disconnect on unmount; move disconnect to explicit close/forget
- Add connection logs panel with `useAcpLogs` hook and `ConnectionLogs` component
- Add i18n strings for connection logs and ACP status
* test(acp): Add tests for connection hook and logs hook
- Test connect/disconnect lifecycle, status events, workspace isolation
- Test cached state restoration and health check on mount
- Verify disconnect is not called on unmount
- Test log capture, filtering and clear behavior
* style(ui): Replace X icon with Trash2 on session delete button
* fix(acp): Reset session init on disconnect and auto-reload on "not found"
- Reset `initRef` when `isConnected` goes false so reconnect triggers
`doInit` and reloads the session in the new copilot process
- Auto-recover in `sendPrompt`: on "Session not found" (-32602), try
`acp_load_session` then retry the prompt before surfacing the error
* fix(acp): Add 60s streaming timeout to auto-clear stale Stop button
If no streaming event arrives within 60s, `isStreaming` is reset to
false automatically. Prevents the Stop button from staying red
indefinitely when `end_turn` is never received.
* fix: replace native button with shadcn Button and localize aria-label in ErrorConsole
- Use Button (variant=ghost, size=icon) from shadcn/ui
- Add useTranslation() and use t('common.dismiss') for aria-label
- Add common.dismiss key to pt-BR.json ("Dispensar") and en.json ("Dismiss")
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(acp): Avoid awaiting async ops while holding connections mutex
In `acp_connect` and `acp_check_health`, `is_alive().await` and
`shutdown().await` were called while the connections mutex was held,
potentially stalling other ACP commands behind long waits.
Refactored both functions to remove the entry under the lock, drop the
lock, perform async checks/shutdown outside it, then re-acquire the
lock only to re-insert (if still alive) or finalize cleanup.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(acp): Ensure `onConnect` runs even if `onDisconnect` rejects
In `handleReconnect`, if `onDisconnect()` rejected the promise chain
would abort before calling `onConnect()`, leaving the UI stuck in an
error state. Wrapped `onDisconnect` in try/catch so `onConnect`
always executes regardless of disconnect errors.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(acp): Fix listener guard and add streaming timer in session-cache
Flip listenerSetup flag only after the Tauri event listener resolves,
allowing retries if setup fails. Also call resetStreamingTimer() in
addUserMessage so the isStreaming flag is always auto-cleared if no
backend events follow.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(ui): Replace hardcoded hex colors with HSL CSS variables in index.css
Add --terminal-code CSS variable and replace all #3dd68c and #56d4dd
hex values with hsl(var(--success)) and hsl(var(--terminal-code))
respectively, ensuring colors respect the app theme system.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(ui): Use semantic token and localize done label in TerminalMessage
Replace hex color class with text-success/60 semantic token and
localize the hardcoded "done" label via useTranslation().
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(acp): Full state reset on session load and clear idle timer on workspace change
Reset all local state fields (currentMode, availableModes,
activeAcpSessionId, agentPlanFilePath) when loading an existing session
to prevent stale UI state. Also clear idleTimerRef in the workspaceId
effect cleanup so timers don't fire on the wrong workspace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(ui): Use @/ alias for ConnectionLogs and re-trigger init on session change
Replace relative import with @/ path alias. Add a dedicated effect to
reset initRef when session identity changes so doInit() reruns when a
different session is mounted on the same connected workspace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(acp): Avoid holding connections mutex during shutdown in acp_disconnect
Drop the connections lock before calling conn.shutdown().await,
consistent with the same fix applied to acp_connect and acp_check_health.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add `suppress_updates` flag to batch message saves during streaming - Handle tool call content updates via `update_message_by_tool_call_id` - Add `is_duplicate_user_message` to prevent re-saving on session reload - Refactor `send_prompt` to persist user messages before sending
- Replace polling with `acp:user-message-saved` and `acp:assistant-message-saved` events - Update `TerminalChat` and `TerminalMessage` for new message flow - Update test setup and mocks to match new architecture
- Add Rust types: `SetConfigOptionParams`, `SessionConfigOption`, `SessionConfigOptionValue` - Add TS types: `AcpSessionConfigOption`, `AcpSessionConfigOptionsState`, `AcpPreferences` - Change `SessionInfo.config_options` to raw JSON for ACP format tolerance - Add `acp_preferences_json` to `SessionRecord`
- Add `acp_session_set_config_option` command - Emit `session_info_update` with full mode objects and config options - Add `acp_preferences_json` column to sessions table with migration - Add `workspace_acp_defaults` table for per-workspace preferences - Add `session_update_acp_preferences`, `workspace_acp_defaults_get/set` commands - Register new commands in `invoke_handler`
- Add config option fields to `SessionEntry` and update subscribers - Add `addSystemNotice`, `normalizeSelectedConfigOptions`, `readConfigState` helpers - Handle `session_info_update` and `config_options_update` events - Change `availableModes` from `string[]` to `AcpSessionMode[]` - Fix plan restore: remove strict path validation, add multi-location search - Add auto-locate plan on mount for resumed sessions
- Add `AcpSessionControls` component with Mode, Model, Agent, Advanced dropdowns - Wire config states and controls into `ActiveSessionView` - Add preferences persistence and auto-apply on session init - Add auto-switch mode notice via `addSystemNotice` - Replace full-screen init error with inline dismissible banners
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds per-session ACP lifecycle (backend + Tauri commands), session/message persistence with DB v2 migration, workspace→ID migration, new frontend per-session hooks/components and tests, removes file-based history, centralizes clipboard utility, and adds ACP choreography documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as React UI
participant Hook as useSessionConnection/useSessionMessages
participant Tauri as Tauri IPC
participant Rust as Rust backend
participant DB as SQLite
participant ACP as Copilot (ACP)
UI->>Hook: connect(sessionId, workspaceId, binary?, ghToken?, acpSessionId?)
Hook->>Tauri: invoke("acp_session_connect", ...)
Tauri->>Rust: acp_session_connect request
Rust->>DB: ensure workspace/session rows, persist config
Rust->>Rust: create AcpSessionInstance in AcpSessionStore (IndexMap)
Rust->>ACP: spawn copilot process (JSON-RPC over stdin/stdout)
ACP->>Rust: streaming and session updates
Rust->>DB: buffer/persist messages (save_to_db/flush_buffer)
Rust->>Tauri: emit events (acp:session-status, acp:user-message-saved, acp:assistant-message-saved)
Tauri->>Hook: event received -> update session-cache & messages hook
Hook->>UI: update status, streaming view, and persisted messages
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/tauri/src/components/TerminalInput.tsx (1)
91-99:⚠️ Potential issue | 🟡 MinorDon't swallow Enter while streaming.
Line 98 leaves the textarea editable during streaming, but
handleKeyDown()still prevents the default Enter behavior beforehandleSend()exits early. The result is that plain Enter becomes a no-op while the user is drafting the next prompt. Either re-disable the textarea during streaming or only treat Enter as “send” when sending is actually allowed.Possible fix
const handleKeyDown = (e: React.KeyboardEvent) => { - if (e.key === "Enter" && !e.shiftKey && !e.nativeEvent.isComposing) { + if (!isStreaming && !disabled && e.key === "Enter" && !e.shiftKey && !e.nativeEvent.isComposing) { e.preventDefault(); handleSend(); } };🤖 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 91 - 99, Textarea is left enabled while streaming but handleKeyDown prevents Enter regardless, making Enter a no-op; update the logic so Enter only triggers send when sending is allowed (or disable the Textarea while streaming). Concretely, either set disabled={disabled || isStreaming} on the Textarea (referencing the streaming state, e.g., isStreaming) or modify handleKeyDown/handleSend to early-return only when send is actually allowed (check the same isStreaming/disabled condition inside handleKeyDown before calling preventDefault and handleSend). Ensure references to Textarea, handleKeyDown, handleSend, and the streaming state (e.g., isStreaming) are updated consistently.apps/tauri/src-tauri/src/sessions.rs (1)
323-329:⚠️ Potential issue | 🟠 MajorDon’t fail
session_deleteafter the row is already gone.
delete_session(&conn, &id)?runs beforedelete_plan. If the plan file is already missing, this command returns an error even though the DB delete succeeded, which leaves the UI believing the delete failed. Treat missing-plan cleanup as best effort here, or handleNotFoundexplicitly.Based on learnings,
delete_planmay returnErrwhen the plan file does not exist, so best-effort cleanup should not fail the operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src-tauri/src/sessions.rs` around lines 323 - 329, The DB row is deleted by delete_session(&conn, &id) but a missing plan file makes delete_plan return an Err and causes session_delete to fail; change the code to treat plan-file removal as best-effort by calling crate::plan_file::delete_plan(&app_data, &id) and handling its Result such that a "not found" error is ignored (do not propagate) while other errors may be logged or returned—reference the delete_session and crate::plan_file::delete_plan calls and adjust the error handling there so deletion succeeds when the DB row was removed even if the plan file is already gone.apps/tauri/src/contexts/AppContext.tsx (1)
179-189:⚠️ Potential issue | 🟠 MajorDon’t delete the workspace record on a plain close.
Sessions now hang off
workspace_id. Removing the workspace row here means reopening the same path creates a fresh id, so the old sessions/messages become unreachable even though the user only closed the card.workspace_deleteshould stay in the forget flow, or reopen needs to preserve the original id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/contexts/AppContext.tsx` around lines 179 - 189, The closeWorkspace handler is currently deleting the persisted workspace row (invoke('workspace_delete', { id })) which causes sessions/messages tied to workspace_id to be orphaned; remove the call to invoke('workspace_delete') from closeWorkspace so closing a card only updates UI state (setWorkspaces filter) and optional teardown (unwatch_file, acp_disconnect, clearWorkspaceCaches), and reserve invoke('workspace_delete') for the explicit "forget" flow; also ensure the reopen path logic reuses an existing workspace row by looking up the persisted workspace by path and preserving its id instead of creating a fresh one.apps/tauri/src-tauri/src/acp/connection.rs (1)
327-340:⚠️ Potential issue | 🟠 MajorTreat ping channel failures as heartbeat failures.
The
Ok(_)arm acceptsOk(Err(_))andErr(RecvError)as healthy, so a dropped or errored ping still resetsconsecutive_failuresand emits"healthy"instead of degrading/disconnecting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src-tauri/src/acp/connection.rs` around lines 327 - 340, The current match on tokio::time::timeout(ping_timeout, rx).await treats any Ok(_) as a successful ping; change the match to distinguish Ok(Ok(_)) as the healthy case and treat Ok(Err(_)) and Err(_) as heartbeat failures: on Ok(Ok(_)) keep the existing behavior (compute latency from start, reset consecutive_failures, update last_activity, emit HeartbeatEvent with status "healthy"), but for Ok(Err(_)) and Err(_) increment consecutive_failures, do not reset last_activity, set latency_ms to None (or omit), and emit a HeartbeatEvent with status like "unreachable"/"failed" (or trigger disconnect logic if consecutive_failures exceeds threshold). Update handling around ping_timeout, rx, consecutive_failures, last_activity, HeartbeatEvent, app_handle.emit, workspace_id, start, and timestamp accordingly.apps/tauri/src/lib/session-cache.ts (1)
118-126:⚠️ Potential issue | 🟠 MajorUpdate tool cards on every status transition.
Only
"completed"events are applied here. Failed, cancelled, or running tool calls will stay stuck at their initial"pending"state.🔧 Suggested fix
- if (p.status !== "completed") break; const rawOutput = p.rawOutput as Record<string, unknown> | undefined; const summary = rawOutput?.content as string; - if (!summary) break; const idx = msgs.findIndex((m) => m.toolCallId === (p.toolCallId as string)); if (idx !== -1) { - msgs[idx] = { ...msgs[idx], content: summary, toolStatus: "completed" }; + msgs[idx] = { + ...msgs[idx], + content: summary || msgs[idx].content, + toolStatus: typeof p.status === "string" ? p.status : msgs[idx].toolStatus, + }; } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/lib/session-cache.ts` around lines 118 - 126, The current case "tool_call_update" only handles p.status === "completed", leaving tool cards stuck at "pending"; update the handler in session-cache.ts to apply every status transition: find the message by toolCallId (using p.toolCallId and msgs.findIndex) and always set msgs[idx].toolStatus = p.status, and if p.rawOutput?.content exists set msgs[idx].content = that summary; keep existing guard to skip if message not found. Ensure you still handle undefined rawOutput gracefully and only overwrite content when present.
🟡 Minor comments (5)
apps/tauri/src/lib/utils.ts-14-16 (1)
14-16:⚠️ Potential issue | 🟡 MinorError message should be localized.
The success toast uses
i18n.t("common.copied")but the error toast has a hardcoded English string. For consistency with the project's i18n approach, the error message should also be localized.🌐 Proposed fix
} catch { // fallback for environments where clipboard API is restricted - toast.error("Failed to copy"); + toast.error(i18n.t("common.copyFailed")); }You'll also need to add the
common.copyFailedkey to your locale files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/lib/utils.ts` around lines 14 - 16, The catch block in apps/tauri/src/lib/utils.ts currently calls toast.error("Failed to copy") with a hardcoded English string; replace that with the i18n-based localized message using i18n.t("common.copyFailed") so the error toast matches the success toast usage (which uses i18n.t("common.copied")), and add the "common.copyFailed" key to the locale files accordingly.apps/tauri/src-tauri/src/messages.rs-93-137 (1)
93-137:⚠️ Potential issue | 🟡 Minor
update_message_by_tool_call_idmay return a stale record if no row matches.The function performs an UPDATE then immediately queries by
(session_id, tool_call_id). If no row matched the UPDATE (the tool_call_id doesn't exist), the subsequent SELECT will fail withQueryReturnedNoRows, which gets mapped to a generic "Query error after update". This doesn't distinguish between "update failed" and "record not found".Consider checking the update count or returning
Option<MessageRecord>to handle missing records gracefully.🛡️ Proposed fix: check rows affected
pub fn update_message_by_tool_call_id( conn: &Connection, session_id: &str, tool_call_id: &str, content: Option<&str>, tool_status: &str, -) -> Result<MessageRecord, String> { +) -> Result<Option<MessageRecord>, String> { - if let Some(new_content) = content { - conn.execute( + let rows_affected = if let Some(new_content) = content { + conn.execute( "UPDATE messages SET content = ?1, tool_status = ?2 WHERE session_id = ?3 AND tool_call_id = ?4", params![new_content, tool_status, session_id, tool_call_id], ) - .map_err(|e| format!("Update error: {}", e))?; + .map_err(|e| format!("Update error: {}", e))? } else { conn.execute( "UPDATE messages SET tool_status = ?1 WHERE session_id = ?2 AND tool_call_id = ?3", params![tool_status, session_id, tool_call_id], ) - .map_err(|e| format!("Update error: {}", e))?; - } + .map_err(|e| format!("Update error: {}", e))? + }; + + if rows_affected == 0 { + return Ok(None); + } conn.query_row( // ... ) - .map_err(|e| format!("Query error after update: {}", e)) + .map(Some) + .map_err(|e| format!("Query error after update: {}", e)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src-tauri/src/messages.rs` around lines 93 - 137, update_message_by_tool_call_id performs an UPDATE then SELECT but never checks the UPDATE row count, so a missing tool_call_id yields a misleading "Query error after update"; update the function to inspect the usize returned by conn.execute (the rows affected) for both branches that run the UPDATE, and if it is 0 return a clear result (either Err("no row found for session_id/X and tool_call_id/Y") or change the signature to return Result<Option<MessageRecord>, String> and return Ok(None)); only run the SELECT when rows_affected > 0 and keep the existing row mapping when present.apps/tauri/src-tauri/src/comments.rs-518-530 (1)
518-530:⚠️ Potential issue | 🟡 MinorFallback to
"unknown"workspace_id may violate foreign key constraint.If no workspace matches the file path,
workspace_iddefaults to"unknown"(line 523), but thecommentstable hasREFERENCES workspaces(id) ON DELETE CASCADE. Inserting a comment with a non-existent workspace_id will fail when foreign keys are enabled.Consider skipping migration for files without a matching workspace, similar to the approach in
migrate_v1_to_v2(lines 303-306).🛡️ Proposed fix: skip if no workspace found
let workspace_id: String = conn.query_row( "SELECT id FROM workspaces WHERE ?1 LIKE path || '/%' OR ?1 = path ORDER BY LENGTH(path) DESC LIMIT 1", params![markdown_path], |row| row.get(0), - ).unwrap_or_else(|_| "unknown".to_string()); + ).ok(); + + let workspace_id = match workspace_id { + Some(id) => id, + None => { + // No matching workspace; skip migration but still delete the legacy file + let _ = std::fs::remove_file(&json_pathbuf); + return Ok(false); + } + }; let data = CommentsData {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src-tauri/src/comments.rs` around lines 518 - 530, The code currently falls back to the literal "unknown" workspace_id when the SELECT query fails, which can violate the comments->workspaces FK; instead, change the workspace lookup so that if conn.query_row (the SELECT that assigns workspace_id) returns an error/NoRows you skip migrating that file (do not call save_comments). Mirror the pattern used in migrate_v1_to_v2: treat the query result as Option/Result, log or note the skipped file, and only call save_comments(conn, markdown_path, &workspace_id, &data) when a real workspace_id was found.apps/tauri/src/components/SessionCard.tsx-44-49 (1)
44-49:⚠️ Potential issue | 🟡 MinorLocalize and theme the connection-status indicator.
The new dot exposes raw enum values in the tooltip and uses hardcoded Tailwind colors. Please switch this to a translated label/
aria-labeland semantic status tokens so it stays accessible and theme-aware.As per coding guidelines, use
useTranslation()hook for all user-facing text in React components. Based on learnings, use semantic SHADCN UI tokens for status indicators instead of hardcoded colors.Also applies to: 99-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/SessionCard.tsx` around lines 44 - 49, The connection-status dot currently exposes raw enum values and hardcoded Tailwind classes (see CONN_COLORS in SessionCard) — update the component to call useTranslation() and map the enum to a translated label for both the tooltip and aria-label, and replace the hardcoded class values in CONN_COLORS with semantic SHADCN UI status tokens (e.g., use token class names for success/warning/disabled/muted instead of literal bg-green-500 / bg-yellow-400 / bg-muted-foreground/30 / bg-muted-foreground/20) so the indicator is localized, accessible (aria-label uses the translated text), and theme-aware; keep the mapping keyed by the same enum values so other references (e.g., where the dot is rendered) remain unchanged.apps/tauri/src/__tests__/hooks/useSessionMessages.test.ts-28-30 (1)
28-30:⚠️ Potential issue | 🟡 MinorUse a block body in
emitTauriEvent.Line 29 returns the callback result from
forEach(), which Biome flags as suspicious and can fail lint for this test file.Suggested fix
function emitTauriEvent(eventName: string, payload: unknown) { - eventListeners.get(eventName)?.forEach((cb) => cb({ payload })); + eventListeners.get(eventName)?.forEach((cb) => { + cb({ payload }); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/__tests__/hooks/useSessionMessages.test.ts` around lines 28 - 30, The arrow passed to forEach in emitTauriEvent currently uses an implicit return (cb => cb({ payload })) which causes the callback result to be returned; change it to a block body so nothing is returned (e.g., (cb) => { cb({ payload }); }) or use a simple for-of loop over eventListeners.get(eventName) to invoke each callback, ensuring you reference the existing emitTauriEvent function and the eventListeners map when making the change.
🧹 Nitpick comments (10)
apps/tauri/src/components/TerminalChat.tsx (1)
113-122: Use the shared shadcn button for the load-more action.This introduces a raw
<button>insideapps/tauri/src/components, so it bypasses the same primitive/variant layer used by the rest of the chat controls.Possible fix
+import { Button } from "@/components/ui/button"; ... - <button - className="text-xs text-muted-foreground/50 hover:text-muted-foreground transition-colors" + <Button + size="sm" + variant="ghost" + className="h-auto px-2 py-1 text-xs text-muted-foreground/50 hover:text-muted-foreground" onClick={() => { prevScrollHeightRef.current = scrollRef.current?.scrollHeight ?? 0; onLoadMore(); }} > {t("chat.loadMore", "Load older messages")} - </button> + </Button>As per coding guidelines,
apps/tauri/src/components/**/*.{ts,tsx}: Use shadcn/ui for all UI primitives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/TerminalChat.tsx` around lines 113 - 122, Replace the raw <button> in TerminalChat.tsx with the shared shadcn Button component: keep the existing condition (!isLoadingMore && hasMore), preserve the onClick handler that sets prevScrollHeightRef.current and calls onLoadMore(), and render the same localized label t("chat.loadMore", "Load older messages") as the Button's children; apply the equivalent visual variant (e.g., ghost/secondary or the project-standard small/text variant) instead of the inline className so styling goes through the shared primitive/variant system; finally add the corresponding Button import at the top of the file and remove the raw <button> element.apps/tauri/src/components/AcpSessionControls.tsx (3)
215-218: Extract duplicated item-mapping logic into a shared helper.The same mapping logic for
option.optionsappears in bothConfigOptionDropdown(lines 161-164) andAdvancedDropdown(lines 215-218). Consider extracting it to reduce duplication and ensure consistent behavior.♻️ Proposed helper extraction
function mapConfigItems(options: AcpSessionConfigOption["options"]): Array<{ id: string; label: string }> { return (options ?? []) .map((o) => { if (typeof o === "string") return { id: o, label: o }; const id = o.id ?? o.value ?? ""; return { id, label: o.label ?? o.name ?? id }; }) .filter((item) => item.id !== ""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/AcpSessionControls.tsx` around lines 215 - 218, Duplicate option-to-item mapping appears in ConfigOptionDropdown and AdvancedDropdown; extract this logic into a single helper (e.g., mapConfigItems) that accepts AcpSessionConfigOption["options"] and returns Array<{id:string,label:string}> by normalizing strings and objects (use id = o.id ?? o.value ?? "", label = o.label ?? o.name ?? id) and filter out empty ids, then replace the inline mapping in both ConfigOptionDropdown and AdvancedDropdown to call this helper to ensure consistent behavior.
221-241: Nested dropdown menus may cause UX accessibility issues.The
AdvancedDropdownrenders a nestedDropdownMenuinside another dropdown's content. This pattern can be problematic for keyboard navigation and screen readers, and may behave inconsistently across platforms.Consider using a flat list with grouped sections, or a submenu pattern that the shadcn/ui library explicitly supports, if available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/AcpSessionControls.tsx` around lines 221 - 241, The nested DropdownMenu usage in AcpSessionControls (the DropdownMenu > DropdownMenuTrigger/DropdownMenuContent block that renders per opt) creates inaccessible nested menus; refactor this to a flat grouped menu or use the shadcn/ui submenu component instead of nesting DropdownMenu instances. Locate the block using DropdownMenu, DropdownMenuTrigger, DropdownMenuContent, and DropdownMenuItem and replace the per-opt nested DropdownMenu with either a single DropdownMenu that renders grouped sections (e.g., group items by opt and render headers) or the library's supported Submenu component so keyboard and screen-reader navigation is preserved.
161-164: Guard against emptyidproducing invalid keys or silent mismatches.The fallback chain
o.id ?? o.value ?? ""can produce an empty string, which would cause React key collisions if multiple options lack bothidandvalue. Additionally, empty IDs passed toonSelectwould be meaningless.♻️ Proposed fix: filter or skip invalid items
function ConfigOptionDropdown({ label, option, selectedValue, onSelect }: ConfigOptionDropdownProps) { const items = (option.options ?? []).map((o) => { if (typeof o === "string") return { id: o, label: o }; - return { id: o.id ?? o.value ?? "", label: o.label ?? o.name ?? o.id ?? o.value ?? "" }; - }); + const id = o.id ?? o.value ?? ""; + const label = o.label ?? o.name ?? id; + return { id, label }; + }).filter((item) => item.id !== "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/AcpSessionControls.tsx` around lines 161 - 164, The items mapping can produce empty string ids which cause React key collisions and meaningless onSelect values; update the transformation for option.options in AcpSessionControls.tsx (the items = ... map) to ensure every item has a non-empty id by either skipping entries with no id/value or generating a stable fallback (e.g., using label + index) and then filter out any items with falsy id before returning; ensure onSelect is only called with these validated non-empty ids so downstream handlers receive meaningful keys.apps/tauri/src/hooks/usePlanWorkflow.ts (2)
84-103: UnusedworkspaceIdinstartPlanningdependency array.
workspaceIdis listed in the dependency array (line 102) but isn't used in the callback body. Either remove it from dependencies or verify it's intentionally included for invalidation purposes.♻️ Suggested fix
}, - [workspaceId, localSessionId] + [localSessionId] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/hooks/usePlanWorkflow.ts` around lines 84 - 103, The startPlanning useCallback lists workspaceId in its dependency array but workspaceId is not referenced in the callback body; update the dependency array for startPlanning to remove workspaceId so it only includes actual dependencies (e.g. localSessionId, sendPromptRef.current callers or functions), ensuring references like startPlanning, sendPromptRef, setModeRef, availableModesRef, onAutoSwitchModeRef and localSessionId remain correct and stable; if workspaceId was intentionally added for invalidation, replace it with an explicit comment explaining that intent instead of leaving an unused dependency.
105-130: Multiple unused dependencies inapprovePlan.
workspaceId,acpSessionId, andactiveSessionIdare in the dependency array but onlyactiveSessionIdRef.currentis used (via ref). The callback uses refs for stable access, making these direct dependencies unnecessary.♻️ Suggested fix
}, - [workspaceId, acpSessionId, activeSessionId, localSessionId] + [localSessionId] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/hooks/usePlanWorkflow.ts` around lines 105 - 130, The approvePlan callback lists unused direct dependencies (workspaceId, acpSessionId, activeSessionId) even though the function reads from refs (activeSessionIdRef.current, setModeRef.current, availableModesRef.current, onAutoSwitchModeRef.current, sendPromptRef.current); remove those unused variables from the dependency array and keep only the real reactive value(s) used (e.g., localSessionId) so the array becomes something like [localSessionId]; leave refs out of the dependencies since their .current access is stable.apps/tauri/src-tauri/src/comments.rs (1)
31-36:unwrap()on system time could panic in edge cases.While extremely rare, if the system clock is set before the UNIX epoch,
duration_since(UNIX_EPOCH)returnsErrand this will panic. Consider usingunwrap_or_default()or explicit handling for robustness.♻️ Defensive alternative
fn now_epoch() -> i64 { std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) - .unwrap() + .unwrap_or_default() .as_secs() as i64 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src-tauri/src/comments.rs` around lines 31 - 36, The now_epoch function uses duration_since(...).unwrap() which can panic if the system clock is before the UNIX epoch; change now_epoch to handle the Err case instead of unwrapping (e.g., map the Ok to seconds and use unwrap_or(0) or match the Result and return a safe fallback) so that now_epoch() always returns an i64 without panicking; update the implementation referenced by the now_epoch function to explicitly handle std::time::SystemTime::duration_since(std::time::UNIX_EPOCH) errors and return a defined fallback value.apps/tauri/src/components/MarkdownViewer.tsx (1)
357-358: Usecn()for the conditional container classes.This new class merge is using a template string. Switching it to
cn()keeps Tailwind composition consistent with the rest of the app.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` around lines 357 - 358, Replace the template-string className in the MarkdownViewer component with the cn() utility: import cn from "lib/utils" if not already imported, then change the container's className to use cn("flex flex-row", isEmbedded ? "h-full" : "flex-1 min-h-0") so Tailwind class merging is consistent with the rest of the app and avoids manual string templates; ensure you update the JSX in the MarkdownViewer function where the div currently uses the template literal.apps/tauri/src/hooks/useSessionConnection.ts (1)
2-2: Use the repo-standard Tauri invoke entrypoint.This hook imports
invokefrom@tauri-apps/api/core; the frontend standard here iswindow.__TAURI__.core.invoke(...), which keeps the runtime path and test mocks consistent.As per coding guidelines, "Use
window.__TAURI__.core.invoke()for calling Rust backend functions (withGlobalTauri: truein config)".Also applies to: 24-25, 55-61, 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/hooks/useSessionConnection.ts` at line 2, The hook useSessionConnection.ts currently imports and calls invoke from "@tauri-apps/api/core"; change all calls to use the repo-standard global entrypoint window.__TAURI__.core.invoke(...) instead (remove the import of invoke and replace usages inside the useSessionConnection hook where invoke(...) is called, including the calls around the session connect/close/fetch logic). Ensure you reference window.__TAURI__.core.invoke in place of invoke so runtime path and test mocks (withGlobalTauri: true) remain consistent and update any local variables or wrappers accordingly.apps/tauri/src/hooks/useSessionMessages.ts (1)
2-2: Use the repo-standard Tauri invoke entrypoint.This hook imports
invokefrom@tauri-apps/api/core; the frontend standard here iswindow.__TAURI__.core.invoke(...), which keeps the runtime path and test mocks consistent.As per coding guidelines, "Use
window.__TAURI__.core.invoke()for calling Rust backend functions (withGlobalTauri: truein config)".Also applies to: 53-57, 124-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/hooks/useSessionMessages.ts` at line 2, The file imports invoke from "@tauri-apps/api/core" instead of using the repo-standard global; update the useSessionMessages hook to call window.__TAURI__.core.invoke(...) wherever invoke is currently imported or used (replace the import and all usages at the locations around the top import and the usage blocks near the sections referenced like the code around lines 53-57 and 124-128), ensuring tests and runtime use the global Tauri entrypoint and removing the direct import of invoke.
🤖 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/acp/commands.rs`:
- Around line 553-571: The current acp_session_send_prompt flow logs full user
prompt and assistant result to stderr via the eprintln! calls; remove those
detailed prints (the eprintln! lines that include text and the serde_json
result) and instead log only non-sensitive metadata such as session_id, message
length, and duration; update the logging around the send_prompt call
(references: the variables session_id, text, conn.send_prompt, and the result)
to record size/timing (e.g., text.len() and result size or success) or a hash/ID
rather than the full bodies, and ensure no user content is emitted to stderr.
- Around line 395-406: The eviction block currently removes the first-inserted
key via instances.keys().next(), which implements FIFO not LRU and also neglects
to remove the session from store.configs; change the logic in the cap
enforcement (the block that checks instances.len() >= store.max_instances) to
scan instances for the entry with the oldest last_activity timestamp (use the
Instance struct's last_activity field), select that key as evict_key, call
instances.shift_remove(&evict_key) as before, then also remove its config via
store.configs.remove(&evict_key), call emit_session_status(&app_handle,
&evict_key, "disconnected"), log the eviction, and finally call
old.connection.shutdown().await to mirror the disconnect handlers.
- Around line 379-387: You are holding store.instances lock across await calls
(e.g., when calling inst.connection.is_alive() and inst.connection.shutdown())
which can block other sessions; instead, inside the locked block locate the
entry by session_id, clone or Arc::clone the AcpConnection/instance (reference
the symbols instances, inst, inst.connection, inst.acp_session_id), then drop
the lock (exit scope) and perform the async is_alive()/shutdown() awaits on the
cloned connection; apply the same pattern for all occurrences flagged (around
the blocks that call is_alive() and shutdown()), leaving acp_session_send_prompt
as-is.
- Around line 434-450: After calling conn.set_suppress_updates(true) before the
session/load request, ensure you immediately re-enable updates once the load
succeeds: after the send_request("session/load", ...) completes and the
SessionInfo (variable info) is parsed and adjusted, call
conn.set_suppress_updates(false) (or the existing method to unset suppression)
prior to returning info so the suppression window is not left open until
send_prompt(); update the block handling
LoadSessionParams/send_request/serde_json::from_value to re-enable updates on
success.
In `@apps/tauri/src-tauri/src/acp/connection.rs`:
- Around line 42-61: The flush_buffer function is saving plain assistant buffer
text with message_type "assistant", causing plain replies to be persisted as
typed messages; change flush_buffer so that when msg_type (from streaming_type)
is the internal marker "assistant" (or otherwise denotes an untyped buffer) you
pass None for message_type to save_to_db instead of that string—i.e., compute
let msg_type = streaming_type.take().filter(|t| t != "assistant").as_deref() (or
equivalent) before calling save_to_db so only real typed messages set
message_type, leaving regular assistant replies null.
In `@apps/tauri/src-tauri/src/lib.rs`:
- Around line 784-785: Tear down AcpSessionStore on app exit by invoking its
shutdown/cleanup method alongside AcpState in the explicit-quit path: locate
where AcpState is disconnected on exit and add a call to
AcpSessionStore::disconnect_all_sessions (or the appropriate method on
acp::commands::AcpSessionStore) so session-scoped ACP processes created via
acp_session_* are also gracefully shut down; ensure you reference the managed
instance (AcpSessionStore) you registered with .manage(...) and call its cleanup
before app termination.
In `@apps/tauri/src/components/ActiveSessionView.tsx`:
- Around line 128-142: persistPreferences currently reads
session.acp_preferences_json then merges and fire-and-forgets two invokes
(session_update_acp_preferences and workspace_acp_defaults_set), causing lost
updates on concurrent writes; change persistPreferences to fetch the latest
prefs from the backend or use an atomic update RPC rather than merging local
JSON, await the invoke results and handle failures (retry or abort) instead of
.catch(console.error), and ensure the restore-defaults logic actually applies
prefs.modeId when computing restored defaults (apply and persist the modeId via
the same atomic update path) so defaults are not skipped on startup; refer to
persistPreferences, session.acp_preferences_json,
session_update_acp_preferences, workspace_acp_defaults_set and the
restore-defaults code that reads prefs.modeId to implement this.
- Around line 183-189: The handler currently deletes DB rows then re-sends
"/clear" via invoke("acp_session_send_prompt", { sessionId: session.id, text:
"/clear" }), which the backend now persists and thus re-populates history;
remove that invoke and only call invoke("messages_delete_session", { sessionId:
session.id }) and sessionMessages.clearMessages() (or replace the persistent
send with a backend API that executes a non-persisted control command if one
exists) so no new "/clear" user message is inserted after deletion.
In `@apps/tauri/src/components/TerminalChat.tsx`:
- Around line 73-82: The effect that preserves scroll position in TerminalChat
currently only clears prevScrollHeightRef.current when diff > 0; update it so
the saved scroll baseline is always cleared at the end of the load cycle
(regardless of whether diff > 0 or onLoadMore returned items). Locate the
useEffect that reads scrollRef.current and prevScrollHeightRef.current and
ensure you set prevScrollHeightRef.current = 0 unconditionally after computing
diff (or in a finally-style path) so stale deltas cannot apply to future
unrelated height changes; keep the existing logic that adjusts el.scrollTop when
diff > 0 but always reset the ref afterward.
In `@apps/tauri/src/contexts/AppContext.tsx`:
- Around line 59-64: Restore a localStorage-backed mirror and storage-event
syncing around the current SQLite fetch: on mount, first read and parse the
saved workspaces and persistedWorkspaceId from localStorage and call
setWorkspaces and setPersistedWorkspaceId (so cold boot and cross-window sync
work), then still call invoke('workspace_list') and map with recordToWorkspace
to reconcile/update state and overwrite localStorage; add a
window.addEventListener('storage') handler to update
setWorkspaces/setPersistedWorkspaceId when other windows change storage, and
ensure every local change to workspaces (the state updated via setWorkspaces)
serializes the latest workspace array and persistedWorkspaceId into localStorage
so both persistence and cross-window sync remain intact.
In `@apps/tauri/src/hooks/useAcpConnection.ts`:
- Around line 146-156: The optimistic disconnect in the disconnect callback only
flips connectedRef and React state, but does not clear the refs that feed
connectionStore, so stale statusRef/errorRef can be persisted and resurrected;
update the disconnect function (disconnect) to also clear statusRef.current (set
to "idle" or equivalent) and errorRef.current (set to undefined/null) and ensure
any connectionStore update mechanism that reads those refs will see the cleared
values so the cleanup effect won't persist a stale error/status after a manual
disconnect.
In `@apps/tauri/src/hooks/useComments.ts`:
- Around line 28-31: The load_comments path is missing workspace scoping: update
the Rust function load_comments (in comments.rs) to accept workspace_id: &str
(matching save_comments), change its DB query to filter by both workspace_id and
file_path (instead of file_path only), and then update the frontend invoke call
invoke("load_comments", { markdownPath: pathRef.current, workspaceId:
workspaceId ?? "" }) in useComments.ts to pass workspaceId so the Rust handler
receives and uses the workspace_id parameter.
In `@apps/tauri/src/hooks/useSessionConnection.ts`:
- Around line 22-26: The effect in useSessionConnection.ts currently maps any
non-"connected" invoke response to "disconnected", which loses a backend
"connecting" state; change the .then handler on invoke("acp_session_status", {
sessionId }) to setStatus to the exact returned state when it is "connected" or
"connecting" (preserve "connecting"), and only fall back to "disconnected" for
any other/unknown values so that setStatus(s) only accepts
"connected"|"connecting" and otherwise "disconnected".
- Around line 30-49: The cleanup race occurs because unlisten may be null when
the listen promise hasn't resolved; change the approach in useEffect to capture
the Promise returned by window.__TAURI__.event.listen(...) (instead of only
assigning its resolved callback to unlisten), store that promise (e.g.,
listenPromise), and in the cleanup call listenPromise.then(fn => fn()) or if
unlisten already set call unlisten(), ensuring the previous listener is always
removed when sessionId changes or the component unmounts; update the variables
referenced (useEffect, unlisten, window.__TAURI__.event.listen and the cleanup
return) accordingly so the listener is reliably cleaned up for the old
sessionId.
In `@apps/tauri/src/hooks/useSessionMessages.ts`:
- Around line 112-117: The effect is clearing in-flight streamed messages on
mount by calling resetSessionMessages(sessionId) immediately after
subscribeSession(sessionId,...); remove that immediate reset so subscribing
preserves cached streamed messages during remounts of ActiveSessionView. If you
still need to clear messages in some cases, invoke resetSessionMessages only on
an explicit “new session” action or when sessionId actually changes (not on
every mount) — update the effect that uses subscribeSession and
resetSessionMessages accordingly.
In `@apps/tauri/src/lib/session-cache.ts`:
- Around line 111-115: The current check that sets agentPlanFilePath uses the
regex /\/plan[^/]*\.md$/i which only matches POSIX slashes and misses Windows
paths like C:\repo\plan.md; update the detection around filePath (in
session-cache.ts where filePath and agentPlanFilePath are used) to normalize or
test for both separators by replacing backslashes with forward slashes before
applying the regex (or use a regex that accepts both / and \\\\ as path
separators, applied to filePath) so plan files with Windows separators correctly
set agentPlanFilePath.
- Around line 161-166: The code currently uses FORMAT_IDLE_MS to flip the
isStreaming session flag after 5s of no chunks, which prematurely marks the turn
finished; instead keep isStreaming governed only by the real streaming lifecycle
(start/stop/60s timeout) and use a separate mechanism to trigger markdown
formatting: preserve FORMAT_IDLE_MS as a formatting-only idle timer and remove
any assignments that set isStreaming = false in the idle callback; introduce a
new boolean or timestamp (e.g., pendingFormat or lastChunkAt) updated in chunk
handlers and have the idle callback call the markdown formatting routine (e.g.,
formatPendingMarkdown) without changing isStreaming; update the places where the
current idle callback is used (the handlers that schedule/clear the
FORMAT_IDLE_MS timer and any code referencing isStreaming in those callbacks) to
use the new formatting-only behavior so slow gaps or long tool calls don't end
the streaming turn.
---
Outside diff comments:
In `@apps/tauri/src-tauri/src/acp/connection.rs`:
- Around line 327-340: The current match on tokio::time::timeout(ping_timeout,
rx).await treats any Ok(_) as a successful ping; change the match to distinguish
Ok(Ok(_)) as the healthy case and treat Ok(Err(_)) and Err(_) as heartbeat
failures: on Ok(Ok(_)) keep the existing behavior (compute latency from start,
reset consecutive_failures, update last_activity, emit HeartbeatEvent with
status "healthy"), but for Ok(Err(_)) and Err(_) increment consecutive_failures,
do not reset last_activity, set latency_ms to None (or omit), and emit a
HeartbeatEvent with status like "unreachable"/"failed" (or trigger disconnect
logic if consecutive_failures exceeds threshold). Update handling around
ping_timeout, rx, consecutive_failures, last_activity, HeartbeatEvent,
app_handle.emit, workspace_id, start, and timestamp accordingly.
In `@apps/tauri/src-tauri/src/sessions.rs`:
- Around line 323-329: The DB row is deleted by delete_session(&conn, &id) but a
missing plan file makes delete_plan return an Err and causes session_delete to
fail; change the code to treat plan-file removal as best-effort by calling
crate::plan_file::delete_plan(&app_data, &id) and handling its Result such that
a "not found" error is ignored (do not propagate) while other errors may be
logged or returned—reference the delete_session and
crate::plan_file::delete_plan calls and adjust the error handling there so
deletion succeeds when the DB row was removed even if the plan file is already
gone.
In `@apps/tauri/src/components/TerminalInput.tsx`:
- Around line 91-99: Textarea is left enabled while streaming but handleKeyDown
prevents Enter regardless, making Enter a no-op; update the logic so Enter only
triggers send when sending is allowed (or disable the Textarea while streaming).
Concretely, either set disabled={disabled || isStreaming} on the Textarea
(referencing the streaming state, e.g., isStreaming) or modify
handleKeyDown/handleSend to early-return only when send is actually allowed
(check the same isStreaming/disabled condition inside handleKeyDown before
calling preventDefault and handleSend). Ensure references to Textarea,
handleKeyDown, handleSend, and the streaming state (e.g., isStreaming) are
updated consistently.
In `@apps/tauri/src/contexts/AppContext.tsx`:
- Around line 179-189: The closeWorkspace handler is currently deleting the
persisted workspace row (invoke('workspace_delete', { id })) which causes
sessions/messages tied to workspace_id to be orphaned; remove the call to
invoke('workspace_delete') from closeWorkspace so closing a card only updates UI
state (setWorkspaces filter) and optional teardown (unwatch_file,
acp_disconnect, clearWorkspaceCaches), and reserve invoke('workspace_delete')
for the explicit "forget" flow; also ensure the reopen path logic reuses an
existing workspace row by looking up the persisted workspace by path and
preserving its id instead of creating a fresh one.
In `@apps/tauri/src/lib/session-cache.ts`:
- Around line 118-126: The current case "tool_call_update" only handles p.status
=== "completed", leaving tool cards stuck at "pending"; update the handler in
session-cache.ts to apply every status transition: find the message by
toolCallId (using p.toolCallId and msgs.findIndex) and always set
msgs[idx].toolStatus = p.status, and if p.rawOutput?.content exists set
msgs[idx].content = that summary; keep existing guard to skip if message not
found. Ensure you still handle undefined rawOutput gracefully and only overwrite
content when present.
---
Minor comments:
In `@apps/tauri/src-tauri/src/comments.rs`:
- Around line 518-530: The code currently falls back to the literal "unknown"
workspace_id when the SELECT query fails, which can violate the
comments->workspaces FK; instead, change the workspace lookup so that if
conn.query_row (the SELECT that assigns workspace_id) returns an error/NoRows
you skip migrating that file (do not call save_comments). Mirror the pattern
used in migrate_v1_to_v2: treat the query result as Option/Result, log or note
the skipped file, and only call save_comments(conn, markdown_path,
&workspace_id, &data) when a real workspace_id was found.
In `@apps/tauri/src-tauri/src/messages.rs`:
- Around line 93-137: update_message_by_tool_call_id performs an UPDATE then
SELECT but never checks the UPDATE row count, so a missing tool_call_id yields a
misleading "Query error after update"; update the function to inspect the usize
returned by conn.execute (the rows affected) for both branches that run the
UPDATE, and if it is 0 return a clear result (either Err("no row found for
session_id/X and tool_call_id/Y") or change the signature to return
Result<Option<MessageRecord>, String> and return Ok(None)); only run the SELECT
when rows_affected > 0 and keep the existing row mapping when present.
In `@apps/tauri/src/__tests__/hooks/useSessionMessages.test.ts`:
- Around line 28-30: The arrow passed to forEach in emitTauriEvent currently
uses an implicit return (cb => cb({ payload })) which causes the callback result
to be returned; change it to a block body so nothing is returned (e.g., (cb) =>
{ cb({ payload }); }) or use a simple for-of loop over
eventListeners.get(eventName) to invoke each callback, ensuring you reference
the existing emitTauriEvent function and the eventListeners map when making the
change.
In `@apps/tauri/src/components/SessionCard.tsx`:
- Around line 44-49: The connection-status dot currently exposes raw enum values
and hardcoded Tailwind classes (see CONN_COLORS in SessionCard) — update the
component to call useTranslation() and map the enum to a translated label for
both the tooltip and aria-label, and replace the hardcoded class values in
CONN_COLORS with semantic SHADCN UI status tokens (e.g., use token class names
for success/warning/disabled/muted instead of literal bg-green-500 /
bg-yellow-400 / bg-muted-foreground/30 / bg-muted-foreground/20) so the
indicator is localized, accessible (aria-label uses the translated text), and
theme-aware; keep the mapping keyed by the same enum values so other references
(e.g., where the dot is rendered) remain unchanged.
In `@apps/tauri/src/lib/utils.ts`:
- Around line 14-16: The catch block in apps/tauri/src/lib/utils.ts currently
calls toast.error("Failed to copy") with a hardcoded English string; replace
that with the i18n-based localized message using i18n.t("common.copyFailed") so
the error toast matches the success toast usage (which uses
i18n.t("common.copied")), and add the "common.copyFailed" key to the locale
files accordingly.
---
Nitpick comments:
In `@apps/tauri/src-tauri/src/comments.rs`:
- Around line 31-36: The now_epoch function uses duration_since(...).unwrap()
which can panic if the system clock is before the UNIX epoch; change now_epoch
to handle the Err case instead of unwrapping (e.g., map the Ok to seconds and
use unwrap_or(0) or match the Result and return a safe fallback) so that
now_epoch() always returns an i64 without panicking; update the implementation
referenced by the now_epoch function to explicitly handle
std::time::SystemTime::duration_since(std::time::UNIX_EPOCH) errors and return a
defined fallback value.
In `@apps/tauri/src/components/AcpSessionControls.tsx`:
- Around line 215-218: Duplicate option-to-item mapping appears in
ConfigOptionDropdown and AdvancedDropdown; extract this logic into a single
helper (e.g., mapConfigItems) that accepts AcpSessionConfigOption["options"] and
returns Array<{id:string,label:string}> by normalizing strings and objects (use
id = o.id ?? o.value ?? "", label = o.label ?? o.name ?? id) and filter out
empty ids, then replace the inline mapping in both ConfigOptionDropdown and
AdvancedDropdown to call this helper to ensure consistent behavior.
- Around line 221-241: The nested DropdownMenu usage in AcpSessionControls (the
DropdownMenu > DropdownMenuTrigger/DropdownMenuContent block that renders per
opt) creates inaccessible nested menus; refactor this to a flat grouped menu or
use the shadcn/ui submenu component instead of nesting DropdownMenu instances.
Locate the block using DropdownMenu, DropdownMenuTrigger, DropdownMenuContent,
and DropdownMenuItem and replace the per-opt nested DropdownMenu with either a
single DropdownMenu that renders grouped sections (e.g., group items by opt and
render headers) or the library's supported Submenu component so keyboard and
screen-reader navigation is preserved.
- Around line 161-164: The items mapping can produce empty string ids which
cause React key collisions and meaningless onSelect values; update the
transformation for option.options in AcpSessionControls.tsx (the items = ...
map) to ensure every item has a non-empty id by either skipping entries with no
id/value or generating a stable fallback (e.g., using label + index) and then
filter out any items with falsy id before returning; ensure onSelect is only
called with these validated non-empty ids so downstream handlers receive
meaningful keys.
In `@apps/tauri/src/components/MarkdownViewer.tsx`:
- Around line 357-358: Replace the template-string className in the
MarkdownViewer component with the cn() utility: import cn from "lib/utils" if
not already imported, then change the container's className to use cn("flex
flex-row", isEmbedded ? "h-full" : "flex-1 min-h-0") so Tailwind class merging
is consistent with the rest of the app and avoids manual string templates;
ensure you update the JSX in the MarkdownViewer function where the div currently
uses the template literal.
In `@apps/tauri/src/components/TerminalChat.tsx`:
- Around line 113-122: Replace the raw <button> in TerminalChat.tsx with the
shared shadcn Button component: keep the existing condition (!isLoadingMore &&
hasMore), preserve the onClick handler that sets prevScrollHeightRef.current and
calls onLoadMore(), and render the same localized label t("chat.loadMore", "Load
older messages") as the Button's children; apply the equivalent visual variant
(e.g., ghost/secondary or the project-standard small/text variant) instead of
the inline className so styling goes through the shared primitive/variant
system; finally add the corresponding Button import at the top of the file and
remove the raw <button> element.
In `@apps/tauri/src/hooks/usePlanWorkflow.ts`:
- Around line 84-103: The startPlanning useCallback lists workspaceId in its
dependency array but workspaceId is not referenced in the callback body; update
the dependency array for startPlanning to remove workspaceId so it only includes
actual dependencies (e.g. localSessionId, sendPromptRef.current callers or
functions), ensuring references like startPlanning, sendPromptRef, setModeRef,
availableModesRef, onAutoSwitchModeRef and localSessionId remain correct and
stable; if workspaceId was intentionally added for invalidation, replace it with
an explicit comment explaining that intent instead of leaving an unused
dependency.
- Around line 105-130: The approvePlan callback lists unused direct dependencies
(workspaceId, acpSessionId, activeSessionId) even though the function reads from
refs (activeSessionIdRef.current, setModeRef.current, availableModesRef.current,
onAutoSwitchModeRef.current, sendPromptRef.current); remove those unused
variables from the dependency array and keep only the real reactive value(s)
used (e.g., localSessionId) so the array becomes something like
[localSessionId]; leave refs out of the dependencies since their .current access
is stable.
In `@apps/tauri/src/hooks/useSessionConnection.ts`:
- Line 2: The hook useSessionConnection.ts currently imports and calls invoke
from "@tauri-apps/api/core"; change all calls to use the repo-standard global
entrypoint window.__TAURI__.core.invoke(...) instead (remove the import of
invoke and replace usages inside the useSessionConnection hook where invoke(...)
is called, including the calls around the session connect/close/fetch logic).
Ensure you reference window.__TAURI__.core.invoke in place of invoke so runtime
path and test mocks (withGlobalTauri: true) remain consistent and update any
local variables or wrappers accordingly.
In `@apps/tauri/src/hooks/useSessionMessages.ts`:
- Line 2: The file imports invoke from "@tauri-apps/api/core" instead of using
the repo-standard global; update the useSessionMessages hook to call
window.__TAURI__.core.invoke(...) wherever invoke is currently imported or used
(replace the import and all usages at the locations around the top import and
the usage blocks near the sections referenced like the code around lines 53-57
and 124-128), ensuring tests and runtime use the global Tauri entrypoint and
removing the direct import of invoke.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9f0b542-c011-4688-a0e3-33a0ed3cc4ad
⛔ Files ignored due to path filters (1)
apps/tauri/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.ai/mcp/mcp.jsonapps/tauri/src-tauri/Cargo.tomlapps/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/cli_installer.rsapps/tauri/src-tauri/src/comments.rsapps/tauri/src-tauri/src/history.rsapps/tauri/src-tauri/src/lib.rsapps/tauri/src-tauri/src/messages.rsapps/tauri/src-tauri/src/sessions.rsapps/tauri/src/__tests__/hooks/useSessionConnection.test.tsapps/tauri/src/__tests__/hooks/useSessionMessages.test.tsapps/tauri/src/__tests__/setup.tsapps/tauri/src/components/AcpSessionControls.tsxapps/tauri/src/components/ActiveSessionView.tsxapps/tauri/src/components/ConnectionLogs.tsxapps/tauri/src/components/DirectoryWorkspace.tsxapps/tauri/src/components/MarkdownViewer.tsxapps/tauri/src/components/SessionCard.tsxapps/tauri/src/components/TerminalChat.tsxapps/tauri/src/components/TerminalInput.tsxapps/tauri/src/components/TerminalMessage.tsxapps/tauri/src/components/settings/DiagnosticsSettings.tsxapps/tauri/src/contexts/AppContext.tsxapps/tauri/src/hooks/useAcpConnection.tsapps/tauri/src/hooks/useAcpSession.tsapps/tauri/src/hooks/useComments.tsapps/tauri/src/hooks/useLocalSessions.tsapps/tauri/src/hooks/usePlanWorkflow.tsapps/tauri/src/hooks/useSessionConnection.tsapps/tauri/src/hooks/useSessionMessages.tsapps/tauri/src/lib/session-cache.tsapps/tauri/src/lib/utils.tsapps/tauri/src/locales/en.jsonapps/tauri/src/locales/pt-BR.jsonapps/tauri/src/types/acp.tsapps/tauri/src/types/index.tsdocs/acp-choreography.md
💤 Files with no reviewable changes (1)
- apps/tauri/src-tauri/src/history.rs
Comment on lines
+553
to
+571
| eprintln!("[acp] acp_session_send_prompt: session={} text={:.60}", session_id, text); | ||
|
|
||
| // Extract what we need and release the lock BEFORE the long-running send_prompt | ||
| let (conn, acp_id) = { | ||
| let instances = store.instances.lock().await; | ||
| let inst = instances.get(&session_id).ok_or("Session not connected")?; | ||
| *inst.last_activity.lock().await = Instant::now(); | ||
| (Arc::clone(&inst.connection), inst.acp_session_id.clone()) | ||
| }; | ||
|
|
||
| // Emit optimistic status | ||
| emit_session_status(&app_handle, &session_id, "streaming"); | ||
|
|
||
| // send_prompt saves the user message to DB then forwards to copilot | ||
| let result = conn | ||
| .send_prompt(acp_id, text, std::time::Duration::from_secs(600)) | ||
| .await?; | ||
|
|
||
| eprintln!("[acp] session={} prompt result: {}", session_id, serde_json::to_string(&result).unwrap_or_default().chars().take(500).collect::<String>()); |
There was a problem hiding this comment.
Remove prompt/result bodies from backend logs.
This writes user prompts and assistant output into stderr. For real workspaces that is a privacy/compliance leak; log session IDs, sizes, or timing instead.
🔧 Suggested fix
- eprintln!("[acp] acp_session_send_prompt: session={} text={:.60}", session_id, text);
+ eprintln!("[acp] acp_session_send_prompt: session={} prompt_len={}", session_id, text.len());
@@
- eprintln!("[acp] session={} prompt result: {}", session_id, serde_json::to_string(&result).unwrap_or_default().chars().take(500).collect::<String>());
+ eprintln!("[acp] session={} prompt completed", session_id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| eprintln!("[acp] acp_session_send_prompt: session={} text={:.60}", session_id, text); | |
| // Extract what we need and release the lock BEFORE the long-running send_prompt | |
| let (conn, acp_id) = { | |
| let instances = store.instances.lock().await; | |
| let inst = instances.get(&session_id).ok_or("Session not connected")?; | |
| *inst.last_activity.lock().await = Instant::now(); | |
| (Arc::clone(&inst.connection), inst.acp_session_id.clone()) | |
| }; | |
| // Emit optimistic status | |
| emit_session_status(&app_handle, &session_id, "streaming"); | |
| // send_prompt saves the user message to DB then forwards to copilot | |
| let result = conn | |
| .send_prompt(acp_id, text, std::time::Duration::from_secs(600)) | |
| .await?; | |
| eprintln!("[acp] session={} prompt result: {}", session_id, serde_json::to_string(&result).unwrap_or_default().chars().take(500).collect::<String>()); | |
| eprintln!("[acp] acp_session_send_prompt: session={} prompt_len={}", session_id, text.len()); | |
| // Extract what we need and release the lock BEFORE the long-running send_prompt | |
| let (conn, acp_id) = { | |
| let instances = store.instances.lock().await; | |
| let inst = instances.get(&session_id).ok_or("Session not connected")?; | |
| *inst.last_activity.lock().await = Instant::now(); | |
| (Arc::clone(&inst.connection), inst.acp_session_id.clone()) | |
| }; | |
| // Emit optimistic status | |
| emit_session_status(&app_handle, &session_id, "streaming"); | |
| // send_prompt saves the user message to DB then forwards to copilot | |
| let result = conn | |
| .send_prompt(acp_id, text, std::time::Duration::from_secs(600)) | |
| .await?; | |
| eprintln!("[acp] session={} prompt completed", session_id); |
🤖 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 553 - 571, The current
acp_session_send_prompt flow logs full user prompt and assistant result to
stderr via the eprintln! calls; remove those detailed prints (the eprintln!
lines that include text and the serde_json result) and instead log only
non-sensitive metadata such as session_id, message length, and duration; update
the logging around the send_prompt call (references: the variables session_id,
text, conn.send_prompt, and the result) to record size/timing (e.g., text.len()
and result size or success) or a hash/ID rather than the full bodies, and ensure
no user content is emitted to stderr.
Contributor
Author
There was a problem hiding this comment.
Won't fix — this is debug logging via eprintln\! (stderr), which is only visible when running from a terminal during development. In production builds of a desktop app, stderr is not captured or persisted. The truncation ({:.60} and .take(500)) already limits exposure. Removing these would hurt debuggability without meaningful privacy benefit.
There was a problem hiding this comment.
@wilcorrea, understood — that's a fair distinction. For a Tauri desktop app, stderr isn't persisted or surfaced to end users in production, and the existing truncation bounds ({:.60} / .take(500)) do limit the worst-case exposure during development sessions. Happy to defer to your judgment here.
🧠 Learnings used
Learnt from: CR
Repo: devitools/arandu PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-02T05:28:33.736Z
Learning: Applies to apps/tauri/src-tauri/src/acp/**/*.rs : ACP integration must use JSON-RPC 2.0 over stdin/stdout for communication with the Copilot binary
Learnt from: wilcorrea
Repo: devitools/arandu PR: 43
File: apps/tauri/src-tauri/src/sessions.rs:329-331
Timestamp: 2026-03-05T02:50:48.745Z
Learning: In Rust/Tauri backend code, delete_plan may return Err when the plan file does not exist. Therefore, patterns like let _ = crate::plan_file::delete_plan(&app_data, id) inside loops (e.g., forget_workspace_data) are intentional best-effort deletions and should not be flagged as swallowing errors. Do not flag this as an error; using ? would abort on the first missing file. When reviewing, ensure this behavior is intentional by verifying that delete_plan returns Err only for non-existent files and that the code relies on best-effort cleanup in iterations.
Comment on lines
+22
to
+26
| useEffect(() => { | ||
| if (!sessionId) return; | ||
| invoke<string>("acp_session_status", { sessionId }) | ||
| .then((s) => setStatus(s === "connected" ? "connected" : "disconnected")) | ||
| .catch(() => setStatus("disconnected")); |
There was a problem hiding this comment.
Preserve "connecting" when hydrating status.
Line 25 maps every non-"connected" response to "disconnected". If the backend reports "connecting" during restore/reconnect, the hook briefly flips to the wrong state and re-enables the connect action.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/tauri/src/hooks/useSessionConnection.ts` around lines 22 - 26, The
effect in useSessionConnection.ts currently maps any non-"connected" invoke
response to "disconnected", which loses a backend "connecting" state; change the
.then handler on invoke("acp_session_status", { sessionId }) to setStatus to the
exact returned state when it is "connected" or "connecting" (preserve
"connecting"), and only fall back to "disconnected" for any other/unknown values
so that setStatus(s) only accepts "connected"|"connecting" and otherwise
"disconnected".
Contributor
Author
There was a problem hiding this comment.
Won't fix — the backend acp_session_status command only returns "connected" or "disconnected". There is no "connecting" state from the backend; that status is purely frontend-driven (set in the connect() callback before the invoke). The mapping is correct.
There was a problem hiding this comment.
Rate Limit Exceeded
@wilcorrea have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 44 seconds before sending another message.
Comment on lines
+112
to
+117
| useEffect(() => { | ||
| const unsub = subscribeSession(sessionId, (entry) => { | ||
| setStreamingMessages(entry.messages); | ||
| }); | ||
| resetSessionMessages(sessionId); | ||
| return () => { unsub(); }; |
There was a problem hiding this comment.
Don't clear streamed session-cache messages on mount.
Line 116 runs resetSessionMessages(sessionId) immediately after subscribeSession(), so remounting ActiveSessionView during an in-flight turn drops cached thinking/tool/text messages before end_turn persists them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/tauri/src/hooks/useSessionMessages.ts` around lines 112 - 117, The
effect is clearing in-flight streamed messages on mount by calling
resetSessionMessages(sessionId) immediately after
subscribeSession(sessionId,...); remove that immediate reset so subscribing
preserves cached streamed messages during remounts of ActiveSessionView. If you
still need to clear messages in some cases, invoke resetSessionMessages only on
an explicit “new session” action or when sessionId actually changes (not on
every mount) — update the effect that uses subscribeSession and
resetSessionMessages accordingly.
Contributor
Author
There was a problem hiding this comment.
Won't fix — resetSessionMessages is already guarded by entry.messages.length > 0 (only clears if there are cached messages). The remount-during-streaming scenario is extremely rare in practice (requires navigating away and back during an active turn), and the streamed messages are reconstructed from new incoming chunks immediately. The brief gap is imperceptible.
There was a problem hiding this comment.
Rate Limit Exceeded
@wilcorrea have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 43 seconds before sending another message.
Comment on lines
+161
to
+166
| /** | ||
| * Idle time (ms) after the last chunk before auto-formatting markdown. | ||
| * Adjust this value to control how quickly streaming text switches | ||
| * from plain text to rendered markdown when no new chunks arrive. | ||
| */ | ||
| const FORMAT_IDLE_MS = 5_000; |
There was a problem hiding this comment.
Don't use isStreaming as the markdown-idle timer.
After 5 seconds of silence this code marks the session non-streaming even though the ACP turn may still be active. Slow chunk gaps or long tool calls will look finished mid-turn, and the 60-second streaming timeout becomes effectively dead for any gap over 5 seconds.
Also applies to: 191-201, 235-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/tauri/src/lib/session-cache.ts` around lines 161 - 166, The code
currently uses FORMAT_IDLE_MS to flip the isStreaming session flag after 5s of
no chunks, which prematurely marks the turn finished; instead keep isStreaming
governed only by the real streaming lifecycle (start/stop/60s timeout) and use a
separate mechanism to trigger markdown formatting: preserve FORMAT_IDLE_MS as a
formatting-only idle timer and remove any assignments that set isStreaming =
false in the idle callback; introduce a new boolean or timestamp (e.g.,
pendingFormat or lastChunkAt) updated in chunk handlers and have the idle
callback call the markdown formatting routine (e.g., formatPendingMarkdown)
without changing isStreaming; update the places where the current idle callback
is used (the handlers that schedule/clear the FORMAT_IDLE_MS timer and any code
referencing isStreaming in those callbacks) to use the new formatting-only
behavior so slow gaps or long tool calls don't end the streaming turn.
Contributor
Author
There was a problem hiding this comment.
Won't fix — this is intentional UX behavior. The FORMAT_IDLE_MS (5s) timer controls when streaming markdown switches from plain text to rendered output during pauses. It's a rendering optimization, not a session lifecycle marker. When new chunks arrive, isStreaming goes back to true and rendering switches back to plain text mode. The 60s STREAMING_TIMEOUT_MS serves as the hard safety timeout for truly stuck sessions.
There was a problem hiding this comment.
Rate Limit Exceeded
@wilcorrea have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 37 seconds before sending another message.
- Release lock before awaiting is_alive/shutdown in session commands - Clean up store.configs on session eviction - Re-enable suppress_updates after session/load completes - Don't persist "assistant" as message_type (use null for plain text) - Tear down AcpSessionStore on app exit - Skip saving slash commands (e.g. /clear) as user messages - Always reset scroll baseline ref in TerminalChat - Fix race condition in useSessionConnection listener cleanup - Handle Windows path separators in plan file detection
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/tauri/src/components/TerminalChat.tsx (1)
114-122: Use shadcn Button component instead of native button.The native
<button>element should be replaced with the shadcnButtoncomponent for consistency with UI primitives.Suggested fix
+import { Button } from "@/components/ui/button";{!isLoadingMore && hasMore && ( - <button - className="text-xs text-muted-foreground/50 hover:text-muted-foreground transition-colors" + <Button + variant="ghost" + size="sm" + className="text-xs text-muted-foreground/50 hover:text-muted-foreground h-auto py-1" onClick={() => { prevScrollHeightRef.current = scrollRef.current?.scrollHeight ?? 0; onLoadMore(); }} > {t("chat.loadMore", "Load older messages")} - </button> + </Button> )}As per coding guidelines: "Use shadcn/ui for all UI primitives."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src/components/TerminalChat.tsx` around lines 114 - 122, Replace the native <button> in TerminalChat.tsx with the shadcn Button component to follow UI primitives; locate the button that uses prevScrollHeightRef.current, scrollRef.current?.scrollHeight and calls onLoadMore() (the "Load older messages" control) and swap it to use the imported Button component while preserving the onClick handler and className/appearance props (map existing classes to Button variant/size or className as appropriate) and ensure the Button import from "ui" (or the project's shadcn entry) is added at the top of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/tauri/src/components/TerminalChat.tsx`:
- Around line 114-122: Replace the native <button> in TerminalChat.tsx with the
shadcn Button component to follow UI primitives; locate the button that uses
prevScrollHeightRef.current, scrollRef.current?.scrollHeight and calls
onLoadMore() (the "Load older messages" control) and swap it to use the imported
Button component while preserving the onClick handler and className/appearance
props (map existing classes to Button variant/size or className as appropriate)
and ensure the Button import from "ui" (or the project's shadcn entry) is added
at the top of the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4c3b97a-ece4-4143-9731-bd8b2ac48ace
📒 Files selected for processing (6)
apps/tauri/src-tauri/src/acp/commands.rsapps/tauri/src-tauri/src/acp/connection.rsapps/tauri/src-tauri/src/lib.rsapps/tauri/src/components/TerminalChat.tsxapps/tauri/src/hooks/useSessionConnection.tsapps/tauri/src/lib/session-cache.ts
Contributor
Author
Code reviewFound 1 issue:
arandu/apps/tauri/src-tauri/src/comments.rs Lines 244 to 254 in bc43999 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Users upgrading from v0.12.0 may not have the `messages` table. The migration now checks with `has_table` before attempting to migrate, creating the table from scratch when it does not exist.
- AcpSessionControls: show refresh button when info unavailable - ActiveSessionView: add reconnect button for existing sessions - Move controls outside `isConnected` guard for visibility - Skip auto-init for sessions with existing `acp_session_id`
- New `useBlockDragSelect` hook for lasso-style block selection - Comment badges on blocks with count indicator - Merge adjacent selected/highlighted blocks visually - Inline task-list checkbox paragraph fix
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/tauri/src-tauri/src/comments.rs (2)
44-47: SQL injection risk inhas_column- safe only with trusted inputs.The
format!macro directly interpolatestableandcolumninto the SQL string without parameterization. This is safe only because all current call sites use hardcoded string literals. If this function is ever exposed to user-controlled input, it becomes vulnerable.Consider adding a comment documenting this constraint, or validating that inputs match
^[a-zA-Z_][a-zA-Z0-9_]*$.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src-tauri/src/comments.rs` around lines 44 - 47, The has_column function currently builds SQL via format! with table and column, which is unsafe for untrusted input; update has_column to either validate that both table and column match the identifier regex (e.g. ^[A-Za-z_][A-Za-z0-9_]*$) before calling conn.prepare, or add a clear doc-comment above has_column stating it only accepts trusted/hardcoded identifiers; reference the has_column function and the call to conn.prepare when making the change so reviewers can see the validation or documentation next to the SQL construction.
395-413: Consider optimizing the N+1 query pattern.The current implementation executes a separate query for each comment to fetch its block_ids, and also prepares the statement inside the loop. While this works correctly and is acceptable for typical low comment counts, it could be optimized:
♻️ Proposed optimization
let mut result = Vec::with_capacity(comments.len()); + let mut block_stmt = conn + .prepare("SELECT block_id FROM comment_blocks WHERE comment_id = ?1") + .map_err(|e| format!("Prepare block_ids: {}", e))?; for (id, text, created_at, resolved) in comments { - let mut block_stmt = conn - .prepare("SELECT block_id FROM comment_blocks WHERE comment_id = ?1") - .map_err(|e| format!("Prepare block_ids: {}", e))?; let block_ids: Vec<String> = block_stmt .query_map(params![id], |row| row.get(0))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tauri/src-tauri/src/comments.rs` around lines 395 - 413, The loop in comments.rs does an N+1 query by preparing and executing "SELECT block_id FROM comment_blocks WHERE comment_id = ?1" for each comment; fix this by moving the prepare call for the block_id query (the variable block_stmt) outside the for loop or, better, replace the per-comment query with one bulk query that selects comment_id, block_id for all comment ids and then group block_ids by comment_id into the Comment.block_ids before building result; update the code that currently uses block_stmt.query_map(...) and the loop that pushes Comment { id, block_ids, ... } to instead pull block_ids from the pre-fetched map (or use the prepared statement reused for each id) so the N+1 queries are eliminated.
🤖 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/comments.rs`:
- Around line 535-547: The code currently sets workspace_id to "unknown" when
the SELECT in conn.query_row for the workspace fails, which will later cause a
foreign-key error in save_comments; change the logic to detect a missing
workspace instead of using "unknown" — call conn.query_row and handle the Err by
skipping this legacy entry (do not call save_comments) or return/continue as
appropriate for the migration loop; specifically update the workspace_id
assignment and the code path before save_comments(conn, markdown_path,
&workspace_id, &data)? to only invoke save_comments when a valid workspace_id
(from the SELECT via conn.query_row) was returned.
---
Nitpick comments:
In `@apps/tauri/src-tauri/src/comments.rs`:
- Around line 44-47: The has_column function currently builds SQL via format!
with table and column, which is unsafe for untrusted input; update has_column to
either validate that both table and column match the identifier regex (e.g.
^[A-Za-z_][A-Za-z0-9_]*$) before calling conn.prepare, or add a clear
doc-comment above has_column stating it only accepts trusted/hardcoded
identifiers; reference the has_column function and the call to conn.prepare when
making the change so reviewers can see the validation or documentation next to
the SQL construction.
- Around line 395-413: The loop in comments.rs does an N+1 query by preparing
and executing "SELECT block_id FROM comment_blocks WHERE comment_id = ?1" for
each comment; fix this by moving the prepare call for the block_id query (the
variable block_stmt) outside the for loop or, better, replace the per-comment
query with one bulk query that selects comment_id, block_id for all comment ids
and then group block_ids by comment_id into the Comment.block_ids before
building result; update the code that currently uses block_stmt.query_map(...)
and the loop that pushes Comment { id, block_ids, ... } to instead pull
block_ids from the pre-fetched map (or use the prepared statement reused for
each id) so the N+1 queries are eliminated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffc30f7b-25d4-493a-ab69-47b01292f5f2
📒 Files selected for processing (1)
apps/tauri/src-tauri/src/comments.rs
Comment on lines
+535
to
+547
| // Find workspace for this file path | ||
| let workspace_id: String = conn.query_row( | ||
| "SELECT id FROM workspaces WHERE ?1 LIKE path || '/%' OR ?1 = path ORDER BY LENGTH(path) DESC LIMIT 1", | ||
| params![markdown_path], | ||
| |row| row.get(0), | ||
| ).unwrap_or_else(|_| "unknown".to_string()); | ||
|
|
||
| let data = CommentsData { | ||
| file_hash: legacy.file_hash, | ||
| comments: legacy.comments, | ||
| }; | ||
|
|
||
| save_comments(conn, markdown_path, &data)?; | ||
| save_comments(conn, markdown_path, &workspace_id, &data)?; |
There was a problem hiding this comment.
Potential FK constraint violation when workspace not found.
If no workspace matches the file path, workspace_id falls back to "unknown", which will likely cause a foreign key constraint violation when save_comments attempts the insert (since no workspace with id "unknown" exists).
Consider skipping the migration when no workspace is found, consistent with the v1→v2 migration approach:
🐛 Proposed fix
// Find workspace for this file path
- let workspace_id: String = conn.query_row(
+ let workspace_id: Option<String> = conn.query_row(
"SELECT id FROM workspaces WHERE ?1 LIKE path || '/%' OR ?1 = path ORDER BY LENGTH(path) DESC LIMIT 1",
params![markdown_path],
|row| row.get(0),
- ).unwrap_or_else(|_| "unknown".to_string());
+ ).ok();
+
+ let workspace_id = match workspace_id {
+ Some(id) => id,
+ None => {
+ // No matching workspace; skip migration but keep file for later
+ return Ok(false);
+ }
+ };
let data = CommentsData {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/tauri/src-tauri/src/comments.rs` around lines 535 - 547, The code
currently sets workspace_id to "unknown" when the SELECT in conn.query_row for
the workspace fails, which will later cause a foreign-key error in
save_comments; change the logic to detect a missing workspace instead of using
"unknown" — call conn.query_row and handle the Err by skipping this legacy entry
(do not call save_comments) or return/continue as appropriate for the migration
loop; specifically update the workspace_id assignment and the code path before
save_comments(conn, markdown_path, &workspace_id, &data)? to only invoke
save_comments when a valid workspace_id (from the SELECT via conn.query_row) was
returned.
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
suppress_updatesfor batched savesacp:user-message-saved,acp:assistant-message-saved) instead of pollingconfigOptionsacp_preferences_jsoncolumn) and per-workspace (workspace_acp_defaultstable)startsWithpath validation, add multi-location search, auto-locate on mountChanges
Rust Backend
connection.rs: Message persistence, tool call content updates, duplicate detection,suppress_updatesflagmessages.rs: Addupdate_message_by_tool_call_id,is_duplicate_user_messagetypes.rs: Add config option types (SetConfigOptionParams,SessionConfigOption, etc.), changeSessionInfo.config_optionsto raw JSONcommands.rs: Addacp_session_set_config_option, emitsession_info_updatewith full mode objects + config optionssessions.rs: Addacp_preferences_jsoncolumn,workspace_acp_defaultstable, preference CRUD commandscomments.rs: SQLite migration for existing databasesFrontend
useSessionMessages.ts: Event-based message flow rewritesession-cache.ts: Config option fields,addSystemNotice, normalize/read helpers, new event handlersusePlanWorkflow.ts:AcpSessionMode[]objects,onAutoSwitchMode, plan restore fixesAcpSessionControls.tsx: New component — Mode/Model/Agent/Advanced dropdownsActiveSessionView.tsx: Wire controls, preferences persistence, auto-apply, inline error bannersTest plan
cargo checkcompilesnpx tsc --noEmitcompilesnpm testpassesSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation