Skip to content

feat: add project-specific command history filtering and clear functionality#73

Merged
gnoviawan merged 2 commits intodevfrom
feat/project-specific-command-history
Mar 11, 2026
Merged

feat: add project-specific command history filtering and clear functionality#73
gnoviawan merged 2 commits intodevfrom
feat/project-specific-command-history

Conversation

@gnoviawan
Copy link
Owner

@gnoviawan gnoviawan commented Mar 11, 2026

Summary

  • Add project filter dropdown to CommandHistoryModal ("This Project" / "All Projects")
  • Add "Clear History" button with confirmation dialog for current project
  • Disable Clear History button when viewing "All Projects" mode
  • Add loading state during clear operation
  • Add error handling with toast notification for persistence failures
  • Add useAllCommandHistory hook to retrieve all entries from store
  • Add comprehensive tests for filter and clear functionality

Test plan

  • Open command history modal with Ctrl+R
  • Verify "This Project" is default filter selection
  • Run commands in different projects, switch filter to "All Projects"
  • Verify all commands from all projects appear
  • Click "Clear History", verify confirmation dialog appears
  • Confirm clear, verify only current project's history is cleared
  • Verify Clear History button is disabled when viewing "All Projects"
  • Verify Clear History button is disabled when history is empty

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Command history modal: switch between current-project and all-project history; search query persists when switching.
    • Global history view added and wired to the modal.
    • Clear History flow: project-scoped clear with confirmation dialog, loading state, and disabled when viewing all-projects or when history is empty.
    • Confirmation dialogs now support a loading/disabled state.
  • Tests

    • Expanded coverage for project filter behavior, multi-project results, clear-history confirmation/cancel flows, disabled states, and empty/match messaging.

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds an "All Projects" vs "This Project" filter and clear-history flow to CommandHistoryModal, exposes aggregated history via a new useAllCommandHistory hook, and wires an onClearHistory handler and allEntries data through WorkspaceLayout with persistence/error handling.

Changes

Cohort / File(s) Summary
Component: CommandHistoryModal
src/renderer/components/CommandHistoryModal.tsx
Adds props allEntries?: CommandHistoryEntry[] and onClearHistory: () => Promise<void>; introduces filterMode select, derives baseEntries from entries or allEntries, adds Clear History button + ConfirmDialog, and manages clear-state/loading.
Tests: CommandHistoryModal
src/renderer/components/CommandHistoryModal.test.tsx
Provides allEntries in defaults; adds tests for project filter behavior (default/switch/search preservation) and Clear History flow (button states, confirm/cancel, onClearHistory invocation).
Hook: command history
src/renderer/hooks/use-command-history.ts
Exports new useAllCommandHistory() returning aggregated newest-first command history; existing per-project hook unchanged.
Layout: WorkspaceLayout
src/renderer/layouts/WorkspaceLayout.tsx, src/renderer/layouts/WorkspaceLayout*.test.tsx
Uses useAllCommandHistory(), passes allEntries and onClearHistory to modal; implements handleClearCommandHistory to clear project entries, persist via persistence API, and surface errors; tests/mocks updated to expose useAllCommandHistory.
UI/Support: ConfirmDialog & Select
src/renderer/components/ConfirmDialog.tsx, src/renderer/components/ui/select usages
ConfirmDialog gains optional isLoading prop to disable actions and show loading label; Select components wired into modal header for filter mode.

Sequence Diagram

sequenceDiagram
    participant User
    participant Modal as CommandHistoryModal
    participant Layout as WorkspaceLayout
    participant Store as CommandHistoryStore
    participant API as PersistenceAPI

    User->>Modal: open modal / choose filter
    Modal->>Layout: request allEntries (prop) / onSelectCommand
    User->>Modal: click "Clear History"
    Modal->>Modal: show ConfirmDialog
    User->>Modal: confirm
    Modal->>Layout: onClearHistory()
    Layout->>Store: clear project entries
    Layout->>API: persist cleared state
    API-->>Layout: success / error
    Layout-->>Modal: close modal / refresh entries
    Modal->>User: updated history view
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Poem

