Skip to content

Commit 9ebcb3a

Browse files
fix: close UX gaps across app surfaces
Sweep from /ux-gap-detector over the whole app, plus the design-review follow-ups. Behaviour-safe focus/contrast/motion polish, one wiring fix, and the DESIGN.md rules the review surfaced. Focus visibility (was: no visible focus on several controls) - SkillItem unlink / delete / bookmark X, AgentItem row, SidebarFooter skills.sh + gear, SourceLink repo-filter + external link, SkillDetail tabs, and the dialog close X all gain focus-visible rings. Theme selector - Drop hover:scale-110 jank on the colour swatches; the hover affordance is now a quiet bg-muted tile (transition-colors) on the swatch button. - Selected-swatch ring ring-white -> ring-foreground so it stays visible in light mode (ring-white was invisible on a light background). Severity + scale - SkillDetail Valid/Broken/Inaccessible counts colour by severity when non-zero (success / amber) and stay muted at zero. - Skills Marketplace heading: 28px + subtitle -> text-lg (no hero-scale type in the shell). Dashboard stat numbers text-3xl -> text-2xl tabular-nums. - CodePreview loading / no-files containers h-48 -> h-full so they fill the panel instead of floating. - Card: rounded-xl + shadow -> rounded-lg, border-only. Trending panel (the wiring fix) - MarketplaceDashboard now owns its own loadLeaderboard('trending') mount fetch. Nothing requested trending on this surface before (only SkillsMarketplace, which defaults to 'all-time'), so its placeholder never resolved. Four-way view (populated / loading / error / empty) via a pure resolveTrendingView helper + ts-pattern; TrendingSkeleton carries role="status" + aria-label; a TrendingError offline notice replaces a silent blank on failure. Settings - General + Appearance ToggleGroups: justify-start (left-aligned under their labels, not stretched). Appearance font-size range gains a focus ring. Bulk-confirm dialog - AlertTriangle icon (destructive=red, copy=amber) via a module-scope helper so MainContent stays at its complexity baseline. DESIGN.md - New "Loading and Skeletons" section (fixed-shape skeleton, role="status" a11y, a skeleton must own its fetch) and a segmented-control left-align rule.
1 parent 20dd459 commit 9ebcb3a

19 files changed

Lines changed: 449 additions & 89 deletions

DESIGN.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,12 @@ Rules:
297297
- Tabs should be compact and stable.
298298
- Use underline, border, or subtle background to communicate selection.
299299
- Do not increase container height on hover or active state.
300+
- A segmented control sits left-aligned under its own label, sized to its
301+
content — it does not stretch full-width or center in the row. Stretching reads
302+
as a primary tab bar; a secondary setting toggle should stay a compact,
303+
left-anchored affordance. Apply `justify-start` to the ToggleGroup whenever its
304+
parent is a full-width flex column. (macOS System Settings, Linear preference
305+
toggles.)
300306
- Keep Radix roles in mind when testing: some toggle groups expose `radio`.
301307

302308
### Lists and Rows
@@ -336,6 +342,24 @@ Rules:
336342
heading for a search miss. This is the operational reading of principle 5's
337343
"no noisy empty states."
338344

345+
### Loading and Skeletons
346+
347+
- A fixed-shape skeleton — not a single "Loading…" text line — is the default
348+
first-load placeholder for any list or panel whose populated layout is known.
349+
Mirror the populated layout (same row count, same chip / text / trailing
350+
structure) so the panel does not reflow when real data lands. (Linear, GitHub,
351+
Vercel load lists this way.)
352+
- 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.
358+
- A panel that renders a skeleton must actually request its data. A skeleton with
359+
no fetch behind it is a permanent broken-looking spinner; own the fetch on
360+
mount (guarded by a cache TTL) wherever the populated state is the intended
361+
resting state.
362+
339363
### Cards and Widgets
340364

341365
- Use cards for repeated standalone items, dashboard widgets, dialogs, and

