feat(dashboard): Classic Split — remove notification widget, restructure into 2-col layout #172

Merged

marcel merged 12 commits from feat/issue-171-dashboard-classic-split into main 2026-03-31 20:56:52 +02:00

Conversation 6 Commits 12 Files Changed 22 +273 -197

marcel commented 2026-03-31 19:54:17 +02:00

Owner

Copy Link

Closes #171

Summary

• Replaces /api/notifications fetch with /api/stats in the server load — dashboard no longer owns notification data
• Restructures dashboard into a lg:grid-cols-[1fr_300px] Classic Split: recent docs on the left, DropZone + metadata queue on the right
• Adds showRightColumn guard so read-only users with a complete archive never see an empty 300 px ghost column
• Adds stats footnote ("248 Documents · 34 Persons") to DashboardRecentDocuments with failure isolation
• Adds min-h-[44px] to doc rows for WCAG 2.5.5 touch target compliance
• Removes DashboardMentions from the page (file kept for future notifications page)

Test plan

• 498 unit tests green (172 server + 326 client)
• New server tests: stats success, stats failure isolation
• New component tests: footnote renders / absent when null / shown for 0 / min-h class
• New page tests: right column absent when canWrite=false + incompleteDocs=[], present otherwise
• npm run lint passes
• npm run check passes (no new type errors in source files)

🤖 Generated with Claude Code

Closes #171 ## Summary - Replaces `/api/notifications` fetch with `/api/stats` in the server load — dashboard no longer owns notification data - Restructures dashboard into a `lg:grid-cols-[1fr_300px]` Classic Split: recent docs on the left, DropZone + metadata queue on the right - Adds `showRightColumn` guard so read-only users with a complete archive never see an empty 300 px ghost column - Adds stats footnote ("248 Documents · 34 Persons") to `DashboardRecentDocuments` with failure isolation - Adds `min-h-[44px]` to doc rows for WCAG 2.5.5 touch target compliance - Removes `DashboardMentions` from the page (file kept for future notifications page) ## Test plan - [x] 498 unit tests green (172 server + 326 client) - [x] New server tests: stats success, stats failure isolation - [x] New component tests: footnote renders / absent when null / shown for 0 / min-h class - [x] New page tests: right column absent when canWrite=false + incompleteDocs=[], present otherwise - [x] `npm run lint` passes - [x] `npm run check` passes (no new type errors in source files) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 5 commits 2026-03-31 19:54:18 +02:00

fix(e2e): replace fragile .last() selector with data-testid on password form submit ... fb636e4152

The password-reset E2E test was using button[type="submit"].last() to target
the password change button on the profile page. The profile page has two submit
buttons with identical text, so .last() is layout-order-dependent and breaks
if the form order ever changes.

Add data-testid="submit-password" to PasswordChangeForm and use getByTestId()
in the test.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(tests): fix 27 failing frontend unit tests ... 6d61297182

Six categories of breakage:

1. date.ts — add formatGermanDateInput(raw: string): string as a pure
   function covering both digit-stream auto-dot and manual-dot-with-padding
   modes. Refactor handleGermanDateInput to delegate to it. Fixes 16 failures
   in date.spec.ts where the function was imported but didn't exist.

2. Admin layout specs (groups/tags/users) — $effect fires on initial mount
   with manualCollapse=false, so the spy captured 'false' before the click's
   effect ran. Fix: move spy setup after render(), add await setTimeout(0) to
   flush Svelte effects before asserting.

3. DashboardMentions — component now renders a persistent
   "Benachrichtigungsverlauf ansehen" link, making getByRole('link') strict-
   mode violations. Fix: scope link queries to the actor name, and check
   absence of the actor link (not all links) in the no-documentId test.

4. Conversations page — empty-state copy changed from "Wählen Sie zwei
   Personen aus" to "Korrespondenz durchsuchen". Update the test.

5. Login page — AuthHeader adds a second aria-label="Familienarchiv" link.
   Use .first() to avoid strict-mode violation.

6. Persons page — alias is rendered with German quotation marks „…" not
   straight quotes "…". Update the test string.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(dashboard): replace notifications fetch with stats in server load ... 20923d04b6

Removes /api/notifications from the dashboard widget fetches and replaces
it with /api/stats so the page no longer needs to own notification data.
Returns stats: StatsDTO | null (null on failure) instead of mentions.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(dashboard): add stats footnote and min-h touch target to DashboardRecentDocuments ... f618364632

Adds stats?: StatsDTO | null prop; renders a quiet footnote showing total
document and person counts. Guards on stats?.totalDocuments != null so
zero is shown but the footnote is absent when stats fails. Adds
min-h-[44px] to doc rows for WCAG 2.5.5 touch target compliance.
Adds dashboard_stats_documents/persons i18n keys in de/en/es.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(dashboard): Classic Split — 2-col layout, remove DashboardMentions widget ... 92e7aa127c

Restructures the dashboard to a lg:grid-cols-[1fr_300px] split:
- Left column: DashboardRecentDocuments (with stats footnote)
- Right column: DropZone (canWrite) + DashboardNeedsMetadata (flex-1)

Adds showRightColumn guard (canWrite || incompleteDocs.length > 0) so
read-only users with a complete archive never see an empty 300px ghost
column. DashboardMentions is removed from the page; the file is kept.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-03-31 19:57:51 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Blockers

None.

Suggestions

1. formatGermanDateInput bundled into a dashboard PR

frontend/src/lib/utils/date.ts grows a substantial new function that handles date input formatting with dot-aware overflow logic. This is unrelated to the Classic Split. Atomic commit principle says one logical change per commit — the date utility extraction should have been its own commit (or PR). Not a blocker since the function is correct and tested, but worth noting for future PRs.

2. The formatGermanDateInput comment explains what, not why

/**
 * Formats a raw date string into German DD.MM.YYYY format.
 *
 * Handles two modes:
 * - Pure digit stream (no dots): auto-inserts dots after position 2 and 4
 * - Manual dot entry: preserves user-typed dots, pads single-digit day/month,
 *   and overflows extra digits from day→month and month→year
 */

The "two modes" explanation is documenting what the function does — which is fine when the logic is truly non-obvious. But the dayOverflowed / monthOverflowed flag names already communicate the logic. I'd keep the comment brief: why are two modes needed (i.e. browsers fire input events differently when the user types vs. pastes), not a re-narration of the branches. Minor nit.

3. await new Promise((r) => setTimeout(r, 0)) in three layout spec files

// admin/groups/layout.svelte.spec.ts:119
// admin/tags/layout.svelte.spec.ts:97
// admin/users/layout.svelte.spec.ts:140
await new Promise((r) => setTimeout(r, 0));

This is a timing hack. It works today, but it's a smell — if the tick count changes, the test silently goes green for the wrong reason. Prefer await vi.waitFor(() => expect(setSpy).toHaveBeenCalled()) so the test polls until the assertion passes or times out with a meaningful message.

4. $derived usage for showRightColumn — ✓ correct

const showRightColumn = $derived(data.canWrite || (data.incompleteDocs?.length ?? 0) > 0);

Exactly right — computed value via $derived, not $state + $effect. Clean.

5. {#each recentDocs as doc (doc.id)} — ✓ keyed

Noted and approved.

6. Stats footnote edge case for totalDocuments === 0 — ✓ tested

The #if stats?.totalDocuments != null guard correctly allows 0 through, and there is a test for it. Good.

Overall the implementation is clean, the tests are well-structured, and the $derived usage is idiomatic Svelte 5. The bundling concern and the timing hack in the layout specs are the only things I'd ask to fix before the next PR in this area.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### Blockers None. ### Suggestions **1. `formatGermanDateInput` bundled into a dashboard PR** `frontend/src/lib/utils/date.ts` grows a substantial new function that handles date input formatting with dot-aware overflow logic. This is unrelated to the Classic Split. Atomic commit principle says one logical change per commit — the date utility extraction should have been its own commit (or PR). Not a blocker since the function is correct and tested, but worth noting for future PRs. **2. The `formatGermanDateInput` comment explains _what_, not _why_** ```typescript /** * Formats a raw date string into German DD.MM.YYYY format. * * Handles two modes: * - Pure digit stream (no dots): auto-inserts dots after position 2 and 4 * - Manual dot entry: preserves user-typed dots, pads single-digit day/month, * and overflows extra digits from day→month and month→year */ ``` The "two modes" explanation is documenting _what_ the function does — which is fine when the logic is truly non-obvious. But the `dayOverflowed` / `monthOverflowed` flag names already communicate the logic. I'd keep the comment brief: _why_ are two modes needed (i.e. browsers fire `input` events differently when the user types vs. pastes), not a re-narration of the branches. Minor nit. **3. `await new Promise((r) => setTimeout(r, 0))` in three layout spec files** ```typescript // admin/groups/layout.svelte.spec.ts:119 // admin/tags/layout.svelte.spec.ts:97 // admin/users/layout.svelte.spec.ts:140 await new Promise((r) => setTimeout(r, 0)); ``` This is a timing hack. It works today, but it's a smell — if the tick count changes, the test silently goes green for the wrong reason. Prefer `await vi.waitFor(() => expect(setSpy).toHaveBeenCalled())` so the test polls until the assertion passes or times out with a meaningful message. **4. `$derived` usage for `showRightColumn` — ✓ correct** ```svelte const showRightColumn = $derived(data.canWrite || (data.incompleteDocs?.length ?? 0) > 0); ``` Exactly right — computed value via `$derived`, not `$state` + `$effect`. Clean. **5. `{#each recentDocs as doc (doc.id)}` — ✓ keyed** Noted and approved. **6. Stats footnote edge case for `totalDocuments === 0` — ✓ tested** The `#if stats?.totalDocuments != null` guard correctly allows `0` through, and there is a test for it. Good. Overall the implementation is clean, the tests are well-structured, and the `$derived` usage is idiomatic Svelte 5. The bundling concern and the timing hack in the layout specs are the only things I'd ask to fix before the next PR in this area.

marcel commented 2026-03-31 19:58:03 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

What I checked

Data flow — load function stays clean

+page.server.ts continues to own all API calls and pass data down via props. No component is fetching its own data. The swap from /api/notifications to /api/stats is a clean, surgical change in exactly the right place.

Promise.allSettled failure isolation — ✓

const [statsResult, incompleteResult, recentResult] = await Promise.allSettled([
    api.GET('/api/stats'),
    api.GET('/api/documents/incomplete', ...),
    api.GET('/api/documents/recent-activity', ...)
]);

Failure of any one widget API does not crash the page. stats defaults to null, incompleteDocs defaults to []. This is the correct pattern for dashboard widgets.

showRightColumn guard — ✓ right layer

const showRightColumn = $derived(data.canWrite || (data.incompleteDocs?.length ?? 0) > 0);

This is display logic computed from server-supplied props — it belongs here, not in the load function. The load function stays declarative, the component decides how to render. Good boundary.

CSS Grid Classic Split — lg:grid-cols-[1fr_300px]

The use of a custom grid track value ([1fr_300px]) instead of a named Tailwind column preset is fine. The showRightColumn conditional correctly avoids emitting the 2-column class entirely rather than hiding a 300px ghost column. Cleaner than visibility: hidden or w-0 tricks.

flex-1 min-h-0 on the NeedsMetadata wrapper

The comment explaining min-h-0 overriding the flex default min-height: auto is the rare case where a comment is justified — this is a known browser gotcha that trips up developers regularly.

Observation (not a blocker)

The +page.svelte now handles two substantially different layouts (dashboard vs. search results) in a single file. As the Classic Split grows in complexity, it may be worth considering a route group split (/(dashboard) and /(search)) to isolate concerns. Not needed now, but worth flagging before the next layout change adds another branch.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** ### What I checked **Data flow — load function stays clean** `+page.server.ts` continues to own all API calls and pass data down via props. No component is fetching its own data. The swap from `/api/notifications` to `/api/stats` is a clean, surgical change in exactly the right place. **`Promise.allSettled` failure isolation — ✓** ```typescript const [statsResult, incompleteResult, recentResult] = await Promise.allSettled([ api.GET('/api/stats'), api.GET('/api/documents/incomplete', ...), api.GET('/api/documents/recent-activity', ...) ]); ``` Failure of any one widget API does not crash the page. `stats` defaults to `null`, `incompleteDocs` defaults to `[]`. This is the correct pattern for dashboard widgets. **`showRightColumn` guard — ✓ right layer** ```svelte const showRightColumn = $derived(data.canWrite || (data.incompleteDocs?.length ?? 0) > 0); ``` This is display logic computed from server-supplied props — it belongs here, not in the load function. The load function stays declarative, the component decides how to render. Good boundary. **CSS Grid Classic Split — `lg:grid-cols-[1fr_300px]`** The use of a custom grid track value (`[1fr_300px]`) instead of a named Tailwind column preset is fine. The `showRightColumn` conditional correctly avoids emitting the 2-column class entirely rather than hiding a 300px ghost column. Cleaner than `visibility: hidden` or `w-0` tricks. **`flex-1 min-h-0` on the NeedsMetadata wrapper** The comment explaining `min-h-0` overriding the flex default `min-height: auto` is the rare case where a comment is justified — this is a known browser gotcha that trips up developers regularly. ### Observation (not a blocker) The `+page.svelte` now handles two substantially different layouts (dashboard vs. search results) in a single file. As the Classic Split grows in complexity, it may be worth considering a route group split (`/(dashboard)` and `/(search)`) to isolate concerns. Not needed now, but worth flagging before the next layout change adds another branch.

marcel commented 2026-03-31 19:58:15 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

What's well covered

• Stats success path — result.stats equals the mocked { totalDocuments: 42, totalPersons: 7 } ✓
• Stats failure isolation — result.stats is null when /api/stats rejects ✓
• Footnote renders / absent when null / shown for 0 — three distinct cases tested ✓
• Touch target class — min-h-[44px] on doc-row-{id} verified via data-testid ✓
• Right column show/hide — three cases: canWrite=false+empty, canWrite=true, incompleteDocs non-empty ✓
• DashboardMentions spec hardened — ambiguous getByRole('link') replaced with getByRole('link', { name: 'Anna Schmidt' }) — this is a direct quality improvement to existing tests ✓

Blockers

None.

Concerns

1. await new Promise((r) => setTimeout(r, 0)) in three layout specs

frontend/src/routes/admin/groups/layout.svelte.spec.ts:119
frontend/src/routes/admin/tags/layout.svelte.spec.ts:97
frontend/src/routes/admin/users/layout.svelte.spec.ts:140

Raw setTimeout(0) is a timing hack that passes today and may silently fail or flake when the reactive update cycle changes. The correct pattern is:

await vi.waitFor(() => {
    expect(setSpy).toHaveBeenCalledWith('admin_groups_list_collapsed', 'true');
});

vi.waitFor retries the assertion up to the configured timeout and fails with a meaningful message. I'd make this change before merging to prevent a future flake.

2. No E2E test for the Classic Split dashboard layout

The test plan lists unit tests only. Given that this PR restructures the primary dashboard layout (2-column grid, right column guard), a Playwright smoke test that visits / as a read-only user (right column absent) and as a write user (right column present + DropZone visible) would be the appropriate E2E coverage. This is the kind of layout regression that unit tests won't catch.

Suggestion — add to the existing Playwright suite:

test('read-only user sees no right column on dashboard', async ({ page }) => {
    // log in as read-only user
    await page.goto('/');
    await expect(page.getByTestId('dashboard-right-column')).not.toBeVisible();
});

Not a blocker for this PR, but track it as a follow-up ticket.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** ### What's well covered - **Stats success path** — `result.stats` equals the mocked `{ totalDocuments: 42, totalPersons: 7 }` ✓ - **Stats failure isolation** — `result.stats` is `null` when `/api/stats` rejects ✓ - **Footnote renders / absent when null / shown for `0`** — three distinct cases tested ✓ - **Touch target class** — `min-h-[44px]` on `doc-row-{id}` verified via `data-testid` ✓ - **Right column show/hide** — three cases: `canWrite=false+empty`, `canWrite=true`, `incompleteDocs non-empty` ✓ - **`DashboardMentions` spec hardened** — ambiguous `getByRole('link')` replaced with `getByRole('link', { name: 'Anna Schmidt' })` — this is a direct quality improvement to existing tests ✓ ### Blockers None. ### Concerns **1. `await new Promise((r) => setTimeout(r, 0))` in three layout specs** `frontend/src/routes/admin/groups/layout.svelte.spec.ts:119` `frontend/src/routes/admin/tags/layout.svelte.spec.ts:97` `frontend/src/routes/admin/users/layout.svelte.spec.ts:140` Raw `setTimeout(0)` is a timing hack that passes today and may silently fail or flake when the reactive update cycle changes. The correct pattern is: ```typescript await vi.waitFor(() => { expect(setSpy).toHaveBeenCalledWith('admin_groups_list_collapsed', 'true'); }); ``` `vi.waitFor` retries the assertion up to the configured timeout and fails with a meaningful message. I'd make this change before merging to prevent a future flake. **2. No E2E test for the Classic Split dashboard layout** The test plan lists unit tests only. Given that this PR restructures the primary dashboard layout (2-column grid, right column guard), a Playwright smoke test that visits `/` as a read-only user (right column absent) and as a write user (right column present + DropZone visible) would be the appropriate E2E coverage. This is the kind of layout regression that unit tests won't catch. Suggestion — add to the existing Playwright suite: ```typescript test('read-only user sees no right column on dashboard', async ({ page }) => { // log in as read-only user await page.goto('/'); await expect(page.getByTestId('dashboard-right-column')).not.toBeVisible(); }); ``` Not a blocker for this PR, but track it as a follow-up ticket.

marcel commented 2026-03-31 19:58:24 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

Attack surface review

New API call: GET /api/stats

The load function now fetches /api/stats via the typed API client:

api.GET('/api/stats')

createApiClient(fetch) in SvelteKit's server context forwards the session cookie automatically, so this call is authenticated. The response (totalDocuments, totalPersons) is aggregate count data — no PII, no document content exposed to the UI. ✓

Stats data rendered in the DOM

{stats.totalDocuments}
{m.dashboard_stats_documents()} · {stats.totalPersons}
{m.dashboard_stats_persons()}

totalDocuments and totalPersons are numbers. Svelte auto-escapes template expressions. No XSS vector here. ✓

showRightColumn — not a security boundary

const showRightColumn = $derived(data.canWrite || (data.incompleteDocs?.length ?? 0) > 0);

This is a display-layer guard only — it controls whether the DropZone and NeedsMetadata widgets render on screen. The actual write-permission enforcement lives on the server (Spring Security @RequirePermission). A user manually enabling the hidden column via DevTools would find that any upload attempts are rejected at the API layer. This is the correct design. ✓

data-testid="submit-password" on the password form button

This attribute is for Playwright test targeting only. It has no security implication. ✓

No findings.

This PR does not introduce any injection vectors, authentication changes, authorization bypasses, information disclosure beyond aggregate counts, or new third-party dependencies.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** ### Attack surface review **New API call: `GET /api/stats`** The load function now fetches `/api/stats` via the typed API client: ```typescript api.GET('/api/stats') ``` `createApiClient(fetch)` in SvelteKit's server context forwards the session cookie automatically, so this call is authenticated. The response (`totalDocuments`, `totalPersons`) is aggregate count data — no PII, no document content exposed to the UI. ✓ **Stats data rendered in the DOM** ```svelte {stats.totalDocuments} {m.dashboard_stats_documents()} · {stats.totalPersons} {m.dashboard_stats_persons()} ``` `totalDocuments` and `totalPersons` are numbers. Svelte auto-escapes template expressions. No XSS vector here. ✓ **`showRightColumn` — not a security boundary** ```svelte const showRightColumn = $derived(data.canWrite || (data.incompleteDocs?.length ?? 0) > 0); ``` This is a display-layer guard only — it controls whether the DropZone and NeedsMetadata widgets render on screen. The actual write-permission enforcement lives on the server (Spring Security `@RequirePermission`). A user manually enabling the hidden column via DevTools would find that any upload attempts are rejected at the API layer. This is the correct design. ✓ **`data-testid="submit-password"` on the password form button** This attribute is for Playwright test targeting only. It has no security implication. ✓ ### No findings. This PR does not introduce any injection vectors, authentication changes, authorization bypasses, information disclosure beyond aggregate counts, or new third-party dependencies.

marcel commented 2026-03-31 19:58:41 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UI/UX Design Lead

Verdict: ⚠️ Approved with concerns

What works well

Touch targets — WCAG 2.5.5 ✓

class="flex min-h-[44px] items-center justify-between ..."

min-h-[44px] is exactly the 44×44px minimum. This directly addresses WCAG 2.5.5 and is a real improvement for senior and mobile users. ✓

showRightColumn guard — UX decision is correct
An empty 300px ghost column on a dashboard is a disorienting layout hole. Suppressing the column entirely when there's nothing to show is the right call. ✓

Mobile-first grid — grid-cols-1 base, lg:grid-cols-[1fr_300px] desktop
Single-column mobile, 2-column desktop. The pattern is mobile-first. ✓

---

Concerns

1. Stats footnote at text-xs (12px) is at the absolute minimum

<p data-testid="dashboard-stats-footnote" class="mt-4 font-sans text-xs text-ink-3">

text-xs renders at 12px. That is my hard floor — never below 12px — but "floor" means absolute minimum, not target. For a footnote that senior family members will read on a phone, text-sm (14px) with sufficient line-height would be more comfortable. The text-ink-3 muted color compound the legibility issue at 12px. Suggest: bump to text-sm text-ink-3 or keep text-xs but use text-ink-2 for higher contrast.

2. Removal of the DashboardMentions widget is a UX regression without a replacement path

The PR correctly notes the file is "kept for future notifications page." But from the user's perspective, the mention/notification feed is gone from the dashboard with no replacement visible anywhere. Family members who relied on it to see who mentioned them in documents now have no discovery surface. The PR description mentions this is tracked but doesn't link a follow-up issue. Before this merges, there should be a Gitea issue open for the notifications replacement so it doesn't get lost.

3. Right column fixed at 300px — verify on narrow desktop viewports

lg: breakpoint is 1024px. At 1024px with 300px right column and gap-4 (16px), the left column gets 1024px - 300px - 16px - (horizontal padding) ≈ 660px. This should be fine, but worth a visual verification at exactly 1024px to confirm the DropZone doesn't feel squeezed.

4. Dark mode — no evidence new elements are token-based

The text-ink-3, border-line, bg-white classes — if these map to CSS custom properties with dark-mode overrides, the new elements will inherit dark mode correctly. If any hardcoded colors crept in, they won't. I can't verify this from the diff alone. A quick visual smoke test in dark mode before merge would confirm.

## 🎨 Leonie Voss — UI/UX Design Lead **Verdict: ⚠️ Approved with concerns** ### What works well **Touch targets — WCAG 2.5.5 ✓** ```svelte class="flex min-h-[44px] items-center justify-between ..." ``` `min-h-[44px]` is exactly the 44×44px minimum. This directly addresses WCAG 2.5.5 and is a real improvement for senior and mobile users. ✓ **`showRightColumn` guard — UX decision is correct** An empty 300px ghost column on a dashboard is a disorienting layout hole. Suppressing the column entirely when there's nothing to show is the right call. ✓ **Mobile-first grid — `grid-cols-1` base, `lg:grid-cols-[1fr_300px]` desktop** Single-column mobile, 2-column desktop. The pattern is mobile-first. ✓ --- ### Concerns **1. Stats footnote at `text-xs` (12px) is at the absolute minimum** ```svelte <p data-testid="dashboard-stats-footnote" class="mt-4 font-sans text-xs text-ink-3"> ``` `text-xs` renders at 12px. That is my hard floor — never below 12px — but "floor" means absolute minimum, not target. For a footnote that senior family members will read on a phone, `text-sm` (14px) with sufficient line-height would be more comfortable. The `text-ink-3` muted color compound the legibility issue at 12px. Suggest: bump to `text-sm text-ink-3` or keep `text-xs` but use `text-ink-2` for higher contrast. **2. Removal of the DashboardMentions widget is a UX regression without a replacement path** The PR correctly notes the file is "kept for future notifications page." But from the user's perspective, the mention/notification feed is gone from the dashboard with no replacement visible anywhere. Family members who relied on it to see who mentioned them in documents now have no discovery surface. The PR description mentions this is tracked but doesn't link a follow-up issue. Before this merges, there should be a Gitea issue open for the notifications replacement so it doesn't get lost. **3. Right column fixed at `300px` — verify on narrow desktop viewports** `lg:` breakpoint is 1024px. At 1024px with `300px` right column and `gap-4` (16px), the left column gets `1024px - 300px - 16px - (horizontal padding)` ≈ 660px. This should be fine, but worth a visual verification at exactly 1024px to confirm the DropZone doesn't feel squeezed. **4. Dark mode — no evidence new elements are token-based** The `text-ink-3`, `border-line`, `bg-white` classes — if these map to CSS custom properties with dark-mode overrides, the new elements will inherit dark mode correctly. If any hardcoded colors crept in, they won't. I can't verify this from the diff alone. A quick visual smoke test in dark mode before merge would confirm.

marcel commented 2026-03-31 19:58:52 +02:00

Author

Owner

Copy Link

🚀 Tobias Wendt — DevOps & Platform Engineer

Verdict: ⚠️ Approved with concerns

What I checked

No changes to docker-compose.yml, CI workflow files, or infrastructure configuration. This PR is frontend-only.

---

Concerns

1. frontend/e2e/.auth/user.json is a tracked file modified in this PR

M frontend/e2e/.auth/user.json

This file stores Playwright's authenticated browser state (session cookies / storage). It is committed to the repository, which means real session tokens may be persisted in git history. Playwright's recommended approach is to either:

• Add .auth/ to .gitignore and generate it at the start of every CI run via playwright/auth.setup.ts
• Or ensure the file contains only placeholder/test credentials from a dedicated test account, never production

If this file contains a real session token (even a test one), rotate it after this PR lands and add .auth/ to .gitignore. If it's already a regenerated test token, that's fine — but I'd still recommend gitignoring and regenerating it in the CI setup step to avoid accidental credential commits in the future.

2. proofshot-artifacts/ binary PNGs committed to git

M proofshot-artifacts/dashboard/dashboard-desktop-dark.png
M proofshot-artifacts/dashboard/dashboard-desktop-light.png
M proofshot-artifacts/dashboard/dashboard-mobile-dark.png
M proofshot-artifacts/dashboard/dashboard-mobile-light.png
M proofshot-artifacts/dashboard/dashboard-tablet-dark.png
M proofshot-artifacts/dashboard/dashboard-tablet-light.png

6 PNG files per PR adds up. Git is not optimized for binary blobs — they are stored as full copies, not deltas. Over time this will inflate the repository size noticeably. Consider using Git LFS for screenshot artifacts, or uploading them as CI job artifacts (ephemeral, not stored in the repo) and referencing the artifact URL in the PR description instead.

No infrastructure blockers

The PR does not touch CI pipelines, compose configuration, or any deployed component beyond the SvelteKit frontend. Safe to merge from an infrastructure perspective once the .auth/user.json situation is clarified.

## 🚀 Tobias Wendt — DevOps & Platform Engineer **Verdict: ⚠️ Approved with concerns** ### What I checked No changes to `docker-compose.yml`, CI workflow files, or infrastructure configuration. This PR is frontend-only. --- ### Concerns **1. `frontend/e2e/.auth/user.json` is a tracked file modified in this PR** ``` M frontend/e2e/.auth/user.json ``` This file stores Playwright's authenticated browser state (session cookies / storage). It is committed to the repository, which means real session tokens may be persisted in git history. Playwright's recommended approach is to either: - Add `.auth/` to `.gitignore` and generate it at the start of every CI run via `playwright/auth.setup.ts` - Or ensure the file contains only placeholder/test credentials from a dedicated test account, never production If this file contains a real session token (even a test one), rotate it after this PR lands and add `.auth/` to `.gitignore`. If it's already a regenerated test token, that's fine — but I'd still recommend gitignoring and regenerating it in the CI setup step to avoid accidental credential commits in the future. **2. `proofshot-artifacts/` binary PNGs committed to git** ``` M proofshot-artifacts/dashboard/dashboard-desktop-dark.png M proofshot-artifacts/dashboard/dashboard-desktop-light.png M proofshot-artifacts/dashboard/dashboard-mobile-dark.png M proofshot-artifacts/dashboard/dashboard-mobile-light.png M proofshot-artifacts/dashboard/dashboard-tablet-dark.png M proofshot-artifacts/dashboard/dashboard-tablet-light.png ``` 6 PNG files per PR adds up. Git is not optimized for binary blobs — they are stored as full copies, not deltas. Over time this will inflate the repository size noticeably. Consider using Git LFS for screenshot artifacts, or uploading them as CI job artifacts (ephemeral, not stored in the repo) and referencing the artifact URL in the PR description instead. ### No infrastructure blockers The PR does not touch CI pipelines, compose configuration, or any deployed component beyond the SvelteKit frontend. Safe to merge from an infrastructure perspective once the `.auth/user.json` situation is clarified.

marcel added 7 commits 2026-03-31 20:46:49 +02:00

test(admin): replace setTimeout timing hack with vi.waitFor in layout specs ... 81ed1ce3ed

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

style(dashboard): bump stats footnote from text-xs to text-sm for legibility ... 38b8804b17

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

chore(e2e): gitignore Playwright auth state — regenerate in CI via auth.setup.ts ... 518019f099

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(e2e): Classic Split — right column absent for read-only user, present for admin ... 595c2eb987

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(dashboard): delete DashboardMentions component — notifications page exists ... 5adef7bec5

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(dashboard): reduce incomplete docs widget from 5 to 3 items to prevent scroll ... 79faee554a

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(dashboard): move right column first in DOM for mobile-first upload zone ... 19035fbeab

On small screens the upload zone now appears above recent docs.
lg:order-last keeps it visually on the right at desktop width.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel merged commit 19035fbeab into main 2026-03-31 20:56:52 +02:00

marcel deleted branch feat/issue-171-dashboard-classic-split 2026-03-31 20:56:54 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#172