🐰 I hopped through entries, near and far,

"This" or "All" — I nudge the switch like a star.
A gentle click, a prompt—I tidy the trail,
Old commands vanish as I wag my tail.
Happy hops for fresh starts, soft and hale.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and accurately describes the main changes: adding project-specific command history filtering (the project filter dropdown) and clear functionality (the Clear History button). The title is concise and specific without noise.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/project-specific-command-history

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gnoviawan gnoviawan added phase-1 Phase 1: Stability & Polish (v0.3.1) v0.3.1 labels Mar 11, 2026
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/renderer/layouts/WorkspaceLayout.tsx (1)

767-776: Consider reusing COMMAND_HISTORY_KEY helper for consistency.

The persistence key is hardcoded as a template string, but there's already a COMMAND_HISTORY_KEY helper exported from use-command-history.ts (line 5 of that file) that generates the same key pattern. Reusing it would ensure consistency and avoid duplication.

♻️ Proposed refactor to use existing helper
 import {
   useCommandHistoryLoader,
   useAddCommand,
   useCommandHistory,
-  useAllCommandHistory
+  useAllCommandHistory,
+  COMMAND_HISTORY_KEY
 } from '@/hooks/use-command-history'
 const handleClearCommandHistory = useCallback(async () => {
   if (!activeProjectId) return
   const { clearHistory } = useCommandHistoryStore.getState()
   clearHistory(activeProjectId)
   // Persist the cleared state
-  const result = await persistenceApi.write(`projects/${activeProjectId}/command-history`, [])
+  const result = await persistenceApi.write(COMMAND_HISTORY_KEY(activeProjectId), [])
   if (!result.success) {
     toast.error(`Failed to clear history: ${result.error}`)
   }
 }, [activeProjectId])
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/layouts/WorkspaceLayout.tsx` around lines 767 - 776, The
persistence key is hardcoded in handleClearCommandHistory; replace the template
string passed to persistenceApi.write with the exported COMMAND_HISTORY_KEY
helper from use-command-history.ts to ensure consistency. Update the call in
handleClearCommandHistory to compute the key via
COMMAND_HISTORY_KEY(activeProjectId) (or the helper's correct signature) and
keep the rest of the logic intact (invoking
useCommandHistoryStore.getState().clearHistory(activeProjectId) and handling
result.success/toast.error).
src/renderer/components/CommandHistoryModal.tsx (1)

92-100: Consider adding error handling for failed clear operations.

The handleClearConfirm callback doesn't handle errors from onClearHistory. If the parent's clear operation fails (e.g., persistence error), the dialog closes without feedback to the user, and the UI may be in an inconsistent state where in-memory history is cleared but the error toast appears.

Consider catching and handling errors to provide better UX:

♻️ Proposed improvement for error handling
 const handleClearConfirm = useCallback(async () => {
   setIsClearing(true)
   try {
     await onClearHistory()
     setShowClearConfirm(false)
+  } catch {
+    // Keep dialog open on error so user can retry or cancel
   } finally {
     setIsClearing(false)
   }
 }, [onClearHistory])
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/CommandHistoryModal.tsx` around lines 92 - 100, The
handleClearConfirm callback currently awaits onClearHistory but doesn't catch
errors, causing the confirmation dialog to close on failure and leaving UI
inconsistent; update handleClearConfirm to wrap the await onClearHistory() in
try/catch/finally, move setShowClearConfirm(false) into the try block so the
dialog only closes on success, in the catch call a provided error feedback
mechanism (e.g., show a toast or set an error state) to surface the failure, and
keep setIsClearing(false) in finally to ensure loading state is always cleared;
reference handleClearConfirm, onClearHistory, setShowClearConfirm, and
setIsClearing when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/renderer/layouts/WorkspaceLayout.tsx`:
- Line 301: The tests mock '@/hooks/use-command-history' but are missing the new
hook export used by WorkspaceLayout; update the mocks in
WorkspaceLayout.test.tsx and WorkspaceLayout.close-persistence.test.tsx to add
useAllCommandHistory (e.g., include useAllCommandHistory: vi.fn(() => [])
alongside useCommandHistory, useAddCommand, and useCommandHistoryLoader) so the
component's const allCommandHistory = useAllCommandHistory() call is satisfied.

---

Nitpick comments:
In `@src/renderer/components/CommandHistoryModal.tsx`:
- Around line 92-100: The handleClearConfirm callback currently awaits
onClearHistory but doesn't catch errors, causing the confirmation dialog to
close on failure and leaving UI inconsistent; update handleClearConfirm to wrap
the await onClearHistory() in try/catch/finally, move setShowClearConfirm(false)
into the try block so the dialog only closes on success, in the catch call a
provided error feedback mechanism (e.g., show a toast or set an error state) to
surface the failure, and keep setIsClearing(false) in finally to ensure loading
state is always cleared; reference handleClearConfirm, onClearHistory,
setShowClearConfirm, and setIsClearing when making the change.

In `@src/renderer/layouts/WorkspaceLayout.tsx`:
- Around line 767-776: The persistence key is hardcoded in
handleClearCommandHistory; replace the template string passed to
persistenceApi.write with the exported COMMAND_HISTORY_KEY helper from
use-command-history.ts to ensure consistency. Update the call in
handleClearCommandHistory to compute the key via
COMMAND_HISTORY_KEY(activeProjectId) (or the helper's correct signature) and
keep the rest of the logic intact (invoking
useCommandHistoryStore.getState().clearHistory(activeProjectId) and handling
result.success/toast.error).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8c966192-3397-4cf5-99be-9cbcd5b0286d

📥 Commits

Reviewing files that changed from the base of the PR and between 6de0374 and a3df48c.

📒 Files selected for processing (4)
  • src/renderer/components/CommandHistoryModal.test.tsx
  • src/renderer/components/CommandHistoryModal.tsx
  • src/renderer/hooks/use-command-history.ts
  • src/renderer/layouts/WorkspaceLayout.tsx

@gnoviawan gnoviawan force-pushed the feat/project-specific-command-history branch from a3df48c to 5a506ab Compare March 11, 2026 15:05
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/renderer/components/CommandHistoryModal.tsx`:
- Around line 92-100: The clear handler can be invoked multiple times while a
clear is pending; modify handleClearConfirm to early-return when a clear is
already in progress by checking the isClearing state at the top (e.g. if
(isClearing) return;), keep the existing setIsClearing(true)/try/finally flow,
and include isClearing in the useCallback dependency array so the closure sees
updates; this prevents double submissions even though ConfirmDialog lacks a
loading/disabled confirm state.
- Around line 54-61: The confirm dialog state (showClearConfirm and isClearing)
is not reset when the modal closes; update the useEffect that watches isOpen
(the one that calls setQuery, setSelectedIndex, setFilterMode and focuses
inputRef) to also clear the dialog state by calling setShowClearConfirm(false)
and setIsClearing(false) so the confirm dialog cannot persist or reappear with
stale state when isOpen toggles; ensure you reference the same useEffect that
uses isOpen and inputRef.

In `@src/renderer/hooks/use-command-history.ts`:
- Around line 77-79: useAllCommandHistory currently returns the raw array from
useCommandHistoryStore which preserves grouping order; fix it by returning a
sorted (newest-first) copy so the "All Projects" view shows a true newest-first
history. In useAllCommandHistory, call useCommandHistoryStore((s) => s.entries),
clone the array (e.g., [...entries]) and sort by the timestamp field on
CommandHistoryEntry (e.g., createdAt or similar) in descending order before
returning to avoid mutating the store.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 12a6c999-9a58-4111-88b4-be5dfc37e052

📥 Commits

Reviewing files that changed from the base of the PR and between a3df48c and 5a506ab.

📒 Files selected for processing (6)
  • src/renderer/components/CommandHistoryModal.test.tsx
  • src/renderer/components/CommandHistoryModal.tsx
  • src/renderer/hooks/use-command-history.ts
  • src/renderer/layouts/WorkspaceLayout.close-persistence.test.tsx
  • src/renderer/layouts/WorkspaceLayout.test.tsx
  • src/renderer/layouts/WorkspaceLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/renderer/layouts/WorkspaceLayout.tsx

@gnoviawan gnoviawan force-pushed the feat/project-specific-command-history branch from 5a506ab to a4a31b2 Compare March 11, 2026 20:13
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
src/renderer/components/CommandHistoryModal.tsx (2)

