ENG-2380: Add PDF report download for privacy assessments#7535
Merged
lucanovera merged 8 commits intomainfrom Mar 6, 2026
Merged
ENG-2380: Add PDF report download for privacy assessments#7535lucanovera merged 8 commits intomainfrom
lucanovera merged 8 commits intomainfrom
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Add a "Download report" button to the privacy assessment detail page that downloads a PDF report when the assessment is complete. - Add downloadAssessmentReport mutation to privacy-assessments.slice.ts - Use file-saver to trigger browser download of PDF blob - Update AssessmentDetail component with download button and handler - Button disabled until assessment is marked complete Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
f844ecd to
5a9063a
Compare
- Use correct endpoint path (/pdf instead of /report) - Add export_mode=external query parameter for clean output - Improve filename extraction from content-disposition header Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR adds a functional "Download report" button to the privacy assessment detail page that triggers PDF download using the existing backend endpoint. Key changes:
The implementation is clean, follows React best practices with Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 27eb180 |
Combine PDF download feature with Slack questionnaire features: - Show "Download report" button when assessment is complete - Show "Request input from team" button when assessment is incomplete Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes add a PDF report download feature to the privacy assessment detail page. A new mutation ( Changes
Possibly related PRs
Poem
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts`:
- Around line 216-237: The responseHandler currently treats every fetch response
as a PDF; add an HTTP error check at the start of responseHandler (in the
responseHandler function) to verify response.ok and handle non-2xx responses
before extracting headers/arrayBuffer; if !response.ok, read the error body
(text/json) or throw a wrapped Error (including status and statusText) so
callers don't attempt to save a failed error payload via saveAs; still preserve
existing filename extraction (contentDisposition) and blob/saveAs only when
response.ok.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3e4dbde-ef5d-41e6-a5ae-dfdd9818e85b
📒 Files selected for processing (3)
changelog/7535-pdf-download.yamlclients/admin-ui/src/features/privacy-assessments/AssessmentDetail.tsxclients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts
clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts
Show resolved
Hide resolved
Check response.ok before treating the response as a PDF blob. If the response is an error, extract the error message and throw so callers don't attempt to save failed error payloads via saveAs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
452426f to
fca049c
Compare
lucanovera
approved these changes
Mar 5, 2026
Contributor
lucanovera
left a comment
There was a problem hiding this comment.
Basic download works correctly, I've adjusted the implementation to account for the button being moved to the header.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/admin-ui/src/pages/privacy-assessments/`[id].tsx:
- Around line 51-54: handleDownloadReport uses the transient route query value
assessmentId when calling downloadReport(assessmentId). Change the call to use
the canonical loaded entity ID (assessment.id) instead so the download mutation
always uses the persisted assessment identifier; update the invocation in
handleDownloadReport to call downloadReport(assessment.id).unwrap() (and ensure
assessment is non-null/defined before calling) to avoid edge cases from query
parsing/state.
- Around line 176-183: Update the Button label text to match the implemented
action: replace the displayed string "Generate report" with "Download report" in
the Button that uses onClick={handleDownloadReport} (the Button with props
disabled={!isComplete} and loading={isDownloading}) so the UI and tests reflect
the actual download behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e8f34d4-e679-4fe7-bd2c-372a48b88166
📒 Files selected for processing (3)
clients/admin-ui/src/features/privacy-assessments/AssessmentDetail.tsxclients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.tsclients/admin-ui/src/pages/privacy-assessments/[id].tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts
- clients/admin-ui/src/features/privacy-assessments/AssessmentDetail.tsx
Comment on lines
+51
to
+54
| const handleDownloadReport = async () => { | ||
| try { | ||
| await downloadReport(assessmentId).unwrap(); | ||
| } catch (error) { |
There was a problem hiding this comment.
Use canonical assessment ID for download mutation.
On Line 53, downloadReport(assessmentId) uses route query state instead of the loaded entity. Prefer assessment.id to avoid edge cases where query parsing/state is transient.
Suggested patch
const handleDownloadReport = async () => {
try {
- await downloadReport(assessmentId).unwrap();
+ if (!assessment?.id) {
+ return;
+ }
+ await downloadReport(assessment.id).unwrap();
} catch (error) {
message.error(📝 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
| const handleDownloadReport = async () => { | |
| try { | |
| await downloadReport(assessmentId).unwrap(); | |
| } catch (error) { | |
| const handleDownloadReport = async () => { | |
| try { | |
| if (!assessment?.id) { | |
| return; | |
| } | |
| await downloadReport(assessment.id).unwrap(); | |
| } catch (error) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/pages/privacy-assessments/`[id].tsx around lines 51 -
54, handleDownloadReport uses the transient route query value assessmentId when
calling downloadReport(assessmentId). Change the call to use the canonical
loaded entity ID (assessment.id) instead so the download mutation always uses
the persisted assessment identifier; update the invocation in
handleDownloadReport to call downloadReport(assessment.id).unwrap() (and ensure
assessment is non-null/defined before calling) to avoid edge cases from query
parsing/state.
Comment on lines
+176
to
183
| <Button | ||
| type="primary" | ||
| disabled={!isComplete} | ||
| loading={isDownloading} | ||
| onClick={handleDownloadReport} | ||
| > | ||
| Generate report | ||
| </Button> |
There was a problem hiding this comment.
Align button copy with the implemented action ("Download report").
The action downloads a PDF, and PR acceptance text references “Download report”. Keeping the button label as “Generate report” may confuse users and test steps.
Suggested patch
- >
- Generate report
- </Button>
+ >
+ Download report
+ </Button>📝 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
| <Button | |
| type="primary" | |
| disabled={!isComplete} | |
| loading={isDownloading} | |
| onClick={handleDownloadReport} | |
| > | |
| Generate report | |
| </Button> | |
| <Button | |
| type="primary" | |
| disabled={!isComplete} | |
| loading={isDownloading} | |
| onClick={handleDownloadReport} | |
| > | |
| Download report | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/pages/privacy-assessments/`[id].tsx around lines 176 -
183, Update the Button label text to match the implemented action: replace the
displayed string "Generate report" with "Download report" in the Button that
uses onClick={handleDownloadReport} (the Button with props
disabled={!isComplete} and loading={isDownloading}) so the UI and tests reflect
the actual download behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts (1)
237-238: Preferparamsover hardcoded query string in URL.At Line [237], embedding
export_mode=externaldirectly in the URL is less maintainable than using theparamsfield.Proposed refactor
- url: `plus/privacy-assessments/${id}/pdf?export_mode=external`, + url: `plus/privacy-assessments/${id}/pdf`, + params: { export_mode: "external" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts` around lines 237 - 238, Replace the hardcoded query string in the request config by removing `?export_mode=external` from the url `plus/privacy-assessments/${id}/pdf` and add a `params` object with `export_mode: "external"` in the same request config (the block that currently contains `url: \`plus/privacy-assessments/${id}/pdf?export_mode=external\`, method: "GET",`). This keeps the `method: "GET"` unchanged but moves query params into `params` for maintainability and better integration with the HTTP client.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts`:
- Around line 259-265: The Content-Disposition parsing around the
contentDisposition variable only handles filename= and misses RFC 5987 filename*
entries; update the parsing logic in the privacy-assessments.slice.ts helper
(the block using contentDisposition.match(/filename="?([^";\n]+)"?/)) to first
check for filename* (e.g., match /filename\*\s*=\s*([^;]+)/), parse the RFC5987
value into charset'lang'encodedValue, percent-decode the encodedValue (using
decodeURIComponent) and, if a charset is provided other than UTF-8, convert
bytes to that charset or at least handle UTF-8 correctly; if filename* is
absent, fall back to the existing filename= regex and trimming behavior so
non-ASCII/encoded filenames are correctly extracted and decoded.
---
Nitpick comments:
In
`@clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts`:
- Around line 237-238: Replace the hardcoded query string in the request config
by removing `?export_mode=external` from the url
`plus/privacy-assessments/${id}/pdf` and add a `params` object with
`export_mode: "external"` in the same request config (the block that currently
contains `url: \`plus/privacy-assessments/${id}/pdf?export_mode=external\`,
method: "GET",`). This keeps the `method: "GET"` unchanged but moves query
params into `params` for maintainability and better integration with the HTTP
client.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5142849-88ab-4694-b19a-a0da0bc54759
📒 Files selected for processing (1)
clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts
Comment on lines
+259
to
+265
| if (contentDisposition) { | ||
| // Try to extract filename from content-disposition header | ||
| // Handles both: filename="name.pdf" and filename=name.pdf | ||
| const match = contentDisposition.match(/filename="?([^";\n]+)"?/); | ||
| if (match && match[1]) { | ||
| filename = match[1].trim(); | ||
| } |
There was a problem hiding this comment.
Handle RFC 5987 filename* in Content-Disposition parsing.
At Lines [259]-[265], parsing only filename= misses common filename*= headers (encoded/non-ASCII), which can lead to incorrect fallback filenames.
Proposed fix
if (contentDisposition) {
- // Try to extract filename from content-disposition header
- // Handles both: filename="name.pdf" and filename=name.pdf
- const match = contentDisposition.match(/filename="?([^";\n]+)"?/);
- if (match && match[1]) {
- filename = match[1].trim();
- }
+ const filenameStarMatch = contentDisposition.match(
+ /filename\*\s*=\s*([^;]+)/i,
+ );
+ if (filenameStarMatch?.[1]) {
+ const encoded = filenameStarMatch[1].replace(/^UTF-8''/i, "").trim();
+ try {
+ filename = decodeURIComponent(encoded);
+ } catch {
+ filename = encoded;
+ }
+ } else {
+ const filenameMatch = contentDisposition.match(
+ /filename\s*=\s*"?(?<name>[^";\n]+)"?/i,
+ );
+ if (filenameMatch?.groups?.name) {
+ filename = filenameMatch.groups.name.trim();
+ }
+ }
}📝 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
| if (contentDisposition) { | |
| // Try to extract filename from content-disposition header | |
| // Handles both: filename="name.pdf" and filename=name.pdf | |
| const match = contentDisposition.match(/filename="?([^";\n]+)"?/); | |
| if (match && match[1]) { | |
| filename = match[1].trim(); | |
| } | |
| if (contentDisposition) { | |
| const filenameStarMatch = contentDisposition.match( | |
| /filename\*\s*=\s*([^;]+)/i, | |
| ); | |
| if (filenameStarMatch?.[1]) { | |
| const encoded = filenameStarMatch[1].replace(/^UTF-8''/i, "").trim(); | |
| try { | |
| filename = decodeURIComponent(encoded); | |
| } catch { | |
| filename = encoded; | |
| } | |
| } else { | |
| const filenameMatch = contentDisposition.match( | |
| /filename\s*=\s*"?(?<name>[^";\n]+)"?/i, | |
| ); | |
| if (filenameMatch?.groups?.name) { | |
| filename = filenameMatch.groups.name.trim(); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts`
around lines 259 - 265, The Content-Disposition parsing around the
contentDisposition variable only handles filename= and misses RFC 5987 filename*
entries; update the parsing logic in the privacy-assessments.slice.ts helper
(the block using contentDisposition.match(/filename="?([^";\n]+)"?/)) to first
check for filename* (e.g., match /filename\*\s*=\s*([^;]+)/), parse the RFC5987
value into charset'lang'encodedValue, percent-decode the encodedValue (using
decodeURIComponent) and, if a charset is provided other than UTF-8, convert
bytes to that charset or at least handle UTF-8 correctly; if filename* is
absent, fall back to the existing filename= regex and trimming behavior so
non-ASCII/encoded filenames are correctly extracted and decoded.
mfbrown
pushed a commit
that referenced
this pull request
Mar 12, 2026
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Lucano Vera <lucanovera@live.com.ar>
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
2 participants
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.
Ticket ENG-2380
Description Of Changes
Adds a "Download report" button to the privacy assessment detail page that downloads a PDF report in external format (clean format suitable for sharing/signing).
This is a simplified UI implementation - no internal/external mode selection in the UI. The API supports both modes, but the UI only uses external format for now.
Code Changes
downloadAssessmentReportmutation toprivacy-assessments.slice.tsAssessmentDetail.tsxwith download button and loading statefile-saverto trigger browser download of PDF blobexport_mode=externalquery parameter for clean output formatSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit