Show progress/status on evaluation tasks#7553
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
…in-progress-evaluation-tasks-that-update-in-real-time
📝 WalkthroughWalkthroughAdds real-time privacy assessment task status: new API endpoints, types, utilities, two UI components that poll tasks, display inline status with a popover, and emit notifications (with optional reload) when tasks finish. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as AssessmentTaskStatusIndicator
participant API as PrivacyAssessmentsAPI
participant Backend as BackendService
User->>UI: mount
UI->>API: getAssessmentTasks()
API->>Backend: GET /assessment-tasks
Backend-->>API: tasks list
API-->>UI: tasks
alt active task exists
UI->>UI: start polling loop
loop while active
UI->>API: getAssessmentTasks() (poll)
API->>Backend: GET /assessment-tasks
Backend-->>API: updated status
API-->>UI: updated tasks
UI->>User: render spinner / progress
end
UI->>API: getAssessmentTemplates()
API->>Backend: GET /assessment-templates
Backend-->>API: templates
API-->>UI: template names
UI->>User: notify completion (optional Reload)
else no active task
alt last completed exists
UI->>User: show last completion timestamp
else
UI->>User: show "No evaluation history"
end
end
User->>UI: hover -> open popover
UI->>API: fetch task details (if needed)
API-->>UI: task details
UI->>User: show AssessmentTaskPopoverContent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
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/features/privacy-assessments/AssessmentTaskPopoverContent.tsx`:
- Around line 44-47: The Progress percent prop is fed directly from
activeTask.progress and should be defensively clamped and validated before
render; update the usage in AssessmentTaskPopoverContent (where <Progress
percent={Math.round(activeTask.progress)} ... /> is set) to first coerce to a
finite number and clamp into [0,100] (e.g., use Number.isFinite check and
Math.max(0, Math.min(100, value)) or a small clamp helper) and then Math.round
the clamped value so the UI never receives NaN/Infinity or out-of-range values.
In
`@clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsx`:
- Around line 96-114: Currently the code always shows a success notification
when hadActiveTaskRef transitions from true to no activeTask; update the branch
inside the effect that checks hadActiveTaskRef.current && !activeTask to inspect
the last terminal status (e.g., use the previous activeTask status or check
activeTask?.status or store lastTaskStatus) and emit notificationApi.success
when the task ended successfully and notificationApi.error (with a message like
"Assessment failed" and optional details) when the terminal status is
TaskStatus.ERROR; keep the existing Reload results button behavior tied to
onTaskFinish for successful completion and avoid showing the success message on
error by branching on TaskStatus (reference hadActiveTaskRef, activeTask,
notificationApi, onTaskFinish, and TaskStatus).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d09f737-d4f6-4faa-ba63-6d8db1d2b4a4
📒 Files selected for processing (9)
changelog/7553-assessment-task-status.yamlclients/admin-ui/src/features/common/api.slice.tsclients/admin-ui/src/features/privacy-assessments/AssessmentTaskPopoverContent.tsxclients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsxclients/admin-ui/src/features/privacy-assessments/index.tsclients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.tsclients/admin-ui/src/features/privacy-assessments/types.tsclients/admin-ui/src/features/privacy-assessments/utils.tsclients/admin-ui/src/pages/privacy-assessments/index.tsx
clients/admin-ui/src/features/privacy-assessments/AssessmentTaskPopoverContent.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsx (1)
172-187: Make evaluation details reachable without hover-only interaction.The details popover is currently hover-triggered on a non-focusable container, which is easy to miss for keyboard/touch users. Please expose a focus/click path.
Proposed refactor
<Popover content={ <AssessmentTaskPopoverContent activeTask={activeTask} lastCompletedTask={lastCompletedTask} templateNamesMap={templateNamesMap} /> } title="Evaluation details" - trigger="hover" + trigger={["hover", "focus", "click"]} placement="bottom" > - <div className={classNames("cursor-pointer", className)}> + <div + className={classNames("cursor-pointer", className)} + tabIndex={0} + aria-label="View evaluation details" + > {inlineContent} </div> </Popover>🤖 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/AssessmentTaskStatusIndicator.tsx` around lines 172 - 187, The popover in AssessmentTaskStatusIndicator.tsx is only triggered on hover and wrapped around a non-focusable div, making it inaccessible to keyboard and touch users; update the Popover props to allow focus/click (e.g., trigger includes "click" and/or "focus") and make the wrapper element keyboard-accessible by replacing or augmenting the div with a focusable control (add tabIndex=0 and appropriate role like "button" or convert to a <button>) and wire keyboard activation (onKeyDown handling Enter/Space) so AssessmentTaskPopoverContent becomes reachable via keyboard and touch while preserving inlineContent styling via className.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsx`:
- Around line 172-187: The popover in AssessmentTaskStatusIndicator.tsx is only
triggered on hover and wrapped around a non-focusable div, making it
inaccessible to keyboard and touch users; update the Popover props to allow
focus/click (e.g., trigger includes "click" and/or "focus") and make the wrapper
element keyboard-accessible by replacing or augmenting the div with a focusable
control (add tabIndex=0 and appropriate role like "button" or convert to a
<button>) and wire keyboard activation (onKeyDown handling Enter/Space) so
AssessmentTaskPopoverContent becomes reachable via keyboard and touch while
preserving inlineContent styling via className.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: adf201a8-6110-4da9-bfea-bb7db5ab500a
📒 Files selected for processing (2)
clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsxclients/admin-ui/src/features/privacy-assessments/utils.ts
There was a problem hiding this comment.
Couple non-blocking code nits.
One thing I think we should change though is the copy.
Last assessment 2 minutes ago and
Last assessment a minute ago is kind of awkward.
I'd suggest changing the copy to something like Last assessment evaluated a minute ago
clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts
Outdated
Show resolved
Hide resolved
clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsx
Show resolved
Hide resolved
Great suggestion on the copy! I'll implement it. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts (1)
88-95: Dead code — remove the unusedgetAssessmentTaskendpoint.The
getAssessmentTaskendpoint is defined but never exported or used. OnlyuseGetAssessmentTasksQuery(plural) is exported and actively used in the codebase. Remove this endpoint to reduce maintenance burden.🤖 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 88 - 95, Remove the unused RTK Query endpoint getAssessmentTask from the privacy-assessments slice: delete the getAssessmentTask entry in the endpoints object (the build.query definition returning url `plus/privacy-assessments/tasks/${taskId}` and providesTags for "Privacy Assessment Tasks") so only the used getAssessmentTasks endpoint remains; ensure there are no lingering references to getAssessmentTask (e.g., exported hooks or types) and run a quick project-wide search to confirm it isn't referenced elsewhere before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts`:
- Around line 88-95: Remove the unused RTK Query endpoint getAssessmentTask from
the privacy-assessments slice: delete the getAssessmentTask entry in the
endpoints object (the build.query definition returning url
`plus/privacy-assessments/tasks/${taskId}` and providesTags for "Privacy
Assessment Tasks") so only the used getAssessmentTasks endpoint remains; ensure
there are no lingering references to getAssessmentTask (e.g., exported hooks or
types) and run a quick project-wide search to confirm it isn't referenced
elsewhere before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a658f2c-7256-45b0-abd8-56b7001ec29f
📒 Files selected for processing (1)
clients/admin-ui/src/features/privacy-assessments/privacy-assessments.slice.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsx (1)
35-54: Polling won't detect externally-started tasks.The polling is gated on
activeTaskbeing truthy. If the component mounts with no active task and a task is later started by another user/session (or via API), this component won't detect it until the page is refreshed.Consider whether a slower background poll (e.g., every 60–120s) when idle would improve the experience, or document this as expected behavior.
🤖 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/AssessmentTaskStatusIndicator.tsx` around lines 35 - 54, The current logic gates polling on activeTask so the component never notices tasks started externally after mount; change the polling strategy in useGetAssessmentTasksQuery so it always polls but with a conditional interval (e.g., ACTIVE_POLL_INTERVAL when activeTask is truthy and a longer IDLE_POLL_INTERVAL when falsy) instead of using skip: !activeTask, or add a second call that polls at a slow IDLE_POLL_INTERVAL when there is no activeTask; update references to useGetAssessmentTasksQuery, activeTask, ACTIVE_POLL_INTERVAL (and introduce IDLE_POLL_INTERVAL) so the component can detect externally-started tasks without requiring a full refresh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsx`:
- Around line 35-54: The current logic gates polling on activeTask so the
component never notices tasks started externally after mount; change the polling
strategy in useGetAssessmentTasksQuery so it always polls but with a conditional
interval (e.g., ACTIVE_POLL_INTERVAL when activeTask is truthy and a longer
IDLE_POLL_INTERVAL when falsy) instead of using skip: !activeTask, or add a
second call that polls at a slow IDLE_POLL_INTERVAL when there is no activeTask;
update references to useGetAssessmentTasksQuery, activeTask,
ACTIVE_POLL_INTERVAL (and introduce IDLE_POLL_INTERVAL) so the component can
detect externally-started tasks without requiring a full refresh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b70b0fb1-faaf-4176-a8c6-18b15f80dea0
📒 Files selected for processing (1)
clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsx
Ticket ENG-2837
Description Of Changes
Add a progress and status indicator for evaluation tasks. It shows a progress percentage that updates when an evaluation task is in progress and a Notification when the task is done. If no task is in progress it will show when was the last time than an evaluation ran.
Code Changes
Steps to Confirm
Note: Nightly currently doesn't have this fix deployed so the progress percentage might not be reporting correctly, but you can still open the network tab to see the indicator polling the task statuses every 30s.
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and worksSummary by CodeRabbit
New Features
Changelog