Skip to content

Commit 683423a

Browse files
chore: apply /review polish for UX-gap branch
Review-driven follow-ups to 9ebcb3a (zero critical findings; 13 informational): - SidebarFooter: full-width skills.sh link gets focus-visible:ring-inset so the ring renders inside the control instead of clipping the footer border (matches AgentItem full-width convention); gear stays ring-2 (icon-button convention). - MarketplaceDashboard test: mount fetch asserted toBe(1) (was >0) — locks the thunk in-flight guard deduping StrictMode's double-mount; verified 3x green. - MarketplaceDashboard test: settled-empty case asserts the fresh TTL cache short-circuits the mount thunk (not.toHaveBeenCalled). - MarketplaceDetailPanel test: rename describe to 'trending placeholders' so it covers the loading/empty/error cases it actually exercises. - resolveTrendingView: compress JSDoc to the 1-line house rule. - DESIGN.md: add Focus Indicators rule, pin amber shades by role (400 status / 500 warning+bookmark), and split the skeleton a11y rule into primary (role=status) vs dense-grid decorative (aria-hidden) surfaces.
1 parent 9ebcb3a commit 683423a

5 files changed

Lines changed: 69 additions & 28 deletions

File tree

DESIGN.md

Lines changed: 57 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -66,14 +66,30 @@ The app uses OKLCH CSS variables, driven by `THEME_PRESETS` in
6666

6767
Status colors must remain semantically stable:
6868

69-
| Status | Color |
70-
| -------------------- | ----------------------- |
71-
| Valid / linked | `--success` fixed green |
72-
| Broken | Amber |
73-
| Missing | `--muted-foreground` |
74-
| Orphan / destructive | `--destructive` |
75-
| Local skill type | Emerald |
76-
| G-Stack skill type | `--gstack` |
69+
| Status | Color |
70+
| --------------------- | ------------------------ |
71+
| Valid / linked | `--success` fixed green |
72+
| Broken / inaccessible | Amber (see amber shades) |
73+
| Missing | `--muted-foreground` |
74+
| Orphan / destructive | `--destructive` |
75+
| Local skill type | Emerald |
76+
| G-Stack skill type | `--gstack` |
77+
78+
Amber shades — Tailwind ships several amber steps; pin them by role so the
79+
"needs review" hue stays consistent and reviews don't churn:
80+
81+
| Role | Class |
82+
| ---------------------------------------------------------- | ------------------------- |
83+
| Broken / inaccessible / needs-review status text + icons | `text-amber-400` |
84+
| Warning-dialog icon glyphs, bookmark / star, stat emphasis | `text-amber-500` |
85+
| Amber tint backgrounds (badges, status chips, fills) | `bg-amber-500/{10,15,20}` |
86+
87+
`text-amber-400` is the app-wide needs-review hue (`SymlinkStatus`, `badge`
88+
broken variant, `HealthWidget`); `text-amber-500` is the slightly stronger amber
89+
for warning affordances and the bookmark accent. Tint backgrounds always use the
90+
`amber-500` base at low alpha regardless of which text shade sits on them. The
91+
lone exception is `text-amber-300` for badge text on a denser amber tint, where
92+
the lighter step is needed for contrast.
7793

7894
Rules:
7995

