Move assessment evaluation form into a modal#7579
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe privacy-assessment evaluation flow was moved from a standalone page into a modal. A new GenerateAssessmentsModal component was added and integrated into the privacy assessments listing and empty state; the old evaluate page and its route constant were removed. createPrivacyAssessment now invalidates assessment-related cache tags. Changes
Sequence DiagramsequenceDiagram
participant User
participant Page as Privacy Assessments Page
participant Modal as GenerateAssessmentsModal
participant API as RTK Query API
participant Cache as RTK Cache
User->>Page: Click "Generate Assessments"
Page->>Modal: set open = true
User->>Modal: Select templates & systems, toggle AI
User->>Modal: Click Submit
Modal->>API: createPrivacyAssessment(payload)
API->>Cache: invalidate "Privacy Assessment", "Privacy Assessment Tasks"
API-->>Modal: success / error
Modal->>User: show success or error
Modal->>Page: onClose (close modal)
Page->>Page: refresh UI / read updated data
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested labels
Suggested Reviewers
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 |
Contributor
Greptile SummaryThis PR consolidates the privacy assessment evaluation flow by moving the standalone
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 8a1b509 |
clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
clients/admin-ui/src/features/privacy-assessments/EmptyState.tsx (1)
20-22: Align CTA copy with the page-level trigger.This button says “Run assessment” while the header action says “Generate assessments” (
clients/admin-ui/src/pages/privacy-assessments/index.tsx, Lines 83-85). Consider using one label consistently.🤖 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/EmptyState.tsx` around lines 20 - 22, The CTA text in the EmptyState component is inconsistent with the page-level action; update the Button label inside EmptyState (the Button rendering with icon={<Icons.Add />} and prop onClick={onRunAssessment}) to use "Generate assessments" so it matches the header action ("Generate assessments") used elsewhere (e.g., the page-level trigger). Ensure the change is made only to the Button's displayed text and leave the handler name onRunAssessment unchanged.
🤖 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/GenerateAssessmentsModal.tsx`:
- Line 103: The user-facing messages in GenerateAssessmentsModal.tsx (component
GenerateAssessmentsModal) use singular wording but the modal can queue multiple
templates; update the success string ("Assessment evaluation queued. Results
will appear on the assessments page shortly,") and the corresponding failure
string near the other occurrence (line referenced as 111) to use plural wording
(e.g., "Assessments queued. Results will appear on the assessments page
shortly." and "Failed to queue assessments.") so messages accurately reflect
multi-select behavior.
---
Nitpick comments:
In `@clients/admin-ui/src/features/privacy-assessments/EmptyState.tsx`:
- Around line 20-22: The CTA text in the EmptyState component is inconsistent
with the page-level action; update the Button label inside EmptyState (the
Button rendering with icon={<Icons.Add />} and prop onClick={onRunAssessment})
to use "Generate assessments" so it matches the header action ("Generate
assessments") used elsewhere (e.g., the page-level trigger). Ensure the change
is made only to the Button's displayed text and leave the handler name
onRunAssessment unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bde305b-80af-45cd-abd5-9ed09834f19e
📒 Files selected for processing (8)
changelog/7579-assessment-evaluation-modal.yamlclients/admin-ui/src/features/common/nav/routes.tsclients/admin-ui/src/features/privacy-assessments/EmptyState.tsxclients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsxclients/admin-ui/src/features/privacy-assessments/index.tsclients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.tsclients/admin-ui/src/pages/privacy-assessments/evaluate.tsxclients/admin-ui/src/pages/privacy-assessments/index.tsx
💤 Files with no reviewable changes (2)
- clients/admin-ui/src/pages/privacy-assessments/evaluate.tsx
- clients/admin-ui/src/features/common/nav/routes.ts
clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx
Outdated
Show resolved
Hide resolved
…ssmentsModal.tsx Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx (1)
101-111:⚠️ Potential issue | 🟡 MinorUse plural user messaging for multi-template queueing
Since
assessment_typesis multi-select (Line 141), the strings on Line 102 and Line 110 should be plural.Proposed wording update
message.success( - "Assessment evaluation queued. Results will appear on the assessments page shortly.", + "Assessments queued. Results will appear on the assessments page shortly.", ); @@ message.error( - `Failed to queue assessment: ${getErrorMessage(error as RTKErrorResult["error"])}`, + `Failed to queue assessments: ${getErrorMessage(error as RTKErrorResult["error"])}`, );🤖 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/GenerateAssessmentsModal.tsx` around lines 101 - 111, Update the user-facing messages in GenerateAssessmentsModal.tsx to use plural wording for multi-template queueing: change the success message passed to message.success (currently "Assessment evaluation queued. Results will appear on the assessments page shortly.") to a plural form like "Assessments queued for evaluation. Results will appear on the assessments page shortly." and change the error prefix in the message.error call (currently "Failed to queue assessment:") to "Failed to queue assessments:" so messaging aligns with the multi-select field assessment_types and the behavior in the submit handler that processes multiple template IDs (see functions/variables form.resetFields, setSelectedTemplateIds, and assessment_types).
🤖 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/GenerateAssessmentsModal.tsx`:
- Around line 40-51: The JSX return in renderTemplateOption (function handling
TemplateOptionData) is missing the closing parenthesis for the multi-line
return; fix by adding the closing ')' immediately after the closing </Flex>
element so the return reads return ( ... ); and then close the function with the
existing '};' — this restores correct JSX/paren pairing and resolves the syntax
error.
---
Duplicate comments:
In
`@clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx`:
- Around line 101-111: Update the user-facing messages in
GenerateAssessmentsModal.tsx to use plural wording for multi-template queueing:
change the success message passed to message.success (currently "Assessment
evaluation queued. Results will appear on the assessments page shortly.") to a
plural form like "Assessments queued for evaluation. Results will appear on the
assessments page shortly." and change the error prefix in the message.error call
(currently "Failed to queue assessment:") to "Failed to queue assessments:" so
messaging aligns with the multi-select field assessment_types and the behavior
in the submit handler that processes multiple template IDs (see
functions/variables form.resetFields, setSelectedTemplateIds, and
assessment_types).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae2abbd3-44b2-47d5-84e3-0b91acfb4a33
📒 Files selected for processing (1)
clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx
clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx (1)
67-68: Hardcoded pagination may miss templates if more than 100 exist.The query uses a fixed limit of 100 templates per page. While the modal includes client-side search via the Select component, this only filters the fetched results—users won't be able to discover templates beyond the first 100. Consider fetching all pages or implementing dynamic pagination. Note that this pattern is used consistently elsewhere in the codebase (e.g., AssessmentTaskStatusIndicator.tsx, privacy-assessments/index.tsx), suggesting awareness of the limitation.
🤖 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/GenerateAssessmentsModal.tsx` around lines 67 - 68, The query in GenerateAssessmentsModal uses a hardcoded page/size ({ page: 1, size: 100 }) when calling useGetAssessmentTemplatesQuery (templatesData, isLoadingTemplates), which can omit templates beyond the first 100 and breaks client-side search; update the data fetching to either (a) paginate through all pages and concatenate results before passing to the Select, or (b) add dynamic pagination/virtualized loading tied to the Select (infinite scroll or server-side search) so you don't rely on a fixed size; locate useGetAssessmentTemplatesQuery usage in GenerateAssessmentsModal and implement the chosen approach (looping requests until templatesData.meta.total/pages are exhausted, or wiring the Select's onSearch/onScroll to request additional pages) and ensure the component consumes the aggregated template list rather than only the first page.
🤖 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/AssessmentTaskStatusIndicator.tsx`:
- Line 132: The effect that depends on activeTask/lastCompletedTask uses the
onTaskFinish callback inside the notification button but omits it from the
dependency array, risking a stale closure; fix by either (A) capturing the
latest onTaskFinish via a ref (create onTaskFinishRef, update it in useEffect/on
prop change, and call onTaskFinishRef.current inside the notification handler)
or (B) if omission is intentional, add an explanatory comment and keep
onTaskFinish out of the dependency array to satisfy the linter; references:
onTaskFinish, the effect with dependency array [activeTask, lastCompletedTask,
notificationApi, dispatch], and the notification button callback.
---
Nitpick comments:
In
`@clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx`:
- Around line 67-68: The query in GenerateAssessmentsModal uses a hardcoded
page/size ({ page: 1, size: 100 }) when calling useGetAssessmentTemplatesQuery
(templatesData, isLoadingTemplates), which can omit templates beyond the first
100 and breaks client-side search; update the data fetching to either (a)
paginate through all pages and concatenate results before passing to the Select,
or (b) add dynamic pagination/virtualized loading tied to the Select (infinite
scroll or server-side search) so you don't rely on a fixed size; locate
useGetAssessmentTemplatesQuery usage in GenerateAssessmentsModal and implement
the chosen approach (looping requests until templatesData.meta.total/pages are
exhausted, or wiring the Select's onSearch/onScroll to request additional pages)
and ensure the component consumes the aggregated template list rather than only
the first page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 031c955c-b441-41a8-af6f-72f281eb7cc7
📒 Files selected for processing (2)
clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsxclients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx
clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx (1)
102-112:⚠️ Potential issue | 🟡 MinorUse plural toast copy for multi-template submissions.
The form allows multiple templates, but the success and failure messages are singular. Update both strings to plural so they match actual behavior.
Suggested text update
- message.success( - "Assessment evaluation queued. Results will appear on the assessments page shortly.", - ); + message.success( + "Assessments queued. Results will appear on the assessments page shortly.", + ); @@ - message.error( - `Failed to queue assessment: ${getErrorMessage(error as RTKErrorResult["error"])}`, - ); + message.error( + `Failed to queue assessments: ${getErrorMessage(error as RTKErrorResult["error"])}`, + );🤖 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/GenerateAssessmentsModal.tsx` around lines 102 - 112, The success and error toast text in GenerateAssessmentsModal use singular wording but the form can submit multiple templates; update the strings passed to message.success and message.error to plural copy (e.g., "Assessments evaluation queued. Results will appear on the assessments page shortly." and "Failed to queue assessments: …") so they reflect multi-template submissions—keep calls to message.success, message.error and the error formatting via getErrorMessage(RTKErrorResult["error"]) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx`:
- Around line 102-112: The success and error toast text in
GenerateAssessmentsModal use singular wording but the form can submit multiple
templates; update the strings passed to message.success and message.error to
plural copy (e.g., "Assessments evaluation queued. Results will appear on the
assessments page shortly." and "Failed to queue assessments: …") so they reflect
multi-template submissions—keep calls to message.success, message.error and the
error formatting via getErrorMessage(RTKErrorResult["error"]) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6219022c-c8d5-4fbf-bb76-736fdc016a3e
📒 Files selected for processing (2)
clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsxclients/admin-ui/src/pages/privacy-assessments/index.tsx
…thyca/fides into 2872-generate-assessment-modal
lucanovera
approved these changes
Mar 6, 2026
Contributor
lucanovera
left a comment
There was a problem hiding this comment.
Modal looks good, the notification on every assessment is a nice improvement. Approved!
mfbrown
pushed a commit
that referenced
this pull request
Mar 12, 2026
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
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-2872
Description Of Changes
Move the assessment evaluation form from a dedicated page (
/privacy-assessments/evaluate) into a modal triggered from the assessments list page. Since the API doesn't yet support the additional fields needed for manual assessment creation (name, DPO/owner, description), the "Generate Assessments" and "Create New Assessment" flows were identical — so we consolidated into a single modal.Code Changes
clients/admin-ui/src/features/privacy-assessments/GenerateAssessmentsModal.tsx- New modal component with the evaluation form (template multi-select, system select, AI-assisted toggle)clients/admin-ui/src/features/privacy-assessments/index.ts- Export the new modalclients/admin-ui/src/pages/privacy-assessments/index.tsx- Replace page link with button that opens the modal, wire EmptyState to modalclients/admin-ui/src/features/privacy-assessments/EmptyState.tsx- AcceptonRunAssessmentcallback instead of linking to a pageclients/admin-ui/src/pages/privacy-assessments/evaluate.tsx- Deletedclients/admin-ui/src/features/common/nav/routes.ts- Remove evaluate routeSteps to Confirm
/privacy-assessments/evaluateno longer resolves (404)Pre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit
Changed
New Features