94-102: ⚠️ Potential issue | 🟡 Minor

Single-flight the confirm handler.

ConfirmDialog does not lock the confirm action here, and handleClearConfirm() has no isClearing guard. Rapid clicks will enqueue duplicate clear/persist calls and can duplicate failure toasts.

Suggested fix
   const handleClearConfirm = useCallback(async () => {
+    if (isClearing) return
     setIsClearing(true)
     try {
       await onClearHistory()
       setShowClearConfirm(false)
     } finally {
       setIsClearing(false)
     }
-  }, [onClearHistory])
+  }, [isClearing, onClearHistory])
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/CommandHistoryModal.tsx` around lines 94 - 102,
handleClearConfirm can be invoked multiple times because there is no guard;
modify it to short-circuit when a clear is already in progress by checking the
isClearing state at the start (if (isClearing) return), then proceed to
setIsClearing(true) and clear in the try/finally as before; also include
isClearing in the useCallback dependency array and ensure ConfirmDialog's
confirm button/prop (the component using handleClearConfirm) is disabled/blocked
while isClearing to prevent rapid duplicate invocations (refer to
handleClearConfirm, isClearing, onClearHistory, setIsClearing,
setShowClearConfirm).

54-63: ⚠️ Potential issue | 🟠 Major

Reset the clear-confirmation state when the modal closes.

ConfirmDialog is rendered outside the isOpen branch, but showClearConfirm and isClearing are only reset on open. If the parent closes the history modal while the confirmation is visible, the destructive dialog can stay mounted on its own.

Suggested fix
   useEffect(() => {
-    if (isOpen) {
-      setQuery('')
-      setSelectedIndex(0)
-      setFilterMode('this-project')
-      setShowClearConfirm(false)
-      setIsClearing(false)
-      setTimeout(() => inputRef.current?.focus(), 50)
-    }
+    if (!isOpen) {
+      setShowClearConfirm(false)
+      setIsClearing(false)
+      return
+    }
+
+    setQuery('')
+    setSelectedIndex(0)
+    setFilterMode('this-project')
+    setShowClearConfirm(false)
+    setIsClearing(false)
+
+    const timer = window.setTimeout(() => inputRef.current?.focus(), 50)
+    return () => window.clearTimeout(timer)
   }, [isOpen])

Also applies to: 243-253

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/CommandHistoryModal.tsx` around lines 54 - 63, The
useEffect that resets modal state on open (watching isOpen) only clears
showClearConfirm and isClearing when isOpen becomes true, leaving ConfirmDialog
potentially mounted when the modal is closed; update the effect to also reset
showClearConfirm and isClearing when isOpen becomes false (or unconditionally on
any isOpen change) so the destructive confirmation cannot remain visible after
the parent closes the modal — modify the effect handling in CommandHistoryModal
(the useEffect referencing isOpen, setShowClearConfirm, setIsClearing, and
inputRef) to clear those flags on close as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/renderer/components/CommandHistoryModal.tsx`:
- Line 17: Change the onClearHistory contract so the modal can detect
persistence failure: update the prop signature in CommandHistoryModal from
onClearHistory: () => Promise<void> to either onClearHistory: () =>
Promise<boolean> (return true on success, false on failure) or keep
Promise<void> but make the parent reject the promise on persistence failure;
then in the CommandHistoryModal component (where onClearHistory is awaited) only
close/dismiss the confirmation when the awaited call resolves with success (or
does not throw), and show an error/toast and keep the modal open when the call
returns false or rejects; also update the parent clear handler (the caller of
onClearHistory) to either return false or throw on write failure (or roll back
in-memory changes on failure) so the modal can react appropriately.

---

Duplicate comments:
In `@src/renderer/components/CommandHistoryModal.tsx`:
- Around line 94-102: handleClearConfirm can be invoked multiple times because
there is no guard; modify it to short-circuit when a clear is already in
progress by checking the isClearing state at the start (if (isClearing) return),
then proceed to setIsClearing(true) and clear in the try/finally as before; also
include isClearing in the useCallback dependency array and ensure
ConfirmDialog's confirm button/prop (the component using handleClearConfirm) is
disabled/blocked while isClearing to prevent rapid duplicate invocations (refer
to handleClearConfirm, isClearing, onClearHistory, setIsClearing,
setShowClearConfirm).
- Around line 54-63: The useEffect that resets modal state on open (watching
isOpen) only clears showClearConfirm and isClearing when isOpen becomes true,
leaving ConfirmDialog potentially mounted when the modal is closed; update the
effect to also reset showClearConfirm and isClearing when isOpen becomes false
(or unconditionally on any isOpen change) so the destructive confirmation cannot
remain visible after the parent closes the modal — modify the effect handling in
CommandHistoryModal (the useEffect referencing isOpen, setShowClearConfirm,
setIsClearing, and inputRef) to clear those flags on close as well.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4c0153a5-d3fa-4e74-b001-c107239a8691

📥 Commits

Reviewing files that changed from the base of the PR and between 5a506ab and a4a31b2.

📒 Files selected for processing (6)
  • src/renderer/components/CommandHistoryModal.test.tsx
  • src/renderer/components/CommandHistoryModal.tsx
  • src/renderer/hooks/use-command-history.ts
  • src/renderer/layouts/WorkspaceLayout.close-persistence.test.tsx
  • src/renderer/layouts/WorkspaceLayout.test.tsx
  • src/renderer/layouts/WorkspaceLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/renderer/hooks/use-command-history.ts
  • src/renderer/layouts/WorkspaceLayout.tsx

entries: CommandHistoryEntry[]
allEntries: CommandHistoryEntry[]
onSelectCommand: (command: string) => void
onClearHistory: () => Promise<void>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Have onClearHistory report whether the clear actually succeeded.

With a Promise<void> contract, this modal cannot distinguish success from a toasted persistence failure. The current parent clear callback clears the in-memory store before writing to disk and still resolves on write failure, so the dialog dismisses and retry becomes impossible even though the clear was not persisted.

Suggested direction
 interface CommandHistoryModalProps {
-  onClearHistory: () => Promise<void>
+  onClearHistory: () => Promise<boolean>
 }

Then only close the confirmation on success:

-      await onClearHistory()
-      setShowClearConfirm(false)
+      const didClear = await onClearHistory()
+      if (didClear) {
+        setShowClearConfirm(false)
+      }

Have the parent return false or reject when persistence fails, or roll the store back on failure.

📝 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
onClearHistory: () => Promise<void>
onClearHistory: () => Promise<boolean>
Suggested change
onClearHistory: () => Promise<void>
const didClear = await onClearHistory()
if (didClear) {
setShowClearConfirm(false)
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/CommandHistoryModal.tsx` at line 17, Change the
onClearHistory contract so the modal can detect persistence failure: update the
prop signature in CommandHistoryModal from onClearHistory: () => Promise<void>
to either onClearHistory: () => Promise<boolean> (return true on success, false
on failure) or keep Promise<void> but make the parent reject the promise on
persistence failure; then in the CommandHistoryModal component (where
onClearHistory is awaited) only close/dismiss the confirmation when the awaited
call resolves with success (or does not throw), and show an error/toast and keep
the modal open when the call returns false or rejects; also update the parent
clear handler (the caller of onClearHistory) to either return false or throw on
write failure (or roll back in-memory changes on failure) so the modal can react
appropriately.

@gnoviawan gnoviawan force-pushed the feat/project-specific-command-history branch from a4a31b2 to edb9d03 Compare March 11, 2026 20:30
…onality

- Add project filter dropdown to CommandHistoryModal ("This Project" / "All Projects")
- Add "Clear History" button with confirmation dialog for current project
- Disable Clear History button when viewing "All Projects" mode
- Add loading state during clear operation with double-submit protection
- Handle persistence failure: keep confirm dialog open, show error toast
- Reset confirmation state when modal closes to prevent stale dialogs
- Add error handling with toast notification for persistence failures
- Add useAllCommandHistory hook returning sorted entries (newest-first)
- Add comprehensive tests for filter and clear functionality

Co-Authored-By: Claude <noreply@anthropic.com>
@gnoviawan gnoviawan force-pushed the feat/project-specific-command-history branch from edb9d03 to dd6d3b8 Compare March 11, 2026 20:51
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/renderer/components/CommandHistoryModal.tsx (1)

246-257: Cancel button remains clickable during async clear operation.

The ConfirmDialog doesn't receive the isClearing state, so users can click Cancel while the clear operation is in progress. This creates a minor race condition where the dialog closes but the async operation continues—if it fails, the error toast appears but the dialog is already gone.

Since ConfirmDialog doesn't currently support a loading/disabled prop, consider either:

  1. Extending ConfirmDialog to accept an isLoading prop that disables both buttons
  2. Or accepting this minor edge case as low-risk (requires very fast Cancel click after Confirm)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/CommandHistoryModal.tsx` around lines 246 - 257, The
ConfirmDialog can be closed while the async clear is running; add an isLoading
(or isDisabled) prop to ConfirmDialog and pass the existing isClearing state
from this component (pass isLoading={isClearing} where ConfirmDialog is used),
then update ConfirmDialog's implementation to disable both the confirm and
cancel buttons and prevent closing while isLoading is true; reference
ConfirmDialog, showClearConfirm, handleClearConfirm, setShowClearConfirm and
isClearing when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/renderer/layouts/WorkspaceLayout.tsx`:
- Around line 767-777: The handler clear flow in handleClearCommandHistory
clears the in-memory store via
useCommandHistoryStore.getState().clearHistory(activeProjectId) before awaiting
persistenceApi.write, which can leave memory and disk out-of-sync on failure;
change the order to first call
persistenceApi.write(`projects/${activeProjectId}/command-history`, []) and only
when result.success is true call clearHistory(activeProjectId), and on failure
keep the store intact and surface the error via toast.error and throw, or
alternatively perform clearHistory optimistically but capture the previous
history (from useCommandHistoryStore.getState().getHistory or equivalent) and
roll it back by restoring that data if persistenceApi.write returns
!result.success.

