fix: show Clear All button on History page header#690
fix: show Clear All button on History page header#690RajdeepKushwaha5 wants to merge 2 commits intoaccomplish-ai:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughHistory page now reads tasks from the task store, shows a localized "Clear All" control only when tasks exist, prompts for confirmation, and calls clearHistory on confirm. A new integration test file validates visibility and confirmation-driven behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant Page as HistoryPage
participant Store as TaskStore
User->>Page: Click "Clear All"
Page->>User: show confirmation dialog
alt User confirms
Page->>Store: clearHistory()
Store-->>Page: success
Page-->>User: update UI (no tasks)
else User cancels
Page-->>User: no action
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 |
There was a problem hiding this comment.
Pull request overview
Fixes the /history page so users can clear task history even when TaskHistory is rendered with showTitle={false} (which previously hid the “Clear all” action).
Changes:
- Add a “Clear all” button to the History page header, shown only when history is non-empty, with a confirmation prompt before clearing.
- Wire the button to
useTaskStore().clearHistory(). - Add integration tests covering visibility and confirmation/clearing behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/web/src/client/pages/History.tsx | Adds header-level “Clear all” action driven by task store state and confirmation prompt. |
| apps/web/tests/integration/renderer/pages/History.integration.test.tsx | Adds integration coverage for button visibility and confirmation flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const handleClearAll = () => { | ||
| if (confirm(t('confirmClear'))) { | ||
| clearHistory(); |
There was a problem hiding this comment.
clearHistory is an async store action (returns a Promise). Calling it without await/error handling can lead to unhandled promise rejections if accomplish.clearTaskHistory() fails. Consider making handleClearAll async and awaiting clearHistory() (with a try/catch to surface an error), or explicitly prefixing with void and handling failures elsewhere.
| const handleClearAll = () => { | |
| if (confirm(t('confirmClear'))) { | |
| clearHistory(); | |
| const handleClearAll = async () => { | |
| if (confirm(t('confirmClear'))) { | |
| try { | |
| await clearHistory(); | |
| } catch (error) { | |
| console.error('Failed to clear task history:', error); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/client/pages/History.tsx (1)
11-15: Handle potentialclearHistoryrejection.
clearHistoryis an async function (seetaskStore.ts:496-500). Calling it withoutawaitand without a.catch()means any failure (IPC error, storage issue) results in an unhandled promise rejection and leaves the user without feedback.♻️ Proposed fix to handle errors
- const handleClearAll = () => { + const handleClearAll = async () => { if (confirm(t('confirmClear'))) { - clearHistory(); + try { + await clearHistory(); + } catch (error) { + console.error('Failed to clear history:', error); + } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/client/pages/History.tsx` around lines 11 - 15, handleClearAll calls the async function clearHistory without awaiting or catching errors, which can produce unhandled promise rejections; make handleClearAll async, await clearHistory() inside a try/catch and handle failures (e.g., log the error and show user feedback via alert/toast using t(...) or similar) so any IPC/storage error is caught and reported instead of being unhandled.
🤖 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/web/src/client/pages/History.tsx`:
- Around line 11-15: handleClearAll calls the async function clearHistory
without awaiting or catching errors, which can produce unhandled promise
rejections; make handleClearAll async, await clearHistory() inside a try/catch
and handle failures (e.g., log the error and show user feedback via alert/toast
using t(...) or similar) so any IPC/storage error is caught and reported instead
of being unhandled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee613542-d316-4402-8c44-184eb6caeb39
📒 Files selected for processing (2)
apps/web/__tests__/integration/renderer/pages/History.integration.test.tsxapps/web/src/client/pages/History.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/client/pages/History.tsx (1)
11-19: Consider using a React dialog component instead of nativeconfirm().The native browser
confirm()is synchronous/blocking and cannot be styled to match the application's design system. It also has accessibility limitations (no keyboard focus management, no customizable ARIA labels).Given the PR description mentions "opens a confirmation dialog before clearing," consider using a proper dialog component (e.g., from your existing UI library) for a consistent UX.
Additionally, when
clearHistory()fails, the error is only logged to console—the user has no indication the operation failed. Consider adding a toast notification or inline error state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/client/pages/History.tsx` around lines 11 - 19, Replace the synchronous native confirm() call in handleClearAll with a proper React dialog component from our UI library (e.g., the existing ConfirmDialog / Modal component) so the confirmation is non-blocking and styleable; open the dialog when the user triggers clear, and call clearHistory() in the dialog’s confirm handler (retain the i18n string t('confirmClear') as the dialog message). Also surface failures to the user instead of only console.error by hooking into the app’s notification/toast system (or setting an inline error state) inside the catch block of the clearHistory() call so users see a friendly error message when the operation fails.
🤖 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/web/src/client/pages/History.tsx`:
- Around line 11-19: Replace the synchronous native confirm() call in
handleClearAll with a proper React dialog component from our UI library (e.g.,
the existing ConfirmDialog / Modal component) so the confirmation is
non-blocking and styleable; open the dialog when the user triggers clear, and
call clearHistory() in the dialog’s confirm handler (retain the i18n string
t('confirmClear') as the dialog message). Also surface failures to the user
instead of only console.error by hooking into the app’s notification/toast
system (or setting an inline error state) inside the catch block of the
clearHistory() call so users see a friendly error message when the operation
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2089051-eb2e-48d3-8d39-a87141616326
📒 Files selected for processing (1)
apps/web/src/client/pages/History.tsx
The History page passed showTitle={false} to TaskHistory which hid the
Clear All button entirely. The button was unreachable for users on the
dedicated /history route.
- Move the Clear All button into the History page header alongside the
title, so it's always visible when history exists
- Add 8 integration tests for the History page covering button
visibility (shown/hidden) and click interactions (confirm / cancel)
Fixes accomplish-ai#685
RajdeepKushwaha5
force-pushed
the
fix/history-clear-all-accessibility
branch
from
412eb37 to
67a1060
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/client/pages/History.tsx (1)
11-19: Consider using a custom confirmation dialog instead of nativeconfirm().The native
confirm()blocks the main thread and offers no styling or accessibility customization. A custom modal component would provide a more consistent UX and better accessibility.Additionally, the error handling only logs to console — the user receives no feedback if
clearHistory()fails.💡 Example approach with error feedback
+import { useState } from 'react'; +// Assume a toast/notification system exists in the app + export default function HistoryPage() { + const [showConfirmDialog, setShowConfirmDialog] = useState(false); // ... const handleClearAll = async () => { - if (confirm(t('confirmClear'))) { - try { - await clearHistory(); - } catch (error) { - console.error('Failed to clear task history:', error); - } + try { + await clearHistory(); + } catch (error) { + console.error('Failed to clear task history:', error); + // Show user-visible error notification } };Then trigger
handleClearAllfrom a custom confirmation dialog's confirm action rather than usingconfirm().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/client/pages/History.tsx` around lines 11 - 19, Replace the blocking native confirm() call in handleClearAll with your app's custom confirmation modal/component (so the modal's confirm action invokes handleClearAll or a new confirmedClear function); locate the handler function handleClearAll and the clearHistory() call and move prompting to a styled, accessible modal (use the existing translation key t('confirmClear') for text). Also add user-facing error feedback when clearHistory() fails (e.g., show a toast or set an error state that renders an inline alert) instead of only console.error so users are informed of failures. Ensure the modal is cancellable and that handleClearAll only calls clearHistory() after the modal confirm action.
🤖 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/web/src/client/pages/History.tsx`:
- Around line 11-19: Replace the blocking native confirm() call in
handleClearAll with your app's custom confirmation modal/component (so the
modal's confirm action invokes handleClearAll or a new confirmedClear function);
locate the handler function handleClearAll and the clearHistory() call and move
prompting to a styled, accessible modal (use the existing translation key
t('confirmClear') for text). Also add user-facing error feedback when
clearHistory() fails (e.g., show a toast or set an error state that renders an
inline alert) instead of only console.error so users are informed of failures.
Ensure the modal is cancellable and that handleClearAll only calls
clearHistory() after the modal confirm action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb90e556-78c6-4518-bf90-531bbf4ce0ea
📒 Files selected for processing (2)
apps/web/__tests__/integration/renderer/pages/History.integration.test.tsxapps/web/src/client/pages/History.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/tests/integration/renderer/pages/History.integration.test.tsx
Summary
The dedicated
/historypage rendered<TaskHistory showTitle={false} />, which permanently hid the "Clear All" button — it lived inside{showTitle && …}inside the component. Users had no way to clear history from the History page.Root cause
TaskHistoryrenders the Clear All action only whenshowTitle={true}. The History page always passesshowTitle={false}(to avoid a duplicate heading), so the button was never visible.Fix
Move the Clear All action to the History page header itself, independent of
TaskHistory. The button is conditionally rendered only when tasks exist, and triggers a confirmation dialog before clearing.Changes
apps/web/src/client/pages/History.tsx— AddeduseTaskStore, confirmation dialog before clearing, and a "Clear All" button in the page header (only visible when history is non-empty)apps/web/__tests__/integration/renderer/pages/History.integration.test.tsx— 8 new integration tests covering button visibility, confirmation flow, and store interactionHow to test
pnpm dev/history)This contribution was developed with AI assistance (GitHub Copilot).
Summary by CodeRabbit
New Features
Tests