ENG-2879: Relocate delete and generate report buttons to page header#7564
ENG-2879: Relocate delete and generate report buttons to page header#7564
Conversation
Move the trash icon and Generate report button from the AssessmentDetail title row into the sticky PageHeader rightContent slot, matching the pattern used by the list page. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe PR relocates delete and generate report buttons from the AssessmentDetail component to the page header of the privacy-assessments detail page. The refactoring adds completion validation for the generate report action and implements a confirmation modal for the delete operation with centralized error handling. Changes
Sequence DiagramsequenceDiagram
actor User
participant PageHeader as Page Header
participant Modal
participant Assessment as Assessment API
participant MessageUI as Message UI
User->>PageHeader: Click Delete Button
PageHeader->>Modal: Open Confirmation Dialog
User->>Modal: Confirm Deletion
Modal->>Assessment: Execute Delete Mutation
alt Success
Assessment-->>Modal: Delete Complete
Modal->>MessageUI: Show Success Message
MessageUI-->>User: Navigate to List
else Error
Assessment-->>Modal: Error Response
Modal->>MessageUI: Display Error Message
MessageUI-->>User: Show Error Notification
end
User->>PageHeader: Hover Generate Report Button
alt Assessment Complete
PageHeader-->>User: Button Enabled
else Assessment Incomplete
PageHeader-->>User: Tooltip: Complete All Questions
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 |
Greptile SummaryThis PR cleanly relocates the delete and generate-report action buttons from the inline title row in Key changes:
Issue identified:
Confidence Score: 4/5
Last reviewed commit: 4cda64c |
| const isComplete = | ||
| !!assessment && | ||
| (assessment.question_groups ?? []) | ||
| .flatMap((g) => g.questions) | ||
| .every((q) => q.answer_text.trim().length > 0); |
There was a problem hiding this comment.
The isComplete computation is now present in both this page component and in AssessmentDetail.tsx (lines 58–61 of that file, where it's memoized). This creates two separate traversals of the same question_groups data structure on every render of the page component, and introduces a maintenance risk — if the completeness criterion ever changes, both places need to be updated.
Consider wrapping this in useMemo to match the memoization pattern already used in the child component:
| const isComplete = | |
| !!assessment && | |
| (assessment.question_groups ?? []) | |
| .flatMap((g) => g.questions) | |
| .every((q) => q.answer_text.trim().length > 0); | |
| const isComplete = useMemo( | |
| () => | |
| !!assessment && | |
| (assessment.question_groups ?? []) | |
| .flatMap((g) => g.questions) | |
| .every((q) => q.answer_text.trim().length > 0), | |
| [assessment], | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
clients/admin-ui/src/pages/privacy-assessments/[id].tsx (1)
47-51: Consider extracting or memoizing theisCompletecomputation.This logic is duplicated in
AssessmentDetail.tsx(lines 58-61) where it's memoized withuseMemo. Here it's computed inline on every render.Options:
- Extract to a shared utility function and memoize in both places
- Compute it once here and pass
isCompleteas a prop toAssessmentDetailGiven that
AssessmentDetailusesisCompletefor the status tag and this page uses it for the "Generate report" button, consolidating would reduce duplication and ensure consistency.♻️ Option 2: Pass isComplete as prop
+ import { useMemo } from "react"; ... - const isComplete = - !!assessment && - (assessment.question_groups ?? []) - .flatMap((g) => g.questions) - .every((q) => q.answer_text.trim().length > 0); + const isComplete = useMemo( + () => + !!assessment && + (assessment.question_groups ?? []) + .flatMap((g) => g.questions) + .every((q) => q.answer_text.trim().length > 0), + [assessment], + ); ... - <AssessmentDetail assessment={assessment} /> + <AssessmentDetail assessment={assessment} isComplete={isComplete} />Then update
AssessmentDetailto accept and use the prop instead of recomputing.🤖 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 47 - 51, Extract the inline completion logic into a shared utility (e.g., computeIsComplete(assessment)) that encapsulates the current check (flatten question_groups -> questions -> answer_text.trim().length > 0), then replace the inline computation in this page ([id].tsx) and in AssessmentDetail.tsx with calls to that utility; in components, wrap the call in useMemo (e.g., useMemo(() => computeIsComplete(assessment), [assessment])) or alternatively compute once here and pass the resulting isComplete boolean into AssessmentDetail via a prop instead of recomputing there so both places use the same memoized/shared logic.
🤖 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 68-72: The onOk async callback currently uses the non-null
assertion assessment! when calling deleteAssessment(assessment!.id) which can
throw if assessment becomes undefined; add a defensive guard at the start of the
onOk handler to check assessment exists and bail out gracefully (e.g., show
message.error('Assessment not found') and return) before calling
deleteAssessment/unwrap and router.push(PRIVACY_ASSESSMENTS_ROUTE), so
deleteAssessment, assessment.id, and subsequent logic only run when assessment
is defined.
---
Nitpick comments:
In `@clients/admin-ui/src/pages/privacy-assessments/`[id].tsx:
- Around line 47-51: Extract the inline completion logic into a shared utility
(e.g., computeIsComplete(assessment)) that encapsulates the current check
(flatten question_groups -> questions -> answer_text.trim().length > 0), then
replace the inline computation in this page ([id].tsx) and in
AssessmentDetail.tsx with calls to that utility; in components, wrap the call in
useMemo (e.g., useMemo(() => computeIsComplete(assessment), [assessment])) or
alternatively compute once here and pass the resulting isComplete boolean into
AssessmentDetail via a prop instead of recomputing there so both places use the
same memoized/shared logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e726d42-a8b0-4bb7-a977-585628161f2d
📒 Files selected for processing (3)
changelog/7564-relocate-assessment-action-buttons.yamlclients/admin-ui/src/features/privacy-assessments/AssessmentDetail.tsxclients/admin-ui/src/pages/privacy-assessments/[id].tsx
| onOk: async () => { | ||
| try { | ||
| await deleteAssessment(assessment!.id).unwrap(); | ||
| message.success("Assessment deleted."); | ||
| router.push(PRIVACY_ASSESSMENTS_ROUTE); |
There was a problem hiding this comment.
Add a guard for the assessment reference in the async callback.
The assessment! non-null assertion assumes assessment remains defined between opening the modal and clicking confirm. While unlikely, if a background refetch fails during this window, assessment could become undefined, causing a runtime error.
🛡️ Proposed defensive guard
onOk: async () => {
+ if (!assessment) {
+ message.error("Assessment no longer available.");
+ return;
+ }
try {
- await deleteAssessment(assessment!.id).unwrap();
+ await deleteAssessment(assessment.id).unwrap();📝 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.
| onOk: async () => { | |
| try { | |
| await deleteAssessment(assessment!.id).unwrap(); | |
| message.success("Assessment deleted."); | |
| router.push(PRIVACY_ASSESSMENTS_ROUTE); | |
| onOk: async () => { | |
| if (!assessment) { | |
| message.error("Assessment no longer available."); | |
| return; | |
| } | |
| try { | |
| await deleteAssessment(assessment.id).unwrap(); | |
| message.success("Assessment deleted."); | |
| router.push(PRIVACY_ASSESSMENTS_ROUTE); |
🤖 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 68 -
72, The onOk async callback currently uses the non-null assertion assessment!
when calling deleteAssessment(assessment!.id) which can throw if assessment
becomes undefined; add a defensive guard at the start of the onOk handler to
check assessment exists and bail out gracefully (e.g., show
message.error('Assessment not found') and return) before calling
deleteAssessment/unwrap and router.push(PRIVACY_ASSESSMENTS_ROUTE), so
deleteAssessment, assessment.id, and subsequent logic only run when assessment
is defined.
lucanovera
left a comment
There was a problem hiding this comment.
UI and code changes look good. Approved
…7564) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Ticket ENG-2879
Description Of Changes
Relocates the trash icon (delete assessment) and Generate report button from the inline title row in
AssessmentDetailto the stickyPageHeadervia itsrightContentprop. This matches the existing pattern used by the assessments list page and keeps action buttons consistently in the upper header area.The delete handler and
isCompletecomputation are lifted to the page component ([id].tsx) where thePageHeaderis rendered.Code Changes
clients/admin-ui/src/pages/privacy-assessments/[id].tsx- Add delete mutation,isCompletecomputation,handleDelete, and render buttons asPageHeaderrightContentclients/admin-ui/src/features/privacy-assessments/AssessmentDetail.tsx- Remove delete button, generate report button, and associated hooks/handlers; simplify title row to a plaindivSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit
Release Notes