---

Nitpick comments:
In `@src/renderer/components/CommandHistoryModal.tsx`:
- Around line 246-257: The ConfirmDialog can be closed while the async clear is
running; add an isLoading (or isDisabled) prop to ConfirmDialog and pass the
existing isClearing state from this component (pass isLoading={isClearing} where
ConfirmDialog is used), then update ConfirmDialog's implementation to disable
both the confirm and cancel buttons and prevent closing while isLoading is true;
reference ConfirmDialog, showClearConfirm, handleClearConfirm,
setShowClearConfirm and isClearing when making the changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1165deae-f9ed-46d3-8c28-88de2003cc82

📥 Commits

Reviewing files that changed from the base of the PR and between a4a31b2 and dd6d3b8.

📒 Files selected for processing (6)
  • src/renderer/components/CommandHistoryModal.test.tsx
  • src/renderer/components/CommandHistoryModal.tsx
  • src/renderer/hooks/use-command-history.ts
  • src/renderer/layouts/WorkspaceLayout.close-persistence.test.tsx
  • src/renderer/layouts/WorkspaceLayout.test.tsx
  • src/renderer/layouts/WorkspaceLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/renderer/layouts/WorkspaceLayout.test.tsx

- Persist empty array before clearing in-memory state to prevent
  inconsistency on persistence failure
