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

docs(legibility): write CONTRIBUTING.md with three concrete walkthroughs #398

New Issue
Closed
opened 2026-05-04 16:08:41 +02:00 by marcel · 10 comments
marcel commented 2026-05-04 16:08:41 +02:00
Owner
Copy Link

Context

Part of Epic #394 — Documentation. This is DOC-4: the operational manual for adding things to the codebase. Successor-X (hypothetical future maintainer) opens this when they get their first ticket.

Per the Legibility Rubric, this addresses C5.1, C5.3, C5.4 (all Major).

Required content

A single CONTRIBUTING.md at repo root. Not in docs/ — convention is to keep CONTRIBUTING at the root for tool discoverability.

1. Environment setup

  • Prerequisite versions (Java 21 via SDKMAN, Node 24 via nvm, Docker)
  • Memory note: source SDKMAN and nvm before running java/mvn/node/npm

2. Daily development workflow

  • How to run dev locally (frontend + backend + docker-compose for db/minio)
  • How to regenerate OpenAPI types (npm run generate:api with backend in dev profile)
  • How to run tests (backend, frontend, e2e)
  • Branch naming convention: <type>/<issue-number>-<short-description>
  • Commit message convention (reference issues with Closes #N, atomic commits)
  • Pre-commit hooks (Husky)

3. Walkthrough A — Add a new domain

Concrete step-by-step:

  1. Create the backend package: backend/src/main/java/.../<domain>/
  2. Add entity, repository, service, controller, DTOs in the package
  3. Add the corresponding frontend folder: frontend/src/lib/<domain>/
  4. Add a per-domain README.md on both stacks (per DOC-6)
  5. Update docs/ARCHITECTURE.md to mention the new domain
  6. Update docs/GLOSSARY.md if any new terms are introduced
  7. Add Flyway migration if entities are persistent
  8. Add @RequirePermission on every state-mutating controller method

4. Walkthrough B — Add a new endpoint to an existing domain

Concrete step-by-step using a real example (e.g., "add POST /api/persons/{id}/aliases"):

  1. Add the controller method in <domain>/.../<Domain>Controller.java
  2. Add the service method in <domain>/.../<Domain>Service.java
  3. Add @RequirePermission if mutating
  4. Mark required fields with @Schema(requiredMode = REQUIRED)
  5. Use DomainException.notFound(ErrorCode.X, ...) for errors; add ErrorCode value if new
  6. Run backend with --spring.profiles.active=dev, then npm run generate:api in frontend/
  7. Add a backend test (controller test for HTTP layer; service test for logic)

5. Walkthrough C — Add a new frontend page

Concrete step-by-step using a real example (e.g., "add /persons/[id]/timeline"):

  1. Create frontend/src/routes/persons/[id]/timeline/+page.svelte
  2. Add +page.server.ts for SSR data load (use the typed API client from $lib/api.server.ts)
  3. For domain-specific components, place them under the corresponding lib/<domain>/ folder
  4. For shared primitives, place them under lib/shared/
  5. Use existing patterns: <BackButton>, German dd.mm.yyyy date input, brand color utilities (brand-navy, brand-mint, brand-sand)
  6. Add Paraglide translation keys in messages/{de,en,es}.json
  7. If introducing a new error code, mirror the backend's ErrorCode enum in frontend/src/lib/errors.ts
  8. Add a Svelte unit test or e2e test

6. Conventions reference

Brief tables (links out to longer docs where appropriate):

  • Error handling pattern (DomainException vs ResponseStatusException; ErrorCode mirroring)
  • DTO pattern (input DTOs in dto/; response = entity)
  • Frontend API client pattern (!result.response.ok, result.error as { code }, result.data!)
  • Date handling (German display, ISO over wire, T12:00:00 UTC suffix)
  • Brand colors and typography
  • Per-stack lint/format commands

Anti-patterns

  • Do NOT migrate the entire CLAUDE.md files into here — only the rules a human contributor needs. Keep CONTRIBUTING tight.
  • Do NOT include the architecture overview here — link to docs/ARCHITECTURE.md instead.

Acceptance criteria

  • CONTRIBUTING.md exists at repo root with the 6 sections above
  • Walkthroughs A, B, C are concrete (file paths, exact commands), not abstract
  • Linked from README.md (DOC-1)
  • Conventions section migrates the rules from backend/CLAUDE.md and frontend/CLAUDE.md (per DOC-7)
  • PR opened and merged

Dependency

Soft dependency on Epic 4 (Refactor) being complete OR planned — walkthroughs A/B reference the target domain-based structure, not the current layered one.

Definition of Done

CONTRIBUTING.md committed on main. Closing comment summarizes the three walkthrough examples.

## Context Part of **Epic #394** — Documentation. This is **DOC-4**: the operational manual for adding things to the codebase. Successor-X (hypothetical future maintainer) opens this when they get their first ticket. Per the Legibility Rubric, this addresses **C5.1, C5.3, C5.4** (all Major). ## Required content A single `CONTRIBUTING.md` at repo root. **Not in `docs/`** — convention is to keep CONTRIBUTING at the root for tool discoverability. ### 1. Environment setup - Prerequisite versions (Java 21 via SDKMAN, Node 24 via nvm, Docker) - Memory note: source SDKMAN and nvm before running java/mvn/node/npm ### 2. Daily development workflow - How to run dev locally (frontend + backend + docker-compose for db/minio) - How to regenerate OpenAPI types (`npm run generate:api` with backend in dev profile) - How to run tests (backend, frontend, e2e) - Branch naming convention: `<type>/<issue-number>-<short-description>` - Commit message convention (reference issues with `Closes #N`, atomic commits) - Pre-commit hooks (Husky) ### 3. Walkthrough A — Add a new domain Concrete step-by-step: 1. Create the backend package: `backend/src/main/java/.../<domain>/` 2. Add entity, repository, service, controller, DTOs in the package 3. Add the corresponding frontend folder: `frontend/src/lib/<domain>/` 4. Add a per-domain `README.md` on both stacks (per DOC-6) 5. Update `docs/ARCHITECTURE.md` to mention the new domain 6. Update `docs/GLOSSARY.md` if any new terms are introduced 7. Add Flyway migration if entities are persistent 8. Add `@RequirePermission` on every state-mutating controller method ### 4. Walkthrough B — Add a new endpoint to an existing domain Concrete step-by-step using a real example (e.g., "add `POST /api/persons/{id}/aliases`"): 1. Add the controller method in `<domain>/.../<Domain>Controller.java` 2. Add the service method in `<domain>/.../<Domain>Service.java` 3. Add `@RequirePermission` if mutating 4. Mark required fields with `@Schema(requiredMode = REQUIRED)` 5. Use `DomainException.notFound(ErrorCode.X, ...)` for errors; add `ErrorCode` value if new 6. Run backend with `--spring.profiles.active=dev`, then `npm run generate:api` in `frontend/` 7. Add a backend test (controller test for HTTP layer; service test for logic) ### 5. Walkthrough C — Add a new frontend page Concrete step-by-step using a real example (e.g., "add `/persons/[id]/timeline`"): 1. Create `frontend/src/routes/persons/[id]/timeline/+page.svelte` 2. Add `+page.server.ts` for SSR data load (use the typed API client from `$lib/api.server.ts`) 3. For domain-specific components, place them under the corresponding `lib/<domain>/` folder 4. For shared primitives, place them under `lib/shared/` 5. Use existing patterns: `<BackButton>`, German `dd.mm.yyyy` date input, brand color utilities (`brand-navy`, `brand-mint`, `brand-sand`) 6. Add Paraglide translation keys in `messages/{de,en,es}.json` 7. If introducing a new error code, mirror the backend's `ErrorCode` enum in `frontend/src/lib/errors.ts` 8. Add a Svelte unit test or e2e test ### 6. Conventions reference Brief tables (links out to longer docs where appropriate): - Error handling pattern (DomainException vs ResponseStatusException; ErrorCode mirroring) - DTO pattern (input DTOs in `dto/`; response = entity) - Frontend API client pattern (`!result.response.ok`, `result.error as { code }`, `result.data!`) - Date handling (German display, ISO over wire, T12:00:00 UTC suffix) - Brand colors and typography - Per-stack lint/format commands ## Anti-patterns - Do NOT migrate the *entire* `CLAUDE.md` files into here — only the rules a human contributor needs. Keep CONTRIBUTING tight. - Do NOT include the architecture overview here — link to `docs/ARCHITECTURE.md` instead. ## Acceptance criteria - [ ] `CONTRIBUTING.md` exists at repo root with the 6 sections above - [ ] Walkthroughs A, B, C are concrete (file paths, exact commands), not abstract - [ ] Linked from `README.md` (DOC-1) - [ ] Conventions section migrates the rules from `backend/CLAUDE.md` and `frontend/CLAUDE.md` (per DOC-7) - [ ] PR opened and merged ## Dependency Soft dependency on Epic 4 (Refactor) being complete OR planned — walkthroughs A/B reference the *target* domain-based structure, not the current layered one. ## Definition of Done `CONTRIBUTING.md` committed on `main`. Closing comment summarizes the three walkthrough examples.
marcel added this to the Codebase Legibility milestone 2026-05-04 16:08:41 +02:00
marcel added the P1-highdocumentationlegibility labels 2026-05-04 16:09:51 +02:00
marcel commented 2026-05-04 21:03:53 +02:00
Author
Owner
Copy Link

🏗️ Markus Keller — Senior Application Architect

Observations

  • The dependency note at the bottom of the issue is the most important sentence in the whole spec: Walkthroughs A and B reference the target domain-based package structure (org.raddatz.familienarchiv.document.DocumentController etc.), but the actual codebase today is still layered (controller/, service/, repository/). I checked — backend/src/main/java/org/raddatz/familienarchiv/ has controller/, service/, repository/ as its top-level packages.
  • If Walkthrough A is written against the target structure, it will be wrong the moment a new contributor opens the project and sees the actual layout. This is worse than no walkthrough — it actively misleads.
  • The issue references docs/ARCHITECTURE.md and docs/GLOSSARY.md as link targets in Walkthrough A. Neither file exists. What does exist: docs/architecture/c4-diagrams.md (C4 diagrams) and 6 ADRs in docs/adr/. The walkthrough needs to reference what's actually there.
  • Walkthrough A step 5 says "Update docs/ARCHITECTURE.md" and step 6 says "Update docs/GLOSSARY.md". Both are dead links on day one.

Recommendations

  • Write the walkthroughs against the current structure, not the planned future structure. Document what a contributor will actually encounter. When the refactor lands (Epic 4), update CONTRIBUTING.md in that same PR — that's the right lifecycle.
  • Add a clearly labelled note at the top of Walkthrough A/B: "The backend is currently organized by layer (not by feature). A refactor to feature-package structure is planned in Epic #X. This walkthrough reflects the current layered structure." This is honest and doesn't confuse newcomers.
  • Replace the dead links in step 5/6 with the real targets: docs/architecture/c4-diagrams.md for architecture, and either create docs/GLOSSARY.md as a stub or link to the domain model table already in CLAUDE.md.
  • The existing COLLABORATING.md and CODESTYLE.md at repo root are solid. CONTRIBUTING.md should link to them for the conventions reference section (Section 6) rather than duplicating their content.
  • No open architectural decisions from my side — the constraint is clear: write for current reality, annotate the planned future state.

Open Decisions

  • Which state to document: current layered OR target feature-package structure? The issue says "reference the target structure" but the soft dependency on Epic 4 being complete means this is a time-bomb. If Epic 4 slips, CONTRIBUTING.md is wrong on day one. Cost of writing against current: a minor update when Epic 4 lands. Cost of writing against target: actively misleading contributors until Epic 4 ships.
## 🏗️ Markus Keller — Senior Application Architect ### Observations - The dependency note at the bottom of the issue is the most important sentence in the whole spec: **Walkthroughs A and B reference the target domain-based package structure** (`org.raddatz.familienarchiv.document.DocumentController` etc.), but the actual codebase today is still layered (`controller/`, `service/`, `repository/`). I checked — `backend/src/main/java/org/raddatz/familienarchiv/` has `controller/`, `service/`, `repository/` as its top-level packages. - If Walkthrough A is written against the *target* structure, it will be wrong the moment a new contributor opens the project and sees the actual layout. This is worse than no walkthrough — it actively misleads. - The issue references `docs/ARCHITECTURE.md` and `docs/GLOSSARY.md` as link targets in Walkthrough A. Neither file exists. What does exist: `docs/architecture/c4-diagrams.md` (C4 diagrams) and 6 ADRs in `docs/adr/`. The walkthrough needs to reference what's actually there. - Walkthrough A step 5 says "Update `docs/ARCHITECTURE.md`" and step 6 says "Update `docs/GLOSSARY.md`". Both are dead links on day one. ### Recommendations - **Write the walkthroughs against the current structure, not the planned future structure.** Document what a contributor will actually encounter. When the refactor lands (Epic 4), update CONTRIBUTING.md in that same PR — that's the right lifecycle. - **Add a clearly labelled note** at the top of Walkthrough A/B: "The backend is currently organized by layer (not by feature). A refactor to feature-package structure is planned in Epic #X. This walkthrough reflects the current layered structure." This is honest and doesn't confuse newcomers. - **Replace the dead links** in step 5/6 with the real targets: `docs/architecture/c4-diagrams.md` for architecture, and either create `docs/GLOSSARY.md` as a stub or link to the domain model table already in `CLAUDE.md`. - The existing `COLLABORATING.md` and `CODESTYLE.md` at repo root are solid. CONTRIBUTING.md should link to them for the conventions reference section (Section 6) rather than duplicating their content. - No open architectural decisions from my side — the constraint is clear: write for current reality, annotate the planned future state. ### Open Decisions - **Which state to document: current layered OR target feature-package structure?** The issue says "reference the *target* structure" but the soft dependency on Epic 4 being complete means this is a time-bomb. If Epic 4 slips, CONTRIBUTING.md is wrong on day one. Cost of writing against current: a minor update when Epic 4 lands. Cost of writing against target: actively misleading contributors until Epic 4 ships.
marcel commented 2026-05-04 21:04:11 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

  • The three walkthroughs are well scoped — A (new domain), B (new endpoint), C (new page) cover the real addition patterns. Good choice of a concrete example for each (POST /api/persons/{id}/aliases, /persons/[id]/timeline).
  • The frontend lib structure in Walkthrough C says "place domain-specific components under lib/<domain>/" — but the actual structure is lib/components/ as a flat-ish collection with some subdirectories (document/, chronik/, user/). The issue's guidance would send a contributor to lib/geschichten/ when the actual pattern appears to be lib/components/. Verified this by inspecting frontend/src/lib/ — no per-domain lib/ subfolders exist beyond components/.
  • Step 7 in Walkthrough C mentions "If introducing a new error code, mirror the backend's ErrorCode enum in frontend/src/lib/errors.ts" — that's correct and exactly right. Good to call this out explicitly.
  • Step 4 in Walkthrough C references lib/shared/ for shared primitives — this directory doesn't exist in the current codebase. The actual shared components live directly in lib/components/ (e.g., BackButton.svelte, DateInput.svelte, Pagination.svelte).
  • The pre-commit hook is Husky and runs cd frontend && npm run lint. The issue mentions Husky correctly — that's good.
  • The OpenAPI regen flow (Section 2) is accurate: backend with --spring.profiles.active=dev, then npm run generate:api. This is the real workflow.

Recommendations

  • Correct the lib/<domain>/ claim in Walkthrough C to match reality: domain-specific components go in lib/components/ or a subdirectory of it (e.g., lib/components/document/). Drop the reference to lib/shared/.
  • Walkthrough B step 6 (regen types) is correct and critical — make it bold or put it in a warning callout. Forgetting this step is the most common "where did my type go?" confusion for new contributors.
  • Add TDD to each walkthrough as an explicit step. Walkthrough B says "add a backend test" as step 7 — but the project's TDD rule (from COLLABORATING.md) is that tests come first. The walkthrough shouldn't teach the wrong order even implicitly. Reorder: write the failing test first, then add the implementation.
  • The "per-domain README.md on both stacks (per DOC-6)" reference in Walkthrough A step 4 should clarify that this is a planned DOC-6 pattern — it doesn't currently exist, and a newcomer looking for examples will find none.

Open Decisions (none — all of the above are concrete fixes)

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The three walkthroughs are well scoped — A (new domain), B (new endpoint), C (new page) cover the real addition patterns. Good choice of a concrete example for each (`POST /api/persons/{id}/aliases`, `/persons/[id]/timeline`). - The frontend lib structure in Walkthrough C says "place domain-specific components under `lib/<domain>/`" — but the actual structure is `lib/components/` as a flat-ish collection with some subdirectories (`document/`, `chronik/`, `user/`). The issue's guidance would send a contributor to `lib/geschichten/` when the actual pattern appears to be `lib/components/`. Verified this by inspecting `frontend/src/lib/` — no per-domain `lib/` subfolders exist beyond `components/`. - Step 7 in Walkthrough C mentions "If introducing a new error code, mirror the backend's `ErrorCode` enum in `frontend/src/lib/errors.ts`" — that's correct and exactly right. Good to call this out explicitly. - Step 4 in Walkthrough C references `lib/shared/` for shared primitives — this directory doesn't exist in the current codebase. The actual shared components live directly in `lib/components/` (e.g., `BackButton.svelte`, `DateInput.svelte`, `Pagination.svelte`). - The pre-commit hook is Husky and runs `cd frontend && npm run lint`. The issue mentions Husky correctly — that's good. - The OpenAPI regen flow (Section 2) is accurate: backend with `--spring.profiles.active=dev`, then `npm run generate:api`. This is the real workflow. ### Recommendations - **Correct the `lib/<domain>/` claim in Walkthrough C** to match reality: domain-specific components go in `lib/components/` or a subdirectory of it (e.g., `lib/components/document/`). Drop the reference to `lib/shared/`. - **Walkthrough B step 6** (regen types) is correct and critical — make it bold or put it in a warning callout. Forgetting this step is the most common "where did my type go?" confusion for new contributors. - **Add TDD to each walkthrough as an explicit step.** Walkthrough B says "add a backend test" as step 7 — but the project's TDD rule (from `COLLABORATING.md`) is that tests come *first*. The walkthrough shouldn't teach the wrong order even implicitly. Reorder: write the failing test first, then add the implementation. - The "per-domain `README.md` on both stacks (per DOC-6)" reference in Walkthrough A step 4 should clarify that this is a planned DOC-6 pattern — it doesn't currently exist, and a newcomer looking for examples will find none. ### Open Decisions _(none — all of the above are concrete fixes)_
marcel commented 2026-05-04 21:04:34 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

  • This is documentation, not application code, so there's no attack surface to audit in the traditional sense. My concern is that CONTRIBUTING.md becomes an implicit security specification — what it teaches will determine how every new endpoint and domain is built.
  • Walkthrough B step 3 says "Add @RequirePermission if mutating" — the word if is a security smell. In this codebase, the rule is: every state-mutating endpoint must have @RequirePermission. "If" suggests it's optional, which it isn't. A contributor who reads "if mutating" may skip it and accidentally publish an unauthenticated write endpoint.
  • Step 4 of Walkthrough B (@Schema(requiredMode = REQUIRED)) is type-codegen guidance, not security — but it's in the right place contextually.
  • The conventions reference table (Section 6) lists "Error handling pattern (DomainException vs ResponseStatusException; ErrorCode mirroring)" — this is exactly right to document because ResponseStatusException for auth errors is a known source of broken error code propagation to the frontend.
  • Section 6 doesn't include input validation guidance. The CLAUDE.md has it (validate and sanitize at the controller boundary, trust internal service code) but it's absent from the walkthrough's conventions table. A new contributor adding an endpoint has no signal to validate at the boundary.
  • The issue says walkthroughs should be concrete. The security checklist for a new endpoint (from CLAUDE.md and SecurityConfig) includes: @RequirePermission, input validation, parameterized queries, no logging of raw user input. None of these appear explicitly as a "security checklist" step in Walkthrough B.

Recommendations

  • Change step 3 in Walkthrough B from "Add @RequirePermission if mutating" to "Add @RequirePermission(Permission.WRITE_ALL) on every POST, PUT, PATCH, and DELETE endpoint — this is not optional." Then clarify read-only endpoints stay unannotated.
  • Add an input validation step to Walkthrough B — after the service method, add: "Validate user-supplied inputs at the controller boundary using ResponseStatusException(BAD_REQUEST, ...) or DomainException.badRequest(...). Never trust data that hasn't been validated at the entry point."
  • Add "Security checklist" as a named subsection in Section 6's conventions table: @RequirePermission required on writes; parameterized queries only (@Param); no raw user input in logs; content-type validation on uploads.
  • These aren't just security concerns — they're the patterns that are already enforced in this codebase. The CONTRIBUTING.md should make them non-negotiable for new contributors, not optional additions.

Open Decisions (none — these are corrections to existing project rules, not new choices)

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - This is documentation, not application code, so there's no attack surface to audit in the traditional sense. My concern is that CONTRIBUTING.md becomes an *implicit security specification* — what it teaches will determine how every new endpoint and domain is built. - Walkthrough B step 3 says "Add `@RequirePermission` if mutating" — the word **if** is a security smell. In this codebase, the rule is: every state-mutating endpoint **must** have `@RequirePermission`. "If" suggests it's optional, which it isn't. A contributor who reads "if mutating" may skip it and accidentally publish an unauthenticated write endpoint. - Step 4 of Walkthrough B (`@Schema(requiredMode = REQUIRED)`) is type-codegen guidance, not security — but it's in the right place contextually. - The conventions reference table (Section 6) lists "Error handling pattern (DomainException vs ResponseStatusException; ErrorCode mirroring)" — this is exactly right to document because `ResponseStatusException` for auth errors is a known source of broken error code propagation to the frontend. - Section 6 doesn't include input validation guidance. The `CLAUDE.md` has it (validate and sanitize at the controller boundary, trust internal service code) but it's absent from the walkthrough's conventions table. A new contributor adding an endpoint has no signal to validate at the boundary. - The issue says walkthroughs should be concrete. The security checklist for a new endpoint (from `CLAUDE.md` and `SecurityConfig`) includes: `@RequirePermission`, input validation, parameterized queries, no logging of raw user input. None of these appear explicitly as a "security checklist" step in Walkthrough B. ### Recommendations - **Change step 3 in Walkthrough B** from "Add `@RequirePermission` if mutating" to "Add `@RequirePermission(Permission.WRITE_ALL)` on every `POST`, `PUT`, `PATCH`, and `DELETE` endpoint — this is **not optional**." Then clarify read-only endpoints stay unannotated. - **Add an input validation step to Walkthrough B** — after the service method, add: "Validate user-supplied inputs at the controller boundary using `ResponseStatusException(BAD_REQUEST, ...)` or `DomainException.badRequest(...)`. Never trust data that hasn't been validated at the entry point." - **Add "Security checklist" as a named subsection** in Section 6's conventions table: `@RequirePermission` required on writes; parameterized queries only (`@Param`); no raw user input in logs; content-type validation on uploads. - These aren't just security concerns — they're the patterns that are already enforced in this codebase. The CONTRIBUTING.md should make them non-negotiable for new contributors, not optional additions. ### Open Decisions _(none — these are corrections to existing project rules, not new choices)_
marcel commented 2026-05-04 21:04:53 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

  • Both walkthrough B (add endpoint) and walkthrough C (add frontend page) include testing as a final step — "add a backend test" and "add a Svelte unit test or e2e test". The project's COLLABORATING.md mandates Red/Green TDD: test first. The walkthroughs as written teach implementation-first, test-last — which is the opposite of the project rule.
  • Walkthrough B step 7 says "add a backend test (controller test for HTTP layer; service test for logic)" without distinguishing which layer to test at which point. The COLLABORATING.md has a clear test-type matrix: business logic → unit, HTTP contract → controller slice, full behavior → E2E. CONTRIBUTING.md should either reproduce that table or link to it.
  • Walkthrough C step 8 says "Add a Svelte unit test or e2e test" — the "or" makes this feel optional and doesn't help a contributor know which to choose. The guidance should be deterministic: a server load function gets a Vitest unit test; a user-facing navigation flow gets an E2E Playwright spec; a shared UI component gets a Vitest browser-mode component test.
  • The issue's acceptance criteria don't include anything about test coverage for the CONTRIBUTING.md itself. Since this is documentation, that's fine — but the walkthroughs should model the TDD discipline the project requires.
  • The issue references docs/ARCHITECTURE.md in walkthroughs — that file doesn't exist, which means step 5 of Walkthrough A is a broken link from day one. This is a correctness defect in the spec.

Recommendations

  • Reorder all walkthrough steps to TDD order: "Write a failing test" → "add the implementation" → "make the test pass". This applies to both B and C. Walkthrough A (new domain) may reasonably put the test step later since there's no behavior to test yet, but the step should still be present and explicit.
  • Replace "add a Svelte unit test or e2e test" with a decision rule: server load functions → Vitest unit test (import directly, mock fetch); UI interaction flows → Playwright E2E; shared components → Vitest browser-mode with render() + getByRole().
  • Add the test-type matrix from COLLABORATING.md to the Section 2 walkthrough preamble — a single table that maps "type of change" to "test type" so contributors can navigate the decision without hunting through multiple files.
  • Fix the acceptance criteria to include: "Walkthroughs present tests before implementation" — otherwise this will ship with the test-last ordering and a code reviewer has to catch it.

Open Decisions (none — test ordering is settled by the project's own COLLABORATING.md)

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - Both walkthrough B (add endpoint) and walkthrough C (add frontend page) include testing as a final step — "add a backend test" and "add a Svelte unit test or e2e test". The project's `COLLABORATING.md` mandates Red/Green TDD: test first. The walkthroughs as written teach implementation-first, test-last — which is the opposite of the project rule. - Walkthrough B step 7 says "add a backend test (controller test for HTTP layer; service test for logic)" without distinguishing *which layer* to test at which point. The `COLLABORATING.md` has a clear test-type matrix: business logic → unit, HTTP contract → controller slice, full behavior → E2E. CONTRIBUTING.md should either reproduce that table or link to it. - Walkthrough C step 8 says "Add a Svelte unit test or e2e test" — the "or" makes this feel optional and doesn't help a contributor know which to choose. The guidance should be deterministic: a server `load` function gets a Vitest unit test; a user-facing navigation flow gets an E2E Playwright spec; a shared UI component gets a Vitest browser-mode component test. - The issue's acceptance criteria don't include anything about test coverage for the CONTRIBUTING.md itself. Since this is documentation, that's fine — but the walkthroughs should model the TDD discipline the project requires. - The issue references `docs/ARCHITECTURE.md` in walkthroughs — that file doesn't exist, which means step 5 of Walkthrough A is a broken link from day one. This is a correctness defect in the spec. ### Recommendations - **Reorder all walkthrough steps to TDD order:** "Write a failing test" → "add the implementation" → "make the test pass". This applies to both B and C. Walkthrough A (new domain) may reasonably put the test step later since there's no behavior to test yet, but the step should still be present and explicit. - **Replace "add a Svelte unit test or e2e test"** with a decision rule: server `load` functions → Vitest unit test (import directly, mock `fetch`); UI interaction flows → Playwright E2E; shared components → Vitest browser-mode with `render()` + `getByRole()`. - **Add the test-type matrix from `COLLABORATING.md`** to the Section 2 walkthrough preamble — a single table that maps "type of change" to "test type" so contributors can navigate the decision without hunting through multiple files. - **Fix the acceptance criteria** to include: "Walkthroughs present tests before implementation" — otherwise this will ship with the test-last ordering and a code reviewer has to catch it. ### Open Decisions _(none — test ordering is settled by the project's own `COLLABORATING.md`)_
marcel commented 2026-05-04 21:05:21 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

  • This is a developer documentation issue, so there's no UI to review for accessibility or brand compliance. My angle here is the reader experience of the document itself, and one specific gap in Walkthrough C.
  • Walkthrough C (add a frontend page) has a step for Paraglide translation keys, a step for the API client pattern, and a step for brand colors — but no mention of the accessibility baseline expected for new pages. The project serves an audience that includes 60+ year-old users. New contributors won't know this unless it's written down.
  • The CLAUDE.md mentions min-h-[44px] touch targets and focus-visible:ring-2 focus rings as established patterns — but neither appears in the CONTRIBUTING.md walkthrough. A contributor creating a new page has no signal to implement these.
  • Walkthrough C step 5 correctly references <BackButton>, German date input, and brand color utilities. These are good concrete anchors. The gap is that the accessibility equivalents of these patterns are missing (touch targets, focus rings, aria-label on icon buttons, aria-live on toasts).
  • Section 6 conventions table lists "Brand colors and typography" — good. There's no corresponding "Accessibility checklist" entry that would list the WCAG AA baseline the project should be targeting.
  • The document format itself: six sections plus anti-patterns is appropriate scope for a CONTRIBUTING.md. Keeping it tight (as the issue's anti-pattern note demands) is the right call. Don't let my additions inflate it — a single bullet per accessibility item is enough.

Recommendations

  • Add one bullet to Walkthrough C step 5: "Minimum 44px touch targets on all interactive elements (min-h-[44px]); focus rings on all focusable elements (focus-visible:ring-2 focus-visible:ring-brand-navy); aria-label on all icon-only buttons."
  • Add "Accessibility baseline" to the Section 6 conventions table as a one-row entry: WCAG 2.1 AA required; touch targets ≥ 44px; focus rings via focus-visible; aria-live="polite" on dynamic status messages; color never used as the sole status indicator.
  • Keep both additions to ≤ 3 bullet points total. The goal is a signal, not a full WCAG audit checklist — contributors who need more detail can reference docs/STYLEGUIDE.md (which exists at docs/STYLEGUIDE.md).

Open Decisions (none — these are corrections reflecting existing project standards, not new requirements)

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - This is a developer documentation issue, so there's no UI to review for accessibility or brand compliance. My angle here is the *reader experience* of the document itself, and one specific gap in Walkthrough C. - Walkthrough C (add a frontend page) has a step for Paraglide translation keys, a step for the API client pattern, and a step for brand colors — but no mention of the accessibility baseline expected for new pages. The project serves an audience that includes 60+ year-old users. New contributors won't know this unless it's written down. - The CLAUDE.md mentions `min-h-[44px]` touch targets and `focus-visible:ring-2` focus rings as established patterns — but neither appears in the CONTRIBUTING.md walkthrough. A contributor creating a new page has no signal to implement these. - Walkthrough C step 5 correctly references `<BackButton>`, German date input, and brand color utilities. These are good concrete anchors. The gap is that the *accessibility* equivalents of these patterns are missing (touch targets, focus rings, `aria-label` on icon buttons, `aria-live` on toasts). - Section 6 conventions table lists "Brand colors and typography" — good. There's no corresponding "Accessibility checklist" entry that would list the WCAG AA baseline the project should be targeting. - The document format itself: six sections plus anti-patterns is appropriate scope for a CONTRIBUTING.md. Keeping it tight (as the issue's anti-pattern note demands) is the right call. Don't let my additions inflate it — a single bullet per accessibility item is enough. ### Recommendations - **Add one bullet to Walkthrough C step 5:** "Minimum 44px touch targets on all interactive elements (`min-h-[44px]`); focus rings on all focusable elements (`focus-visible:ring-2 focus-visible:ring-brand-navy`); `aria-label` on all icon-only buttons." - **Add "Accessibility baseline" to the Section 6 conventions table** as a one-row entry: WCAG 2.1 AA required; touch targets ≥ 44px; focus rings via `focus-visible`; `aria-live="polite"` on dynamic status messages; color never used as the sole status indicator. - Keep both additions to ≤ 3 bullet points total. The goal is a signal, not a full WCAG audit checklist — contributors who need more detail can reference `docs/STYLEGUIDE.md` (which exists at `docs/STYLEGUIDE.md`). ### Open Decisions _(none — these are corrections reflecting existing project standards, not new requirements)_
marcel commented 2026-05-04 21:05:48 +02:00
Author
Owner
Copy Link

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

  • From a platform perspective, the environment setup section (Section 1) is the highest-risk part of this document. If a new contributor gets the environment wrong, nothing else works.
  • The CLAUDE.md memory already notes: "source SDKMAN and nvm before running java/mvn/node/npm". The issue correctly includes this as a memory note in Section 1. Good. But the issue doesn't specify the exact SDKMAN init command, which trips people up — source "$HOME/.sdkman/bin/sdkman-init.sh" and export NVM_DIR + source are non-obvious if you've never used them.
  • Section 2 mentions "How to run dev locally (frontend + backend + docker-compose for db/minio)". The docker-compose up -d command in the root correctly starts PostgreSQL, MinIO, and the backend. But contributors also need to know that npm run dev for the frontend runs on port 3000 and needs the backend already running. The sequencing should be explicit: (1) docker-compose up, (2) backend via IDE or ./mvnw spring-boot:run, (3) npm run dev.
  • The pre-commit hook runs cd frontend && npm run lint — CONTRIBUTING.md should warn contributors that their first commit will trigger this and they need node_modules installed (npm install) in frontend/ before committing.
  • The issue mentions "Husky" but the Husky setup is configured via frontend/package.json's prepare script (git -C .. config core.hooksPath .husky). Contributors who npm install in /frontend will have hooks configured automatically — but only if they installed from within the frontend directory. This is non-obvious and should be spelled out.
  • There's a docker-compose.ci.yml overlay for CI — the issue doesn't mention it. New contributors shouldn't need to know about it for local dev, but noting its existence ("don't use the CI compose file locally — it removes bind mounts") prevents a common support question.

Recommendations

  • Section 1 — add exact shell commands for SDKMAN and nvm activation. A code block with source "$HOME/.sdkman/bin/sdkman-init.sh" and the nvm equivalent is worth 10 words of prose.
  • Section 2 — add startup order explicitly: docker-compose → backend → frontend. A new contributor will start npm run dev before docker-compose up and wonder why the API calls fail.
  • Section 2 — add cd frontend && npm install as a prerequisite step. The Husky hook setup runs as part of npm install's prepare script — contributors who skip this step will get a hook failure on first commit with no clear error.
  • Add one note about docker-compose.ci.yml: "Do not use the CI compose overlay locally — it is for the CI pipeline and disables bind mounts that the dev workflow depends on."
  • These are all factual corrections about the actual setup, not opinions.

Open Decisions (none)

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations - From a platform perspective, the environment setup section (Section 1) is the highest-risk part of this document. If a new contributor gets the environment wrong, nothing else works. - The CLAUDE.md memory already notes: "source SDKMAN and nvm before running java/mvn/node/npm". The issue correctly includes this as a memory note in Section 1. Good. But the issue doesn't specify the exact SDKMAN init command, which trips people up — `source "$HOME/.sdkman/bin/sdkman-init.sh"` and `export NVM_DIR` + `source` are non-obvious if you've never used them. - Section 2 mentions "How to run dev locally (frontend + backend + docker-compose for db/minio)". The `docker-compose up -d` command in the root correctly starts PostgreSQL, MinIO, and the backend. But contributors also need to know that `npm run dev` for the frontend runs on port 3000 and needs the backend already running. The sequencing should be explicit: (1) docker-compose up, (2) backend via IDE or `./mvnw spring-boot:run`, (3) `npm run dev`. - The pre-commit hook runs `cd frontend && npm run lint` — CONTRIBUTING.md should warn contributors that their first commit will trigger this and they need `node_modules` installed (`npm install`) in `frontend/` before committing. - The issue mentions "Husky" but the Husky setup is configured via `frontend/package.json`'s `prepare` script (`git -C .. config core.hooksPath .husky`). Contributors who `npm install` in `/frontend` will have hooks configured automatically — but only if they installed from within the frontend directory. This is non-obvious and should be spelled out. - There's a `docker-compose.ci.yml` overlay for CI — the issue doesn't mention it. New contributors shouldn't need to know about it for local dev, but noting its existence ("don't use the CI compose file locally — it removes bind mounts") prevents a common support question. ### Recommendations - **Section 1 — add exact shell commands** for SDKMAN and nvm activation. A code block with `source "$HOME/.sdkman/bin/sdkman-init.sh"` and the nvm equivalent is worth 10 words of prose. - **Section 2 — add startup order** explicitly: docker-compose → backend → frontend. A new contributor will start `npm run dev` before `docker-compose up` and wonder why the API calls fail. - **Section 2 — add `cd frontend && npm install`** as a prerequisite step. The Husky hook setup runs as part of `npm install`'s `prepare` script — contributors who skip this step will get a hook failure on first commit with no clear error. - **Add one note** about `docker-compose.ci.yml`: "Do not use the CI compose overlay locally — it is for the CI pipeline and disables bind mounts that the dev workflow depends on." - These are all factual corrections about the actual setup, not opinions. ### Open Decisions _(none)_
marcel commented 2026-05-04 21:06:14 +02:00
Author
Owner
Copy Link

📋 Elicit — Requirements Engineer

Observations

  • The issue is well-structured: six concrete sections, three named walkthroughs with real example paths, anti-patterns, and an explicit acceptance criteria checklist. This is above average quality for a documentation issue.
  • One acceptance criterion reads: "Conventions section migrates the rules from backend/CLAUDE.md and frontend/CLAUDE.md (per DOC-7)". The use of "migrates" is ambiguous — does it mean copy? Summarize? Replace? Link? The difference matters for maintainability. If the rules are copied, you now have three places to update whenever a convention changes (CONTRIBUTING.md, backend/CLAUDE.md, frontend/CLAUDE.md). If they are linked, CONTRIBUTING.md stays thin and the canonical rule lives in one place.
  • The dependency section says "Soft dependency on Epic 4 (Refactor) being complete OR planned". "Planned" is not defined. Does "planned" mean an issue exists? A milestone is assigned? A PR is open? This soft dependency is ambiguous enough that two contributors could interpret it differently and produce walkthroughs that contradict each other.
  • The issue is part of Epic #394 (Documentation) and addresses C5.1, C5.3, C5.4. These Legibility Rubric criteria are defined elsewhere — a reader of this issue in isolation doesn't know what C5.1 etc. mean. This is fine for an internal team that knows the rubric, but worth noting if the contributor base grows.
  • The acceptance criterion "Linked from README.md (DOC-1)" implies README.md exists and has a place for this link. README.md does not currently exist at the repo root (verified by inspection). This is a blocking dependency that isn't flagged as such in the issue.
  • The Definition of Done says: "Closing comment summarizes the three walkthrough examples." Good — this creates a natural verification step.

Recommendations

  • Clarify "migrates" in the acceptance criteria: Change to "Conventions section links to or summarizes the key rules from backend/CLAUDE.md and frontend/CLAUDE.md — do not duplicate content that already has a canonical home." This prevents a three-way sync problem.
  • Sharpen the dependency condition: Change "Soft dependency on Epic 4 being complete OR planned" to "Write walkthroughs against the current package structure. Add a callout box noting the planned feature-package refactor (Epic #X). Update walkthroughs when the refactor lands." This makes the dependency concrete and unambiguous.
  • Add README.md as an explicit prerequisite: The acceptance criterion "Linked from README.md (DOC-1)" is blocked if DOC-1 (create README.md) hasn't shipped yet. Either re-order the epic or add a note: "This AC requires DOC-1 to be completed first."
  • Add one acceptance criterion: "All file paths, directory names, and command examples in the walkthroughs have been verified against the current codebase structure." Several paths in the current spec are mismatched with reality (see Markus and Felix's reviews).

Open Decisions

  • "Migrate" vs. "link" for the conventions section: Copy rules from backend/CLAUDE.md/frontend/CLAUDE.md into CONTRIBUTING.md (single entry point, three-way sync risk) vs. summarize and link to the canonical sources (thin CONTRIBUTING, reader follows links). The right answer depends on whether the CLAUDE.md files are considered stable canonical sources or LLM-specific overrides that human contributors shouldn't need to read.
## 📋 Elicit — Requirements Engineer ### Observations - The issue is well-structured: six concrete sections, three named walkthroughs with real example paths, anti-patterns, and an explicit acceptance criteria checklist. This is above average quality for a documentation issue. - One acceptance criterion reads: "Conventions section migrates the rules from `backend/CLAUDE.md` and `frontend/CLAUDE.md` (per DOC-7)". The use of "migrates" is ambiguous — does it mean copy? Summarize? Replace? Link? The difference matters for maintainability. If the rules are *copied*, you now have three places to update whenever a convention changes (CONTRIBUTING.md, backend/CLAUDE.md, frontend/CLAUDE.md). If they are *linked*, CONTRIBUTING.md stays thin and the canonical rule lives in one place. - The dependency section says "Soft dependency on Epic 4 (Refactor) being complete OR planned". "Planned" is not defined. Does "planned" mean an issue exists? A milestone is assigned? A PR is open? This soft dependency is ambiguous enough that two contributors could interpret it differently and produce walkthroughs that contradict each other. - The issue is part of Epic #394 (Documentation) and addresses C5.1, C5.3, C5.4. These Legibility Rubric criteria are defined elsewhere — a reader of this issue in isolation doesn't know what C5.1 etc. mean. This is fine for an internal team that knows the rubric, but worth noting if the contributor base grows. - The acceptance criterion "Linked from `README.md` (DOC-1)" implies `README.md` exists and has a place for this link. `README.md` does not currently exist at the repo root (verified by inspection). This is a blocking dependency that isn't flagged as such in the issue. - The Definition of Done says: "Closing comment summarizes the three walkthrough examples." Good — this creates a natural verification step. ### Recommendations - **Clarify "migrates" in the acceptance criteria:** Change to "Conventions section *links to or summarizes* the key rules from `backend/CLAUDE.md` and `frontend/CLAUDE.md` — do not duplicate content that already has a canonical home." This prevents a three-way sync problem. - **Sharpen the dependency condition:** Change "Soft dependency on Epic 4 being complete OR planned" to "Write walkthroughs against the *current* package structure. Add a callout box noting the planned feature-package refactor (Epic #X). Update walkthroughs when the refactor lands." This makes the dependency concrete and unambiguous. - **Add `README.md` as an explicit prerequisite:** The acceptance criterion "Linked from `README.md` (DOC-1)" is blocked if DOC-1 (create README.md) hasn't shipped yet. Either re-order the epic or add a note: "This AC requires DOC-1 to be completed first." - **Add one acceptance criterion:** "All file paths, directory names, and command examples in the walkthroughs have been verified against the current codebase structure." Several paths in the current spec are mismatched with reality (see Markus and Felix's reviews). ### Open Decisions - **"Migrate" vs. "link" for the conventions section:** Copy rules from `backend/CLAUDE.md`/`frontend/CLAUDE.md` into CONTRIBUTING.md (single entry point, three-way sync risk) vs. summarize and link to the canonical sources (thin CONTRIBUTING, reader follows links). The right answer depends on whether the CLAUDE.md files are considered stable canonical sources or LLM-specific overrides that human contributors shouldn't need to read.
marcel commented 2026-05-04 21:06:47 +02:00
Author
Owner
Copy Link

🗳️ Decision Queue — Action Required

2 decisions need your input before implementation starts.

Architecture / Documentation

  • Which package structure to document in the walkthroughs: current layered OR target feature-package? The issue says walkthroughs A and B should reference the target domain-based structure — but the backend is still layered today (controller/, service/, repository/). If Epic 4 slips, CONTRIBUTING.md is factually wrong on day one. Option A: Write against current structure, add a callout box about the planned refactor, update CONTRIBUTING.md when Epic 4 lands (one small PR). Option B: Write against target structure as-is, accept that the walkthroughs will be inconsistent with reality until Epic 4 ships. Cost of A: a small follow-up edit. Cost of B: actively misleading new contributors until the refactor is done. (Raised by: Markus, Felix, Elicit)

Requirements

  • "Migrate" vs. "link" for the conventions section: The AC says "Conventions section migrates the rules from backend/CLAUDE.md and frontend/CLAUDE.md." If "migrate" means copy, you create three places to maintain the same rules. If it means link, CONTRIBUTING.md stays thin but requires contributors to follow links. Option A (link): Summarize in 1–2 bullets per area, link to the canonical file for details. Single source of truth. Option B (copy): Full rules in CONTRIBUTING.md for a single-document experience; accept that they may drift from the CLAUDE.md files over time. The CLAUDE.md files are currently flagged as LLM-specific — if that's their primary audience, copying the human-relevant subset into CONTRIBUTING.md is the right call. (Raised by: Elicit)
## 🗳️ Decision Queue — Action Required _2 decisions need your input before implementation starts._ ### Architecture / Documentation - **Which package structure to document in the walkthroughs: current layered OR target feature-package?** The issue says walkthroughs A and B should reference the *target* domain-based structure — but the backend is still layered today (`controller/`, `service/`, `repository/`). If Epic 4 slips, CONTRIBUTING.md is factually wrong on day one. **Option A:** Write against current structure, add a callout box about the planned refactor, update CONTRIBUTING.md when Epic 4 lands (one small PR). **Option B:** Write against target structure as-is, accept that the walkthroughs will be inconsistent with reality until Epic 4 ships. Cost of A: a small follow-up edit. Cost of B: actively misleading new contributors until the refactor is done. _(Raised by: Markus, Felix, Elicit)_ ### Requirements - **"Migrate" vs. "link" for the conventions section:** The AC says "Conventions section migrates the rules from `backend/CLAUDE.md` and `frontend/CLAUDE.md`." If "migrate" means *copy*, you create three places to maintain the same rules. If it means *link*, CONTRIBUTING.md stays thin but requires contributors to follow links. **Option A (link):** Summarize in 1–2 bullets per area, link to the canonical file for details. Single source of truth. **Option B (copy):** Full rules in CONTRIBUTING.md for a single-document experience; accept that they may drift from the CLAUDE.md files over time. The CLAUDE.md files are currently flagged as LLM-specific — if that's their primary audience, copying the human-relevant subset into CONTRIBUTING.md is the right call. _(Raised by: Elicit)_
marcel commented 2026-05-05 21:18:58 +02:00
Author
Owner
Copy Link

✅ Decision Queue — Resolved

The 2 decisions raised in #398#issuecomment-6290:

1. Current vs target package structure → write against current; add a callout for the planned refactor (Option A)

Same shape as DOC-2's resolution. Markus, Felix, and Elicit all converge: Anja opens the backend on day one and sees controller/, service/, repository/, model/. A walkthrough that says "create org.raddatz.familienarchiv.tag.TagController.java" while the repo has controller/TagController.java is actively misleading.

Implementation pattern:

  • Walkthroughs A, B, C reference the current structure, with exact paths verified against main.
  • Each walkthrough opens with a one-line callout: "The backend is currently organised by layer (controller/, service/, model/, repository/). Epic 4 (Refactor) will restructure it into per-domain packages — when that lands, this walkthrough is updated in the same PR."
  • All file paths and command examples must be verified against the current codebase before merge — explicit AC.

2. "Migrate" vs "link" for the conventions section → migrate (Option B), but only because DOC-7 makes CONTRIBUTING.md the canonical home

Per epic-level Decision Queue D3 (the pointer-comment policy resolved on #394), CLAUDE.md sections become 1-line pointers TO CONTRIBUTING.md (and other human docs). Once that's done:

  • CONTRIBUTING.md is the canonical source for human-readable rules.
  • CLAUDE.md keeps only LLM-workflow content (it's not a parallel home for the same rules).
  • There is no three-way sync risk because the pointer comment in CLAUDE.md just dereferences the CONTRIBUTING.md heading.

So Option B (copy into CONTRIBUTING.md) is correct — it's not duplication; it's the migration target. The follow-up is DOC-7 (#401) replacing the now-stale CLAUDE.md sections with pointers.

Sequencing: DOC-4 ships with the conventions content fully written. DOC-7 then strips the migrated sections out of CLAUDE.md.

📌 Additional persona feedback to fold into implementation

  • Markus: replace dead links — "docs/ARCHITECTURE.md" → use the actual path that will exist after DOC-2 (#396) lands; if not yet, link to docs/architecture/c4-diagrams.md provisionally. "docs/GLOSSARY.md" same.
  • Felix: fix Walkthrough C's frontend lib path claim. Current frontend has lib/components/ (flat-ish) and recent refactors are moving things to per-domain lib/<domain>/ (issue #408). Verify against main at implementation time and use the structure that's actually there.
  • Felix: Walkthrough B step 6 (regen types) deserves a warning callout — most common "where did my type go?" confusion.
  • Felix + Sara: reorder ALL walkthroughs to TDD order. The project's Red/Green TDD rule (per COLLABORATING.md and memories feedback_tdd / feedback_tdd_flow) is non-negotiable. "Write a failing test" comes first, "make it pass" comes second. Walkthrough A may put TDD later (no behaviour to test until controller exists), but it must be present.
  • Sara: add the test-type decision matrix — server load → Vitest unit (mock fetch); UI flow → Playwright E2E; shared component → Vitest browser-mode render() + getByRole(); backend service logic → @ExtendWith(MockitoExtension.class); HTTP contract → @WebMvcTest slice. Replace the vague "unit test or e2e test" wording.
  • Nora: change Walkthrough B step 3 from "Add @RequirePermission if mutating" to "Add @RequirePermission(Permission.WRITE_ALL) on every POST, PUT, PATCH, DELETE — this is not optional. Read-only endpoints stay unannotated."
  • Nora: add input-validation step to Walkthrough B — "Validate user-supplied inputs at the controller boundary using ResponseStatusException(BAD_REQUEST, …) or DomainException.badRequest(…). Trust internal service code; never trust unvalidated boundary input."
  • Nora: add a Security checklist sub-section to Section 6 — @RequirePermission required on writes; parameterised queries only (@Param); no raw user input in logs (use {} placeholders); content-type validation on uploads.
  • Leonie: add accessibility baseline to Walkthrough C step 5 + Section 6:
    • Touch targets ≥ 44px (min-h-[44px])
    • Focus rings (focus-visible:ring-2 focus-visible:ring-brand-navy)
    • aria-label on icon-only buttons
    • aria-live="polite" on dynamic status messages
    • Color is never the sole status indicator
      Cap at ≤3 bullets; link to docs/STYLEGUIDE.md for full WCAG checklist.
  • Tobias: Section 1 — add exact shell commands for SDKMAN and nvm activation: 
    source "$HOME/.sdkman/bin/sdkman-init.sh"
export NVM_DIR="$HOME/.nvm" && [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"
  • Tobias: Section 2 — explicit startup order: (1) docker compose up -d, (2) backend (./mvnw spring-boot:run or IDE), (3) cd frontend && npm install && npm run dev. Note that npm install triggers Husky hook setup via the prepare script — required before the first commit.
  • Tobias: add note about docker-compose.ci.yml — "Do not use the CI compose overlay locally; it disables bind mounts that the dev workflow depends on."
  • Elicit: add AC: "All file paths, directory names, and command examples in the walkthroughs have been verified against current main before merge."
  • Elicit: sharpen rubric reference — DOC-4 addresses C5.1, C5.2, C5.3, plus C7.1 (per-domain context for the conventions reference).

Status: Ready for implementation. Read frontend/src/lib/ and backend/src/main/java/... against the latest main before writing exact paths — they shift as #408 lands.

## ✅ Decision Queue — Resolved The 2 decisions raised in [#398#issuecomment-6290](http://heim-nas:3005/marcel/familienarchiv/issues/398#issuecomment-6290): ### 1. Current vs target package structure → **write against current; add a callout for the planned refactor (Option A)** Same shape as DOC-2's resolution. Markus, Felix, and Elicit all converge: Anja opens the backend on day one and sees `controller/`, `service/`, `repository/`, `model/`. A walkthrough that says "_create `org.raddatz.familienarchiv.tag.TagController.java`_" while the repo has `controller/TagController.java` is actively misleading. **Implementation pattern:** - Walkthroughs A, B, C reference the **current** structure, with exact paths verified against `main`. - Each walkthrough opens with a one-line callout: "_The backend is currently organised by layer (`controller/`, `service/`, `model/`, `repository/`). Epic 4 (Refactor) will restructure it into per-domain packages — when that lands, this walkthrough is updated in the same PR._" - All file paths and command examples must be verified against the current codebase before merge — explicit AC. ### 2. "Migrate" vs "link" for the conventions section → **migrate (Option B), but only because DOC-7 makes CONTRIBUTING.md the canonical home** Per **epic-level Decision Queue D3** (the pointer-comment policy resolved on #394), CLAUDE.md sections become 1-line pointers TO CONTRIBUTING.md (and other human docs). Once that's done: - CONTRIBUTING.md is **the canonical source** for human-readable rules. - CLAUDE.md keeps only LLM-workflow content (it's not a parallel home for the same rules). - There is no three-way sync risk because the pointer comment in CLAUDE.md just dereferences the CONTRIBUTING.md heading. So **Option B (copy into CONTRIBUTING.md)** is correct — it's not duplication; it's the migration target. The follow-up is DOC-7 (#401) replacing the now-stale CLAUDE.md sections with pointers. **Sequencing:** DOC-4 ships with the conventions content fully written. DOC-7 then strips the migrated sections out of CLAUDE.md. --- ## 📌 Additional persona feedback to fold into implementation - **Markus:** replace dead links — "_`docs/ARCHITECTURE.md`_" → use the actual path that will exist after DOC-2 (#396) lands; if not yet, link to `docs/architecture/c4-diagrams.md` provisionally. "_`docs/GLOSSARY.md`_" same. - **Felix:** **fix Walkthrough C's frontend lib path claim**. Current frontend has `lib/components/` (flat-ish) and recent refactors are moving things to per-domain `lib/<domain>/` (issue #408). Verify against `main` at implementation time and use the structure that's actually there. - **Felix:** Walkthrough B step 6 (regen types) deserves a warning callout — most common "where did my type go?" confusion. - **Felix + Sara:** **reorder ALL walkthroughs to TDD order**. The project's Red/Green TDD rule (per `COLLABORATING.md` and memories `feedback_tdd` / `feedback_tdd_flow`) is non-negotiable. "_Write a failing test_" comes first, "_make it pass_" comes second. Walkthrough A may put TDD later (no behaviour to test until controller exists), but it must be present. - **Sara:** add the **test-type decision matrix** — server `load` → Vitest unit (mock `fetch`); UI flow → Playwright E2E; shared component → Vitest browser-mode `render()` + `getByRole()`; backend service logic → `@ExtendWith(MockitoExtension.class)`; HTTP contract → `@WebMvcTest` slice. Replace the vague "_unit test or e2e test_" wording. - **Nora:** **change Walkthrough B step 3** from "_Add `@RequirePermission` if mutating_" to "_Add `@RequirePermission(Permission.WRITE_ALL)` on every `POST`, `PUT`, `PATCH`, `DELETE` — this is **not optional**. Read-only endpoints stay unannotated._" - **Nora:** add input-validation step to Walkthrough B — "_Validate user-supplied inputs at the controller boundary using `ResponseStatusException(BAD_REQUEST, …)` or `DomainException.badRequest(…)`. Trust internal service code; never trust unvalidated boundary input._" - **Nora:** add a **Security checklist** sub-section to Section 6 — `@RequirePermission` required on writes; parameterised queries only (`@Param`); no raw user input in logs (use `{}` placeholders); content-type validation on uploads. - **Leonie:** add **accessibility baseline** to Walkthrough C step 5 + Section 6: - Touch targets ≥ 44px (`min-h-[44px]`) - Focus rings (`focus-visible:ring-2 focus-visible:ring-brand-navy`) - `aria-label` on icon-only buttons - `aria-live="polite"` on dynamic status messages - Color is never the sole status indicator Cap at ≤3 bullets; link to `docs/STYLEGUIDE.md` for full WCAG checklist. - **Tobias:** Section 1 — **add exact shell commands** for SDKMAN and nvm activation: ```bash source "$HOME/.sdkman/bin/sdkman-init.sh" export NVM_DIR="$HOME/.nvm" && [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh" ``` - **Tobias:** Section 2 — explicit startup order: (1) `docker compose up -d`, (2) backend (`./mvnw spring-boot:run` or IDE), (3) `cd frontend && npm install && npm run dev`. Note that `npm install` triggers Husky hook setup via the `prepare` script — required before the first commit. - **Tobias:** add note about `docker-compose.ci.yml` — "_Do not use the CI compose overlay locally; it disables bind mounts that the dev workflow depends on._" - **Elicit:** add AC: "_All file paths, directory names, and command examples in the walkthroughs have been verified against current `main` before merge._" - **Elicit:** sharpen rubric reference — DOC-4 addresses C5.1, C5.2, C5.3, plus C7.1 (per-domain context for the conventions reference). **Status:** Ready for implementation. Read `frontend/src/lib/` and `backend/src/main/java/...` against the latest `main` before writing exact paths — they shift as #408 lands.
marcel commented 2026-05-05 22:56:42 +02:00
Author
Owner
Copy Link

DOC-4 implemented — PR #442

Walkthrough examples:

  • A — Add a new domain: citation domain — 12 steps from creating the backend package through Flyway migration, TDD-first tests, frontend lib/citation/, docs update, and ESLint boundary rule
  • B — Add a new endpoint: POST /api/persons/{id}/aliases — TDD Red step first, then @RequirePermission(Permission.WRITE_ALL) (mandatory, not optional), input validation at boundary, DomainException for errors, and the ⚠️ type-regen warning
  • C — Add a new frontend page: /persons/[id]/timeline — TDD Red first (Playwright E2E), SSR load with typed API client, domain component in lib/person/, shared primitives in lib/shared/primitives/, accessibility baseline, Paraglide i18n

All file paths verified against current main. All walkthroughs in TDD order.

PR: http://heim-nas:3005/marcel/familienarchiv/pulls/442

## DOC-4 implemented — PR #442 **Walkthrough examples:** - **A — Add a new domain:** `citation` domain — 12 steps from creating the backend package through Flyway migration, TDD-first tests, frontend `lib/citation/`, docs update, and ESLint boundary rule - **B — Add a new endpoint:** `POST /api/persons/{id}/aliases` — TDD Red step first, then `@RequirePermission(Permission.WRITE_ALL)` (mandatory, not optional), input validation at boundary, `DomainException` for errors, and the ⚠️ type-regen warning - **C — Add a new frontend page:** `/persons/[id]/timeline` — TDD Red first (Playwright E2E), SSR load with typed API client, domain component in `lib/person/`, shared primitives in `lib/shared/primitives/`, accessibility baseline, Paraglide i18n All file paths verified against current `main`. All walkthroughs in TDD order. PR: http://heim-nas:3005/marcel/familienarchiv/pulls/442
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 P1-high documentation legibility
Milestone
No items
No Milestone Codebase Legibility
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#398
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?