UX-26: Review collapsible detail sections and risk color-coding#641
UX-26: Review collapsible detail sections and risk color-coding#641Chris0Jeky merged 3 commits intomainfrom
Conversation
- Make Affected cards, Planned changes, and Technical details sections collapsible (collapsed by default) to reduce visual overload - Color-code risk levels: Low=green, Medium=amber, High/Critical=red using existing design tokens, with text labels for accessibility - Hide triage run IDs under Technical details toggle - Consolidate provenance link buttons (Open Capture, Review Link, Open Board) into a Links dropdown menu - Show affected cards as count badge on collapsed toggle header - Update tests to expand sections before asserting collapsed content Closes #626
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Self-review of #641What works well
Potential concerns reviewed
VerdictNo blocking issues found. The implementation matches the issue requirements. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ReviewView component to introduce collapsible sections for proposal details and a new color-coded risk badge system. Feedback focuses on improving keyboard accessibility for the links dropdown, adopting more idiomatic Vue 3 reactivity for managing expanded states, and simplifying the risk level class logic. Additionally, it is recommended to hide decorative icons from screen readers to ensure a better experience for users with assistive technologies.
| function closeLinkDropdown() { | ||
| openLinkDropdown.value = null | ||
| } |
There was a problem hiding this comment.
The current closeLinkDropdown implementation, when paired with @blur on the trigger button, breaks keyboard accessibility. When a user tabs from the button into the dropdown menu, the blur event fires and closes the menu before the focus can reach the links inside.
Updating the logic to check relatedTarget ensures the dropdown only closes if focus actually leaves the dropdown component area, allowing keyboard users to navigate the menu items.
function closeLinkDropdown(event: FocusEvent) {
const nextFocus = event.relatedTarget as HTMLElement
if (nextFocus?.closest('.td-review-card__links-dropdown-wrapper')) {
return
}
openLinkDropdown.value = null
}
| const expandedSections = ref<Record<string, Set<string>>>({}) | ||
|
|
||
| function isSectionExpanded(proposalId: string, section: string): boolean { | ||
| return expandedSections.value[proposalId]?.has(section) ?? false | ||
| } | ||
|
|
||
| function toggleSection(proposalId: string, section: string) { | ||
| if (!expandedSections.value[proposalId]) { | ||
| expandedSections.value[proposalId] = new Set() | ||
| } | ||
| const sections = expandedSections.value[proposalId] | ||
| if (sections.has(section)) { | ||
| sections.delete(section) | ||
| } else { | ||
| sections.add(section) | ||
| } | ||
| // Trigger reactivity | ||
| expandedSections.value = { ...expandedSections.value } | ||
| } |
There was a problem hiding this comment.
The current implementation of expandedSections using a Record of Sets requires manual reactivity triggering via object spreading ({ ...expandedSections.value }) because Vue's reactivity system does not automatically track internal mutations of a Set unless the Set itself is made reactive.
In Vue 3, it is more idiomatic and efficient to use a nested reactive object. Since ref on an object is deep, mutations to nested properties (like expandedSections.value[id][section]) will be tracked automatically, avoiding the need to recreate the entire state object on every toggle.
const expandedSections = ref<Record<string, Record<string, boolean>>>({})
function isSectionExpanded(proposalId: string, section: string): boolean {
return !!expandedSections.value[proposalId]?.[section]
}
function toggleSection(proposalId: string, section: string) {
if (!expandedSections.value[proposalId]) {
expandedSections.value[proposalId] = {}
}
expandedSections.value[proposalId][section] = !expandedSections.value[proposalId][section]
}
| function riskLevelClass(riskLevel: ApiProposal['riskLevel']): string { | ||
| const normalized = normalizeProposalRiskLevel(riskLevel) | ||
| if (normalized === 'Low') return 'td-risk--low' | ||
| if (normalized === 'Medium') return 'td-risk--medium' | ||
| if (normalized === 'High') return 'td-risk--high' | ||
| if (normalized === 'Critical') return 'td-risk--critical' | ||
| return 'td-risk--low' | ||
| } |
There was a problem hiding this comment.
This function can be simplified by using a mapping object instead of multiple if statements. This improves readability and makes the code easier to maintain if more risk levels are added in the future.
function riskLevelClass(riskLevel: ApiProposal['riskLevel']): string {
const riskClasses: Record<string, string> = {
Low: 'td-risk--low',
Medium: 'td-risk--medium',
High: 'td-risk--high',
Critical: 'td-risk--critical',
}
return riskClasses[normalizeProposalRiskLevel(riskLevel)] ?? 'td-risk--low'
}
| @click="toggleSection(proposal.id, 'entities')" | ||
| > | ||
| Open Board | ||
| <span class="td-review-card__collapse-icon" :class="{ 'td-review-card__collapse-icon--open': isSectionExpanded(proposal.id, 'entities') }">▶</span> |
There was a problem hiding this comment.
The decorative collapse icon should be hidden from screen readers using aria-hidden="true" to avoid redundant announcements, as the parent button already provides a descriptive label and an aria-expanded state. This applies to all three collapsible sections in this view.
<span class="td-review-card__collapse-icon" :class="{ 'td-review-card__collapse-icon--open': isSectionExpanded(proposal.id, 'entities') }" aria-hidden="true">▶</span>
Adversarial Code Review -- PR #641CRITICAL: E2E test will breakSeverity: Critical | File: await proposalCard.getByRole('button', { name: 'Open Board' }).click()The E2E test expects "Open Board" to be a directly visible button on the proposal card. After this PR, that button is buried two levels deep: first inside a collapsed "Technical details" section, then inside a "Links" dropdown. This test will fail because Playwright cannot find a hidden element with HIGH: Dropdown close-on-blur is fragile and racySeverity: High | File: The Links dropdown uses
HIGH: Missing
|
| Severity | Count |
|---|---|
| Critical | 1 (E2E breakage) |
| High | 2 (dropdown behavior, missing aria-controls) |
| Medium | 3 (stale state, dead types, dropped test coverage) |
| Low | 2 (menu pattern, duplicate removal) |
The critical E2E breakage at first-run.spec.ts:142 must be fixed before merge. The dropdown @blur close mechanism needs a more robust approach (e.g., a click-outside directive or focus-within CSS check). The accessibility gaps (missing aria-controls, incomplete menu keyboard pattern) should be addressed to avoid regression against accessibility standards.
Address Gemini review findings: - Replace Set-based expandedSections with Record<bool> for proper Vue reactivity tracking without manual object spreading - Fix dropdown @blur to check relatedTarget so keyboard users can tab into the dropdown menu without it closing - Add aria-hidden="true" to decorative collapse icons
The "Open Board" button moved inside a collapsed Technical details section and Links dropdown. Update first-run E2E to expand section and dropdown before clicking.
1 participant
Summary
Test plan
npm run typecheckpassesnpm run buildsucceedsCloses #626