@@ -350,11 +366,20 @@ Rules:
350366
structure) so the panel does not reflow when real data lands. (Linear, GitHub,
351367
Vercel load lists this way.)
352368
- A skeleton is silent to assistive tech: the visible "Loading…" text it
353-
replaces was announced; pulsing bars are not. So the skeleton container MUST
354-
carry `role="status"` plus a descriptive `aria-label` (e.g. "Loading trending
355-
skills"), and the inner placeholder rows are `aria-hidden`. Swapping announced
356-
text for a bare skeleton is an accessibility regression, not a neutral visual
357-
change.
369+
replaces was announced; pulsing bars are not. How to restore the announcement
370+
depends on whether the skeleton is a **primary loading surface** or a
371+
**decorative one in a dense grid**:
372+
- **Primary loading surface** — a main panel or list the user is actively
373+
waiting on (e.g. the Trending panel). The skeleton container MUST carry
374+
`role="status"` plus a descriptive `aria-label` (e.g. "Loading trending
375+
skills"), and the inner placeholder rows are `aria-hidden`. Swapping
376+
announced text for a bare skeleton here is an accessibility regression.
377+
- **Decorative skeleton in a dense widget grid** — many small widgets loading
378+
at once (e.g. `dashboard/widgets/*Skeleton`). It MAY instead be fully
379+
`aria-hidden`, because announcing each of N silhouettes would fire a burst of
380+
competing "Loading…" messages; the surrounding region's own status copy
381+
covers the wait. Pick one mode — never put `role="status"` and `aria-hidden`
382+
on the same element (they contradict).
358383
- A panel that renders a skeleton must actually request its data. A skeleton with
359384
no fetch behind it is a permanent broken-looking spinner; own the fetch on
360385
mount (guarded by a cache TTL) wherever the populated state is the intended
@@ -398,6 +423,25 @@ Baseline:
398423
- Use native controls before custom roles.
399424
- Do not remove focus outlines without a visible replacement.
400425

426+
Focus indicators:
427+
428+
The shared `button.tsx` is the source of truth — every variant ships
429+
`focus-visible:ring-1 ring-ring` (see Buttons). Use it for any standard button so
430+
the focus treatment is inherited, not re-invented. For focusable elements that
431+
genuinely can't use it (clickable rows, footer links, hand-rolled icon buttons):
432+
433+
- Always pair `focus-visible:outline-none` with a visible `focus-visible:ring-*
434+
ring-ring`, and always use `focus-visible`, never bare `focus` — bare `focus`
435+
also fires on pointer click, flashing a ring at mouse users.
436+
- Ring width tracks prominence, not a rigid tier: compact in-surface controls may
437+
match `button.tsx`'s `ring-1`; standalone hand-rolled controls commonly use
438+
`ring-2` (e.g. `BookmarkItem`, the sidebar gear). Both are fine as long as the
439+
ring reads clearly against `--ring`. Do not treat a single width as mandatory.
440+
- Full-width or edge-adjacent controls add `focus-visible:ring-inset` so the ring
441+
renders inside the control instead of clipping against a container border —
442+
full-width rows (`AgentItem`), the footer `skills.sh` link, and underline-style
443+
tabs all use the inset ring.
444+
401445
Target sizing:
402446

403447
This is a pointer-driven desktop app (mouse and trackpad), so the mobile 44px

src/renderer/src/components/marketplace/MarketplaceDashboard.browser.test.tsx

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -204,9 +204,10 @@ describe('MarketplaceDashboard — trending placeholders', () => {
204204
// Assert — the dashboard owns its own trending fetch (it is not piggy-backing
205205
// on SkillsMarketplace's default 'all-time' request), keyed to the trending
206206
// filter so the skeleton resolves without the user opening the ranking tab.
207-
await expect
208-
.poll(() => mockLeaderboard.mock.calls.length)
209-
.toBeGreaterThan(0)
207+
// Exactly one IPC call: the thunk's in-flight guard dedupes StrictMode's
208+
// double-mount (the first dispatch sets status:'loading' synchronously, so
209+
// the second dispatch's `condition` short-circuits before reaching IPC).
210+
await expect.poll(() => mockLeaderboard.mock.calls.length).toBe(1)
210211
expect(mockLeaderboard).toHaveBeenCalledWith({ filter: 'trending' })
211212
})
212213

@@ -242,6 +243,9 @@ describe('MarketplaceDashboard — trending placeholders', () => {
242243
await expect
243244
.element(screen.getByText('No trending skills available'))
244245
.toBeInTheDocument()
246+
// …and the fresh (idle, just-fetched) cache entry short-circuits the mount
247+
// thunk via its TTL guard, so no redundant IPC refetch fires.
248+
expect(mockLeaderboard).not.toHaveBeenCalled()
245249
})
246250

247251
it('shows an offline notice when the trending fetch failed and nothing is cached', async () => {

src/renderer/src/components/marketplace/MarketplaceDetailPanel.browser.test.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ describe('MarketplaceDetailPanel routing', () => {
144144
})
145145
})
146146

147-
describe('MarketplaceDashboard empty state', () => {
147+
describe('MarketplaceDashboard trending placeholders', () => {
148148
it('shows a loading skeleton, announced to screen readers, before trending skills have been fetched', async () => {
149149
// Arrange
150150
const store = await createStore()

src/renderer/src/components/marketplace/marketplaceDashboardHelpers.ts

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,9 @@ import type { LeaderboardStatus } from '@/shared/types'
44
export type TrendingView = 'populated' | 'loading' | 'error' | 'empty'
55

66
/**
7-
* Decide which Trending panel state the marketplace dashboard renders from the
8-
* cached skill count and fetch status. Extracted as a pure function so the
9-
* four-way visual branch is unit-testable without React Testing Library (per
10-
* the src/renderer `.coderabbit.yaml` guideline) and stays exhaustive.
11-
*
12-
* Precedence is stale-while-revalidate: any cached skills win first (so a
13-
* background refresh never blanks the list), then an in-flight/never-started
14-
* fetch shows the skeleton, then a failed fetch shows the offline notice, and
15-
* only a successful-but-empty result is treated as a genuine empty.
16-
*
7+
* Pick the dashboard Trending panel's visual state from cached count + fetch
8+
* status, stale-while-revalidate: cached skills win, then in-flight → skeleton,
9+
* then failure → offline notice, then a successful-but-empty result → empty.
1710
* @param trendingSkillCount - Number of trending skills available to render.
1811
* @param status - Cache fetch status for the trending filter, or `undefined` when never fetched.
1912
* @returns

src/renderer/src/components/sidebar/SidebarFooter.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ export const SidebarFooter = React.memo(
3636
<button
3737
type="button"
3838
onClick={handleSkillsLinkClick}
39-
className="flex flex-1 items-center justify-center gap-1.5 rounded-sm text-xs font-medium font-mono text-muted-foreground hover:text-primary transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring"
39+
className="flex flex-1 items-center justify-center gap-1.5 rounded-sm text-xs font-medium font-mono text-muted-foreground hover:text-primary transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-inset"
4040
>
4141
<span>skills.sh</span>
4242
<ExternalLink className="h-3 w-3" />

0 commit comments

Comments
 (0)