fixed: Replaced Scans alert() failures with toast feedback#626
fixed: Replaced Scans alert() failures with toast feedback#626pranavv2707 wants to merge 5 commits into
Conversation
utksh1
left a comment
There was a problem hiding this comment.
The toast replacement is a useful direction, but this head is behind and required checks are failing. Please rebase on latest main/current CI baseline and fix frontend-checks/backend-lint before requesting review again.
a6a39cc to
fb56811
Compare
utksh1
left a comment
There was a problem hiding this comment.
Re-reviewed the latest push. This still has broad Scans/Workflows/phase test churn, no-final-newline changes, and production formatting churn around the toast change. Please narrow the PR to replacing alert failures with toast feedback, remove unrelated test rewrites, and rerun frontend checks.
utksh1
left a comment
There was a problem hiding this comment.
Rechecking after the latest commit: this is still blocked.
The frontend-checks job is failing on the current head. Please fix CI and keep the patch focused on replacing Scans alert failures with toast handling; unrelated Scans/Workflows churn should stay out of this PR.
Hi @utksh1 The ToastContext mock additions to Scans.test.tsx, ScansPhases.test.tsx, and Scans.polling.test.tsx are necessary because adding useToast() to Scans.tsx causes those tests to fail and those are minimal 5-line additions with no other changes to those files. Please do let know if i may remove all of them and commit just the scans alert page or any other alternative approach. |
utksh1
left a comment
There was a problem hiding this comment.
Replacing blocking browser alerts with accessible toasts is a great UX/accessibility win. The failure test coverage looks comprehensive as well. Merging!
utksh1
left a comment
There was a problem hiding this comment.
I was just about to merge this, but another PR that was merged today caused a merge conflict! Could you please pull from main and rebase so I can merge this in?
|
Hello @utksh1 , |
Description
Replaced
alert()dialogs in the Scans page destructive-action failure paths with accessible toast notifications using the existingToastContext. Also adds test coverage for all three failure paths:Related Issues
Closes issue number #469
Type of Change
How Has This Been Tested?
Unit tests — 5 new tests in
Scans.failures.test.tsx:deleteTaskfailure shows error toast and keeps task in listbulkDeleteTasksfailure shows error toast and keeps tasks in listclearAllTasksfailure shows error toastChecklist
Issues Fixed
alert()dialogs for delete and clear-history failure paths, which block the page, cannot be styled, and are inaccessible to screen readers.alert()calls inhandleTaskDelete,handleClearAll, andhandleBulkDeletefailure paths withaddToast()from the existingToastContext.role="alert"so screen readers announce them automatically.Tests Added
frontend/testing/unit/pages/Scans.failures.test.tsx— 5 new tests covering:deleteTaskfailure shows error toast and keeps task in listbulkDeleteTasksfailure shows error toast and keeps tasks in listclearAllTasksfailure shows error toastHow to Test
cd frontend && npm run test— all 5 new tests should pass.npm run build— build should complete with no errors.Comments:
The npm audit failure is a pre-existing vulnerability on upstream main, not introduced by this PR.
This change only modifies frontend/src/pages/Scans.tsx and frontend/testing/unit/pages/Scans.failures.test.tsx.
I have sought assistance from LLMs and gituhub copilot to understand the codebase and guide implementation, as disclosed per the contributing guidelines.
I apologise for the large number of commits.
Screenshots for checks
Description: The screenshot shows the passing of all 5 tests.
Description: The screenshot shows the successful building after the running of the tests, generating no warnings.