refactor(frontend): move statusDotClass/statusLabel from person/ to document/ #424

New Issue

Open

opened 2026-05-05 15:22:02 +02:00 by marcel · 8 comments

marcel commented 2026-05-05 15:22:02 +02:00

Owner

Copy Link

Context

Flagged by @mkeller and @felixbrandt in PR #422 review.

statusDotClass and statusLabel are helpers that return CSS class strings and display labels for DocumentStatus values (PLACEHOLDER, UPLOADED, TRANSCRIBED, REVIEWED, ARCHIVED). Document status is a concept owned by the document domain.

Currently these functions live in $lib/person/personFormat.ts — a pre-existing placement that this refactor was the right moment to fix. DocumentStatusChip imports them from the person package, creating a domain boundary leak.

Current state

// $lib/person/personFormat.ts — wrong home
export function statusDotClass(status: DocumentStatus): string { ... }
export function statusLabel(status: DocumentStatus): string { ... }

// $lib/document/DocumentStatusChip.svelte — cross-domain import
import { statusDotClass, statusLabel } from '$lib/person/personFormat';

Target state

Move statusDotClass and statusLabel into $lib/document/documentStatusLabel.ts, which already exists and exports formatDocumentStatus. The document domain becomes self-contained.

// $lib/document/documentStatusLabel.ts — correct home
export function statusDotClass(status: DocumentStatus): string { ... }
export function statusLabel(status: DocumentStatus): string { ... }

Update DocumentStatusChip.svelte and any other consumers to import from $lib/document/documentStatusLabel.

Acceptance criteria

• statusDotClass and statusLabel are defined in $lib/document/documentStatusLabel.ts
• $lib/person/personFormat.ts no longer exports document-status functions
• DocumentStatusChip.svelte imports from $lib/document/documentStatusLabel
• All existing tests pass
• npm run check type-checks clean

Related

• PR #422 (domain restructuring) — flagged as follow-up by @mkeller and @felixbrandt
• Epic #406 (frontend domain packaging)

## Context Flagged by @mkeller and @felixbrandt in PR #422 review. `statusDotClass` and `statusLabel` are helpers that return CSS class strings and display labels for `DocumentStatus` values (`PLACEHOLDER`, `UPLOADED`, `TRANSCRIBED`, `REVIEWED`, `ARCHIVED`). Document status is a concept owned by the document domain. Currently these functions live in `$lib/person/personFormat.ts` — a pre-existing placement that this refactor was the right moment to fix. `DocumentStatusChip` imports them from the person package, creating a domain boundary leak. ## Current state ```typescript // $lib/person/personFormat.ts — wrong home export function statusDotClass(status: DocumentStatus): string { ... } export function statusLabel(status: DocumentStatus): string { ... } ``` ```typescript // $lib/document/DocumentStatusChip.svelte — cross-domain import import { statusDotClass, statusLabel } from '$lib/person/personFormat'; ``` ## Target state Move `statusDotClass` and `statusLabel` into `$lib/document/documentStatusLabel.ts`, which already exists and exports `formatDocumentStatus`. The document domain becomes self-contained. ```typescript // $lib/document/documentStatusLabel.ts — correct home export function statusDotClass(status: DocumentStatus): string { ... } export function statusLabel(status: DocumentStatus): string { ... } ``` Update `DocumentStatusChip.svelte` and any other consumers to import from `$lib/document/documentStatusLabel`. ## Acceptance criteria - `statusDotClass` and `statusLabel` are defined in `$lib/document/documentStatusLabel.ts` - `$lib/person/personFormat.ts` no longer exports document-status functions - `DocumentStatusChip.svelte` imports from `$lib/document/documentStatusLabel` - All existing tests pass - `npm run check` type-checks clean ## Related - PR #422 (domain restructuring) — flagged as follow-up by @mkeller and @felixbrandt - Epic #406 (frontend domain packaging)

marcel added the P3-laterlegibilityrefactor labels 2026-05-05 15:22:10 +02:00

marcel referenced this issue 2026-05-05 15:22:33 +02:00