src/renderer/settings/sections/Appearance.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ const BackgroundBlurRangeInput = React.memo(function BackgroundBlurRangeInput({
7373
value={value}
7474
onChange={handleInputChange}
7575
aria-label={label}
76-
className="h-2 min-w-0 flex-1 accent-primary"
76+
className="h-2 min-w-0 flex-1 accent-primary rounded-full focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-background"
7777
/>
7878
)
7979
})
@@ -171,6 +171,7 @@ export const Appearance = React.memo(function Appearance(): React.ReactElement {
171171
type="single"
172172
variant="outline"
173173
size="sm"
174+
className="justify-start"
174175
value={installedSearchCountDisplay}
175176
onValueChange={handleSearchCountDisplayChange}
176177
aria-label={INSTALLED_SEARCH_COUNT_DISPLAY_LABEL}

src/renderer/settings/sections/General.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,7 @@ export const General = React.memo(function General(): React.ReactElement {
301301
type="single"
302302
variant="outline"
303303
size="sm"
304+
className="justify-start"
304305
value={settings.defaultSkillTab}
305306
onValueChange={handleDefaultTabChange}
306307
aria-label="Default tab when opening a skill"

src/renderer/src/components/layout/MainContent.tsx

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {
2+
AlertTriangle,
23
ArrowDownAZ,
34
ArrowUpAZ,
45
CheckSquare,
@@ -280,6 +281,23 @@ function getBulkDeleteTargetSummary(
280281
}
281282
}
282283

284+
/**
285+
* Pick the bulk-confirm dialog's warning-icon tint: red for the irreversible
286+
* delete, amber for the lighter unlink. Module-scope (taking the whole state,
287+
* so the call site needs no optional chain) keeps this branch out of
288+
* MainContent's complexity budget.
289+
* @param bulkConfirm - Pending bulk confirm dialog state, or null when closed.
290+
* @returns Tailwind text-color class for the AlertTriangle glyph.
291+
* @example
292+
* bulkConfirmIconColorClass({ kind: 'delete', ... }) // => 'text-destructive'
293+
* bulkConfirmIconColorClass({ kind: 'unlink', ... }) // => 'text-amber-500'
294+
*/
295+
function bulkConfirmIconColorClass(
296+
bulkConfirm: BulkConfirmState | null,
297+
): string {
298+
return bulkConfirm?.kind === 'delete' ? 'text-destructive' : 'text-amber-500'
299+
}
300+
283301
/**
284302
* Disable destructive confirm when every reviewed row is stale and no cleanup can run.
285303
* @param bulkConfirm - Pending bulk confirm dialog state.
@@ -1371,11 +1389,16 @@ export const MainContent = React.memo(
13711389
>
13721390
<DialogContent className="max-w-md">
13731391
<DialogHeader>
1374-
<DialogTitle>
1375-
{bulkConfirm?.kind === 'delete'
1376-
? `Delete ${bulkConfirm.skillNames.length} ${pluralize(bulkConfirm.skillNames.length, 'skill')}?`
1377-
: `Unlink ${bulkConfirm?.skillNames.length ?? 0} ${pluralize(bulkConfirm?.skillNames.length ?? 0, 'skill')} from ${bulkConfirm?.agentName ?? 'agent'}?`}
1378-
</DialogTitle>
1392+
<div className="flex items-center gap-2">
1393+
<AlertTriangle
1394+
className={`h-5 w-5 ${bulkConfirmIconColorClass(bulkConfirm)}`}
1395+
/>
1396+
<DialogTitle>
1397+
{bulkConfirm?.kind === 'delete'
1398+
? `Delete ${bulkConfirm.skillNames.length} ${pluralize(bulkConfirm.skillNames.length, 'skill')}?`
1399+
: `Unlink ${bulkConfirm?.skillNames.length ?? 0} ${pluralize(bulkConfirm?.skillNames.length ?? 0, 'skill')} from ${bulkConfirm?.agentName ?? 'agent'}?`}
1400+
</DialogTitle>
1401+
</div>
13791402
<DialogDescription>
13801403
{bulkConfirm?.kind === 'delete'
13811404
? renderBulkDeleteDescription({

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

Lines changed: 72 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { configureStore } from '@reduxjs/toolkit'
22
import { Provider } from 'react-redux'
3-
import { describe, expect, it } from 'vitest'
3+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
44
import { render } from 'vitest-browser-react'
55

66
import type { RootState } from '@/renderer/src/redux/store'
@@ -20,7 +20,30 @@ import type {
2020
// is active with no skill previewed. It reads three slices (installed count,
2121
// bookmark count, trending leaderboard cache) and lets the user pick a trending
2222
// row for preview. This file guards: the trending-row click → previewSkill
23-
// selection, and the loading vs settled-empty trending placeholders.
23+
// selection, the loading/error/settled-empty trending placeholders, and the
24+
// mount fetch that populates the panel.
25+
26+
// The dashboard dispatches `loadLeaderboard('trending')` on mount, whose thunk
27+
// reads `window.electron.marketplace.leaderboard`. Browser-mode tests replace
28+
// the preload bridge, so the IPC surface is stubbed. The default is a
29+
// never-resolving promise: it lets each test pin the leaderboard state it seeded
30+
// without a late `fulfilled` action racing in and overwriting the rendered
31+
// branch (the fresh-cache TTL guard already blocks the fetch for seeded tests).
32+
const mockLeaderboard = vi.fn()
33+
34+
beforeEach(() => {
35+
mockLeaderboard.mockReset()
36+
mockLeaderboard.mockReturnValue(new Promise<SkillSearchResult[]>(() => {}))
37+
vi.stubGlobal('electron', {
38+
marketplace: {
39+
leaderboard: mockLeaderboard,
40+
},
41+
})
42+
})
43+
44+
afterEach(() => {
45+
vi.unstubAllGlobals()
46+
})
2447

2548
/**
2649
* Build a `SkillSearchResult` fixture so each test varies only the field it
@@ -58,8 +81,10 @@ function makeLeaderboardData(
5881

5982
/**
6083
* Render MarketplaceDashboard over the real marketplace + skills + bookmark
61-
* reducers, seeded with `preloadedState`. The dashboard has no mount thunk, so
62-
* the seeded state is rendered verbatim with no async settling.
84+
* reducers, seeded with `preloadedState`. The dashboard fires a mount fetch, but
85+
* the stubbed IPC defaults to a never-resolving promise so seeded state renders
86+
* without a late `fulfilled` overwriting it; fresh seeded entries also short-
87+
* circuit the thunk via its cache-TTL guard.
6388
* @param preloadedState - Partial marketplace + skills + bookmarks state to seed
6489
*/
6590
async function renderDashboard(preloadedState: {
@@ -170,16 +195,31 @@ describe('MarketplaceDashboard — trending preview selection', () => {
170195
})
171196

172197
describe('MarketplaceDashboard — trending placeholders', () => {
173-
it('shows a loading placeholder while the trending leaderboard has not loaded yet', async () => {
198+
it('requests the trending leaderboard on mount so the panel populates itself', async () => {
199+
// Arrange — an unseeded trending slot means the cache-TTL guard cannot
200+
// short-circuit, so the mount thunk must reach the IPC bridge.
201+
// Act
202+
await renderDashboard({ marketplace: { leaderboard: {} } })
203+
204+
// Assert — the dashboard owns its own trending fetch (it is not piggy-backing
205+
// on SkillsMarketplace's default 'all-time' request), keyed to the trending
206+
// filter so the skeleton resolves without the user opening the ranking tab.
207+
await expect
208+
.poll(() => mockLeaderboard.mock.calls.length)
209+
.toBeGreaterThan(0)
210+
expect(mockLeaderboard).toHaveBeenCalledWith({ filter: 'trending' })
211+
})
212+
213+
it('shows a loading skeleton, announced to screen readers, while the trending leaderboard has not loaded yet', async () => {
174214
// Arrange — no trending cache entry means trendingData is undefined, which
175215
// the dashboard treats as still loading.
176216
const { screen } = await renderDashboard({
177217
marketplace: { leaderboard: {} },
178218
})
179219

180-
// Act + Assert
220+
// Act + Assert — pulsing rows expose an accessible loading status
181221
await expect
182-
.element(screen.getByText('Loading trending skills...'))
222+
.element(screen.getByRole('status', { name: 'Loading trending skills' }))
183223
.toBeInTheDocument()
184224
})
185225

@@ -203,4 +243,29 @@ describe('MarketplaceDashboard — trending placeholders', () => {
203243
.element(screen.getByText('No trending skills available'))
204244
.toBeInTheDocument()
205245
})
246+
247+
it('shows an offline notice when the trending fetch failed and nothing is cached', async () => {
248+
// Arrange — an errored trending entry with zero cached skills is the
249+
// failure state, distinct from both loading and a settled-empty list. The
250+
// mount thunk re-fetches errored filters (errors bypass the TTL gate), so
251+
// the IPC mock must reject for state to settle back on the error branch
252+
// instead of getting stuck on the in-flight skeleton.
253+
mockLeaderboard.mockRejectedValue(new Error('network down'))
254+
const { screen } = await renderDashboard({
255+
marketplace: {
256+
leaderboard: {
257+
trending: makeLeaderboardData({
258+
skills: [],
259+
status: 'error',
260+
error: 'network down',
261+
}),
262+
},
263+
},
264+
})
265+
266+
// Act + Assert
267+
await expect
268+
.element(screen.getByText('Trending unavailable'))
269+
.toBeInTheDocument()
270+
})
206271
})

0 commit comments

Comments
 (0)