UI polish: branded states, mobile reflow, WCAG AA#72
Conversation
Final craftsmanship pass across the dashboard SPA: Branded states (web/src/components/states.tsx) - EmptyState gains optional light line-art illustrations and a next-step CTA (backward compatible: existing title/hint callers unchanged). Added four on-brand illustrations (search, shield-check, chart, box). - NotFoundState is branded with the box illustration + CTA slot. - New RouteErrorBoundary: a friendly, full-page recovery screen wired as the router's errorElement so render/loader crashes never show a blank page. - Findings / Leaderboard / PRDetail empty states now use illustrations + CTAs. Mobile reflow (Leaderboard + base.css) - Leaderboard table + author-drilldown PRs table now carry data-label / cell-primary, so they reflow into stacked cards on phones via the existing 640px pattern (Findings / PRDetail already did). - Touch targets: interactive controls get a >=44px hit area at <=640px (buttons, segmented pickers, PR tabs, nav rows, bulk-select checkboxes). WCAG AA + keyboard (tokens.css + Leaderboard) - Darkened/lightened the muted text tokens to clear AA: --text-3 >= 4.5:1 (normal text) and --text-4 >= 3:1 (graphical floor) on every surface they render on, in both themes. --text-2 already passed and is unchanged. - Sortable column headers and clickable author rows are now keyboard-operable (tabindex + Enter/Space) with aria-sort / aria-pressed. Verified: web build + smoke:dashboard pass; mobile widths spot-checked at 390px (reflow, branded empty state, 404); no console errors; no unfocusable clickables. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughPolished the dashboard UI styling by adding branded empty and error-state presentation, improving mobile touch targets, and refining sortable table interactions. Strengthened accessibility and responsive behavior without changing backend or API functionality. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested Labels
Risk AssessmentScore: 10/100 — 🟢 Low
🧭 Description Drift
Compares PR description claims to the actual diff. Update the description (or the code) so they tell the same story. ✍️ Commit Message Coach2 of 8 commit messages could be stronger.
Tip: imperative mood ( 🔁 Changes since last reviewed
📊 Confidence breakdown2 findings — 🟢 1 high · 🟡 1 medium · 🔴 0 low. Treat the medium / low ones as hypotheses to verify. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in your ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Generate docstrings (beta)
🧹 Simplify (beta)
🪄 Autofix unresolved comments (beta)
Comment |
![diffsentry[bot]](https://avatars.githubusercontent.com/in/3360270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
The UI polish is strong overall: the new branded states are cohesive, the router-level error boundary is a meaningful UX improvement, and the mobile/card reflow changes are consistent with the existing dashboard patterns. I found two accessibility regressions in the new keyboard support: sortable table headers are made focusable without button semantics, and data rows are turned into faux buttons without preserving native table interaction semantics. Those should be corrected before merge.
🧹 Nitpick comments (1)
web/src/components/states.tsx (1)
Line 302: Hardcoded user-facing string in JSX text📝 Documentation | 🟡 Minor
Hardcoded user-facing string in JSX text
Hardcoded strings inside JSX bypass the translation pipeline and break for non-English users.
Suggested fix: Wrap in your i18n helper, e.g.
{t('users.greeting', { name })}instead of literal text.DiffSentry built-in pattern check — disable globally with
reviews.builtin_patterns: false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/components/states.tsx at line 302, the line matches the "Hardcoded user-facing string in JSX text" anti-pattern. Wrap in your i18n helper, e.g. `{t('users.greeting', { name })}` instead of literal text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/components/states.tsx at line 302, the line matches the "Hardcoded user-facing string in JSX text" anti-pattern. Wrap in your i18n helper, e.g. `{t('users.greeting', { name })}` instead of literal text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `web/src/components/states.tsx`:
- Line 302: In web/src/components/states.tsx at line 302, the line matches the "Hardcoded user-facing string in JSX text" anti-pattern. Wrap in your i18n helper, e.g. `{t('users.greeting', { name })}` instead of literal text.
In `web/src/pages/Leaderboard.tsx`:
- Line 122: In web/src/pages/Leaderboard.tsx around line 122, replace the keyboard/click behavior attached directly to the sortable `th` elements with a real button rendered inside each header cell. Keep `aria-sort` on the `th`, move `onClick` to the inner button, and remove `tabIndex`/`onKeyDown` from the header so sorting uses native interactive semantics.
- Line 184: In web/src/pages/Leaderboard.tsx around line 184, remove the faux-button semantics from the author `<tr>` elements and move the selection interaction onto a real control inside the row, preferably in the author cell. Keep the visual clickable-row styling if desired, but use a native `button` or `link` to call `selectAuthor` and expose the selected state accessibly.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 3c550f43-cdae-4568-a81b-50612a567037
📥 Commits
Reviewing files at c101453 (base SHA unavailable).
📒 Files selected for processing (7)
web/src/components/states.tsxweb/src/pages/Findings.tsxweb/src/pages/Leaderboard.tsxweb/src/pages/PRDetail.tsxweb/src/router.tsxweb/src/styles/base.cssweb/src/styles/tokens.css
📌 Status — last updated
|
| Risk score | 10/100 (Low) █▄▂▂▂▅▄▁ |
| Unresolved threads | 2 |
| Failing checks | 0 |
| Pending checks | 1 |
| Files reviewed | 1 (6 skipped) |
| Updated | 2026-06-28 22:13Z |
Live-updated by DiffSentry on every push. Use @diffsentry ship for a verdict, @diffsentry timeline for full history.
Address DiffSentry review: replace the focusable-th / role="button"-row workarounds with real nested controls (ARIA sortable-table pattern). - Sortable column headers render a <button class="th-sort"> inside each <th>; aria-sort stays on the th. Removed tabIndex/onKeyDown from the header. - Author drill-in moves onto a real <button class="row-link"> in the author cell with aria-pressed; the <tr> no longer carries role/tabindex/onKeyDown. Row keeps a visual selected state (.row-selected). Subtitle updated to "click an author to drill in". - CSS: add .th-sort / .row-link / .row-selected, retire .tbl tr.clickable rules; mobile 44px touch target now targets .row-link. web build + smoke:dashboard pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
The UI polish is generally solid and moves the dashboard toward better mobile and accessibility behavior. The main regression risk is in the new sortable-header button styling: the CSS currently restyles the inner button but does not neutralize native button chrome, so sortable table headers are likely to render with browser-default button appearance and inconsistent sizing across browsers/themes. I also found a small semantic/accessibility gap around the row-selection control lacking an explicit accessible name that describes the toggle action.
🧹 Nitpick comments (1)
web/src/pages/Leaderboard.tsx (1)
Line 184: The author drill-in toggle should expose an explicit action-oriented accessible name.🛠️ Refactor suggestion | 🟡 Minor
The author drill-in toggle should expose an explicit action-oriented accessible name.
🤔 Medium confidence — verify against intent before acting.
The row interaction is now on a real button, which is the right accessibility improvement, but the button text is only the raw author name. Screen-reader users will hear something like
alice, button, pressed, which does not explain what pressing it does or what the pressed state means.Because this control toggles a drill-down panel, give it an explicit label such as
Show details for alice/Hide details for alice, or addaria-expandedtied to the drill-down state if it conceptually controls expanded content. That makes the control's purpose clear without relying on surrounding table context.🔧 Proposed fix
<button type="button" className="row-link" aria-pressed={isSel} aria-label={`${isSel ? "Hide" : "Show"} details for ${a.author}`} onClick={() => selectAuthor(isSel ? null : a.author)} > {a.author} </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/pages/Leaderboard.tsx around line 184, update the `row-link` button rendered for each author so its accessible name describes the toggle action instead of exposing only the raw author text. Add an `aria-label` based on `isSel` and `a.author`, and consider `aria-expanded` as well if this button is intended to represent opening and closing the author drilldown.♻️ Suggested fix
<button type="button" className="row-link" aria-pressed={isSel} aria-label={`${isSel ? "Hide" : "Show"} details for ${a.author}`} onClick={() => selectAuthor(isSel ? null : a.author)} > {a.author} </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/pages/Leaderboard.tsx around line 184, update the `row-link` button rendered for each author so its accessible name describes the toggle action instead of exposing only the raw author text. Add an `aria-label` based on `isSel` and `a.author`, and consider `aria-expanded` as well if this button is intended to represent opening and closing the author drilldown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `web/src/styles/base.css`:
- Line 5011: In web/src/styles/base.css around line 5011, fully reset the new `.th-sort` button so it visually behaves like the old clickable header cell; remove native button chrome, zero out padding/border/background, and make it fill the header width. Keep `aria-sort` on the `<th>`, but ensure the button itself does not introduce cross-browser default styling or shrink the clickable area.
In `web/src/pages/Leaderboard.tsx`:
- Line 184: In web/src/pages/Leaderboard.tsx around line 184, update the `row-link` button rendered for each author so its accessible name describes the toggle action instead of exposing only the raw author text. Add an `aria-label` based on `isSel` and `a.author`, and consider `aria-expanded` as well if this button is intended to represent opening and closing the author drilldown.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 18308698-907a-4f6c-a845-6ae7dc7f9542
📥 Commits
Reviewing files that changed from c101453 to a998470. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (2)
web/src/pages/Leaderboard.tsxweb/src/styles/base.css
🚧 Files skipped from review as they are similar to previous changes (5)
web/src/components/states.tsxweb/src/pages/Findings.tsxweb/src/pages/PRDetail.tsxweb/src/router.tsxweb/src/styles/tokens.css
Address DiffSentry re-review of the sort/row refactor:
- .th-sort: full native-button reset (appearance:none, width:100%) so the
control adopts the header cell's presentation with no default button chrome
in Safari/Firefox and the clickable area fills the cell. Numeric/right
headers keep their label flush-right via justify-content (not text-align,
which wouldn't move flex items).
- Author drill-in button: aria-expanded (disclosure semantics for the
drilldown panel) + an action-oriented aria-label ("Show/Hide details for
<author>") so the accessible name conveys purpose and state, not just the
raw name.
Verified: web build + smoke pass; computed styles confirmed in-browser
(appearance:none, width fills cell, left/right header alignment) past the PWA
service-worker cache; selected-row styling intact.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
The UI polish is generally solid and moves the leaderboard interactions onto native controls, which is a meaningful accessibility improvement. I found one regression in the table sorting UX: the active sort state is no longer exposed on hover/focus because the color styling moved to the nested button while aria-sort remains on the <th>, leaving the sorted header visually indistinct unless the caret is noticed. I also have a minor suggestion to make the selected author control semantics align with its expand/collapse behavior.
🧹 Nitpick comments (1)
web/src/pages/Leaderboard.tsx (1)
Line 184: The author row control exposes disclosure semantics without a controlled region.🛠️ Refactor suggestion | 🟡 Minor
The author row control exposes disclosure semantics without a controlled region.
The new author selector button uses
aria-expanded, which implies it controls the visibility of an associated region. In this diff there is no matchingaria-controlsor directly-associated disclosure panel per row, and the drilldown content is rendered separately below the table. That makes the state announcement less precise than it could be and may confuse assistive tech users because the button sounds like an in-place expander.If this is intended as a toggle/select action rather than a true disclosure widget,
aria-pressedis a better semantic fit. It communicates selection state without implying DOM ownership of a controlled panel.🔧 Proposed fix
<button type="button" className="row-link" aria-pressed={isSel} aria-label={`${isSel ? "Hide" : "Show"} details for ${a.author}`} onClick={() => selectAuthor(isSel ? null : a.author)} > {a.author} </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/pages/Leaderboard.tsx around line 184, change the author selection button semantics from disclosure to toggle by replacing `aria-expanded` with `aria-pressed` on `row-link`. Keep the existing selection behavior and label text, but make the accessibility state match the actual interaction pattern where `selectAuthor` toggles the selected author rather than expanding an owned panel.♻️ Suggested fix
<button type="button" className="row-link" aria-pressed={isSel} aria-label={`${isSel ? "Hide" : "Show"} details for ${a.author}`} onClick={() => selectAuthor(isSel ? null : a.author)} > {a.author} </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/pages/Leaderboard.tsx around line 184, change the author selection button semantics from disclosure to toggle by replacing `aria-expanded` with `aria-pressed` on `row-link`. Keep the existing selection behavior and label text, but make the accessibility state match the actual interaction pattern where `selectAuthor` toggles the selected author rather than expanding an owned panel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `web/src/pages/Leaderboard.tsx`:
- Line 184: In web/src/pages/Leaderboard.tsx around line 184, change the author selection button semantics from disclosure to toggle by replacing `aria-expanded` with `aria-pressed` on `row-link`. Keep the existing selection behavior and label text, but make the accessibility state match the actual interaction pattern where `selectAuthor` toggles the selected author rather than expanding an owned panel.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 9f5f85c8-6141-4dad-8038-96fb032c4a69
📥 Commits
Reviewing files that changed from a998470 to bf4e096. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (2)
web/src/pages/Leaderboard.tsxweb/src/styles/base.css
🚧 Files skipped from review as they are similar to previous changes (5)
web/src/components/states.tsxweb/src/pages/Findings.tsxweb/src/pages/PRDetail.tsxweb/src/router.tsxweb/src/styles/tokens.css
The author button has no aria-controls / per-row disclosure panel — the drilldown renders separately below the table — so aria-expanded overstated the relationship. The control is a toggle-selection (one author at a time), so aria-pressed is the accurate state semantic; the action-oriented aria-label still conveys the show/hide effect. Per DiffSentry review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
The UI polish is solid overall: the leaderboard interactions were moved onto native controls, the mobile reflow hooks are applied consistently, and the empty state improvements are straightforward and low risk. I found one accessibility regression in the sortable header implementation: the current button label does not expose sort state to screen readers, so non-visual users lose important feedback that was previously implied by the interactive header pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `web/src/pages/Leaderboard.tsx`:
- Line 128: In web/src/pages/Leaderboard.tsx around line 128, update the sortable header `button.th-sort` inside `Th` so the control itself exposes sort state and action to assistive technology. Keep `aria-sort` on the `<th>`, but add an `aria-label` (or equivalent button-level accessible text) based on `sort.key`, `sort.dir`, and `label`, and mark the visual `sort-caret` as `aria-hidden` so it does not become the only state signal.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 3fb70506-7803-49b0-b3b5-6b476aed947c
📥 Commits
Reviewing files that changed from bf4e096 to 921758a. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (1)
web/src/pages/Leaderboard.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
web/src/components/states.tsxweb/src/pages/Findings.tsxweb/src/pages/PRDetail.tsxweb/src/router.tsxweb/src/styles/base.cssweb/src/styles/tokens.css
A tabbing screen-reader user hears the focused button's name, not the th's
aria-sort, so the sort control could read as just "Author, button". Give each
.th-sort button an aria-label spelling out the current sort state and the next
action ("Author, sorted ascending. Activate to sort descending."), and mark the
decorative caret glyph aria-hidden. aria-sort stays on the th; the accessible
name still leads with the visible column label (WCAG 2.5.3). Per DiffSentry.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
The accessibility and mobile-reflow work is directionally solid: moving sort and drilldown interactions onto native buttons is a clear improvement, and the empty-state enhancements fit the page well. I found one significant regression in the sort-header semantics and one smaller maintainability issue around the inline width styling.
🧹 Nitpick comments (1)
web/src/pages/Leaderboard.tsx (1)
Line 205: The fixed inline trend-column width makes the mobile card reflow harder to maintain.🛠️ Refactor suggestion | 🟡 Minor
The fixed inline trend-column width makes the mobile card reflow harder to maintain.
The new
data-labelmarkup is intended to let the existing small-screen table-to-card CSS take over, but this cell still carriesstyle={{ width: 130 }}inline. Inline width rules are harder to override in the responsive stylesheet and can leave the trend row cramped or inconsistent once the layout collapses into stacked cards.Move this width into a semantic class instead of hardcoding it inline. That keeps the responsive behavior controllable from CSS and avoids mixing presentational concerns into the render path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/pages/Leaderboard.tsx around line 205, remove the inline `style={{ width: 130 }}` from the Trend `<td>` and replace it with a dedicated class name such as `trend-col`; then define the desktop-only width in the relevant stylesheet so the mobile card reflow can override it cleanly. Keep the existing `data-label="Trend"` and alignment classes unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/pages/Leaderboard.tsx around line 205, remove the inline `style={{ width: 130 }}` from the Trend `<td>` and replace it with a dedicated class name such as `trend-col`; then define the desktop-only width in the relevant stylesheet so the mobile card reflow can override it cleanly. Keep the existing `data-label="Trend"` and alignment classes unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `web/src/pages/Leaderboard.tsx`:
- Line 124: In web/src/pages/Leaderboard.tsx around line 124, update the `Th` helper so the focused `button.th-sort` exposes sort state directly instead of relying only on `aria-sort` on the parent `th`; add `aria-pressed` for the active sort column while preserving the existing `aria-label` text. Keep `aria-sort` on the `th` for table semantics, but make sure the interactive control itself communicates state to assistive technology.
- Line 205: In web/src/pages/Leaderboard.tsx around line 205, remove the inline `style={{ width: 130 }}` from the Trend `<td>` and replace it with a dedicated class name such as `trend-col`; then define the desktop-only width in the relevant stylesheet so the mobile card reflow can override it cleanly. Keep the existing `data-label="Trend"` and alignment classes unchanged.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 95009631-0413-4bf7-b5c2-9c909973f387
📥 Commits
Reviewing files that changed from 921758a to 247cde9. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (1)
web/src/pages/Leaderboard.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
web/src/components/states.tsxweb/src/pages/Findings.tsxweb/src/pages/PRDetail.tsxweb/src/router.tsxweb/src/styles/base.cssweb/src/styles/tokens.css
…ride
The Trend cell carried an inline style={{ width: 130 }}, whose specificity
beats the card-reflow's `.tbl td { width: 100% }`, pinning the cell to 130px on
phones. Moved it to `.tbl td.trend-col` scoped under @media (min-width: 641px),
so desktop keeps the 130px column and the ≤640px reflow expands it to full card
width. Verified in-browser: 130px at 1381px, full width at 390px. Per DiffSentry.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
The accessibility direction here is solid: the sortable headers now use native buttons, the author drilldown is keyboard-operable, and the mobile/card reflow work is consistent with the existing table pattern. I found one functional regression in the mobile touch-target styling and one maintainability issue where the new button reset is incomplete and can produce inconsistent rendering across browsers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `web/src/styles/base.css`:
- Line 2888: In web/src/styles/base.css around line 2888, change the mobile checkbox rules so the actual tap target for the bulk-select control is at least 44px, not just the checkbox glyph size. Update the `td[data-label="Select"]` area and its immediate interactive wrapper/label so the hit area expands on phones while keeping the visible checkbox at 20px; verify this against the findings table markup.
- Line 5012: In web/src/styles/base.css around line 5012, add an explicit `:focus-visible` style for `.th-sort` because the button reset removes most native affordances. Use the existing accent tokens to create a clear keyboard-focus ring on sortable column headers and verify it remains visible in both themes.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 2e95e97d-e675-49fd-9328-93142f7e7ad4
📥 Commits
Reviewing files that changed from 247cde9 to 2d526b5. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (2)
web/src/pages/Leaderboard.tsxweb/src/styles/base.css
🚧 Files skipped from review as they are similar to previous changes (5)
web/src/components/states.tsxweb/src/pages/Findings.tsxweb/src/pages/PRDetail.tsxweb/src/router.tsxweb/src/styles/tokens.css
DiffSentry re-review of the mobile/sort work: - Bulk-select checkbox: sizing the <input> alone didn't enlarge its hit area. Wrapped each checkbox in a <label class="check-tap"> (Findings.tsx); on phones the label is a 44x44 tap target while the checkbox stays 20px. The label toggles the input natively. Verified 44x44 / 20x20 at 390px. - .th-sort: the native-button reset stripped affordances, so added an explicit :focus-visible ring (2px --accent-bright, inset via outline-offset:-2px so the card's overflow:hidden can't clip it). Verified offset -2px on keyboard focus, both themes via --accent-bright. web build passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
The UI polish is mostly solid and aligns with the stated goals around branded states, mobile touch targets, and semantic table controls. The CSS refactor improves semantics and responsiveness, but there are two accessibility regressions in the new error-page viewport sizing and the mobile checkbox hit-area implementation that should be fixed before merge.
🧹 Nitpick comments (1)
web/src/styles/base.css (1)
Line 2896: The mobile tap-target expansion misses the header select-all checkbox.
⚠️ Potential issue | 🟡 MinorThe mobile tap-target expansion misses the header select-all checkbox.
The new touch-target treatment only enlarges
.tbl td[data-label="Select"] .check-tap, but the bulk select control inFindings.tsxalso appears in the header inside a<th>. On phones, the row checkboxes get a 44px target while the select-all checkbox remains small, which breaks the stated consistency goal and leaves one of the key bulk actions below the intended touch size.Expand the selector to include the header wrapper as well, or target
.tbl .check-tapwithin the mobile breakpoint if there are no conflicting table uses.🔧 Proposed fix
diff @media (max-width: 640px) { .btn { min-height: 44px; } @@ - .tbl td[data-label="Select"] .check-tap { + .tbl th .check-tap, + .tbl td[data-label="Select"] .check-tap { min-width: 44px; min-height: 44px; } /* Drill-in author button gets a full-height tap target. */ .tbl tbody tr .row-link {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/styles/base.css around line 2896, extend the mobile `.check-tap` sizing so the select-all checkbox in table headers gets the same 44px touch target as row checkboxes. Update the selector in the `@media (max-width: 640px)` block to include the `<th>` wrapper used by `FindingsPage`.♻️ Suggested fix
diff @media (max-width: 640px) { .btn { min-height: 44px; } @@ - .tbl td[data-label="Select"] .check-tap { + .tbl th .check-tap, + .tbl td[data-label="Select"] .check-tap { min-width: 44px; min-height: 44px; } /* Drill-in author button gets a full-height tap target. */ .tbl tbody tr .row-link {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/styles/base.css around line 2896, extend the mobile `.check-tap` sizing so the select-all checkbox in table headers gets the same 44px touch target as row checkboxes. Update the selector in the `@media (max-width: 640px)` block to include the `<th>` wrapper used by `FindingsPage`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `web/src/styles/base.css`:
- Line 1935: In web/src/styles/base.css around line 1935, harden `.error-page` so the route error boundary remains usable when its content is taller than the viewport; add scrolling and avoid strict vertical centering on small screens. Update the `.error-page` rule and add a mobile media override so the recovery actions are always reachable under zoom or large-text conditions.
- Line 2896: In web/src/styles/base.css around line 2896, extend the mobile `.check-tap` sizing so the select-all checkbox in table headers gets the same 44px touch target as row checkboxes. Update the selector in the `@media (max-width: 640px)` block to include the `<th>` wrapper used by `FindingsPage`.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 29098076-1f7e-49a3-8a4a-724cf0d516cd
📥 Commits
Reviewing files that changed from 2d526b5 to 61fbf2b. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (2)
web/src/pages/Findings.tsxweb/src/styles/base.css
🚧 Files skipped from review as they are similar to previous changes (5)
web/src/components/states.tsxweb/src/pages/Leaderboard.tsxweb/src/pages/PRDetail.tsxweb/src/router.tsxweb/src/styles/tokens.css
DiffSentry re-review: - .error-page: top-align (align-items: flex-start) at <=640px so recovery content/actions are never centered partly off-screen on short/zoomed phone viewports. (The page already scrolls via min-height — not height — but top-alignment is the robust default for an error boundary.) - Extend the mobile 44px .check-tap rule to `.tbl th .check-tap` so the header select-all wrapper matches the row checkboxes. (The thead is clip-hidden in card mode, so this is belt-and-suspenders, but keeps the selector consistent with intent.) web build passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
The CSS changes are generally well-structured and align with the stated goals around branded empty states, mobile touch targets, and improved table interactivity. I found one accessibility regression in the sortable-header focus treatment that can hide the only visible keyboard focus indicator at the table edge, plus one maintainability cleanup around duplicated viewport-height declarations. Overall this is close, but the focus-visible issue should be fixed before merging.
🧹 Nitpick comments (1)
web/src/styles/base.css (1)
Line 1939: Duplicate viewport-height declarations should be consolidated with a fallback comment or utility.🛠️ Refactor suggestion | 🟢 Trivial
Duplicate viewport-height declarations should be consolidated with a fallback comment or utility.
🤔 Medium confidence — verify against intent before acting.
Using both
min-height: 100vhandmin-height: 100dvhis a valid compatibility pattern, but repeating it ad hoc makes future full-page layouts harder to keep consistent. This file is already accumulating several full-screen and responsive primitives, so this is a good place to centralize the viewport-height fallback pattern.A small utility class or a shared comment block would reduce drift if the team later adjusts the strategy for mobile browser UI chrome. This is low risk, but worth cleaning up while the error-page layout is being introduced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/styles/base.css around line 1939, consolidate the `100vh`/`100dvh` fallback pattern used by `.error-page` into a shared utility or clearly documented reusable block. Keep the current behavior unchanged, but reduce one-off duplication so future full-page layouts can reuse the same viewport-height convention consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In web/src/styles/base.css around line 1939, consolidate the `100vh`/`100dvh` fallback pattern used by `.error-page` into a shared utility or clearly documented reusable block. Keep the current behavior unchanged, but reduce one-off duplication so future full-page layouts can reuse the same viewport-height convention consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `web/src/styles/base.css`:
- Line 5072: In web/src/styles/base.css around line 5072, update the `:focus-visible` styling for `.tbl thead th.sortable .th-sort` so the keyboard focus indicator is reliably visible at table edges. Replace the negative `outline-offset` with a visible alternative such as a positive offset or an inset focus ring; verify the focus state remains obvious in both themes and on dense table headers.
- Line 1939: In web/src/styles/base.css around line 1939, consolidate the `100vh`/`100dvh` fallback pattern used by `.error-page` into a shared utility or clearly documented reusable block. Keep the current behavior unchanged, but reduce one-off duplication so future full-page layouts can reuse the same viewport-height convention consistently.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 714246ea-b66b-45f0-8270-d477ece31272
📥 Commits
Reviewing files that changed from 61fbf2b to 6016d9b. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (1)
web/src/styles/base.css
🚧 Files skipped from review as they are similar to previous changes (6)
web/src/components/states.tsxweb/src/pages/Findings.tsxweb/src/pages/Leaderboard.tsxweb/src/pages/PRDetail.tsxweb/src/router.tsxweb/src/styles/tokens.css
|
@diffsentry release-notes |
📣 Release Notes✨ What's new
🛠 Improvements
🐛 Fixes
Marketing-speak version of this PR. Re-run with |
1 participant
Final polish pass across the dashboard SPA. No backend or API changes — pure
web/UI work.1. Branded empty states + error boundary
EmptyStatenow accepts optional light line-artillustrationand a next-stepaction(CTA). Backward compatible — existingtitle/hint-only callers are unchanged.currentColorso a single accent-keyed highlight tints them.NotFoundStaterebranded with illustration + CTA; the in-app 404 links back to Overview.RouteErrorBoundarywired as the router'serrorElement— a friendly full-page recovery screen (logo, message, “Back to overview” / “Reload”) replaces the blank-screen/dev-overlay on any render or loader crash.2. Mobile reflow to card stacks (44px+ touch targets)
data-label/cell-primary, so they reflow into stacked cards on phones via the existing ≤640px pattern (Findings & PRDetail already had it).3. WCAG AA contrast + keyboard nav
tokens.css) to clear AA on every surface they render on, in both themes:--text-3≥ 4.5:1 (normal-text AA) — was 3.3–4.0 (failing).--text-4≥ 3:1 (graphical/large floor) for faint placeholder/zero/tick text.--text-2already passed and is left unchanged; the 3-tier hierarchy is preserved.tabindex+ Enter/Space) witharia-sort/aria-pressed.Verification
npm --prefix web run build✓ (typecheck + vite)npm run smoke:dashboard✓🤖 Generated with Claude Code
Summary
Polished the dashboard UI styling by adding branded empty and error-state presentation, improving mobile touch targets, and refining sortable table interactions. Strengthened accessibility and responsive behavior without changing backend or API functionality.
Changes
web/src/styles/base.css