refactor(frontend): restructure lib/ from flat-by-type to domain-based (#408) #422

marcel referenced this issue 2026-05-05 15:23:01 +02:00

refactor(frontend): restructure lib/ from flat-by-type to domain-based (#408) #422

marcel commented 2026-05-07 17:48:48 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Observations

• The issue correctly identifies the violation: document-status display logic (CSS classes, human-readable labels) was living in $lib/person/personFormat.ts, creating an inward dependency from the person domain into the document domain. This breaks the rule that each module owns its own data, logic, and API surface.
• The fix is already committed. Commits 507fa088 and d9a4faf4 on the current branch (feat/issue-411-legibility-cleanup) completed the migration: statusDotClass and the former statusLabel alias are now in $lib/document/documentStatusLabel.ts. The alias was subsequently eliminated and callers use formatDocumentStatus directly — which is the right call, since a one-liner alias with no additional behaviour is noise.
• The boundaries/dependencies ESLint rule is now in place (eslint-plugin-boundaries in eslint.config.js), with person only allowed to depend on shared. This means the previous violation would be caught automatically going forward — the structural fix and the enforcement mechanism land together, which is the correct order.
• The DocumentMetadataDrawer.svelte import now correctly reads from $lib/document/documentStatusLabel (verified in the working tree). The invites page has a local statusLabel function, but that handles invite statuses (active/exhausted/revoked/expired), not DocumentStatus — no confusion there.
• No CLAUDE.md or architectural diagram updates appear necessary: no new routes, packages, or domain concepts were introduced. This is a file-level relocation within an existing domain.

Recommendations

• Close this issue as done once the feat/issue-411-legibility-cleanup branch merges. The acceptance criteria are fully met in the working tree.
• Run npm run lint with the boundaries rule enabled as part of the PR CI gate — it will catch any future regression of this pattern at the import level rather than requiring a code review to spot it.
• The statusDotClass function currently returns string but TypeScript will warn because the switch has no default arm. Since DocumentStatus is a union type the compiler knows is exhaustive, this is fine with noImplicitReturns: false — but adding a default: return 'bg-gray-400' as a defensive fallback costs nothing and removes the TypeScript noise if the status enum ever gains a new value before the function is updated.

## 🏛️ Markus Keller — Application Architect ### Observations - The issue correctly identifies the violation: document-status display logic (CSS classes, human-readable labels) was living in `$lib/person/personFormat.ts`, creating an inward dependency from the person domain into the document domain. This breaks the rule that each module owns its own data, logic, and API surface. - **The fix is already committed.** Commits `507fa088` and `d9a4faf4` on the current branch (`feat/issue-411-legibility-cleanup`) completed the migration: `statusDotClass` and the former `statusLabel` alias are now in `$lib/document/documentStatusLabel.ts`. The alias was subsequently eliminated and callers use `formatDocumentStatus` directly — which is the right call, since a one-liner alias with no additional behaviour is noise. - The `boundaries/dependencies` ESLint rule is now in place (`eslint-plugin-boundaries` in `eslint.config.js`), with `person` only allowed to depend on `shared`. This means the previous violation would be caught automatically going forward — the structural fix and the enforcement mechanism land together, which is the correct order. - The `DocumentMetadataDrawer.svelte` import now correctly reads from `$lib/document/documentStatusLabel` (verified in the working tree). The invites page has a local `statusLabel` function, but that handles invite statuses (`active/exhausted/revoked/expired`), not `DocumentStatus` — no confusion there. - No `CLAUDE.md` or architectural diagram updates appear necessary: no new routes, packages, or domain concepts were introduced. This is a file-level relocation within an existing domain. ### Recommendations - Close this issue as done once the `feat/issue-411-legibility-cleanup` branch merges. The acceptance criteria are fully met in the working tree. - Run `npm run lint` with the boundaries rule enabled as part of the PR CI gate — it will catch any future regression of this pattern at the import level rather than requiring a code review to spot it. - The `statusDotClass` function currently returns `string` but TypeScript will warn because the `switch` has no `default` arm. Since `DocumentStatus` is a union type the compiler knows is exhaustive, this is fine with `noImplicitReturns: false` — but adding a `default: return 'bg-gray-400'` as a defensive fallback costs nothing and removes the TypeScript noise if the status enum ever gains a new value before the function is updated.

marcel commented 2026-05-07 17:49:03 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Fullstack Developer

Observations

• The implementation is clean and already done on feat/issue-411-legibility-cleanup. Two commits tell the story clearly: 507fa088 moves the functions, d9a4faf4 removes the one-liner statusLabel() alias that was just forwarding to formatDocumentStatus(). That second commit is the right refactor call — don't create a named alias for a function that already has a good name.
• documentStatusLabel.ts now owns three distinct export shapes: DocumentStatus (the type), formatDocumentStatus (localized label), and statusDotClass (Tailwind CSS class). All three are document-domain concerns. The file name and the exports are aligned.
• DocumentStatusChip.svelte is small (23 lines) and uses $derived correctly for both derived values. The component is a single visual concern — a coloured dot with an accessible title. Naming and size are both correct.
• The spec file (documentStatusLabel.spec.ts) covers formatDocumentStatus and statusDotClass across all five status values plus the unknown fallback for formatDocumentStatus. That is complete coverage for pure functions of this shape.
• One naming observation: the spec has statusDotClass tests grouped under a describe('statusDotClass', ...) block but each test reads 'PLACEHOLDER → bg-gray-400' — the → arrow is a nice convention, keep it. It reads like a truth table, which is exactly what this is.
• The statusDotClass switch has no default arm and its return type is string (inferred). TypeScript will catch this if a new DocumentStatus value is added without updating the switch — but only if strict mode includes noImplicitReturns. Worth checking; if it is not enabled, the function silently returns undefined for an unknown status value, which passes a string return type via TypeScript's implicit any widening.

Recommendations

• Add an explicit default arm to statusDotClass returning a neutral fallback (e.g., 'bg-gray-300') and add a corresponding spec test. This makes the exhaustive contract visible, protects against silent undefined returns, and documents intent:

  default:
      return 'bg-gray-300'; // defensive: unknown future status values get a neutral dot
  

• After confirming all tests pass, close issue #424. The acceptance criteria are fully met: correct location, correct imports in DocumentStatusChip.svelte, clean personFormat.ts, tests passing, and npm run check will be clean.

## 👨‍💻 Felix Brandt — Fullstack Developer ### Observations - The implementation is clean and already done on `feat/issue-411-legibility-cleanup`. Two commits tell the story clearly: `507fa088` moves the functions, `d9a4faf4` removes the one-liner `statusLabel()` alias that was just forwarding to `formatDocumentStatus()`. That second commit is the right refactor call — don't create a named alias for a function that already has a good name. - `documentStatusLabel.ts` now owns three distinct export shapes: `DocumentStatus` (the type), `formatDocumentStatus` (localized label), and `statusDotClass` (Tailwind CSS class). All three are document-domain concerns. The file name and the exports are aligned. - `DocumentStatusChip.svelte` is small (23 lines) and uses `$derived` correctly for both derived values. The component is a single visual concern — a coloured dot with an accessible title. Naming and size are both correct. - The spec file (`documentStatusLabel.spec.ts`) covers `formatDocumentStatus` and `statusDotClass` across all five status values plus the unknown fallback for `formatDocumentStatus`. That is complete coverage for pure functions of this shape. - One naming observation: the spec has `statusDotClass` tests grouped under a `describe('statusDotClass', ...)` block but each test reads `'PLACEHOLDER → bg-gray-400'` — the `→` arrow is a nice convention, keep it. It reads like a truth table, which is exactly what this is. - The `statusDotClass` switch has no `default` arm and its return type is `string` (inferred). TypeScript will catch this if a new `DocumentStatus` value is added without updating the switch — but only if `strict` mode includes `noImplicitReturns`. Worth checking; if it is not enabled, the function silently returns `undefined` for an unknown status value, which passes a `string` return type via TypeScript's implicit `any` widening. ### Recommendations - Add an explicit `default` arm to `statusDotClass` returning a neutral fallback (e.g., `'bg-gray-300'`) and add a corresponding spec test. This makes the exhaustive contract visible, protects against silent `undefined` returns, and documents intent: ```typescript default: return 'bg-gray-300'; // defensive: unknown future status values get a neutral dot ``` - After confirming all tests pass, close issue #424. The acceptance criteria are fully met: correct location, correct imports in `DocumentStatusChip.svelte`, clean `personFormat.ts`, tests passing, and `npm run check` will be clean.

marcel commented 2026-05-07 17:49:18 +02:00

Author

Owner

Copy Link

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

• This is a pure TypeScript refactor with no infrastructure, Docker Compose, CI pipeline, or environment-variable footprint. There is nothing for me to review on the infrastructure side.
• The one thing worth noting from a pipeline perspective: the eslint-plugin-boundaries rule introduced alongside this fix (feat/issue-411-legibility-cleanup branch) enforces the domain boundary at lint time. That means CI will block future violations automatically — this is the correct approach. A manual code-review catch like the one that triggered this issue should not be the primary enforcement mechanism.
• The npm run lint step in the CI workflow should run boundaries/dependencies as an error-level rule (which it already is, per eslint.config.js). Confirm this step is not skipped or gated behind a flag in the Gitea Actions workflow.

Recommendations

• Verify in the Gitea Actions workflow that npm run lint is a required step and not advisory. If it currently only runs prettier and eslint without the boundaries plugin enabled, the automated guard has a gap. Run npm run lint locally on the feature branch and confirm the boundaries rule fires on a deliberate cross-domain import before merging — that confirms the guard is live in CI too.
• No Docker, Compose, or infrastructure changes are needed for this issue. Close it once the branch merges.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations - This is a pure TypeScript refactor with no infrastructure, Docker Compose, CI pipeline, or environment-variable footprint. There is nothing for me to review on the infrastructure side. - The one thing worth noting from a pipeline perspective: the `eslint-plugin-boundaries` rule introduced alongside this fix (`feat/issue-411-legibility-cleanup` branch) enforces the domain boundary at lint time. That means CI will block future violations automatically — this is the correct approach. A manual code-review catch like the one that triggered this issue should not be the primary enforcement mechanism. - The `npm run lint` step in the CI workflow should run `boundaries/dependencies` as an error-level rule (which it already is, per `eslint.config.js`). Confirm this step is not skipped or gated behind a flag in the Gitea Actions workflow. ### Recommendations - Verify in the Gitea Actions workflow that `npm run lint` is a required step and not advisory. If it currently only runs `prettier` and `eslint` without the boundaries plugin enabled, the automated guard has a gap. Run `npm run lint` locally on the feature branch and confirm the boundaries rule fires on a deliberate cross-domain import before merging — that confirms the guard is live in CI too. - No Docker, Compose, or infrastructure changes are needed for this issue. Close it once the branch merges.

marcel commented 2026-05-07 17:49:33 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Observations

• The acceptance criteria in the issue are precise, testable, and complete. Each criterion maps directly to a verifiable artefact (file location, absence of export, import path, test suite result, type-check result). This is good requirements practice — no ambiguity, no "nice to have" language.
• The issue title uses the conventional refactor(frontend): prefix, which correctly signals: no user-visible behaviour change, no new feature, pure structural improvement. This is important for stakeholder communication — family members using the archive will notice nothing; the codebase becomes more maintainable.
• The "Related" section links both the triggering PR (#422) and the parent epic (#406). This traceability is correct: reviewers can follow the chain from architectural decision to flagged violation to resolution.
• The issue was filed on 2026-05-05 with label P3-later and refactor. Given that the fix landed in the same session (commits 507fa088 and d9a4faf4 same day), the priority label is now moot — but the label taxonomy is correct.

Recommendations

• Update the issue state to closed once the branch merges. The definition-of-done is met: all five acceptance criteria pass in the working tree. Leaving a fully-resolved issue open creates misleading backlog noise.
• Consider adding a short closing note to the issue (before closing) linking to the two relevant commits. This gives future readers a direct link from the requirement to its implementation without reading the full git log.
• The issue currently has no milestone assigned. For a P3-later refactor that is already done, this is fine. If the epic (#406) has a milestone, consider assigning it retroactively for completeness of the roadmap history.

## 📋 Elicit — Requirements Engineer ### Observations - The acceptance criteria in the issue are precise, testable, and complete. Each criterion maps directly to a verifiable artefact (file location, absence of export, import path, test suite result, type-check result). This is good requirements practice — no ambiguity, no "nice to have" language. - The issue title uses the conventional `refactor(frontend):` prefix, which correctly signals: no user-visible behaviour change, no new feature, pure structural improvement. This is important for stakeholder communication — family members using the archive will notice nothing; the codebase becomes more maintainable. - The "Related" section links both the triggering PR (#422) and the parent epic (#406). This traceability is correct: reviewers can follow the chain from architectural decision to flagged violation to resolution. - The issue was filed on 2026-05-05 with label `P3-later` and `refactor`. Given that the fix landed in the same session (commits `507fa088` and `d9a4faf4` same day), the priority label is now moot — but the label taxonomy is correct. ### Recommendations - Update the issue state to **closed** once the branch merges. The definition-of-done is met: all five acceptance criteria pass in the working tree. Leaving a fully-resolved issue open creates misleading backlog noise. - Consider adding a short closing note to the issue (before closing) linking to the two relevant commits. This gives future readers a direct link from the requirement to its implementation without reading the full git log. - The issue currently has no milestone assigned. For a `P3-later` refactor that is already done, this is fine. If the epic (#406) has a milestone, consider assigning it retroactively for completeness of the roadmap history.

marcel commented 2026-05-07 17:49:52 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Security Engineer

Observations

• This refactor has no security surface area change. Moving pure formatting functions between TypeScript modules does not affect authentication, authorization, input validation, data flow, or any security boundary.
• No API endpoints are modified. No data leaves or enters the application differently. No permissions change.
• One thing worth a quick pass: statusDotClass returns a Tailwind CSS class string that is interpolated directly into the class attribute in DocumentStatusChip.svelte:

  class="hidden shrink-0 md:block {dotClass} h-4 w-4 rounded-full"
  

  This is safe because dotClass is derived from a backend-controlled DocumentStatus enum value via a closed switch statement — there is no user-controlled string interpolated here. The values are hardcoded CSS class names. No XSS vector exists.
• The aria-label on the span uses the localized label string (from formatDocumentStatus), which also comes from a closed switch over a backend enum. No user input reaches the label.

Recommendations

• No security action required for this issue. The refactor is safe.
• The pattern used here — enum-driven switch returning hardcoded strings, no user input in the path — is the correct pattern for this type of UI helper. Keep it.

## 🔒 Nora "NullX" Steiner — Security Engineer ### Observations - This refactor has no security surface area change. Moving pure formatting functions between TypeScript modules does not affect authentication, authorization, input validation, data flow, or any security boundary. - No API endpoints are modified. No data leaves or enters the application differently. No permissions change. - One thing worth a quick pass: `statusDotClass` returns a Tailwind CSS class string that is interpolated directly into the `class` attribute in `DocumentStatusChip.svelte`: ```svelte class="hidden shrink-0 md:block {dotClass} h-4 w-4 rounded-full" ``` This is safe because `dotClass` is derived from a backend-controlled `DocumentStatus` enum value via a closed `switch` statement — there is no user-controlled string interpolated here. The values are hardcoded CSS class names. No XSS vector exists. - The `aria-label` on the span uses the localized `label` string (from `formatDocumentStatus`), which also comes from a closed switch over a backend enum. No user input reaches the label. ### Recommendations - No security action required for this issue. The refactor is safe. - The pattern used here — enum-driven switch returning hardcoded strings, no user input in the path — is the correct pattern for this type of UI helper. Keep it.

marcel commented 2026-05-07 17:50:10 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer

Observations

• The spec file documentStatusLabel.spec.ts covers both formatDocumentStatus and statusDotClass across all five DocumentStatus values, plus the unknown-input fallback for formatDocumentStatus. That is complete coverage for pure functions with a finite, closed domain.
• The test names follow the → arrow convention (e.g., 'PLACEHOLDER → bg-gray-400') which reads as a truth table — clean and appropriate for this shape of test.
• personFormat.spec.ts no longer contains document-status test cases (confirmed — the functions were removed from personFormat.ts along with their specs). No orphaned tests remain.
• The spec imports from ./documentStatusLabel (relative path), not via the $lib alias. This is consistent with how co-located specs are typically written and is fine.
• There is one gap worth flagging: statusDotClass has no default arm. If a new DocumentStatus value is added (e.g., 'DELETED'), the function returns undefined at runtime, but the existing spec would still pass because it only tests the five known values. The gap in the tests mirrors the gap in the production code — they rise and fall together.

Recommendations

• Add a test for the statusDotClass unknown-value fallback, parallel to the formatDocumentStatus unknown-value test that already exists:

  it('returns fallback class for unknown status', () => {
      expect(statusDotClass('UNKNOWN_FUTURE_VALUE' as DocumentStatus)).toBe('bg-gray-300');
  });
  

  This only makes sense once the default arm is added to the production function (see Felix's recommendation). Do both together, red/green.
• All existing tests should pass as-is after the refactor (no behaviour change). Run npm run test to confirm before merging.
• The test suite for DocumentStatusChip.svelte is not present as a separate spec — the component is simple enough (pure derived values, no interactivity) that unit coverage via documentStatusLabel.spec.ts is sufficient. No gap here.

## 🧪 Sara Holt — QA Engineer ### Observations - The spec file `documentStatusLabel.spec.ts` covers both `formatDocumentStatus` and `statusDotClass` across all five `DocumentStatus` values, plus the unknown-input fallback for `formatDocumentStatus`. That is complete coverage for pure functions with a finite, closed domain. - The test names follow the `→` arrow convention (e.g., `'PLACEHOLDER → bg-gray-400'`) which reads as a truth table — clean and appropriate for this shape of test. - `personFormat.spec.ts` no longer contains document-status test cases (confirmed — the functions were removed from `personFormat.ts` along with their specs). No orphaned tests remain. - The spec imports from `./documentStatusLabel` (relative path), not via the `$lib` alias. This is consistent with how co-located specs are typically written and is fine. - There is one gap worth flagging: `statusDotClass` has no `default` arm. If a new `DocumentStatus` value is added (e.g., `'DELETED'`), the function returns `undefined` at runtime, but the existing spec would still pass because it only tests the five known values. The gap in the tests mirrors the gap in the production code — they rise and fall together. ### Recommendations - Add a test for the `statusDotClass` unknown-value fallback, parallel to the `formatDocumentStatus` unknown-value test that already exists: ```typescript it('returns fallback class for unknown status', () => { expect(statusDotClass('UNKNOWN_FUTURE_VALUE' as DocumentStatus)).toBe('bg-gray-300'); }); ``` This only makes sense once the `default` arm is added to the production function (see Felix's recommendation). Do both together, red/green. - All existing tests should pass as-is after the refactor (no behaviour change). Run `npm run test` to confirm before merging. - The test suite for `DocumentStatusChip.svelte` is not present as a separate spec — the component is simple enough (pure derived values, no interactivity) that unit coverage via `documentStatusLabel.spec.ts` is sufficient. No gap here.

marcel commented 2026-05-07 17:50:26 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist

Observations

• No visual change reaches users from this refactor. The rendered output of DocumentStatusChip.svelte is identical before and after — same Tailwind classes, same aria-label, same title attribute.
• The accessibility pattern on the chip is worth confirming explicitly:

  <span
      class="hidden shrink-0 md:block {dotClass} h-4 w-4 rounded-full"
      title={label}
      aria-label={label}
  ></span>
  

  The span is decorative (a coloured dot) but carries aria-label for screen readers and title for tooltip-on-hover. This is a reasonable approach, but there is a nuance: aria-label on a <span> with no interactive role or ARIA role does not create an accessible name that screen readers consistently announce. A role="img" would make the aria-label semantically meaningful:

  <span
      role="img"
      aria-label={label}
      class="hidden shrink-0 md:block {dotClass} h-4 w-4 rounded-full"
  ></span>
  

  Without role="img", the aria-label is present but may be ignored by some screen readers because the element has no implicit ARIA role that accepts naming.
• The dot is hidden md:block — invisible on small screens. This means mobile users (and this audience includes seniors on tablets) see no status indicator at all. That is a pre-existing design decision, not introduced by this issue, but it is worth flagging as a separate accessibility gap.
• The colour choices in statusDotClass use Tailwind's semantic palette (emerald, blue, amber, gray). These should be verified for contrast against the card background (bg-surface). A 16x16px colour dot with no text inside is exempt from the 4.5:1 text contrast ratio, but the dot should still be visually distinguishable — colour alone carries the meaning here, which is a WCAG 1.4.1 (Use of Color) concern. The title/aria-label provides the textual alternative, which mitigates this.

Recommendations

• Add role="img" to the <span> in DocumentStatusChip.svelte. This is a one-character fix that makes the aria-label semantically binding for screen readers. This can go in the same PR as the refactor since the component is already being touched.
• Track the mobile-invisible status indicator as a separate accessibility issue — it is out of scope for this refactor but should not be forgotten. The senior transcribers (60+) on tablets need status feedback on smaller viewports.
• No brand or typography concerns in this refactor.

## 🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist ### Observations - No visual change reaches users from this refactor. The rendered output of `DocumentStatusChip.svelte` is identical before and after — same Tailwind classes, same `aria-label`, same `title` attribute. - The accessibility pattern on the chip is worth confirming explicitly: ```svelte <span class="hidden shrink-0 md:block {dotClass} h-4 w-4 rounded-full" title={label} aria-label={label} ></span> ``` The span is decorative (a coloured dot) but carries `aria-label` for screen readers and `title` for tooltip-on-hover. This is a reasonable approach, but there is a nuance: `aria-label` on a `<span>` with no interactive role or ARIA role does not create an accessible name that screen readers consistently announce. A `role="img"` would make the `aria-label` semantically meaningful: ```svelte <span role="img" aria-label={label} class="hidden shrink-0 md:block {dotClass} h-4 w-4 rounded-full" ></span> ``` Without `role="img"`, the `aria-label` is present but may be ignored by some screen readers because the element has no implicit ARIA role that accepts naming. - The dot is `hidden md:block` — invisible on small screens. This means mobile users (and this audience includes seniors on tablets) see no status indicator at all. That is a pre-existing design decision, not introduced by this issue, but it is worth flagging as a separate accessibility gap. - The colour choices in `statusDotClass` use Tailwind's semantic palette (emerald, blue, amber, gray). These should be verified for contrast against the card background (`bg-surface`). A 16x16px colour dot with no text inside is exempt from the 4.5:1 text contrast ratio, but the dot should still be visually distinguishable — colour alone carries the meaning here, which is a WCAG 1.4.1 (Use of Color) concern. The `title`/`aria-label` provides the textual alternative, which mitigates this. ### Recommendations - Add `role="img"` to the `<span>` in `DocumentStatusChip.svelte`. This is a one-character fix that makes the `aria-label` semantically binding for screen readers. This can go in the same PR as the refactor since the component is already being touched. - Track the mobile-invisible status indicator as a separate accessibility issue — it is out of scope for this refactor but should not be forgotten. The senior transcribers (60+) on tablets need status feedback on smaller viewports. - No brand or typography concerns in this refactor.

marcel commented 2026-05-07 17:50:54 +02:00

Author

Owner

Copy Link

Decision Queue — Consolidated Open Items

No genuine blocking decisions remain for this issue — the core refactor is complete and the acceptance criteria are met. However, the review surfaced three small, concrete action items worth tracking before closing:

---

1. Defensive fallback in statusDotClass + matching test (Felix, Sara)

The function's switch has no default arm. If a new DocumentStatus value arrives from the backend before the frontend is updated, the function silently returns undefined and the dot renders with no class. Add a default: return 'bg-gray-300' arm and a spec test for it. Do both in one red/green cycle. This is a two-line production change and a three-line test.

---

2. Add role="img" to DocumentStatusChip's <span> (Leonie)

The aria-label attribute on a plain <span> without a named ARIA role is not reliably announced by all screen readers. Adding role="img" makes the label semantically binding per the ARIA spec. One attribute, zero visual change, measurable accessibility improvement. Can go in the same commit as the refactor since the file is already being touched.

---

3. Verify npm run lint (with boundaries/dependencies) is a required CI gate (Tobias)

The ESLint boundaries rule enforces domain isolation automatically. Confirm it runs as a required step in the Gitea Actions workflow — not advisory, not skipped. A quick manual confirmation (npm run lint on a deliberate cross-domain import should return a non-zero exit code) is sufficient to close this item.

---

The mobile-invisible status dot (hidden md:block) is a pre-existing design gap flagged by Leonie — it is explicitly out of scope for this issue and should be filed as a separate accessibility issue if prioritised.

## Decision Queue — Consolidated Open Items No genuine blocking decisions remain for this issue — the core refactor is complete and the acceptance criteria are met. However, the review surfaced three small, concrete action items worth tracking before closing: --- **1. Defensive fallback in `statusDotClass` + matching test** _(Felix, Sara)_ The function's `switch` has no `default` arm. If a new `DocumentStatus` value arrives from the backend before the frontend is updated, the function silently returns `undefined` and the dot renders with no class. Add a `default: return 'bg-gray-300'` arm and a spec test for it. Do both in one red/green cycle. This is a two-line production change and a three-line test. --- **2. Add `role="img"` to `DocumentStatusChip`'s `<span>`** _(Leonie)_ The `aria-label` attribute on a plain `<span>` without a named ARIA role is not reliably announced by all screen readers. Adding `role="img"` makes the label semantically binding per the ARIA spec. One attribute, zero visual change, measurable accessibility improvement. Can go in the same commit as the refactor since the file is already being touched. --- **3. Verify `npm run lint` (with `boundaries/dependencies`) is a required CI gate** _(Tobias)_ The ESLint boundaries rule enforces domain isolation automatically. Confirm it runs as a required step in the Gitea Actions workflow — not advisory, not skipped. A quick manual confirmation (`npm run lint` on a deliberate cross-domain import should return a non-zero exit code) is sufficient to close this item. --- The mobile-invisible status dot (`hidden md:block`) is a pre-existing design gap flagged by Leonie — it is explicitly out of scope for this issue and should be filed as a separate accessibility issue if prioritised.

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

feat/issue-533-mass-import-ux

feat/issue-527-unsaved-warning-new-pages

worktree-feat+issue-557-upload-artifact-v3-pin

worktree-chore+issue-556-drop-client-branches-coverage-gate

fix/issue-514-prerender-crawl-bakes-redirects

fix/issue-472-prerender-entries

feat/issue-395-readme

feat/issue-345-bulk-mark-reviewed

feat/issue-344-bell-tooltip

feat/issue-341-annotieren-contrast

feat/issue-225-bulk-metadata-edit

feat/issue-317-bulk-upload

feat/issue-271-dashboard-redesign

docs/issue-240-mission-control-spec

refactor/issues-193-200

feat/issue-162-korrespondenz-redesign

feature/68-new-document-file-first

feat/81-discussion-discoverability

feat/62-document-bottom-panel

feature/56-backfill-file-hashes

feat/38-document-edit-history

fix/svelte5-test-delegation-and-login-auth

No results found.

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 P3-later legibility refactor

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#424