docs(legibility): DOC-4 — write CONTRIBUTING.md with three concrete walkthroughs #442
Reference in New Issue
Block a user
Delete Branch "feat/issue-398-contributing"
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?
Summary
Adds
CONTRIBUTING.mdat the repo root — the operational manual for adding things to the codebase.What's covered (6 sections)
POST /api/persons/{id}/aliasesexample; TDD order;@RequirePermissionmarked not optional; input validation step; error code mirroring/persons/[id]/timelineexample; TDD order; correctlib/person/andlib/shared/primitives/paths; accessibility baseline; i18nKey decisions reflected
main.@RequirePermissionon mutations is not optional (per Nora's security feedback).Note on README link
README.md(DOC-1, PR #440) has a TODO row for CONTRIBUTING.md in its "Where to go next" table. That link will be filled in once PR #440 merges.Test plan
CONTRIBUTING.mdin Gitea — verify all 6 sections render correctlymain(already verified before writing)$lib/shared/primitives/BackButton.svelteimport path is correctCloses #398
🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Good structure, correct import paths throughout, and the TDD Red-before-Green order is maintained consistently in all three walkthroughs — that part reads exactly as it should. Two blockers and a handful of suggestions.
Blockers
1.
docs/ARCHITECTURE.mddoes not exist — dead link in the intro and Walkthrough A step 10The intro section links to
./docs/ARCHITECTURE.mdand Walkthrough A step 10 says:The actual file is
docs/architecture/c4-diagrams.md. There is nodocs/ARCHITECTURE.mdat all. Anyone following this guide will get a 404 in Gitea and anlsthat returns nothing.Fix: change both references to
docs/architecture/c4-diagrams.md. Also remove "Section 2" — the file has no numbered sections.2.
./mvnw checkstyle:checkcommand in the Lint section will failThe Lint and format block at the bottom of section 6 says:
There is no
maven-checkstyle-plugininbackend/pom.xml. Running this command returns:Either remove the line, or add the plugin to pom.xml (separate PR). Right now it's a misleading command that will confuse contributors on their first run.
Suggestions
3. DTOs in sub-packages — "flat in the domain package" is not always true
Section 6 Conventions says:
The
person/relationship/sub-domain already has adto/sub-package with 6 DTOs (CreateRelationshipRequest.java,FamilyMemberPatchDTO.java, etc.). This is a correct architectural choice for a sub-domain, but the blanket statement in Conventions is misleading for contributors adding sub-domain endpoints. A note like "for sub-domains, a nesteddto/package is acceptable" would make this accurate.4. DOC-6 per-domain READMEs referenced as a current convention
Walkthrough A step 9 says:
DOC-6 is a future issue. No per-domain READMEs exist yet in any backend package folder or frontend lib domain folder (only Paraglide's generated ones). The forward reference to an unreleased convention will confuse contributors — they'll go looking for examples to copy and find nothing.
Fix: either note that this is a forthcoming convention and link the issue, or remove the DOC-6 reference and just state the expectation without anchoring it to a not-yet-implemented standard.
5.
import { PageServerLoad }is missing from the Walkthrough C+page.server.tssnippetThe type
PageServerLoadis not imported in the snippet. A contributor pasting this will get a TypeScript error. Addimport type { PageServerLoad } from './$types';at the top.The core conventions —
$lib/shared/api.server,$lib/shared/errors,$lib/shared/primitives/BackButton.svelte, the dateT12:00:00pattern,@RequirePermissionnot-optional — are all verified correct against the codebase. Good to see CLAUDE.md's stale$lib/components/BackButton.sveltepath was not carried forward here.🏗️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
The domain-first principle is stated correctly and consistently across both stacks. The layering rules (Controller → Service → Repository; cross-domain access through the owning service) are accurate. The Flyway migration step is in the right position. Two structural issues worth fixing before this ships as the canonical reference.
Blockers
1.
docs/ARCHITECTURE.mdis a dead link — the guide's own consistency principle is violated on page 1The intro and Walkthrough A both reference
docs/ARCHITECTURE.md. This file does not exist. The architecture documentation lives atdocs/architecture/c4-diagrams.md. A contributor following the "add a domain" walkthrough will reach step 10 and find a path that resolves to nothing.This is not a minor nit: the guide is being promoted as the canonical operational manual. A dead link on the first instruction the contributor would follow is a credibility problem.
2. The "flat in the domain package" DTO rule is stated too broadly
Conventions section:
The
person/relationship/dto/sub-package already exists with 6 DTOs. The flat rule is correct for top-level domains; it is not the pattern used for sub-domains. The guide should acknowledge the sub-domain DTO pattern explicitly so contributors don't flatten a sub-domain's DTO surface into the parent package.Suggestions
3. "Update
docs/ARCHITECTURE.mdSection 2" — architectural decision responsibility is underspecifiedEven after fixing the path, "update the architecture doc" as a step in a contributor walkthrough implies that any developer adding a domain self-edits the architecture docs. In practice, architectural doc updates are reviewed as part of the PR; the walkthrough should say "propose the update in the PR description" rather than treating it as a code-like step.
4. ESLint boundary allow-list — good, but the walkthrough should clarify what "needs to import from another domain" actually means
Step 12 of Walkthrough A tells contributors to update
frontend/eslint.config.js"if the domain needs to import from another domain." The existing config showsdocumentimporting from bothpersonanddocument, andconversationimporting fromshared. A concrete one-line example of what an allow-list entry looks like would make this actionable:Without this, contributors will open
eslint.config.js, see an unfamiliar DSL, and either skip the step or make a wrong edit.5. The "startup order" section says
docker composenotdocker-composeThis is correct for Docker Compose v2 (plugin syntax). The existing
CLAUDE.mdstill usesdocker-compose up -d. The guide should be consistent — and since v2 is the right call,CLAUDE.mdis the one that needs updating, not this file. Worth noting as a cross-document inconsistency to clean up in the same DOC series.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
The security posture documented here is solid.
@RequirePermissionon write endpoints is called out as not optional in two separate places (Walkthrough A step 3, Walkthrough B step 4) — exactly right. The security checklist in section 6 covers the five points that matter most for this stack: permission annotations, input validation, parameterized queries, log injection prevention, and upload validation.A few observations — none are blockers.
No blockers
Suggestions
1. The security checklist item on log injection uses SLF4J placeholder notation correctly
This is accurate and prevents Log4Shell-class attacks. Good.
2. Upload validation checklist item is appropriately scoped
The phrasing "before reading the stream" is the critical detail that prevents resource exhaustion attacks. Good call including it.
3. Missing: CSRF posture is not explained
The guide doesn't say anything about CSRF. For a contributor adding a new
POSTendpoint, it would be useful to know that CSRF is disabled at the framework level (Spring Security config) and why that is safe — the existingSecurityConfig.javahas a comment explaining this (Basic Auth + Authorization header; browsers block cross-origin custom headers). A single sentence like "CSRF is disabled application-wide; the explanation is inSecurityConfig.java" would prevent contributors from re-enabling it thinking it's missing.4.
@CrossOriginis not mentioned — consider adding a "don't add this" noteA contributor who hits a browser CORS error during local development might instinctively add
@CrossOrigin(origins = "*")to the controller. The guide doesn't warn against this. Given that the backend uses session-based auth (Spring Session JDBC), wildcard CORS would be a real issue. A one-line callout in the security checklist ("never add@CrossOrigin— CORS is configured globally inSecurityConfig") would head off this common mistake.5. The
@RequirePermissionnote in Walkthrough B (step 4) uses the right annotation placementPermission annotation is on the method, above the HTTP method annotation. This matches the existing codebase pattern and ensures the AOP advice fires correctly.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
The test-type decision matrix is the strongest part of this document — it maps each testing scenario to the correct tool and layer, which is exactly what a contributor needs before they start. TDD Red-before-Green order is maintained throughout. One blocker and a few gaps.
Blockers
1.
./mvnw checkstyle:checkwill fail — no Checkstyle plugin inpom.xmlThe "Lint and format" block in section 6 tells contributors to run:
There is no
maven-checkstyle-pluginin the backend'spom.xml. This command will fail with a "No plugin found" error. A contributor running the full test cycle before their first commit will hit this and either be blocked or lose confidence in the guide.Fix: remove the command, or add the plugin in a separate PR and update this guide once it's wired up.
Suggestions
2. The test-type matrix doesn't mention integration tests (Testcontainers /
@SpringBootTest)The matrix covers: unit (JUnit + Mockito), controller slice (
@WebMvcTest), Vitest unit, Vitest browser-mode, and E2E (Playwright). It has no row for integration tests against a real database — the layer where Flyway migrations, JPA queries, and constraint behavior are verified. A contributor adding a new domain would not know from this matrix that a Testcontainers-backed integration test belongs alongside the unit and slice tests.Suggested row to add:
| JPA queries, Flyway migrations, constraint behavior | Integration test |
@SpringBootTest+ Testcontainers (postgres:16-alpine) |3. No mention of where test files live relative to production code
The walkthroughs tell contributors to write failing tests for service logic and controller slices, but don't say where those test files should go. The codebase puts test files alongside production code in the same package (
DocumentServiceTest.javanext toDocumentService.java). First-time contributors may default tosrc/test/java/with a duplicated package tree — which works but isn't the project convention.4. Playwright E2E snippet is missing
test.usefixture / auth setupThe Walkthrough C Playwright example:
This will redirect to
/loginin the test environment without auth context. The actual E2E tests use astorageStatefixture ortest.use({ storageState: ... })for the authenticated user. Without this, a contributor copying this snippet will write a test that always fails for the wrong reason. A one-line note like "seee2e/fixtures/for the auth setup" would prevent the confusion.5. "Run the full test suite — all green before committing" (Walkthrough B step 9) — which suite?
Step 9 says to run the full test suite but doesn't specify the commands. By the time a contributor has added a backend endpoint they should be running both
./mvnw testandnpx playwright test. Being explicit here mirrors the precision of the test matrix above it.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
The frontend conventions are well-documented from a UI and accessibility standpoint. The brand color tokens (
brand-navy,brand-mint,brand-sand) are named correctly, theT12:00:00date fix is included, and the accessibility baseline (touch targets ≥ 44px, focus rings,aria-label,aria-live) matches the project standard.I verified the
BackButtonimport path against the codebase —$lib/shared/primitives/BackButton.svelteis correct. The old CLAUDE.md path ($lib/components/BackButton.svelte) was stale; this document has it right.No blockers
Suggestions
1. Brand color
layout.csspath is correct but the comment would benefit from a note about Tailwind CSS 4The guide says brand colors are "defined in
src/routes/layout.css" — this is correct. A contributor new to Tailwind CSS 4's first-party custom utility approach might be confused about why brand colors are defined in a CSS file insideroutes/rather than intailwind.config.js. A single parenthetical "(Tailwind CSS 4 — utilities defined in CSS, not in config)" would eliminate that confusion.2. Card pattern and save bar pattern are not documented in this guide
CLAUDE.md has explicit patterns for cards and save bars (with code examples). CONTRIBUTING.md omits them. For Walkthrough C specifically — adding a new frontend page — a contributor doesn't know which card pattern to use or whether to use sticky-full-bleed vs card-style save bar. The guide tells them about colors and accessibility but not the structural layout patterns they'll need on day one.
Suggestion: add a "Page layout patterns" subsection in section 6 that references the card pattern and the two save bar variants (or links to CLAUDE.md for now).
3. Accessibility baseline is accurate but the
aria-liveexample lacks contextThe baseline says:
This is correct. But a contributor won't know where this typically appears (save confirmations, search result counts, loading states). A one-line example like
<p role="status" aria-live="polite">{saveMessage}</p>would make it immediately actionable.4. The senior audience constraint (60+ users) is not mentioned in Walkthrough C
The device-split for this project is documented internally (transcribers on laptop/tablet, readers on phones, 60+ demographic). Walkthrough C mentions "touch targets ≥ 44px" but doesn't explain why this is especially important here. Without context, a contributor may treat it as a generic rule rather than a critical constraint for the primary readership. One sentence noting the senior-reader demographic would make the 44px requirement feel like a design decision, not a bureaucratic rule.
🖥️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ⚠️ Approved with concerns
The startup instructions are correct (
docker composev2 syntax, correct service ordering:db miniobefore backend, backend before frontend). Thedocker-compose.ci.ymlwarning is accurate. One blocker on a command that will fail.Blockers
1.
./mvnw checkstyle:checkwill fail — Checkstyle is not configuredSection 6, Lint and format:
I checked
backend/pom.xml— there is nomaven-checkstyle-plugin. This command fails immediately with:The comment says "checkstyle is enforced via Maven" — this is not currently true. Either the plugin needs to be added to
pom.xml(justified — consistent code style enforcement is worth having) or this line should be removed. Either way, as written it's a command that breaks a contributor's first run.Suggestions
2. The
npm installnote is correct but the Husky warning deserves more prominenceThe startup section includes:
This is accurate. The
preparescript inpackage.jsonrunshusky. However, the note is easy to miss buried in the blockquote below the startup commands. Given that a failing pre-commit hook is a common first-commit frustration, consider making this a top-level note rather than a nested blockquote.3. No mention of the
.envfile setupThe startup sequence assumes environment variables are in place (database credentials, MinIO credentials, etc.) but the guide doesn't mention an
.envfile,.env.example, or how to obtain the required values. A first-time contributor following the startup steps will hit a credentials error before the stack comes up.If there's an
.env.exampleat the repo root, add a note: "Copy.env.exampleto.envand fill in the local values before starting the stack." If there isn't one, that's a gap worth raising separately.4.
docker compose up -d db miniovsdocker-compose up -dThe startup guide correctly uses
docker compose(v2). CLAUDE.md still documentsdocker-compose up -d(v1 compatibility wrapper). This will cause confusion when contributors switch between documents. The DOC series that includes this PR should also update CLAUDE.md to v2 syntax for consistency.5. No mention of what to do when the backend fails to start
The most common startup failure is "backend can't connect to DB." The guide doesn't mention checking
docker compose logs dbor the health check endpoint (/actuator/health). A single troubleshooting note ("if the backend crashes on startup, checkdocker compose logs db minio— the most common cause is the DB not being healthy yet") would save contributors 20 minutes of confusion.📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
As a requirements document, CONTRIBUTING.md is structured well: it separates environment setup, workflow, walkthroughs, and conventions into distinct sections, and each walkthrough follows a consistent Red → Green pattern. The intended audience (contributor adding a new domain, endpoint, or page) is implicit but clear. Two concerns on completeness and one on forward references.
Concerns
1. Forward references to unreleased conventions (DOC-6)
Walkthrough A step 9 says:
DOC-6 is a future issue, not a shipped convention. No per-domain READMEs exist anywhere in the codebase right now — a contributor following this guide will have no example to work from. The guide presents this as an existing rule ("per DOC-6") but it's actually a forthcoming one.
This creates a requirements contradiction: the guide says "do X" but the codebase provides no examples of X having been done, and the rule doesn't yet formally exist.
Recommendation: either (a) defer this step to DOC-6 entirely and remove it from CONTRIBUTING.md until DOC-6 ships, or (b) note explicitly "this convention is being introduced — you are the first to apply it; see issue #XXX for context."
2. The "Documentation" steps in Walkthrough A are aspirational, not verifiable
Steps 10 and 11:
Beyond the path error (
docs/ARCHITECTURE.mddoesn't exist — it'sdocs/architecture/c4-diagrams.md), there is no acceptance criterion for either step. "Section 2" doesn't exist in the c4-diagrams file. A contributor completing the walkthrough has no way to verify they've done this step correctly. These need either more specificity ("add a row to the Domain table indocs/architecture/c4-diagrams.md") or acknowledgment that this is a judgment call reviewed in the PR.3. The intro links to
docs/ARCHITECTURE.mdanddocs/GLOSSARY.md— one of these does not existOpening paragraph:
docs/GLOSSARY.mdexists.docs/ARCHITECTURE.mddoes not — the architecture documentation is atdocs/architecture/c4-diagrams.md. This is a broken link in the very first paragraph of the document, which undermines confidence in the rest of it.Observation (not a blocker)
The three walkthroughs share a consistent structure (Red → Green → documentation) and the conventions reference section provides a compact lookup table. The overall document organization is appropriate for the stated audience. The addition of a "test-type decision matrix" is particularly valuable — it answers the question contributors ask most often ("where does this test go?") at the point in the workflow where it's most needed.
Follow-up commit
dfbd958caddresses the two blockers from the review:docs/ARCHITECTURE.mddead link fixed — added an interim pointer todocs/architecture/c4-diagrams.mduntil DOC-2 (PR #441) merges; the primary link stays asdocs/ARCHITECTURE.mdsince that file will exist before this PR can merge./mvnw checkstyle:checkremoved — nomaven-checkstyle-plugininpom.xml; replaced with./mvnw testand./mvnw clean package -DskipTestswhich are the actual commands that compile and catch code quality issues🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
This is exactly the kind of document that prevents architecture decay. When contributors can read a walkthrough and arrive at the correct package structure without guessing, the domain-first organization stays intact. A few observations:
What works well
citation/example is unambiguous.c4-diagrams.mdpointer fordocs/ARCHITECTURE.mdis a good call. Linking to a not-yet-merged doc would have sent contributors into a 404.Suggestions (non-blocking)
README.mdin the backend package folder. Since CLAUDE.md already notes "per DOC-6", this will be redundant once that PR merges. Worth adding a small parenthetical: (added as part of DOC-6 — until that merges, this step is optional for new domains) — prevents confusion for a contributor who looks and sees existing domains without those READMEs.git log --allor the migration folder onmainbefore numbering would be a practical guard. Tiny addition, high value.eslint.config.jsor noting which variable to modify would prevent a contributor from spending 20 minutes locating it.Overall: the document accurately reflects the architecture as it stands. No corrections required.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Solid contributor guide. The TDD order is correct throughout — Red before Green is stated as a first-class rule in all three walkthroughs, not buried in a footnote. That matters: I've seen guides where it's mentioned once and then the implementation steps come first anyway. This one stays consistent.
What works well
!result.response.okpattern is documented correctly in the frontend API client section. The note about not usingresult.erroris exactly right and saves a debug session.T12:00:00UTC offset trick is documented where it will actually be found — in the date handling reference and in the Walkthrough C inline example. Good placement.BackButton.sveltepath verified — I checked:frontend/src/lib/shared/primitives/BackButton.svelteexists. The import path in the guide is correct.createApiClient(fetch)from$lib/shared/api.server— also verified, file exists atfrontend/src/lib/shared/api.server.ts. Correct.Suggestions (non-blocking)
Walkthrough B, Step 5 —
ResponseStatusExceptionfor validation. The guide shows:The Reliable Code section of CODESTYLE.md flags
ResponseStatusExceptionas something to avoid for domain errors — useDomainException.badRequest()instead. For simple controller boundary validation this is a documented exception (the Conventions reference table at the bottom of this same file shows it), but it would be cleaner to either useDomainException.badRequest(ErrorCode.INVALID_INPUT, "...")here as well, or add a single-sentence explanation in the code comment: // controller boundary validation — domain errors use DomainException. The inconsistency between the walkthrough code and the conventions table could confuse a new contributor.Walkthrough C, Step 3 — load function snippet. The
+page.server.tsexample throws a SvelteKiterror()but does not import it. Fine for a doc snippet, but a beginner will copy-paste and get a compile error. Consider addingimport { error } from '@sveltejs/kit';to the snippet.The test-type decision matrix is excellent. One gap: there is no row for
+page.server.tsaction functions. A contributor adding a form action won't know where action logic fits. Suggest adding a row: Form action handler | MockMvc (server-side) or Vitest (direct import) | depends on where the logic lives.No blockers. The TDD discipline is documented correctly and the code samples are accurate.
🔧 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: ✅ Approved
This is a developer-facing document, not an infrastructure file, so my usual checklist (image tags, named volumes, exposed ports, hardcoded secrets) doesn't apply directly. What I do care about is whether the startup instructions and CI-related commands are accurate for this stack. Let me go through what I checked.
What works well
docker compose up -d db minio→ backend → frontend. This is the actual dependency chain. A new contributor who skips step 1 will get a datasource connection failure on backend start — documenting the sequence prevents that.docker-compose.ci.ymlwarning is correct. The note "Do not usedocker-compose.ci.ymllocally — it disables the bind mounts" is accurate and prevents a real footgun. Good placement right after the startup block.npm installwires the pre-commit hook — without it, the first commit silently skips the hook and contributors wonder why CI catches things their terminal didn't../mvnw testinstead of./mvnw checkstyle:check— confirmed correct. There is no Checkstyle plugin configured in this project'spom.xml. The removed command would have thrown an unsupported goal error.Suggestions (non-blocking)
npm run generate:apirequires the backend running with--spring.profiles.active=dev, which is correctly noted. However, the startup section shows./mvnw spring-boot:runwithout a dev profile flag. A contributor following the guide top-to-bottom would start the backend on the default profile, then fail at the regeneration step. Consider either adding./mvnw spring-boot:run -Dspring-boot.run.profiles=devto the startup section, or adding a small note: regeneration requires the backend started with the dev profile (enables the OpenAPI endpoint).npx playwright test— the guide usesnpx playwright testrather than a dedicated npm script. If there is annpm run test:e2e(or similar) inpackage.json, that would be more consistent with the other commands listed above it. Minor consistency issue only.No mention of
docker compose downor cleanup. Not a blocker, but contributors who accumulate stale containers will eventually run into port conflicts. A brief note about stopping the stack cleanly would be practical.Overall the infrastructure-relevant commands are accurate. No corrections needed.
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
I'm reviewing this as a requirements and specification document. My lens: Is it unambiguous? Is it complete for its stated audience? Are there any hidden assumptions that could mislead a contributor?
What works well
citationdomain andPOST /api/persons/{id}/aliasesexamples are specific enough that a contributor can directly adapt them without interpretation. Abstract rules without examples ("create a service") are how contributors arrive at five different structures for five domains.Concerns
Concern 1 — "conversations" route is stale (non-blocking but worth flagging)
The document does not mention
conversations/directly, but CLAUDE.md (which this PR says will be partially replaced by CONTRIBUTING.md) references it as/briefwechselin actual routes. I verified: the live route isfrontend/src/routes/briefwechsel/, notconversations/. If DOC-7 strips the route table from CLAUDE.md and leaves CONTRIBUTING.md as the primary reference, the route walkthrough examples should use a route that actually exists. Thepersons/[id]/timelineexample in Walkthrough C is hypothetical, which is fine — but if a contributor tries to navigate the existing structure to orient themselves, this could create confusion.Concern 2 —
docs/ARCHITECTURE.mdlink is fragile (minor)The interim note says: "(introduced in DOC-2; until that PR merges, see docs/architecture/c4-diagrams.md)". This creates a time-boxed requirement: this note must be updated once DOC-2 merges, or it becomes permanently stale. There is no mechanism to ensure that happens. Suggest tracking it as a follow-up item in the issue for DOC-2, or flagging it with a
<!-- TODO: remove interim note once DOC-2 merges -->HTML comment in the markdown (invisible in rendered view).Concern 3 — Missing "when to create a new domain" criteria (suggestion)
Walkthrough A tells a contributor how to add a domain but not when it is appropriate to introduce one versus extending an existing domain. A contributor adding "formal references to documents" might reasonably ask: should this be a subdomain of
document/(likedocument/annotation/) or a top-level domain? Without criteria, this decision will vary by contributor instinct. A two-sentence heuristic ("new domain when it has its own entity, its own lifecycle, and no shared service methods with an existing domain") would close this gap.Concern 4 — i18n step in Walkthrough B is missing
Walkthrough B documents adding a new error code (Step 6: add to
ErrorCode.java, mirror inerrors.ts, add translation keys). However it does not mention that the new endpoint response may require new i18n keys for success messages or labels in the frontend. Walkthrough C has an explicit i18n step (Step 7). The asymmetry could cause contributors to skip i18n when adding backend endpoints. Suggest adding a brief Step 8.5 or note in Walkthrough B: "If the endpoint returns data displayed to the user, add display labels tomessages/{de,en,es}.json."No blockers. The document accurately specifies the contribution process. The concerns above are refinements that would reduce future ambiguity.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Security documentation is often the weakest part of a contributor guide — either absent, vague, or so buried that developers skip it. This guide gets the fundamentals right. Let me walk through what I checked.
What works well
@RequirePermissionis documented as non-optional with emphasis. Both Walkthrough A (Step 3) and Walkthrough B (Step 4) include the phrase "this is not optional" in bold. That phrasing matters — it matches the AOP enforcement mechanism and is unambiguous. A contributor who skips this will be caught in code review with this document as the reference.{}placeholder) is a specific and correct callout.ResponseStatusExceptionfor validation is acknowledged as a documented exception for controller boundary checks (distinct fromDomainExceptionfor domain errors). The Conventions reference table makes this explicit, which is good — it prevents the confusion of "why is one approach used in Walkthrough B and another in the service error table?"Suggestions (non-blocking)
The security checklist does not mention CORS. Given this is a session-cookie-based application (Spring Session JDBC), wildcard CORS with credentialed requests would be a real risk. A one-liner: "CORS: whitelist specific origins in
SecurityConfig— no@CrossOrigin(origins = \"*\")on session-based endpoints" would close this. It's the one item in the OWASP Top 10 that commonly appears in Spring Boot apps and is easy to misconfigure.The
@WebMvcTestslice examples do not show@Import({SecurityConfig.class, PermissionAspect.class})in the TDD samples. If a new contributor writes the controller test shown in Walkthrough B without importing the security config, the@RequirePermissionannotation will silently not fire — the 403 test will pass vacuously, giving false confidence that the endpoint is protected. The correct import should be included in the code example, or a note added: "IncludeSecurityConfig.classandPermissionAspect.classin your@WebMvcTestimports, or your@RequirePermissionannotation will not be enforced during testing."This is the one finding I'd call a near-blocker because it directly affects whether security tests are actually testing security. A contributor following this guide exactly would produce tests that prove nothing about authorization.
Mass assignment is not mentioned anywhere. In a Spring Boot app with
@RequestBodyand@Data-annotated DTOs, if a DTO exposes a field that should not be user-settable (e.g., astatusorcreatedByfield), a user can set it by sending extra JSON fields. A sentence noting that DTOs should only expose fields the user is permitted to set, and that@JsonIgnoreor separate input/output DTOs address this, would cover the gap.Summary
The near-blocker is the missing
@Importnote for@WebMvcTestsecurity tests. Everything else is suggestions. The security checklist structure is solid — it's one of the better developer-facing security references I've seen in a project this size.🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
My focus is on whether the testing guidance is complete, accurate, and will produce a trustworthy test suite. The TDD discipline is correct throughout, which is the biggest thing. But I have a few structural concerns.
What works well
@WebMvcTestis recommended over@SpringBootTestfor controller tests. That's correct and important — full context tests for controller logic are a common antipattern in Spring Boot projects.@ExtendWith(MockitoExtension.class)for service unit tests — explicitly named. Correct.Concerns
Concern 1 — No mention of Testcontainers (non-blocking but significant gap)
The test-type matrix lists
@WebMvcTestfor HTTP contract tests but does not mention integration tests at all. There is no row for "persistence layer test" or "full-stack integration test." A contributor adding a domain with a complex query, a Flyway migration, or a PostgreSQL-specific constraint (partial index, CHECK) has no guidance on how to test it. The absence will push contributors toward H2 or skipping persistence testing entirely — both produce false confidence. Suggest adding a row: Service + DB integration test |@SpringBootTest+ Testcontainers |@DataJpaTestwith real Postgres.Concern 2 — No mention of the quality gate thresholds
The guide tells contributors to run
./mvnw testandnpm run testbefore committing, but doesn't state what "passing" means beyond exit code 0. Is there a coverage threshold? Are there CI gates that will block the PR if coverage drops? New contributors often add features without tests, not out of laziness but because they don't know a gate exists. A sentence in Section 2: "CI enforces >= 80% branch coverage via JaCoCo for Java and Vitest coverage for TypeScript — PRs that drop below this threshold will not merge" would set expectations before submission.Concern 3 — Playwright test snippet missing
test.describe/beforeEachsetupThe Walkthrough C E2E example:
This hardcodes
persons/1which will fail in a clean CI database. The existing Playwright tests in this project use authentication setup and test-specific data. A contributor following this snippet literally will write a test that fails in CI but passes locally against seeded data. The example should either note that auth setup is required (pointing to an existing fixture), or use a more realistic pattern that mirrors what the existing E2E suite does.Concern 4 — No mention of
@DisabledpolicyThe guide says "run all tests green before committing" but doesn't mention what to do when a test is temporarily broken during development (before the feature is complete). Contributors will either skip the commit, leave broken tests, or use
@Disabled. The project should have a policy on this — even a one-liner: "Never commit@Disabledwithout a linked Gitea issue and a comment explaining why."Summary
No hard blockers, but the missing Testcontainers guidance and the fragile E2E snippet are the two findings most likely to produce real test quality problems. The TDD structure of the walkthroughs is correct. The gaps are in completeness, not accuracy.
🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
This is a documentation PR, not a UI change, so most of my usual concerns (touch targets, contrast, mobile layout) don't apply to the diff itself. What I do care about is whether the accessibility baseline documented here is complete and accurate — because this document will govern what contributors build.
What works well
min-h-[44px], focus rings withfocus-visible:ring-2 focus-visible:ring-brand-navy,aria-labelon icon-only buttons,aria-live="polite"on dynamic status messages — these are the four most commonly missed items in new pages. Getting them into the walkthrough means they arrive as a checklist, not as review feedback.brand-navy,brand-mint,brand-sandwith their correct hex values and usage guidance. The note that they are defined insrc/routes/layout.cssis correct (verified). The typography split (Merriweather for body, Montserrat for labels/UI) is also correctly stated.BackButton.svelteis documented as the correct pattern for back navigation instead of a static<a href>. The import path is verified correct.T12:00:00for date display is documented with the right pattern. Dates rendered without the noon offset look wrong to German users due to UTC timezone shifts — this saves a class of subtle date-off-by-one bugs that are hard to reproduce.Suggestions (non-blocking)
The accessibility baseline mentions
aria-live="polite"on dynamic status messages but does not mention the companion requirement: color is never the sole status indicator. This is in the Section 6 accessibility baseline checklist (the last bullet), which is good — but it's missing from the Walkthrough C step where the other a11y items are listed. A contributor reading only Walkthrough C would have an incomplete checklist. Suggest adding it to Step 6's bullet list for consistency.No mention of semantic HTML structure. The walkthrough instructs contributors to create a new page but doesn't mention wrapping the content in
<main>, using<article>for document content, or providing an<h1>. A new page without<main>won't integrate correctly with the screen reader navigation that users of the existing pages depend on. A one-liner: "Wrap page content in<main>and provide an<h1>heading" in Walkthrough C would close this.brand-minton white fails WCAG AA for normal text (2.8:1 contrast). The documentation correctly listsbrand-mintas an accent color for "hover underlines, icons" — not text. But a contributor reading the brand colors section might use it as a text color. The existing STYLEGUIDE.md covers this, but a small parenthetical "(accent only — fails contrast on white text)" next to the brand-mint entry in Section 6 would prevent the misuse inline.The accessibility guidance is more specific than most contributor guides I've reviewed. The gaps are omissions, not errors.