feat(dashboard): reader dashboard spec alignment #483 #484
Reference in New Issue
Block a user
Delete Branch "feat/issue-483-reader-dashboard-spec-align"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #483
Summary
ReaderHeaderBar— replacesReaderStatsStrip; white header card with greeting (morning/afternoon/evening) + 3 stat columns (Dokumente / Personen / Geschichten), each a touch-target linkReaderPersonChips— redesigned from flex-wrap togrid grid-cols-2 sm:grid-cols-4; card tiles with responsive avatar, doc count as mint pillReaderRecentDocs— compact card-head (h3 + "Alle Dokumente" link); row-based list with thumb SVG, mint-pill "Neu" badge, "Aktualisiert" badge removedReaderRecentStories— same compact card pattern; "Alle Geschichten" link moved into card-head; excerpt usestext-ink-2; story rows withmin-h-[44px]ReaderDraftsModule— mint left-border (border-l-[3px] border-l-brand-mint), card-head h3, draft rows with meta viadashboard_reader_draft_meta, chevron SVG+page.svelte— h1 greeting moved into author branch; reader branch usesReaderHeaderBar+grid grid-cols-1 sm:grid-cols-2content rowReaderStatsStrip.svelte+ specmint-soft,line-soft,link-quiet,ink-4(WCAG AA verified)dashboard_badge_updated)Touch target decision
min-h-[44px]applied to every interactive region (stat links, person chips, doc rows, story rows, draft rows, "Alle …" links).Test plan
npm run lintcleannpm run check: 277 errors — all pre-existing in unrelated files, zero in touched filessrc/lib/paraglide/directory — unrelated to this PR🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
What I checked
TDD evidence, naming, function size, Svelte 5 rules, component responsibility, keyed
{#each}, dead code.Blockers
1. Sender name is no longer a navigable link (
ReaderRecentDocs.svelte)The old code rendered the sender as
<a href="/persons/{doc.sender.id}">. The new code renders it as plain text inside the doc-row link. The spec test was renamed from'renders sender link when sender is present'to'renders sender name text when sender is present'— confirming this is intentional, not an accident.This removes a navigation path users had. If the spec explicitly drops it, that's fine — but it's worth flagging because it's a behavioral regression, not just a visual change.
Suggestions
2. Tests coupled to className strings
Many new tests do:
For touch-target compliance this is understandable (you want to catch regressions when someone removes
min-h-[44px]), but className assertions are brittle: a Tailwind upgrade or class rename breaks the test without any functional change. Consider a data-testid + computed style approach for the touch-target tests if this becomes a maintenance issue.3. Missing edge case at boundary hour 12 in
ReaderHeaderBartestsTests cover
hour: 8(morning),hour: 14(afternoon),hour: 20(evening). There's no test forhour: 12exactly — which the implementation assigns to "afternoon" (if (h < 12) ... if (h < 18) ...). Given this is a boundary, a quick one-liner would close the gap.4. DOM-traversal in
ReaderDraftsModule.svelte.spec.tsWalking
.closest()→.parentElementto get to the root card is fragile. Adata-testid="drafts-card"on the outer<div>would make this onepage.locator('[data-testid=drafts-card]')call.5.
border-accentinReaderPersonChips.svelte— possible undefined tokenaccentis not one of the new tokens added inlayout.css(onlymint-soft,line-soft,link-quiet,ink-4were added). In Tailwind CSS 4,border-accentresolves to the browser's system accent color or is silently ignored. Either way, the hover border colour is undefined. Probably should behover:border-brand-mint.6.
hover:border-l-4causes a 3px layout shift on hoverThe person chips go from
border(1px) tohover:border-l-4(4px left border) on hover. The 3px delta shifts the card content visibly right. The old design usedhover:border-brand-mintwhich kept the same 1px width. Layout shifts on hover feel glitchy, especially on touch devices.What's great
$derived.by()fortimeLabelcomputation inReaderHeaderBar— correct Svelte 5 pattern ✓{#each}blocks keyed:(p.id),(doc.id),(draft.id),(story.id)✓aria-hidden="true"on all decorative SVGs ✓<section aria-label>forReaderPersonChips— better semantics than the old<div>✓<header>element forReaderHeaderBar✓hourprop for deterministic time-of-day tests — clean testability design ✓🏗️ Markus Keller — Application Architect
Verdict: ✅ Approved
Doc update check
Per the trigger table, I check each category against this PR:
@ManyToMany/ FK+page.svelte)+page.svelteErrorCode/PermissionSuggestion (not a blocker): Four new CSS design tokens are introduced —
mint-soft,line-soft,link-quiet,ink-4— with verified contrast values and dual dark/light definitions. These are now reusable utilities that other developers will reach for. CLAUDE.md's styling conventions table documents onlybrand-navy,brand-mint, and--palette-sand. Consider a follow-up ticket to extend that table with the new tokens so the next developer doesn't have to greplayout.cssto discover them.Architecture observations
Layer boundaries respected: All changes are confined to
frontend/src/lib/shared/dashboard/andfrontend/src/routes/+page.svelte. No cross-domain leakage, no repository access from a component. ✓SSR preserved: Data still flows from
+page.server.ts→dataprops. NoonMountfetches introduced. ✓Component responsibility: Each new/modified component maps to a single named visual region.
ReaderHeaderBar,ReaderPersonChips,ReaderRecentDocs,ReaderRecentStories,ReaderDraftsModuleare all appropriately scoped. ✓Deleted
ReaderStatsStrip: Clean deletion with no orphaned imports or references. The replacementReaderHeaderBaris strictly a superset in functionality and responsibility. ✓Tech debt note (pre-existing, not this PR): The PR description notes 277 pre-existing
npm run checkerrors in unrelated files, plus a production build blocked by root-ownedsrc/lib/paraglide/. These should be tracked as separate issues if they aren't already.Summary
This is a well-scoped UI change that respects every architectural boundary. The only open item is the informal documentation of the four new CSS tokens — a quick CLAUDE.md table update in a follow-up would complete the picture.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
What I audited
OWASP Top 10 sweep, XSS vectors, data exposure, auth boundaries, external link hygiene, user-controlled interpolation.
Findings
No security issues found. This PR is a pure frontend UI restructuring with no backend changes, no new API calls, no new form handling, and no authentication/authorization changes.
Specific checks passed:
innerHTML/eval/document.writewith user inputfetch('/api/...')insideonMount(bypasses SSR auth)rel="noopener noreferrer"i18n interpolation note — the following pattern appears in
ReaderDraftsModule.svelte:relativeTimeDe()returns a plain string from aDate, and Paraglide's message function performs string interpolation without HTML evaluation. This is safe — no XSS vector.Data flow remains server-driven:
draft.updatedAtarrives via+page.server.ts→ props. No client-side fetch path was opened.Nothing to fix. LGTM from a security perspective.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
What I checked
Test coverage per layer, naming, factory patterns, behavioral vs. implementation assertions, missing edge cases, test brittleness.
Suggestions
1. className assertions are brittle for touch-target compliance
Multiple tests do:
This verifies that a specific Tailwind class is present in the DOM string, not that the element actually has a
minHeight >= 44px. The problem: a future Tailwind version or class rename breaks the test without any functional regression. Consider:This tests the actual rendered height, which is what the touch target requirement actually means.
2. Missing boundary test:
hour === 12ReaderHeaderBar.svelte.spec.tscovershour: 8,hour: 14,hour: 20. The implementation usesh < 12andh < 18— making 12 the boundary between morning and afternoon. No test covershour: 12. A one-liner:3. DOM traversal brittleness in
ReaderDraftsModule.svelte.spec.tsIf the component's DOM structure changes (e.g., one wrapper
<div>is added), this test silently tests the wrong element. Adata-testidattribute would make this robust.4.
ReaderPersonChips— no test for thedocumentCount === 0renderingThe new code hides the doc-count pill when
documentCountis 0 or absent:There's no test that verifies the pill is absent when count is 0. Given the old code always showed the count, this is a behavioral change worth covering explicitly.
5.
ReaderRecentDocs—updatedBadgeremoval is tested, but the removed"Aktualisiert"key deserves an i18n snapshotThe test covers
expect.element(page.getByText(/^Aktualisiert$/i)).not.toBeInTheDocument()— good. But the deleted i18n keydashboard_badge_updatedis not covered by any test that would catch it being accidentally re-added.What's good
afterEach(() => cleanup())consistently present in all spec files ✓vitest-browser-svelteused throughout (real DOM, not JSDOM) ✓{#each}behaviors covered: populated, empty states verified ✓ReaderStatsStripcorrectly removed alongside the component ✓🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate
Verdict: ⚠️ Approved with concerns
What I reviewed
Brand compliance, WCAG contrast, touch targets, semantic HTML, focus management, dark mode, 320px mobile behavior, redundant cues.
Blockers
1.
border-accentis an undefined token onReaderPersonChipshoveraccentis not registered inlayout.css. In Tailwind CSS 4, this resolves to the browser system accent color (typically blue), which violates brand consistency and is unpredictable across operating systems. The likely intent ishover:border-brand-mint. Fix:2.
hover:border-l-4causes a 3px layout shift on person cardsThe card has
border(1px) normally andhover:border-l-4(4px) on hover. This pushes card content 3px to the right on every hover, creating a visible jank — especially noticeable for the senior audience (60+) who are sensitive to unexpected movement. Use a constantborder-l-4and change only the color:This keeps the spacing stable while giving the hover affordance.
Suggestions
3.
mint-softpill background contrast (WCAG 1.4.11)The CSS comment is honest:
--c-mint-soft: #d4f0ee; /* decorative fill (1.1:1 on white, WCAG carve-out) */. WCAG 1.4.11 requires 3:1 for non-text contrast on UI components. A pill badge is a UI component, so 1.1:1 technically fails 1.4.11. The text ON the pill usestext-ink(navy, ~14:1 on#d4f0ee) — so the text contrast is fine. The concern is whether the pill's boundary is distinguishable as a UI component. Consider addingborder border-brand-mint/30to give the pill a subtle visible edge that meets 3:1 even when the fill is soft.4. Person chip vertical padding is very generous (
py-6= 24px top + 24px bottom)The grid cards have
px-4 py-6. On a 320px screen withgrid-cols-2, each card is ~144px wide and 100px+ tall for a single name. This is arguably fine for the senior audience (lots of touch area), but check whether on small phones the grid stacks feel overcrowded. The WCAG 2.2 minimum is 44px target height — you're well above that, which is good.5.
<header>inside<main>is semantically valid but creates an implicit landmarkReaderHeaderBarrenders as a<header>element. Inside<main>, this creates abannerlandmark (as confirmed by the testpage.getByRole('banner')). Screen reader users navigating by landmarks will encounter "Banner" inside the main content area, which is correct for a section header. This is fine but worth being intentional about — thearia-labelon the<main>or anaria-labelledbypointing to the greeting h1 would complete the landmark structure for users of screen readers.What's great
min-h-[44px]consistently applied to all interactive regions ✓focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:outline-noneon all links ✓aria-hidden="true"on all decorative SVGs (chevron, thumb, divider) ✓<section aria-label>forReaderPersonChips,<header>forReaderHeaderBar✓text-inkon stat numbers (replacestext-brand-navy) — correct, brand-navy can fail AA in dark mode on certain surfaces ✓sm:block/sm:hidden) — thoughtful mobile ergonomics for the senior audience ✓font-black text-2xlfor stat numbers — high legibility, appropriate for 60+ users ✓📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
What I reviewed
Behavioral completeness, acceptance criteria coverage, removed functionality, and spec-code alignment.
Concerns
1. Sender navigation removed from
ReaderRecentDocs— unspecified behavioral regressionThe original component rendered the document sender as a clickable link:
The new component renders the sender as non-navigable text inside the doc-row link. Users who previously used the sender name to jump to a person's detail page have lost that path.
The test was explicitly renamed from
'renders sender link when sender is present'→'renders sender name text when sender is present', so this appears intentional. But:ReaderPersonChipsThis is worth confirming against the original spec before merging.
2.
dashboard_badge_updated("Aktualisiert") removed without a documented decisionThe PR removes the "Aktualisiert" badge entirely. Previously, users had a visible signal distinguishing new documents (created recently) from updated documents (edited since upload). Now:
For the archive use case (readers reviewing transcription progress), knowing something was recently updated is valuable. If the spec explicitly removed this signal, that's a product decision — but the PR description only says
'dashboard_badge_updated' deletedwithout explaining why. A follow-up acceptance criterion in issue #483 would document the reasoning.3. Missing NFR verification for the new CSS tokens
The PR adds 4 new CSS tokens and documents WCAG ratios in CSS comments — good practice. However, WCAG 1.4.11 (non-text contrast, 3:1 minimum for UI components) for
mint-softis called a "WCAG carve-out" at 1.1:1. The mint pill is a visible UI component (badge/pill). This is an accessibility NFR that should have an explicit acceptance criterion in issue #483 rather than a silent code comment.What's good
hourprop makes time-of-day greeting deterministically testable without mockingDate.now()— a clean requirements-implementation alignment ✓🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
Docker/Compose changes, CI workflow changes, infrastructure configuration, environment variable handling, secrets.
Findings
This PR is a pure frontend UI change — Svelte components, CSS tokens, and i18n message keys. No infrastructure-relevant files were modified:
docker-compose.yml/ overlay files.gitea/workflows/)Dockerfile/ build configspackage.jsondependency additionsvite.config.*changesThe PR description notes that the production build is blocked by a pre-existing root-owned
src/lib/paraglide/directory. This is a DevOps-adjacent issue that should be tracked separately. The affected directory is likely owned by root due to a Docker volume mount during a previous CI run. Fix:If this is happening in CI, the relevant Docker Compose service should ensure the
paraglide/output directory is owned by the non-root build user. Worth a separate ticket.LGTM from infrastructure perspective. Nothing to block here.