- Add isLoading prop to ConfirmDialog to disable buttons during
  async operations

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gnoviawan
Copy link
Owner Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/renderer/components/ConfirmDialog.tsx (1)

35-60: ⚠️ Potential issue | 🟠 Major

Keyboard and backdrop handlers bypass disabled state during loading.

The buttons are correctly disabled when isLoading is true, but the Escape handler (line 41), Enter handler (line 53), and backdrop click (line 70) still fire their callbacks. Users can dismiss or re-confirm the dialog via keyboard or backdrop while the async operation is in progress.

🛡️ Proposed fix to guard handlers against isLoading
   useEffect(() => {
-    if (!isOpen) return
+    if (!isOpen || isLoading) return

     const handleEscape = (e: globalThis.KeyboardEvent): void => {
       if (e.key === 'Escape') {
         e.preventDefault()
         onCancel()
       }
     }

     window.addEventListener('keydown', handleEscape)
     return () => window.removeEventListener('keydown', handleEscape)
-  }, [isOpen, onCancel])
+  }, [isOpen, isLoading, onCancel])

   const handleKeyDown = useCallback(
     (e: KeyboardEvent<HTMLDivElement>) => {
+      if (isLoading) return
       if (e.key === 'Enter') {
         e.preventDefault()
         onConfirm()
       } else if (e.key === 'Escape') {
         e.preventDefault()
         onCancel()
       }
     },
-    [onConfirm, onCancel]
+    [isLoading, onConfirm, onCancel]
   )

Also guard the backdrop click on line 70:

-          onClick={onCancel}
+          onClick={isLoading ? undefined : onCancel}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/ConfirmDialog.tsx` around lines 35 - 60, The
Escape/Enter/backdrop handlers currently call onCancel/onConfirm even when
isLoading is true; update the keyboard escape handler defined inside useEffect
(handleEscape), the memoized handleKeyDown function, and the backdrop click
handler to first check if isLoading and return early if so, and also include
isLoading in their dependency arrays where relevant (e.g., the useEffect and
useCallback dependencies) so the guards reflect the current loading state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/renderer/components/ConfirmDialog.tsx`:
- Around line 35-60: The Escape/Enter/backdrop handlers currently call
onCancel/onConfirm even when isLoading is true; update the keyboard escape
handler defined inside useEffect (handleEscape), the memoized handleKeyDown
function, and the backdrop click handler to first check if isLoading and return
early if so, and also include isLoading in their dependency arrays where
relevant (e.g., the useEffect and useCallback dependencies) so the guards
reflect the current loading state.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 77274531-df32-40bb-8bca-009e156ff7db

📥 Commits

Reviewing files that changed from the base of the PR and between dd6d3b8 and 9f2708e.

📒 Files selected for processing (3)
  • src/renderer/components/CommandHistoryModal.tsx
  • src/renderer/components/ConfirmDialog.tsx
  • src/renderer/layouts/WorkspaceLayout.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/renderer/layouts/WorkspaceLayout.tsx

@gnoviawan gnoviawan merged commit 1d9595c into dev Mar 11, 2026
10 checks passed
This was referenced Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

phase-1 Phase 1: Stability & Polish (v0.3.1) v0.3.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant