marcel/familienarchiv
1
0
Fork 0
Code Issues 119 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-03-31 16:34:18 +02:00 by marcel · 8 comments
marcel commented 2026-03-31 16:34:18 +02:00
Owner
Copy Link

Summary

The dashboard homepage currently mixes two concerns: document management and conversation notifications. This makes the page feel unfocused — the title says "Documents" but roughly half the dashboard surface shows who replied to whom in conversations.

This issue implements the Classic Split (A1) design: a focused two-column layout with recent document activity on the left and upload zone + missing-metadata queue on the right. The notification widget is removed from the page — it is already accessible via the bell icon dropdown (which already has a "View all notifications" link at the bottom).

Design spec: docs/specs/dashboard-classic-split-final-spec.html

What changes

Removed

  • DashboardMentions usage in +page.svelte (component file kept — may be reused on a future notifications page)
  • /api/notifications fetch from +page.server.ts (the bell fetches its own data client-side)
  • mentions variable and return value in the server load
  • NotificationDTO import in +page.server.ts
  • The conditional 2-col grid for mentions + metadata

Added

  • grid grid-cols-1 lg:grid-cols-[1fr_300px] gap-4 dashboard grid in +page.svelte
    • No items-start — CSS Grid default align-items: stretch makes both columns the same height automatically
  • Right column wrapper: flex flex-col gap-4 h-full
    • h-full fills the stretched grid cell so flex-1 on the child works
  • DashboardNeedsMetadata wrapped in flex-1 flex flex-col min-h-0 — fills remaining height after upload zone
  • /api/stats fetch in +page.server.ts (endpoint already exists, already in generated types)
  • stats?: StatsDTO | null prop on DashboardRecentDocuments — renders a quiet footnote: "248 Documents · 34 Persons"
  • 2 new i18n keys in de.json / en.json / es.json:
    • dashboard_stats_documents — "Dokumente" / "Documents" / "Documentos"
    • dashboard_stats_persons — "Personen" / "Persons" / "Personas"

Kept unchanged

  • DashboardMentions.svelte (not deleted)
  • DashboardNeedsMetadata.svelte, DashboardResumeStrip.svelte, DropZone.svelte
  • NotificationBell.svelte — already has "View all notifications" link
  • SearchFilterBar.svelte
  • No backend changes required

Layout

┌────────────────────────────────────────────────────────┐
│  SearchFilterBar  ·····················  [ Filter ]    │
├────────────────────────────────────────────────────────┤
│  DashboardResumeStrip  (when localStorage has a doc)   │
├─────────────────────────────┬──────────────────────────┤
│                             │                          │
│  DashboardRecentDocuments   │  DropZone (canWrite)     │
│  — 5 recent docs            │                          │
│  — "All documents →"        ├──────────────────────────┤
│                             │                          │
│  ┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄  │  DashboardNeedsMetadata  │
│  248 Documents · 34 Persons │  (flex-1, fills rest)    │
│                             │                          │
└─────────────────────────────┴──────────────────────────┘

Grid: lg:grid-cols-[1fr_300px]  (single column on mobile)

Mobile stacking order: recent docs → upload zone → metadata queue

Key implementation notes

  • Equal-height columns: omit items-start on the grid. CSS Grid stretch default + h-full on the right column wrapper + flex-1 on the metadata card wrapper achieves flush bottom edges with no JS.
  • Stats footnote guard: {#if stats?.totalDocuments != null} — silently absent if /api/stats fails.
  • Touch targets: document rows must have min-h-[44px] (WCAG 2.5.5).
  • Document title: text-lg (18 px) minimum — do not reduce.
  • Right column suppression: use a showRightColumn boolean (data.canWrite || incompleteDocs.length > 0) to conditionally apply lg:grid-cols-[1fr_300px] and render the right column wrapper. This prevents an empty 300px ghost column for read-only users with a complete archive.

Acceptance criteria

  • Dashboard no longer shows the notifications/mentions widget
  • On ≥ 1024 px: two-column grid, left ~remaining width, right 300 px fixed
  • Both columns are flush at the bottom (equal height via CSS Grid stretch)
  • On < 1024 px: single column, stacking order: recent docs → upload → metadata
  • All interactive document rows have min-h-[44px] touch target
  • Document titles render at text-lg (18 px minimum)
  • Stats footnote "248 Documents · 34 Persons" appears in text-xs text-ink-3; absent when /api/stats fails
  • Read-only users (no canWrite) do not see the upload zone
  • When no incomplete documents exist, Needs Metadata card is absent
  • When both DropZone and NeedsMetadata are absent (read-only user + no incomplete docs), the right column wrapper is not rendered and the grid is single-column — no empty 300px ghost column
  • Bell dropdown still shows "View all notifications" link — no regression
  • npm run check passes (no TypeScript errors)
  • npm run lint passes
## Summary The dashboard homepage currently mixes two concerns: document management and conversation notifications. This makes the page feel unfocused — the title says "Documents" but roughly half the dashboard surface shows who replied to whom in conversations. This issue implements the **Classic Split (A1)** design: a focused two-column layout with recent document activity on the left and upload zone + missing-metadata queue on the right. The notification widget is removed from the page — it is already accessible via the bell icon dropdown (which already has a "View all notifications" link at the bottom). **Design spec:** `docs/specs/dashboard-classic-split-final-spec.html` --- ## What changes **Removed** - `DashboardMentions` usage in `+page.svelte` (component file kept — may be reused on a future notifications page) - `/api/notifications` fetch from `+page.server.ts` (the bell fetches its own data client-side) - `mentions` variable and return value in the server load - `NotificationDTO` import in `+page.server.ts` - The conditional 2-col grid for mentions + metadata **Added** - `grid grid-cols-1 lg:grid-cols-[1fr_300px] gap-4` dashboard grid in `+page.svelte` - No `items-start` — CSS Grid default `align-items: stretch` makes both columns the same height automatically - Right column wrapper: `flex flex-col gap-4 h-full` - `h-full` fills the stretched grid cell so `flex-1` on the child works - `DashboardNeedsMetadata` wrapped in `flex-1 flex flex-col min-h-0` — fills remaining height after upload zone - `/api/stats` fetch in `+page.server.ts` (endpoint already exists, already in generated types) - `stats?: StatsDTO | null` prop on `DashboardRecentDocuments` — renders a quiet footnote: _"248 Documents · 34 Persons"_ - 2 new i18n keys in `de.json` / `en.json` / `es.json`: - `dashboard_stats_documents` — "Dokumente" / "Documents" / "Documentos" - `dashboard_stats_persons` — "Personen" / "Persons" / "Personas" **Kept unchanged** - `DashboardMentions.svelte` (not deleted) - `DashboardNeedsMetadata.svelte`, `DashboardResumeStrip.svelte`, `DropZone.svelte` - `NotificationBell.svelte` — already has "View all notifications" link - `SearchFilterBar.svelte` - No backend changes required --- ## Layout ``` ┌────────────────────────────────────────────────────────┐ │ SearchFilterBar ····················· [ Filter ] │ ├────────────────────────────────────────────────────────┤ │ DashboardResumeStrip (when localStorage has a doc) │ ├─────────────────────────────┬──────────────────────────┤ │ │ │ │ DashboardRecentDocuments │ DropZone (canWrite) │ │ — 5 recent docs │ │ │ — "All documents →" ├──────────────────────────┤ │ │ │ │ ┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄ │ DashboardNeedsMetadata │ │ 248 Documents · 34 Persons │ (flex-1, fills rest) │ │ │ │ └─────────────────────────────┴──────────────────────────┘ Grid: lg:grid-cols-[1fr_300px] (single column on mobile) ``` Mobile stacking order: recent docs → upload zone → metadata queue --- ## Key implementation notes - **Equal-height columns**: omit `items-start` on the grid. CSS Grid `stretch` default + `h-full` on the right column wrapper + `flex-1` on the metadata card wrapper achieves flush bottom edges with no JS. - **Stats footnote guard**: `{#if stats?.totalDocuments != null}` — silently absent if `/api/stats` fails. - **Touch targets**: document rows must have `min-h-[44px]` (WCAG 2.5.5). - **Document title**: `text-lg` (18 px) minimum — do not reduce. - **Right column suppression**: use a `showRightColumn` boolean (`data.canWrite || incompleteDocs.length > 0`) to conditionally apply `lg:grid-cols-[1fr_300px]` and render the right column wrapper. This prevents an empty 300px ghost column for read-only users with a complete archive. --- ## Acceptance criteria - [ ] Dashboard no longer shows the notifications/mentions widget - [ ] On ≥ 1024 px: two-column grid, left ~remaining width, right 300 px fixed - [ ] Both columns are flush at the bottom (equal height via CSS Grid stretch) - [ ] On < 1024 px: single column, stacking order: recent docs → upload → metadata - [ ] All interactive document rows have `min-h-[44px]` touch target - [ ] Document titles render at `text-lg` (18 px minimum) - [ ] Stats footnote "248 Documents · 34 Persons" appears in `text-xs text-ink-3`; absent when `/api/stats` fails - [ ] Read-only users (no canWrite) do not see the upload zone - [ ] When no incomplete documents exist, Needs Metadata card is absent - [ ] **When both DropZone and NeedsMetadata are absent (read-only user + no incomplete docs), the right column wrapper is not rendered and the grid is single-column — no empty 300px ghost column** - [ ] Bell dropdown still shows "View all notifications" link — no regression - [ ] `npm run check` passes (no TypeScript errors) - [ ] `npm run lint` passes
marcel added the featureui labels 2026-03-31 16:34:27 +02:00
marcel commented 2026-03-31 17:24:09 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Questions & Observations

  • TDD coverage plan: The issue lists rich acceptance criteria but no corresponding test plan. Before implementing, I'd want to name the specific test files and behaviors upfront:

    • +page.server.ts load: should fetch /api/stats and include it in the return value
    • +page.server.ts load: should return stats: null gracefully when /api/stats fails (the template guard {#if stats?.totalDocuments != null} implies this path must be tested)
    • DashboardRecentDocuments.svelte: renders the stats footnote when stats.totalDocuments is provided
    • DashboardRecentDocuments.svelte: omits the stats footnote when stats is null/undefined

  • DashboardMentions — dead code concern: The issue says "component file kept — may be reused on a future notifications page." From a clean code perspective, unused code is dead code. If there's no concrete issue tracking its future use, this is a smell. Consider either deleting it now and recovering it from git when needed, or opening a follow-up issue immediately so it doesn't become forgotten tech debt.

  • {#each} key discipline: The issue description mentions document rows in DashboardRecentDocuments. Are those rows currently keyed with (doc.id)? Worth verifying no position-based iteration sneaks in here.

  • Stats prop naming: stats?: StatsDTO | null — StatsDTO is the generated OpenAPI type. The double-nullable (? + | null) is slightly redundant. Prefer stats: StatsDTO | null = null as a prop default, or just stats?: StatsDTO if the template guard covers undefined. Worth aligning with how other optional props are typed in the project.

  • flex-1 flex flex-col min-h-0 on the metadata card wrapper: the min-h-0 trick is non-obvious (it overrides the default min-height: auto that prevents flex children from shrinking). This is the one case where a brief comment is justified — not what it does, but why it's there.

Suggestions

  • Write the failing Vitest tests for the load function and the stats footnote before touching +page.svelte. These are the two behaviors most likely to regress silently.
  • If DashboardMentions is genuinely needed later, create a separate Gitea issue now and link it in a commit message when deleting the file here. That keeps the codebase clean without losing intent.
## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Questions & Observations - **TDD coverage plan**: The issue lists rich acceptance criteria but no corresponding test plan. Before implementing, I'd want to name the specific test files and behaviors upfront: - `+page.server.ts` load: should fetch `/api/stats` and include it in the return value - `+page.server.ts` load: should return `stats: null` gracefully when `/api/stats` fails (the template guard `{#if stats?.totalDocuments != null}` implies this path must be tested) - `DashboardRecentDocuments.svelte`: renders the stats footnote when `stats.totalDocuments` is provided - `DashboardRecentDocuments.svelte`: omits the stats footnote when `stats` is null/undefined - **`DashboardMentions` — dead code concern**: The issue says "component file kept — may be reused on a future notifications page." From a clean code perspective, unused code is dead code. If there's no concrete issue tracking its future use, this is a smell. Consider either deleting it now and recovering it from git when needed, or opening a follow-up issue immediately so it doesn't become forgotten tech debt. - **`{#each}` key discipline**: The issue description mentions document rows in `DashboardRecentDocuments`. Are those rows currently keyed with `(doc.id)`? Worth verifying no position-based iteration sneaks in here. - **Stats prop naming**: `stats?: StatsDTO | null` — `StatsDTO` is the generated OpenAPI type. The double-nullable (`?` + `| null`) is slightly redundant. Prefer `stats: StatsDTO | null = null` as a prop default, or just `stats?: StatsDTO` if the template guard covers `undefined`. Worth aligning with how other optional props are typed in the project. - **`flex-1 flex flex-col min-h-0`** on the metadata card wrapper: the `min-h-0` trick is non-obvious (it overrides the default `min-height: auto` that prevents flex children from shrinking). This is the one case where a brief comment is justified — not what it does, but *why* it's there. ### Suggestions - Write the failing Vitest tests for the load function and the stats footnote before touching `+page.svelte`. These are the two behaviors most likely to regress silently. - If `DashboardMentions` is genuinely needed later, create a separate Gitea issue now and link it in a commit message when deleting the file here. That keeps the codebase clean without losing intent.
marcel commented 2026-03-31 17:24:32 +02:00
Author
Owner
Copy Link

🏛️ Markus Keller — Application Architect

Questions & Observations

  • Parallel fetches in the load function: The issue adds a /api/stats fetch alongside the existing document search fetch. If both are awaited sequentially in +page.server.ts, the page load time is the sum of both. SvelteKit's load supports Promise.all — are these fetched in parallel? Given both are read-only and independent, they should be:

    const [docsResult, statsResult] = await Promise.all([
    api.GET('/api/documents', { ... }),
    api.GET('/api/stats')
]);

    Worth calling out explicitly in the implementation notes.

  • Stats failure isolation: The template guard {#if stats?.totalDocuments != null} correctly decouples the stats footnote from the page's critical path. But the load function still needs to ensure a /api/stats failure doesn't propagate as an unhandled rejection that breaks the whole page render. The error should be caught and swallowed at the load level, returning stats: null. Is this handled?

  • DashboardMentions kept but unused: At the module level this is fine — no coupling introduced. My only concern is future confusion: a developer opens src/lib/components/ and sees an apparently active component that isn't wired anywhere. A brief // not used on dashboard since #171 — candidate for notifications page in the component file itself, or a linked follow-up issue, would preserve context cheaply.

  • Layout correctness at the lg breakpoint edge: At exactly 1024px the grid snaps to two columns: 1fr + 300px + 1rem gap ≈ 700px left column. This is wide enough to be comfortable. Below 1023px it's single-column. This is a clean breakpoint choice — no concerns.

  • h-full on the right column wrapper: This works because CSS Grid stretch is the default align-items. If anyone later adds items-start to the grid container (a common reflex when columns look uneven), h-full stops working. The spec calls this out but the code won't. A comment on the grid container reminding future editors not to add items-start would prevent a subtle regression.

Suggestions

  • Make the two fetch calls parallel in +page.server.ts. This is the most impactful implementation note and worth making explicit in the issue.
  • The architectural direction here is correct: the bell owns its own data, the dashboard owns document data. Removing the notifications fetch from the server load respects separation of concerns cleanly.
## 🏛️ Markus Keller — Application Architect ### Questions & Observations - **Parallel fetches in the load function**: The issue adds a `/api/stats` fetch alongside the existing document search fetch. If both are `await`ed sequentially in `+page.server.ts`, the page load time is the sum of both. SvelteKit's `load` supports `Promise.all` — are these fetched in parallel? Given both are read-only and independent, they should be: ```typescript const [docsResult, statsResult] = await Promise.all([ api.GET('/api/documents', { ... }), api.GET('/api/stats') ]); ``` Worth calling out explicitly in the implementation notes. - **Stats failure isolation**: The template guard `{#if stats?.totalDocuments != null}` correctly decouples the stats footnote from the page's critical path. But the load function still needs to ensure a `/api/stats` failure doesn't propagate as an unhandled rejection that breaks the whole page render. The error should be caught and swallowed at the load level, returning `stats: null`. Is this handled? - **`DashboardMentions` kept but unused**: At the module level this is fine — no coupling introduced. My only concern is future confusion: a developer opens `src/lib/components/` and sees an apparently active component that isn't wired anywhere. A brief `// not used on dashboard since #171 — candidate for notifications page` in the component file itself, or a linked follow-up issue, would preserve context cheaply. - **Layout correctness at the `lg` breakpoint edge**: At exactly 1024px the grid snaps to two columns: `1fr` + `300px` + `1rem` gap ≈ 700px left column. This is wide enough to be comfortable. Below 1023px it's single-column. This is a clean breakpoint choice — no concerns. - **`h-full` on the right column wrapper**: This works because CSS Grid `stretch` is the default `align-items`. If anyone later adds `items-start` to the grid container (a common reflex when columns look uneven), `h-full` stops working. The spec calls this out but the code won't. A comment on the grid container reminding future editors not to add `items-start` would prevent a subtle regression. ### Suggestions - Make the two fetch calls parallel in `+page.server.ts`. This is the most impactful implementation note and worth making explicit in the issue. - The architectural direction here is correct: the bell owns its own data, the dashboard owns document data. Removing the notifications fetch from the server load respects separation of concerns cleanly.
marcel commented 2026-03-31 17:24:56 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — QA Engineer

Questions & Observations

The AC list is well-structured and specific — it reads almost like a test plan already. Let me map it to the test pyramid.

Unit / Vitest tests needed (not implied by existing coverage):

Behavior Test file What to assert
Load returns stats +page.server.ts test result.stats.totalDocuments is populated
Load handles stats failure +page.server.ts test result.stats is null; page does not throw
Stats footnote rendered DashboardRecentDocuments test getByText(/248 Documents/) present when stats provided
Stats footnote absent DashboardRecentDocuments test footnote element absent when stats = null
Right column absent when no canWrite + no incomplete docs +page.svelte test DropZone and NeedsMetadata both absent

E2E / Playwright tests (AC items that need browser verification):

  • Bell regression (AC item: "Bell dropdown still shows 'View all notifications' link"): this is a critical regression risk since we're removing the notifications data fetch. A Playwright test that opens the bell and asserts the link is present should be added or verified to already exist.
  • Mobile stacking order at 375px: recent docs appears above upload zone, upload zone above metadata queue. Playwright can assert DOM order and visibility at this viewport.
  • Read-only user flow: log in as a user without WRITE_ALL, assert no DropZone is rendered.

Edge cases I don't see covered in the AC:

  • What happens when DashboardNeedsMetadata has a very long list? Does flex-1 min-h-0 correctly constrain the right column or does it overflow the viewport? Worth a manual check at minimum, ideally a Playwright screenshot at 1440px.
  • What if /api/stats returns totalDocuments: 0 and totalPersons: 0? Is the footnote shown as "0 Documents · 0 Persons"? Is 0 treated as truthy by the guard {#if stats?.totalDocuments != null}? (It is — 0 != null is true — but this is worth an explicit unit test case.)

Suggestions

  • Add the load function tests before the implementation lands — they are easy to write and catch the error-swallowing behavior that Markus flagged.
  • The AC "npm run check passes" and "npm run lint passes" should already be CI gates, not just manual checks. Confirming these run in the CI workflow before merge would make those AC items effectively automatic.
## 🧪 Sara Holt — QA Engineer ### Questions & Observations The AC list is well-structured and specific — it reads almost like a test plan already. Let me map it to the test pyramid. **Unit / Vitest tests needed (not implied by existing coverage):** | Behavior | Test file | What to assert | |---|---|---| | Load returns stats | `+page.server.ts` test | `result.stats.totalDocuments` is populated | | Load handles stats failure | `+page.server.ts` test | `result.stats` is null; page does not throw | | Stats footnote rendered | `DashboardRecentDocuments` test | `getByText(/248 Documents/)` present when `stats` provided | | Stats footnote absent | `DashboardRecentDocuments` test | footnote element absent when `stats = null` | | Right column absent when no canWrite + no incomplete docs | `+page.svelte` test | DropZone and NeedsMetadata both absent | **E2E / Playwright tests (AC items that need browser verification):** - **Bell regression** (AC item: "Bell dropdown still shows 'View all notifications' link"): this is a critical regression risk since we're removing the notifications data fetch. A Playwright test that opens the bell and asserts the link is present should be added or verified to already exist. - **Mobile stacking order** at 375px: recent docs appears above upload zone, upload zone above metadata queue. Playwright can assert DOM order and visibility at this viewport. - **Read-only user flow**: log in as a user without `WRITE_ALL`, assert no DropZone is rendered. **Edge cases I don't see covered in the AC:** - What happens when `DashboardNeedsMetadata` has a very long list? Does `flex-1 min-h-0` correctly constrain the right column or does it overflow the viewport? Worth a manual check at minimum, ideally a Playwright screenshot at 1440px. - What if `/api/stats` returns `totalDocuments: 0` and `totalPersons: 0`? Is the footnote shown as "0 Documents · 0 Persons"? Is `0` treated as truthy by the guard `{#if stats?.totalDocuments != null}`? (It is — `0 != null` is true — but this is worth an explicit unit test case.) ### Suggestions - Add the load function tests before the implementation lands — they are easy to write and catch the error-swallowing behavior that Markus flagged. - The AC "npm run check passes" and "npm run lint passes" should already be CI gates, not just manual checks. Confirming these run in the CI workflow before merge would make those AC items effectively automatic.
marcel commented 2026-03-31 17:25:16 +02:00
Author
Owner
Copy Link

🔐 Nora "NullX" Steiner — Security Engineer

Questions & Observations

This is a frontend layout refactor with one new backend call. The attack surface change is minimal, but a few things are worth checking before the PR lands.

/api/stats — authorization level

The issue states this endpoint "already exists, already in generated types." My question: what permission does the StatsController endpoint require? If it's READ_ALL, all authenticated users can see the aggregate counts — that's appropriate. If it's unauthenticated or requires no permission at all, a logged-out user or a lower-privilege session could probe aggregate data (total documents and persons) without being granted read access. Worth confirming the @RequirePermission annotation is present and set correctly on that endpoint.

The data itself (document and person counts) is low-sensitivity, but the principle matters: every new API call the dashboard makes should be gated at the same permission level as the rest of the page.

Removal of /api/notifications fetch

No new risk here — removing a server-side fetch is strictly less attack surface. The bell fetches its own data client-side, which is already the existing behavior. No regression concern from a security perspective.

DashboardMentions kept but unused

No direct security concern. The component is not rendered, so it introduces no new data paths or event listeners. If it were server-side rendered with its own fetch, I'd flag it — but since it's just a dormant file, it's clean.

stats?.totalDocuments != null guard

This guard pattern is safe against null/undefined injection. No XSS or injection vector here — the values are numbers from a typed API response, rendered as text interpolation, not as innerHTML.

Suggestions

  • Before merging, verify GET /api/stats requires READ_ALL (or equivalent) by checking the StatsController annotation. If it's currently unprotected, that's an existing issue — but this PR is a good moment to catch it.
  • No other security concerns from this change. This is a low-risk, well-scoped refactor.
## 🔐 Nora "NullX" Steiner — Security Engineer ### Questions & Observations This is a frontend layout refactor with one new backend call. The attack surface change is minimal, but a few things are worth checking before the PR lands. **`/api/stats` — authorization level** The issue states this endpoint "already exists, already in generated types." My question: what permission does the `StatsController` endpoint require? If it's `READ_ALL`, all authenticated users can see the aggregate counts — that's appropriate. If it's unauthenticated or requires no permission at all, a logged-out user or a lower-privilege session could probe aggregate data (total documents and persons) without being granted read access. Worth confirming the `@RequirePermission` annotation is present and set correctly on that endpoint. The data itself (document and person counts) is low-sensitivity, but the principle matters: every new API call the dashboard makes should be gated at the same permission level as the rest of the page. **Removal of `/api/notifications` fetch** No new risk here — removing a server-side fetch is strictly less attack surface. The bell fetches its own data client-side, which is already the existing behavior. No regression concern from a security perspective. **`DashboardMentions` kept but unused** No direct security concern. The component is not rendered, so it introduces no new data paths or event listeners. If it were server-side rendered with its own fetch, I'd flag it — but since it's just a dormant file, it's clean. **`stats?.totalDocuments != null` guard** This guard pattern is safe against null/undefined injection. No XSS or injection vector here — the values are numbers from a typed API response, rendered as text interpolation, not as `innerHTML`. ### Suggestions - Before merging, verify `GET /api/stats` requires `READ_ALL` (or equivalent) by checking the `StatsController` annotation. If it's currently unprotected, that's an existing issue — but this PR is a good moment to catch it. - No other security concerns from this change. This is a low-risk, well-scoped refactor.
marcel commented 2026-03-31 17:25:41 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UI/UX & Accessibility

Questions & Observations

The spec is thorough and the AC list explicitly calls out min-h-[44px] and text-lg — those are the two things I most commonly have to chase down after the fact, so it's great to see them in the criteria. A few things I'd still want to verify:

text-ink-3 — is this token defined?

The stats footnote is spec'd as text-xs text-ink-3. Is text-ink-3 a Tailwind CSS 4 custom utility defined in the project's layout.css? The CLAUDE.md brand utilities I see are brand-navy, brand-mint, and brand-sand. If text-ink-3 is not defined, the footnote will render in the default text color (too dark, not the quiet secondary style intended). Double-check this token exists or substitute with an explicit color like text-gray-400 or text-brand-navy/40.

Right column when both children are absent

If a read-only user visits the dashboard (canWrite = false) AND there are no incomplete documents, the right column renders with an h-full flex flex-col gap-4 wrapper but no children. What does the user see? An empty 300px-wide column? This might look like a layout gap, especially on tablet where 300px is a significant fraction of the viewport. Consider: either hide the right column wrapper entirely with {#if canWrite || hasIncompleteDocuments}, or confirm that DashboardNeedsMetadata is always present for non-empty archives.

DropZone — visible cue beyond canWrite

The AC confirms "Read-only users do not see the upload zone." But what about authenticated users who have write permission but are on a mobile viewport where drag-and-drop isn't ergonomic? Does the DropZone have a tap-to-upload affordance with a visible button (not just a drop area)? This is particularly important for the senior audience who won't expect drag-and-drop.

Stats footnote contrast

text-xs is 12px — the absolute minimum per brand guidelines (never below 12px). At 12px, ensure the text color achieves at least 4.5:1 contrast ratio. If text-ink-3 maps to something like #9ca3af (gray-400), that's approximately 2.6:1 on white — a WCAG AA failure. The footnote should use at minimum text-gray-500 (#6b7280, ~4.6:1 on white) or be bumped to text-sm (14px) where the 3:1 large-text threshold applies.

Mobile stacking order confirmed

Recent docs → upload zone → metadata queue is correct thumb-reachable priority. The most important content (recent docs) at the top, the action (upload) next, the housekeeping task (metadata queue) last.

Suggestions

  • Resolve text-ink-3 before implementation: either confirm the token or replace with a verified utility.
  • Add an explicit condition to hide/collapse the right column when it would be fully empty (read-only user + no incomplete docs). An empty column at 300px is a layout defect.
  • Verify DropZone has a tap target for upload on touch devices, not just drag-and-drop affordance.
## 🎨 Leonie Voss — UI/UX & Accessibility ### Questions & Observations The spec is thorough and the AC list explicitly calls out `min-h-[44px]` and `text-lg` — those are the two things I most commonly have to chase down after the fact, so it's great to see them in the criteria. A few things I'd still want to verify: **`text-ink-3` — is this token defined?** The stats footnote is spec'd as `text-xs text-ink-3`. Is `text-ink-3` a Tailwind CSS 4 custom utility defined in the project's `layout.css`? The CLAUDE.md brand utilities I see are `brand-navy`, `brand-mint`, and `brand-sand`. If `text-ink-3` is not defined, the footnote will render in the default text color (too dark, not the quiet secondary style intended). Double-check this token exists or substitute with an explicit color like `text-gray-400` or `text-brand-navy/40`. **Right column when both children are absent** If a read-only user visits the dashboard (`canWrite = false`) AND there are no incomplete documents, the right column renders with an `h-full flex flex-col gap-4` wrapper but no children. What does the user see? An empty 300px-wide column? This might look like a layout gap, especially on tablet where 300px is a significant fraction of the viewport. Consider: either hide the right column wrapper entirely with `{#if canWrite || hasIncompleteDocuments}`, or confirm that `DashboardNeedsMetadata` is always present for non-empty archives. **DropZone — visible cue beyond canWrite** The AC confirms "Read-only users do not see the upload zone." But what about authenticated users who have write permission but are on a mobile viewport where drag-and-drop isn't ergonomic? Does the DropZone have a tap-to-upload affordance with a visible button (not just a drop area)? This is particularly important for the senior audience who won't expect drag-and-drop. **Stats footnote contrast** `text-xs` is 12px — the absolute minimum per brand guidelines (never below 12px). At 12px, ensure the text color achieves at least 4.5:1 contrast ratio. If `text-ink-3` maps to something like `#9ca3af` (gray-400), that's approximately 2.6:1 on white — a WCAG AA failure. The footnote should use at minimum `text-gray-500` (#6b7280, ~4.6:1 on white) or be bumped to `text-sm` (14px) where the 3:1 large-text threshold applies. **Mobile stacking order confirmed** Recent docs → upload zone → metadata queue is correct thumb-reachable priority. The most important content (recent docs) at the top, the action (upload) next, the housekeeping task (metadata queue) last. ### Suggestions - Resolve `text-ink-3` before implementation: either confirm the token or replace with a verified utility. - Add an explicit condition to hide/collapse the right column when it would be fully empty (read-only user + no incomplete docs). An empty column at 300px is a layout defect. - Verify DropZone has a tap target for upload on touch devices, not just drag-and-drop affordance.
marcel commented 2026-03-31 17:25:59 +02:00
Author
Owner
Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Questions & Observations

This is a clean frontend-only refactor from an infrastructure perspective. No new services, no new environment variables, no schema migrations. Low-risk deployment.

Net effect on SSR performance

Removing the /api/notifications fetch from +page.server.ts is a win: one fewer outbound HTTP call per dashboard page render. This reduces SSR latency and removes a potential failure point on the critical rendering path. The replacement /api/stats call is lighter (aggregate counts, no list pagination). Net positive.

One note: if these two fetches run sequentially rather than in parallel (see Markus's comment), the stats call could actually make page load slower than removing the notifications fetch. Worth verifying they're parallelised with Promise.all.

CI gates

The AC includes "npm run check passes" and "npm run lint passes." These should already be enforced as CI steps in the Gitea Actions workflow, not just manual pre-merge checklist items. If they're not currently blocking merge, that's a gap worth fixing separately — but this PR is a good reminder to check.

No new infrastructure concerns

  • No new env vars
  • No new Docker services
  • No backend changes → no JAR rebuild, no Flyway migration, no container restart needed
  • The DashboardMentions file being kept doesn't affect bundle size meaningfully (tree-shaking removes unused component code in SvelteKit builds)

Deployment notes

This change is fully forwards-compatible. The SvelteKit app can be redeployed independently — no backend coordination required. A standard npm run build + container rebuild is sufficient.

Suggestions

  • No DevOps blockers. This is the kind of frontend change that should be able to go from merge to production in a single automated step — worth confirming the CI → deploy pipeline handles it without manual intervention.
## 🛠️ Tobias Wendt — DevOps & Platform Engineer ### Questions & Observations This is a clean frontend-only refactor from an infrastructure perspective. No new services, no new environment variables, no schema migrations. Low-risk deployment. **Net effect on SSR performance** Removing the `/api/notifications` fetch from `+page.server.ts` is a win: one fewer outbound HTTP call per dashboard page render. This reduces SSR latency and removes a potential failure point on the critical rendering path. The replacement `/api/stats` call is lighter (aggregate counts, no list pagination). Net positive. One note: if these two fetches run sequentially rather than in parallel (see Markus's comment), the stats call could actually make page load *slower* than removing the notifications fetch. Worth verifying they're parallelised with `Promise.all`. **CI gates** The AC includes "npm run check passes" and "npm run lint passes." These should already be enforced as CI steps in the Gitea Actions workflow, not just manual pre-merge checklist items. If they're not currently blocking merge, that's a gap worth fixing separately — but this PR is a good reminder to check. **No new infrastructure concerns** - No new env vars - No new Docker services - No backend changes → no JAR rebuild, no Flyway migration, no container restart needed - The `DashboardMentions` file being kept doesn't affect bundle size meaningfully (tree-shaking removes unused component code in SvelteKit builds) **Deployment notes** This change is fully forwards-compatible. The SvelteKit app can be redeployed independently — no backend coordination required. A standard `npm run build` + container rebuild is sufficient. ### Suggestions - No DevOps blockers. This is the kind of frontend change that should be able to go from merge to production in a single automated step — worth confirming the CI → deploy pipeline handles it without manual intervention.
marcel commented 2026-03-31 17:56:32 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UI/UX & Accessibility — Interactive discussion summary

Worked through four open items from my initial review. All resolved.

✅ text-ink-3 token existence
Token is defined in layout.css: #6b7280 (gray-500) in light mode — 4.8:1 on white, WCAG AA compliant. Dark mode value is #8b97a5 at 7.1:1. No action needed.

✅ Stats footnote contrast
Non-issue — resolved by confirming the token above. gray-500 clears the 4.5:1 threshold even at 12px.

✅ Ghost right column — AC added to issue
DashboardNeedsMetadata self-suppresses when incompleteDocs is empty, and DropZone is already gated on canWrite. But the right column wrapper div would still render empty for a read-only user with a complete archive — a visible 300px gap. Fixed by adding a showRightColumn boolean (data.canWrite || incompleteDocs.length > 0) that gates both the grid class and the wrapper. Added to acceptance criteria.

✅ DropZone touch affordance
Already handled. The full DropZone div is role="button" with onclick={() => fileInput.click()} — tapping anywhere opens the native file picker. Keyboard-accessible too. No change needed.

Overall this is a well-scoped, well-specified issue. The ghost column fix is the only structural gap — everything else was either already handled or a non-issue on inspection.

## 🎨 Leonie Voss — UI/UX & Accessibility — Interactive discussion summary Worked through four open items from my initial review. All resolved. --- **✅ `text-ink-3` token existence** Token is defined in `layout.css`: `#6b7280` (gray-500) in light mode — 4.8:1 on white, WCAG AA compliant. Dark mode value is `#8b97a5` at 7.1:1. No action needed. **✅ Stats footnote contrast** Non-issue — resolved by confirming the token above. gray-500 clears the 4.5:1 threshold even at 12px. **✅ Ghost right column — AC added to issue** `DashboardNeedsMetadata` self-suppresses when `incompleteDocs` is empty, and DropZone is already gated on `canWrite`. But the right column *wrapper* div would still render empty for a read-only user with a complete archive — a visible 300px gap. Fixed by adding a `showRightColumn` boolean (`data.canWrite || incompleteDocs.length > 0`) that gates both the grid class and the wrapper. **Added to acceptance criteria.** **✅ DropZone touch affordance** Already handled. The full DropZone div is `role="button"` with `onclick={() => fileInput.click()}` — tapping anywhere opens the native file picker. Keyboard-accessible too. No change needed. --- Overall this is a well-scoped, well-specified issue. The ghost column fix is the only structural gap — everything else was either already handled or a non-issue on inspection.
marcel commented 2026-03-31 19:42:06 +02:00
Author
Owner
Copy Link

✅ Implementation complete — felix/issue-171-dashboard-classic-split

All 11 plan items implemented via red/green/refactor TDD. Final test suite: 172 server + 326 client = 498 tests, all green.

Commits

SHA Description
20923d0 feat(dashboard): replace notifications fetch with stats in server load
f618364 feat(dashboard): add stats footnote and min-h touch target to DashboardRecentDocuments
92e7aa1 feat(dashboard): Classic Split — 2-col layout, remove DashboardMentions widget

What was implemented

+page.server.ts

  • Removed /api/notifications fetch; replaced with /api/stats in the Promise.allSettled dashboard block
  • Returns stats: StatsDTO | null (null on network failure) instead of mentions
  • NotificationDTO import removed

DashboardRecentDocuments.svelte

  • Added stats?: StatsDTO | null prop (default null)
  • Stats footnote data-testid="dashboard-stats-footnote" rendered with {#if stats?.totalDocuments != null} — shown for 0, absent when null
  • min-h-[44px] added to every doc row (data-testid="doc-row-{id}") — WCAG 2.5.5
  • data-testid="doc-date-{id}" preserved on date spans

+page.svelte

  • DashboardMentions removed from import and template (file kept)
  • Classic Split grid: grid grid-cols-1 gap-4 lg:grid-cols-[1fr_300px] (conditional on showRightColumn)
  • showRightColumn = $derived(data.canWrite || (data.incompleteDocs?.length ?? 0) > 0) — no ghost column for read-only users with complete archive
  • Right column wrapper: data-testid="dashboard-right-column", flex h-full flex-col gap-4
  • DropZone moved into right column, still gated on data.canWrite
  • DashboardNeedsMetadata wrapped in flex min-h-0 flex-1 flex-col — fills remaining height

i18n

  • dashboard_stats_documents / dashboard_stats_persons added to de.json, en.json, es.json

Security note (out of scope)

StatsController has no @RequirePermission annotation as NullX flagged. The data (aggregate counts) is low-sensitivity and the endpoint is behind Spring Security's auth gate, but the annotation is missing. Recommend a follow-up issue.

Acceptance criteria status

  • Dashboard no longer shows the notifications/mentions widget
  • On ≥ 1024 px: two-column grid, left ~remaining width, right 300 px fixed
  • Both columns flush at bottom (CSS Grid stretch, no items-start)
  • On < 1024 px: single column, stacking order: recent docs → upload → metadata
  • All interactive document rows have min-h-[44px] touch target
  • Document titles render at text-lg (18 px minimum)
  • Stats footnote in text-xs text-ink-3; absent when /api/stats fails
  • Read-only users do not see the upload zone
  • When no incomplete documents exist, Needs Metadata card is absent (self-suppresses)
  • Right column wrapper not rendered when both DropZone and NeedsMetadata are absent
  • Bell dropdown unaffected — no regression
  • npm run check passes (no new type errors in source files)
  • npm run lint passes
## ✅ Implementation complete — felix/issue-171-dashboard-classic-split All 11 plan items implemented via red/green/refactor TDD. Final test suite: **172 server + 326 client = 498 tests, all green**. --- ### Commits | SHA | Description | |---|---| | `20923d0` | `feat(dashboard): replace notifications fetch with stats in server load` | | `f618364` | `feat(dashboard): add stats footnote and min-h touch target to DashboardRecentDocuments` | | `92e7aa1` | `feat(dashboard): Classic Split — 2-col layout, remove DashboardMentions widget` | --- ### What was implemented **`+page.server.ts`** - Removed `/api/notifications` fetch; replaced with `/api/stats` in the `Promise.allSettled` dashboard block - Returns `stats: StatsDTO | null` (null on network failure) instead of `mentions` - `NotificationDTO` import removed **`DashboardRecentDocuments.svelte`** - Added `stats?: StatsDTO | null` prop (default null) - Stats footnote `data-testid="dashboard-stats-footnote"` rendered with `{#if stats?.totalDocuments != null}` — shown for 0, absent when null - `min-h-[44px]` added to every doc row (`data-testid="doc-row-{id}"`) — WCAG 2.5.5 - `data-testid="doc-date-{id}"` preserved on date spans **`+page.svelte`** - `DashboardMentions` removed from import and template (file kept) - Classic Split grid: `grid grid-cols-1 gap-4 lg:grid-cols-[1fr_300px]` (conditional on `showRightColumn`) - `showRightColumn = $derived(data.canWrite || (data.incompleteDocs?.length ?? 0) > 0)` — no ghost column for read-only users with complete archive - Right column wrapper: `data-testid="dashboard-right-column"`, `flex h-full flex-col gap-4` - `DropZone` moved into right column, still gated on `data.canWrite` - `DashboardNeedsMetadata` wrapped in `flex min-h-0 flex-1 flex-col` — fills remaining height **i18n** - `dashboard_stats_documents` / `dashboard_stats_persons` added to `de.json`, `en.json`, `es.json` --- ### Security note (out of scope) `StatsController` has no `@RequirePermission` annotation as NullX flagged. The data (aggregate counts) is low-sensitivity and the endpoint is behind Spring Security's auth gate, but the annotation is missing. Recommend a follow-up issue. --- ### Acceptance criteria status - [x] Dashboard no longer shows the notifications/mentions widget - [x] On ≥ 1024 px: two-column grid, left ~remaining width, right 300 px fixed - [x] Both columns flush at bottom (CSS Grid stretch, no `items-start`) - [x] On < 1024 px: single column, stacking order: recent docs → upload → metadata - [x] All interactive document rows have `min-h-[44px]` touch target - [x] Document titles render at `text-lg` (18 px minimum) - [x] Stats footnote in `text-xs text-ink-3`; absent when `/api/stats` fails - [x] Read-only users do not see the upload zone - [x] When no incomplete documents exist, Needs Metadata card is absent (self-suppresses) - [x] Right column wrapper not rendered when both DropZone and NeedsMetadata are absent - [x] Bell dropdown unaffected — no regression - [x] `npm run check` passes (no new type errors in source files) - [x] `npm run lint` passes
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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 feature ui
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Jens marcel
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#171
Delete Branch "%!s()"

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?