feat(download): add error notifications when document downloads fail#981
Open
ctron wants to merge 2 commits intoguacsec:mainfrom
Open
feat(download): add error notifications when document downloads fail#981ctron wants to merge 2 commits intoguacsec:mainfrom
ctron wants to merge 2 commits intoguacsec:mainfrom
Conversation
Add .catch() handlers to all three download functions in useDownload hook (advisory, SBOM, SBOM licenses) that display danger toast notifications with the error message extracted via getAxiosErrorMessage. Implements TC-4045 Assisted-by: Claude Code
Contributor
Reviewer's GuideAdds centralized error handling and danger toast notifications to the useDownload hook’s advisory, SBOM, and SBOM license download flows, and introduces unit tests to verify both error notification and successful download behavior. Sequence diagram for failed advisory download with error notificationsequenceDiagram
actor User
participant ReactComponent
participant useDownload
participant downloadAdvisory
participant NotificationsContext
participant getAxiosErrorMessage
User->>ReactComponent: clickDownloadAdvisory
ReactComponent->>useDownload: downloadAdvisory(id, filename)
useDownload->>downloadAdvisory: request advisory
downloadAdvisory-->>useDownload: reject(error)
useDownload->>NotificationsContext: notifyDownloadError(title, error)
NotificationsContext->>getAxiosErrorMessage: getAxiosErrorMessage(error)
getAxiosErrorMessage-->>NotificationsContext: message
NotificationsContext-->>User: pushNotification{variant: danger, title, message}
Sequence diagram for successful SBOM license downloadsequenceDiagram
actor User
participant ReactComponent
participant useDownload
participant downloadSbomLicense
participant getFilenameFromContentDisposition
participant saveAs
User->>ReactComponent: clickDownloadSbomLicenses
ReactComponent->>useDownload: downloadSBOMLicenses(id)
useDownload->>downloadSbomLicense: request SBOM licenses
downloadSbomLicense-->>useDownload: resolve(response)
useDownload->>getFilenameFromContentDisposition: getFilenameFromContentDisposition(header)
getFilenameFromContentDisposition-->>useDownload: filename
useDownload->>saveAs: saveAs(Blob, filenameOrDefault)
saveAs-->>User: browser downloads tarGzFile
Flow diagram for useDownload advisory download with error handlingflowchart TD
A[User triggers downloadAdvisory] --> B[useDownload called with id and filename]
B --> C[Call downloadAdvisory with client and request options]
C --> D{Promise resolved or rejected}
D -->|resolved| E[Create Blob from response data]
E --> F[Call saveAs with Blob and filename or idJson]
F --> G[Browser downloads advisory file]
D -->|rejected| H[Call notifyDownloadError with title and error]
H --> I[Call getAxiosErrorMessage with error]
I --> J[Construct notification with title, variant danger, message]
J --> K[pushNotification shows danger toast to user]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the tests, consider rendering the hook within a real
NotificationsContext.Providerinstead of mockingReact.useContextglobally, to avoid tightly coupling the hook’s behavior to an implementation detail and to keep other potentialuseContextcalls working as expected. - You can import
useContextdirectly fromreactinstead of importing the fullReactnamespace (import { useContext } from "react";) to keep the hook’s dependencies a bit more focused.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the tests, consider rendering the hook within a real `NotificationsContext.Provider` instead of mocking `React.useContext` globally, to avoid tightly coupling the hook’s behavior to an implementation detail and to keep other potential `useContext` calls working as expected.
- You can import `useContext` directly from `react` instead of importing the full `React` namespace (`import { useContext } from "react";`) to keep the hook’s dependencies a bit more focused.
## Individual Comments
### Comment 1
<location path="client/src/app/hooks/domain-controls/useDownload.ts" line_range="16" />
<code_context>
+/** Hook providing download functions for advisories, SBOMs, and SBOM licenses with error notifications. */
export const useDownload = () => {
+ const { pushNotification } = React.useContext(NotificationsContext);
+
+ /** Pushes a danger toast notification with the given title and the error message extracted from the error. */
</code_context>
<issue_to_address>
**question:** Consider how this hook behaves when used outside a NotificationsContext provider.
`React.useContext(NotificationsContext)` will throw if `useDownload` is used under a tree without that provider. If that’s acceptable, no change needed. Otherwise, consider either giving the context a safe default or guarding against a missing provider and no-op’ing notifications in that case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| /** Hook providing download functions for advisories, SBOMs, and SBOM licenses with error notifications. */ | ||
| export const useDownload = () => { | ||
| const { pushNotification } = React.useContext(NotificationsContext); |
Contributor
There was a problem hiding this comment.
question: Consider how this hook behaves when used outside a NotificationsContext provider.
React.useContext(NotificationsContext) will throw if useDownload is used under a tree without that provider. If that’s acceptable, no change needed. Otherwise, consider either giving the context a safe default or guarding against a missing provider and no-op’ing notifications in that case.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #981 +/- ##
==========================================
- Coverage 66.79% 66.72% -0.07%
==========================================
Files 221 221
Lines 3882 3889 +7
Branches 898 898
==========================================
+ Hits 2593 2595 +2
- Misses 949 954 +5
Partials 340 340 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The error parameter was typed as `unknown` which caused a build-time type error when passed to `getAxiosErrorMessage`. Implements TC-4045 Assisted-by: Claude Code
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
.catch()error handlers to all three download functions in theuseDownloadhook (advisory, SBOM, SBOM licenses)getAxiosErrorMessageImplements TC-4045
Test plan
pushNotificationcalled withvariant: "danger"whendownloadAdvisoryrejectspushNotificationcalled withvariant: "danger"whendownloadSbomrejectspushNotificationcalled withvariant: "danger"whendownloadSbomLicenserejectsgetAxiosErrorMessageused to extract the error messagesaveAsfor advisory, SBOM, and SBOM license downloadsnpm run checkpasses (biome lint/format)npm testpasses (all 9 tests)🤖 Generated with Claude Code
Summary by Sourcery
Add error-handling and user notifications to document download hooks while verifying behavior with new unit tests.
New Features:
Tests: