Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdds an evidence drawer feature to privacy assessments: new UI components (drawer, section, card group), drawer state and search in AssessmentDetail, evidence deduplication/filtering utilities, label constants, and simplified EvidenceItem types. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AssessmentDetail as "AssessmentDetail"
participant EvidenceDrawer as "EvidenceDrawer"
participant EvidenceSection as "EvidenceSection"
participant EvidenceCardGroup as "EvidenceCardGroup"
User->>AssessmentDetail: Click "View evidence"
AssessmentDetail->>AssessmentDetail: handleViewEvidence() sets drawerOpen, focusedGroupId
AssessmentDetail->>EvidenceDrawer: render(open=true, group, evidence, searchQuery)
EvidenceDrawer->>EvidenceSection: pass group, evidence, searchQuery
EvidenceSection->>EvidenceSection: categorize (system vs human), build collapse items
EvidenceSection->>EvidenceCardGroup: render groups with items
EvidenceCardGroup->>EvidenceCardGroup: render individual cards
User->>EvidenceDrawer: type search query
EvidenceDrawer->>AssessmentDetail: onSearchChange(query)
AssessmentDetail->>AssessmentDetail: dedupe + filterEvidence(query)
AssessmentDetail->>EvidenceDrawer: re-render with filtered evidence
EvidenceDrawer->>EvidenceSection: update evidence prop
User->>EvidenceDrawer: close drawer
EvidenceDrawer->>AssessmentDetail: onClose()
AssessmentDetail->>AssessmentDetail: handleCloseDrawer() clears state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
lucanovera
requested review from
jpople and
kruulik
and removed request for
a team and
jpople
Contributor
Greptile SummaryThis PR adds an evidence drawer to the privacy assessments detail page, allowing users to view the complete evidence trail for any question group. Clicking "View evidence" on an expanded group panel opens a 600px right-side Ant Design Key changes:
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 4a59fd4 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+31
to
+38
| <div className={styles.drawerTitle}> | ||
| <Title level={5} className={styles.drawerHeading}> | ||
| Evidence | ||
| </Title> | ||
| <Text type="secondary" size="sm"> | ||
| Complete evidence trail organized by question | ||
| </Text> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Avoid div — use Ant Design Space or Flex instead
The div.drawerTitle wrapper (line 31) and the div.searchBar / div.body structural sections (lines 46, 55) should use semantic or component-library alternatives rather than plain div elements where possible.
The same pattern also appears in:
EvidenceSection.tsx:56andEvidenceSection.tsx:79—divwrappingFlex+Textinside collapse panel labelsEvidenceCardGroup.tsx:16—divused as the evidence card container
For the drawer title, a Space direction="vertical" size={0} or Flex vertical would remove the need for the bare div. For the collapse labels, the same Ant Design Space or Flex vertical approach applies. The card container in EvidenceCardGroup could use a semantic element such as article to better describe the content.
Suggested change
| <div className={styles.drawerTitle}> | |
| <Title level={5} className={styles.drawerHeading}> | |
| Evidence | |
| </Title> | |
| <Text type="secondary" size="sm"> | |
| Complete evidence trail organized by question | |
| </Text> | |
| </div> | |
| <Space direction="vertical" size={0} className={styles.drawerTitle}> | |
| <Title level={5} className={styles.drawerHeading}> | |
| Evidence | |
| </Title> | |
| <Text type="secondary" size="sm"> | |
| Complete evidence trail organized by question | |
| </Text> | |
| </Space> |
Context Used: Rule from dashboard - Avoid using div elements when possible. Use semantic HTML elements or component library alternativ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const [expandedKeys, setExpandedKeys] = useState<string[]>([]); | ||
| const [drawerOpen, setDrawerOpen] = useState(false); | ||
| const [focusedGroupId, setFocusedGroupId] = useState<string | null>(null); | ||
| const [evidenceSearchQuery, setEvidenceSearchQuery] = useState(""); |
Contributor
There was a problem hiding this comment.
Prefer useSearch hook over custom useState
The evidence search is managed with a plain useState (evidenceSearchQuery) and a manual reset in handleCloseDrawer. The existing useSearch hook already handles in-memory (non-URL-synced) search state via disableUrlState: true, and also provides a resetSearch utility, which would simplify the close handler.
// Instead of:
const [evidenceSearchQuery, setEvidenceSearchQuery] = useState("");
// ...
const handleCloseDrawer = useCallback(() => {
setDrawerOpen(false);
setEvidenceSearchQuery(""); // manual reset
}, []);
// Prefer:
const { searchQuery: evidenceSearchQuery, updateSearch: setEvidenceSearchQuery, resetSearch: resetEvidenceSearch } = useSearch({ disableUrlState: true });
// ...
const handleCloseDrawer = useCallback(() => {
setDrawerOpen(false);
resetEvidenceSearch();
}, [resetEvidenceSearch]);Context Used: Rule from dashboard - Use the existing useSearch hook for search functionality to maintain URL state synchronization ins... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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
🧹 Nitpick comments (2)
clients/admin-ui/src/features/privacy-assessments/EvidenceSection.tsx (1)
104-106: Consider expanding sections by default for better discoverability.The
Collapsehas nodefaultActiveKey, so both sections start collapsed. Users must click to reveal evidence items. If the drawer is opened specifically to view evidence, consider expanding sections by default.♻️ Proposed: Expand all sections by default
<div className={styles.collapseWrapper}> - <Collapse items={collapseItems} ghost /> + <Collapse + items={collapseItems} + defaultActiveKey={collapseItems.map((item) => item.key)} + ghost + /> </div>🤖 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/EvidenceSection.tsx` around lines 104 - 106, The Collapse sections are all collapsed because no defaultActiveKey is provided; modify the EvidenceSection component so the Collapse (symbol: Collapse) opens sections by default by passing a defaultActiveKey prop that contains the keys of collapseItems (symbol: collapseItems) — e.g., compute the keys from collapseItems and pass them as defaultActiveKey so all sections are expanded when the drawer opens; ensure keys match the item.key values used to build collapseItems.clients/admin-ui/src/features/privacy-assessments/AssessmentDetail.tsx (1)
239-247: Consider passingfocusedGroupIdonly when the drawer is open.The
focusedGroupIdprop is passed regardless of whethergroupis defined. IfEvidenceDrawerorEvidenceSectionrelies ongroupIdbeing non-null whengroupis defined, the current flow is fine. However, iffocusedGroupIdcan be stale after closing and reopening with a new group, verify that the downstream components handle the transition gracefully.🤖 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/AssessmentDetail.tsx` around lines 239 - 247, The EvidenceDrawer is always receiving focusedGroupId even when closed, which can leave a stale id active; update the prop usage in the AssessmentDetail render so focusedGroupId is only passed when the drawer is open (use drawerOpen to gate passing focusedGroupId, otherwise pass undefined/null), and ensure this change targets the EvidenceDrawer prop in the JSX where EvidenceDrawer is instantiated (symbols: EvidenceDrawer, focusedGroupId, drawerOpen, focusedGroup).
🤖 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/utils.ts`:
- Around line 50-63: The filter uses the untrimmed query for matching: after the
early-return check it sets lower = query.toLowerCase() but query wasn't trimmed;
change to compute lower from query.trim() (e.g., const lower =
query.trim().toLowerCase()) so items.filter comparisons (item.value,
SOURCE_TYPE_LABELS[item.source_type], FIELD_NAME_LABELS[item.field_name] and
item.field_name.replace(/_/g, " ")) use the normalized/truncated query for
matching.
---
Nitpick comments:
In `@clients/admin-ui/src/features/privacy-assessments/AssessmentDetail.tsx`:
- Around line 239-247: The EvidenceDrawer is always receiving focusedGroupId
even when closed, which can leave a stale id active; update the prop usage in
the AssessmentDetail render so focusedGroupId is only passed when the drawer is
open (use drawerOpen to gate passing focusedGroupId, otherwise pass
undefined/null), and ensure this change targets the EvidenceDrawer prop in the
JSX where EvidenceDrawer is instantiated (symbols: EvidenceDrawer,
focusedGroupId, drawerOpen, focusedGroup).
In `@clients/admin-ui/src/features/privacy-assessments/EvidenceSection.tsx`:
- Around line 104-106: The Collapse sections are all collapsed because no
defaultActiveKey is provided; modify the EvidenceSection component so the
Collapse (symbol: Collapse) opens sections by default by passing a
defaultActiveKey prop that contains the keys of collapseItems (symbol:
collapseItems) — e.g., compute the keys from collapseItems and pass them as
defaultActiveKey so all sections are expanded when the drawer opens; ensure keys
match the item.key values used to build collapseItems.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 600d47d4-8d70-4095-a80d-f0fdc58454e2
📒 Files selected for processing (13)
changelog/7560-evidence-drawer.yamlclients/admin-ui/src/features/privacy-assessments/AssessmentDetail.tsxclients/admin-ui/src/features/privacy-assessments/EvidenceCardGroup.module.scssclients/admin-ui/src/features/privacy-assessments/EvidenceCardGroup.tsxclients/admin-ui/src/features/privacy-assessments/EvidenceDrawer.module.scssclients/admin-ui/src/features/privacy-assessments/EvidenceDrawer.tsxclients/admin-ui/src/features/privacy-assessments/EvidenceSection.module.scssclients/admin-ui/src/features/privacy-assessments/EvidenceSection.tsxclients/admin-ui/src/features/privacy-assessments/QuestionGroupPanel.tsxclients/admin-ui/src/features/privacy-assessments/constants.tsclients/admin-ui/src/features/privacy-assessments/index.tsclients/admin-ui/src/features/privacy-assessments/types.tsclients/admin-ui/src/features/privacy-assessments/utils.ts
Comment on lines
+50
to
+63
| if (!query.trim()) { | ||
| return items; | ||
| } | ||
| const lower = query.toLowerCase(); | ||
| return items.filter( | ||
| (item) => | ||
| item.value.toLowerCase().includes(lower) || | ||
| (SOURCE_TYPE_LABELS[item.source_type] ?? item.source_type) | ||
| .toLowerCase() | ||
| .includes(lower) || | ||
| (FIELD_NAME_LABELS[item.field_name] ?? item.field_name.replace(/_/g, " ")) | ||
| .toLowerCase() | ||
| .includes(lower), | ||
| ); |
There was a problem hiding this comment.
Normalize the trimmed query before filtering.
Line 50 trims only for emptiness, but Line 53 uses the untrimmed query for matching. Inputs like " policy " won’t match expected results.
Proposed fix
export const filterEvidence = (
items: EvidenceItem[],
query: string,
): EvidenceItem[] => {
- if (!query.trim()) {
+ const normalizedQuery = query.trim().toLowerCase();
+ if (!normalizedQuery) {
return items;
}
- const lower = query.toLowerCase();
return items.filter(
(item) =>
- item.value.toLowerCase().includes(lower) ||
+ item.value.toLowerCase().includes(normalizedQuery) ||
(SOURCE_TYPE_LABELS[item.source_type] ?? item.source_type)
.toLowerCase()
- .includes(lower) ||
+ .includes(normalizedQuery) ||
(FIELD_NAME_LABELS[item.field_name] ?? item.field_name.replace(/_/g, " "))
.toLowerCase()
- .includes(lower),
+ .includes(normalizedQuery),
);
};🤖 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/utils.ts` around lines 50 -
63, The filter uses the untrimmed query for matching: after the early-return
check it sets lower = query.toLowerCase() but query wasn't trimmed; change to
compute lower from query.trim() (e.g., const lower = query.trim().toLowerCase())
so items.filter comparisons (item.value, SOURCE_TYPE_LABELS[item.source_type],
FIELD_NAME_LABELS[item.field_name] and item.field_name.replace(/_/g, " ")) use
the normalized/truncated query for matching.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
clients/admin-ui/src/features/privacy-assessments/AssessmentCard.tsx (1)
127-129: Add contextual accessible label to the “View” button.“View” is generic in assistive-tech button lists. Add an
aria-labelincluding the assessment name.Suggested tweak
- <Button type="link" className="p-0" onClick={onClick}> + <Button + type="link" + className="p-0" + onClick={onClick} + aria-label={`View assessment ${assessment.name}`} + > View </Button>🤖 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/AssessmentCard.tsx` around lines 127 - 129, The "View" Button in AssessmentCard.tsx is too generic for screen-reader users; update the Button (inside the AssessmentCard component) to include an aria-label that incorporates the assessment's name/title (e.g., use the assessment.name or assessment.title prop/state used in this component) so each button reads something like "View assessment {name}" and preserve the existing onClick handler and className. Ensure the aria-label value is derived from the same identifier used elsewhere in the card to avoid mismatches.
🤖 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/AssessmentCard.tsx`:
- Around line 62-65: The component's completion logic (completionDate) and the
completion badge render in AssessmentCard incorrectly treat assessments with
completeness === 100 as completed even when status ===
AssessmentStatus.OUTDATED; update the logic in the Completion computation and
the area that renders the completion badge/text (references: completionDate,
isComplete, assessment.updated_at, and the rendering block that shows
"Assessment complete") to first check for assessment.status ===
AssessmentStatus.OUTDATED and, if outdated, use the statusLabel (or render the
outdated label/badge) instead of the completed text/date so outdated assessments
are never shown as "Completed".
---
Nitpick comments:
In `@clients/admin-ui/src/features/privacy-assessments/AssessmentCard.tsx`:
- Around line 127-129: The "View" Button in AssessmentCard.tsx is too generic
for screen-reader users; update the Button (inside the AssessmentCard component)
to include an aria-label that incorporates the assessment's name/title (e.g.,
use the assessment.name or assessment.title prop/state used in this component)
so each button reads something like "View assessment {name}" and preserve the
existing onClick handler and className. Ensure the aria-label value is derived
from the same identifier used elsewhere in the card to avoid mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f09f3282-999c-4cf3-a817-1e4b7a7fac57
📒 Files selected for processing (1)
clients/admin-ui/src/features/privacy-assessments/AssessmentCard.tsx
Comment on lines
+62
to
+65
| const completionDate = | ||
| isComplete && assessment.updated_at | ||
| ? `Completed on ${formatDate(assessment.updated_at, { showTime: false })}` | ||
| : statusLabel; |
There was a problem hiding this comment.
Outdated assessments can be mislabeled as completed.
For AssessmentStatus.OUTDATED with completeness === 100, Line 64 renders “Completed on …” and Lines 119-121 still show “Assessment complete,” which hides the outdated state.
Proposed fix
const riskLabel = riskLevel ? RISK_LEVEL_LABELS[riskLevel] : "N/A";
const statusLabel = status ? ASSESSMENT_STATUS_LABELS[status] : "N/A";
const isComplete = completeness === 100;
+ const isOutdated = status === AssessmentStatus.OUTDATED;
const completionDate =
- isComplete && assessment.updated_at
+ isOutdated
+ ? statusLabel
+ : isComplete && assessment.updated_at
? `Completed on ${formatDate(assessment.updated_at, { showTime: false })}`
: statusLabel;
@@
- <Text strong type="success" size="sm">
- Assessment complete
- </Text>
+ <Text strong type={isOutdated ? "danger" : "success"} size="sm">
+ {isOutdated ? "Assessment outdated" : "Assessment complete"}
+ </Text>Also applies to: 119-124
🤖 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/AssessmentCard.tsx` around
lines 62 - 65, The component's completion logic (completionDate) and the
completion badge render in AssessmentCard incorrectly treat assessments with
completeness === 100 as completed even when status ===
AssessmentStatus.OUTDATED; update the logic in the Completion computation and
the area that renders the completion badge/text (references: completionDate,
isComplete, assessment.updated_at, and the rendering block that shows
"Assessment complete") to first check for assessment.status ===
AssessmentStatus.OUTDATED and, if outdated, use the statusLabel (or render the
outdated label/badge) instead of the completed text/date so outdated assessments
are never shown as "Completed".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clients/admin-ui/src/features/privacy-assessments/types.ts (1)
220-220: UsePartial<Record<>>forby_typeto accurately represent sparse API responses.At Line 220,
Record<EvidenceType, EvidenceItem[]>requires all 4 enum keys to exist wheneverby_typeis present. In practice, grouped responses contain only a subset of evidence types, making the current type overly strict and potentially causing type-checking friction when accessing the property.Proposed change
- by_type?: Record<EvidenceType, EvidenceItem[]>; + by_type?: Partial<Record<EvidenceType, EvidenceItem[]>>;🤖 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/types.ts` at line 220, Update the optional by_type property to allow sparse keys by changing its type from Record<EvidenceType, EvidenceItem[]> to Partial<Record<EvidenceType, EvidenceItem[]>>; locate the by_type declaration in the same module (the symbol is by_type and the related types are EvidenceType and EvidenceItem) and replace the type accordingly, and then update any call sites that assumed every EvidenceType key exists to handle possibly undefined entries (e.g., check for undefined or use optional chaining/defaults).
🤖 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/types.ts`:
- Line 220: Update the optional by_type property to allow sparse keys by
changing its type from Record<EvidenceType, EvidenceItem[]> to
Partial<Record<EvidenceType, EvidenceItem[]>>; locate the by_type declaration in
the same module (the symbol is by_type and the related types are EvidenceType
and EvidenceItem) and replace the type accordingly, and then update any call
sites that assumed every EvidenceType key exists to handle possibly undefined
entries (e.g., check for undefined or use optional chaining/defaults).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88a3e952-513b-4436-8ae1-e7319a0a241e
📒 Files selected for processing (2)
clients/admin-ui/src/features/privacy-assessments/constants.tsclients/admin-ui/src/features/privacy-assessments/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- clients/admin-ui/src/features/privacy-assessments/constants.ts
This was referenced Mar 6, 2026
mfbrown
pushed a commit
that referenced
this pull request
Mar 12, 2026
Co-authored-by: Karolis Krulis <karolis@ethyca.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-2748
Description Of Changes
Adds an evidence drawer to the privacy assessments detail page, allowing users to view the complete evidence trail for any question group. Clicking "View evidence" on a question group panel opens a right-side drawer showing all evidence items collected for that section, organized into "System-derived data" and "Human input" categories using collapsible panels.
Evidence items are deduplicated (same source/field/value across multiple questions are shown once) and filterable via a search bar that matches against evidence values, source type labels, and field name labels.
Note: I have only been able to test system data, team data will be tested with Slack once all PRs are merged.
Code Changes
clients/admin-ui/src/features/privacy-assessments/AssessmentDetail.tsx- Wires up drawer open/close state, focused group tracking, evidence deduplication, and search filtering; rendersEvidenceDrawerclients/admin-ui/src/features/privacy-assessments/EvidenceDrawer.tsx- New right-side Ant DesignDrawerwith search bar andEvidenceSectioncontentclients/admin-ui/src/features/privacy-assessments/EvidenceDrawer.module.scss- Drawer layout styles (sticky search bar, scrollable body, layout background)clients/admin-ui/src/features/privacy-assessments/EvidenceSection.tsx- New component rendering a question group's evidence split into system-derived and human-input collapse panels; shows empty/no-match statesclients/admin-ui/src/features/privacy-assessments/EvidenceSection.module.scss- Collapse padding overridesclients/admin-ui/src/features/privacy-assessments/EvidenceCardGroup.tsx- New component rendering a list of flatEvidenceItemcards with source type, field name, value, and dateclients/admin-ui/src/features/privacy-assessments/EvidenceCardGroup.module.scss- Card layout stylesclients/admin-ui/src/features/privacy-assessments/QuestionGroupPanel.tsx- Enables the "View evidence" button (wasdisabled); usesanswered_count/total_countfrom the group directly instead of computing from question answer textclients/admin-ui/src/features/privacy-assessments/types.ts- SimplifiesEvidenceItemto a flat interface matching the API; addsanswered_count/total_counttoQuestionGroup; removes the complex discriminated union and associated sub-typesclients/admin-ui/src/features/privacy-assessments/utils.ts- AddsdeduplicateEvidenceandfilterEvidencepure helpersclients/admin-ui/src/features/privacy-assessments/constants.ts- AddsSOURCE_TYPE_LABELSandFIELD_NAME_LABELSmaps for display formattingclients/admin-ui/src/features/privacy-assessments/index.ts- Exports newEvidenceDrawerandEvidenceSectioncomponentsSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit
New Features
Style