🐛 Fix some applications query issues #2606
Conversation
WalkthroughRefactors application static-report download flow: the UI now uses mutateAsync/isPending from useDownloadStaticReport; query layer internalizes downloadStaticReport, applies HEADERS.yaml for YAML Accept, uses saveAs to write blobs, and updates the delete mutation onSuccess to pass the deleted id. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as DownloadButton
participant Hook as useDownloadStaticReport
participant Func as downloadStaticReport (internal)
participant API as REST API
participant FS as file-saver
User->>UI: Click "Download"
UI->>Hook: mutateAsync({ application, mimeType })
Hook->>Func: invoke download
alt mimeType == YAML
Func->>API: GET /applications/{id}/report\nHeaders: ... + HEADERS.yaml
else
Func->>API: GET /applications/{id}/report\nHeaders: default
end
API-->>Func: 200 Blob
Func->>FS: saveAs(blob, "application-name.ext")
FS-->>Hook: confirm save
Hook-->>UI: resolve (isPending=false)
UI-->>User: Download complete
%% Error path
API-->>Func: Error
Func-->>Hook: reject
Hook-->>UI: isError=true
UI-->>User: show Alert
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing Touches
🧪 Generate unit tests
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/app/pages/applications/application-detail-drawer/components/download-button.tsx (1)
27-32: Prevent clicks when visually disabled (link-variant + isAriaDisabled).With PatternFly’s link-variant Button, isAriaDisabled does not block onClick. Guard the handler to avoid downloads when disabled.
- const handleDownload = () => { - downloadFile({ + const handleDownload = () => { + if (!isDownloadEnabled) return; + void downloadFile({ application: application, mimeType: mimeType, }); };Also applies to: 50-51
🧹 Nitpick comments (5)
client/src/app/pages/applications/application-detail-drawer/components/download-button.tsx (2)
2-3: Consolidate PatternFly imports into one line.Minor cleanup to avoid duplicate module imports.
-import { Button } from "@patternfly/react-core"; -import { Alert, Spinner } from "@patternfly/react-core"; +import { Button, Alert, Spinner } from "@patternfly/react-core";
45-45: Remove redundant key prop.key is only needed in arrays/lists; here it’s a single element.
- <Button - key={mimeType} + <Button onClick={handleDownload}client/src/app/queries/applications.ts (3)
232-241: Minor: simplify URL selection.The switch falls back to the same value for most cases. A simple conditional is clearer.
- switch (mimeType) { - case MimeType.YAML: - url = `${APPLICATIONS}/${application.id}/analysis`; - break; - case MimeType.TAR: - default: - url = `${APPLICATIONS}/${application.id}/analysis/report`; - } + url = + mimeType === MimeType.YAML + ? `${APPLICATIONS}/${application.id}/analysis` + : `${APPLICATIONS}/${application.id}/analysis/report`;
244-244: Type the Axios response as Blob and avoid re-wrapping blobs.Avoid constructing a new Blob around an existing Blob; also add a generic to axios.get for type safety.
- const response = await axios.get(url, { + const response = await axios.get<Blob>(url, { responseType: "blob", ...(mimeType === MimeType.YAML && { headers: HEADERS.yaml }), }); @@ - const blob = new Blob([response.data]); - saveAs(blob, `analysis-report-app-${application.name}.${mimeType}`); + // response.data is already a Blob + saveAs(response.data, `analysis-report-app-${application.name}.${mimeType}`);Optional: preserve content-type explicitly.
// saveAs(new Blob([response.data], { type: response.headers["content-type"] }), filename);Also applies to: 253-255
253-255: Optional: sanitize filename.Application names may contain characters invalid in filenames on some platforms. Consider normalizing.
Example helper (outside this diff):
const safeName = (s: string) => s.replace(/[\\/:*?"<>|]/g, "_"); saveAs(response.data, `analysis-report-app-${safeName(application.name)}.${mimeType}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/app/pages/applications/application-detail-drawer/components/download-button.tsx(3 hunks)client/src/app/queries/applications.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/app/queries/applications.ts (1)
client/src/app/api/rest.ts (1)
HEADERS(101-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (8)
client/src/app/pages/applications/application-detail-drawer/components/download-button.tsx (2)
6-7: Imports look good.Correct sources for Application/MimeType and the updated hook path.
36-41: Pending-first rendering is correct.Showing a spinner while pending before error state is the right priority.
client/src/app/queries/applications.ts (6)
5-5: LGTM: centralized refetch interval.Good reuse of DEFAULT_REFETCH_INTERVAL.
14-15: LGTM: standardized headers import.Bringing HEADERS from rest centralizes MIME handling.
27-27: LGTM: cross-query invalidation key available.This enables dependent cache invalidations.
205-208: Fix: pass deleted id through onSuccess.Using vars.id resolves the incorrect id issue and aligns with consumer expectations.
244-247: YAML-only Accept header is correct.Scoping HEADERS.yaml to only the YAML request fixes the MIME negotiation issue.
3-3: Default import safe with current TS config
esModuleInterop and allowSyntheticDefaultImports are enabled in tsconfig (lines 19, 21, 63, 64), soimport saveAs from "file-saver";works. You may still opt to useimport { saveAs } from "file-saver";to decouple from interop flags.
client/src/app/pages/applications/application-detail-drawer/components/download-button.tsx
Show resolved
Hide resolved
Two issues noticed while testing the application form: - `useDeleteApplicationMutation()` was not sending the correct application id to `onSuccess()` - `downloadStaticReport()` was not testing for mime type properly when setting the accept headers for yaml types - `DownloadButton` changed: - Using `mutateAsync` instead of `mutate` - Use `isPending` instead of `isLoading` as per the deprecation comment Signed-off-by: Scott J Dickerson <[email protected]>
sjd78
force-pushed
the
application_query_fixes
branch
from
d105a75 to
66533a7
Compare
sjd78
changed the title
Resolves some issues noticed while updating the application form: - `useDeleteApplicationMutation()` was not sending the correct application id to `onSuccess()` - `downloadStaticReport()` was not testing for mime type properly when setting the accept headers for yaml types - `DownloadButton` changed: - Using `mutateAsync` instead of `mutate` - Use `isPending` instead of `isLoading` as per the deprecation comment Signed-off-by: Scott J Dickerson <[email protected]>
2 participants
Resolves some issues noticed while updating the application form:
useDeleteApplicationMutation()was not sending the correct application id toonSuccess()downloadStaticReport()was not testing for mime type properly when setting the accept headers for yaml typesDownloadButtonchanged:mutateAsyncinstead ofmutateisPendinginstead ofisLoadingas per the deprecation commentSummary by CodeRabbit
Improvements
Bug Fixes