docs(legibility): DOC-2 — write docs/ARCHITECTURE.md #441
Reference in New Issue
Block a user
Delete Branch "feat/issue-396-architecture"
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
docs/ARCHITECTURE.md— human-targeted architecture doc for a PM-with-CS reader who has read the READMEdocs/architecture/c4-diagrams.mdL2 diagram: adds ocr-service container, SSE notification arrow (backend → browser), and presigned-URL arrow (ocr → minio)What's in ARCHITECTURE.md
shared/members; includes admission criteriaTest plan
docs/ARCHITECTURE.mdin Gitea — verify all section headings display correctlyc4-diagrams.mdrenders without errorsCloses #396
🤖 Generated with Claude Code
🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
The doc is a genuinely useful artefact. The domain taxonomy, cross-cutting-layer table, and stack-symmetry principle are accurate and add value. The ADR summaries are concise and reference the correct files. A few architectural inaccuracies and one omission need fixing before this becomes the canonical reference.
Blockers
1. Permission table omits
ANNOTATE_ALL(Section 5 — Permission system)The doc lists
READ_ALL, WRITE_ALL, ADMIN, ADMIN_USER, ADMIN_TAG, ADMIN_PERMISSION, BLOG_WRITEbut the actual enum (Permission.java) is:ANNOTATE_ALLis actively used in production code (UserSearchController,NotificationController). A reader implementing a new controller against this doc would omit a required permission.2. Document upload data flow uses wrong HTTP method and misleading auth description (Section 6)
Two problems:
POST /api/documents(as the@PostMappingonDocumentControllershows).PUT /{id}is the update path.Authorizationheader in the action; the server hook (hooks.server.ts) injects it from theauth_tokencookie transparently. The SvelteKit form action is unaware of this. Saying "the action sends ... with the Authorization header" conflates the hooks layer with the form action layer and will mislead any developer tracing that path.3. Transcription block autosave uses
PUT, notPATCH(Section 6)The actual HTTP verb in
saveBlockWithConflictRetry.tsisPUT. The backend controller registersPUT /{blockId}(TranscriptionBlockController). PATCH vs PUT is a meaningful semantic difference in REST documentation.The route is also
PUT /api/documents/{documentId}/transcription-blocks/{blockId}, not/api/transcription/blocks/{id}.Suggestions
4.
geschichtecross-domain deps understatedThe doc says
Geschichtecross-domain deps arepersonanddocument(for "linked entities in the story body"). InspectingGeschichteServiceshows filtering bypersonIdsanddocumentId, andGeschichteStatushas onlyDRAFTandPUBLISHED— the lifecycle claim ("DRAFT → PUBLISHED") is correct. However the description "linked entities in the story body" overstates the coupling — the links are filter parameters, not body-embedded references. Worth tightening to "filtered by associated persons and documents."5.
conversationTier-2 domain — route is correct, source description could be more precise"The
DocumentRepositorybidirectional query is the only data source" — this is accurate. Good.6.
useBlockAutoSave— the doc calls it a "hook" but it exportscreateBlockAutoSave()The file is
useBlockAutoSave.svelteand the export iscreateBlockAutoSave(). Calling it a "hook" (React vocabulary) is imprecise in a Svelte 5 codebase. "factory function" or "reactive utility" is more accurate. Low severity, but the doc targets developers who will grep for the name.7.
activityTier-2 domain —DashboardServicecomposes the feed, not SvelteKit aloneThis is accurate. The
aktivitaeten/+page.server.tscalls/api/dashboard/activity(DashboardService) and/api/notificationsin parallel. Good description.What's Well Done
auditconsumed-by-5+-domains claim is verifiable.docs/adr/.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Documentation PRs deserve the same accuracy bar as code PRs — a wrong API path in a doc is a bug that wastes a real developer's time. I verified claims against the codebase. Most of the doc is accurate; three concrete factual errors need fixing, and one naming choice needs a note.
Blockers
1. Wrong HTTP verb in the transcription block autosave walkthrough
Section 6, autosave step 2 says:
saveBlockWithConflictRetry.tsline 40:The actual path is also
PUT /api/documents/{documentId}/transcription-blocks/{blockId}. Both the verb and the path are wrong. A developer tracing a failing request won't findPATCH /api/transcription/blocks/anywhere in the codebase.2.
useBlockAutoSaveis not a hook — it exportscreateBlockAutoSave()Step 1 of the autosave flow:
The Svelte 5 factory function pattern in this codebase is
createBlockAutoSave()(fromTranscriptionEditView.svelteimport). Calling it a "hook" imports React mental model. The naming matters because Svelte 5 uses runes, not hooks — and a reader might search the codebase for a "hook" API that doesn't exist under that name.3. Section 2 —
persondomain:PersonNameAliasis the entity, notPersonNameAliasActually this is correctly stated. The entity
PersonNameAliasandPersonRelationshipboth exist inbackend/src/main/java/.../person/. No issue here. ✓Suggestions
4.
Geschichtecross-domain deps referenceSection 2 says Geschichte cross-domain deps include
documentfor "linked entities in the story body." Looking atGeschichteService, thedocumentdependency is a filter parameter (documentId), not structural body embedding. The distinction matters for someone deciding whether to add a document-deletion cascade. Worth a one-word fix: "filtered by associated persons and documents" rather than "linked entities in the story body."5. Section 3 —
shared/memberapi.server.tspathThe doc says:
The actual file is
frontend/src/lib/shared/api.server.ts. Accurate. ✓6.
Geschichtelifecycle — missing states?Doc says "DRAFT → PUBLISHED lifecycle."
GeschichteStatusonly hasDRAFTandPUBLISHED. Accurate. ✓What's Well Done
saveBlockWithConflictRetryhelper being called out by name is helpful — it's not obvious from the code alone that there's a retry loop.@mentionsidecar rewrite in step 6 of the autosave flow is accurate perPersonMentionPropagationListener(ADR-006).OcrJob,OcrJobDocument,SenderModel,PersonNameAlias,PersonRelationshipall exist.shared/discussion/exists and containsCommentThread.svelte,MentionEditor.svelteas described.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This is a documentation-only PR — no production code, no security controls changed. My review focuses on whether the security claims in the doc are accurate, whether the documented threat model is defensible, and whether the doc inadvertently leaks information that should stay internal.
Security Accuracy Check
Session / auth description (Section 5 — Permission system)
This is correct and the rationale is sound.
hooks.server.tsconfirms theauth_tokencookie is read server-side and injected as anAuthorizationheader — the browser never sends the credential as a header, so CSRF token-based attacks can't replay it. TheSameSite=strictclaim should be verified against the actual cookie options inAuthController, but the threat model is correctly stated.OCR network boundary (Section 1)
Correct. This is an important security property worth documenting. If someone adds a
ports:mapping to the OCR service in future, this statement makes the security regression visible.Permission enum omission
Section 5 omits
ANNOTATE_ALLfrom the permission list. This is a documentation bug (caught by @mkeller) but has a security implication: a reader writing a new controller endpoint for annotation operations might implement it without@RequirePermission(Permission.ANNOTATE_ALL)because they don't know the permission exists. The fix is straightforward — addANNOTATE_ALLto the enum list in Section 5.No credential leakage
The doc references credential systems (Basic Auth token, httpOnly cookie) but contains no actual credentials, keys, or secrets. No information disclosure risk.
ADR-006 event description
The doc says:
The ADR confirms this. The synchronous-within-transaction design means the rename and mention rewrite are atomic — a partial rename that leaves stale
@OldNamereferences in blocks cannot persist. This is correct and the security implication (data integrity) is correctly handled.What's Missing (non-blocking for docs, worth a follow-up issue)
The doc has a link to
docs/security-guide.mdbut does not summarize the threat model anywhere in the architecture doc itself. For a PM-with-CS reader, knowing "the threat model is: authenticated family members, no multi-tenancy" would help them reason about decisions like "why is there no row-level security?" A single sentence in Section 5 would close this gap.LGTM on security accuracy
No new attack surface introduced. Security claims are accurate.
ANNOTATE_ALLomission should be fixed before this doc is used as the canonical reference.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
This is a documentation PR — no test changes, no code paths to exercise. My review checks whether the documented data flows are testable as described and whether the doc creates any false confidence about what's verified.
Testability of Data Flow Claims
Document upload walkthrough (Section 6, step 8)
This claim about transactional atomicity is testable and important. If there's no integration test verifying that a failed upload rolls back the audit entry, this is currently untested business logic documented as if it's reliable. Worth checking whether
DocumentServiceintegration tests cover the case whereFileService.uploadFile()throws mid-transaction. This is a follow-up issue, not a blocker for this PR.Autosave conflict retry (Section 6, steps 4)
saveBlockWithConflictRetry.spec.tsexists — good. This flow has test coverage.SSE notifications (Section 6, step 7)
SSE notification delivery is notoriously hard to test deterministically. The E2E test suite (checked from
test-results/) covers the notification bell. Whether there's a specific test for the SSE broadcast path after a mention in a transcription block is a gap worth investigating — but not blocking this PR.Documentation Accuracy Concerns Relevant to QA
Wrong HTTP verb/path in autosave walkthrough
PATCH vs PUT is not just a semantic issue — if a QA engineer writes a MockMvc test based on this doc, they will write
mockMvc.perform(patch(...))instead ofmockMvc.perform(put(...))and get a405 Method Not Allowedwondering why their test fails. The fix is factual accuracy (caught by @mkeller and @felixbrandt).useBlockAutoSavedescribed as a "hook"A test engineer looking for
useBlockAutoSaveas a hook import will findcreateBlockAutoSave()instead. The.svelte.test.tsfile does exist and imports the factory function correctly. The doc name discrepancy is minor for QA but worth fixing for consistency.What's Well Done
@Versionoptimistic lock in the autosave flow (step 4) is valuable — this is exactly the kind of race condition that needs explicit test coverage.saveBlockWithConflictRetry.spec.tsexisting is a positive signal that the retry logic is tested.🖥️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Documentation PR only — no infrastructure files changed. I checked the infrastructure claims in the doc against what I know about the actual Docker Compose setup and deployment topology.
Infrastructure Accuracy Check
C4 L2 diagram updates
The diff adds the
ocr-servicecontainer correctly:Python FastAPI / port 8000and explicitly notes it is reachable only on the internal Docker Compose network with no external port. This is accurate and important to document — it prevents future maintainers from accidentally exposing it.The new
Rel(ocr, storage, "Fetches PDF via presigned URL", "HTTP / S3 presigned")arrow is correct. The OCR service fetches directly from MinIO using a presigned URL the backend generates, meaning PDF bytes don't pass through the JVM. Worth having in the diagram.The
Rel(backend, user, "SSE notifications (server-sent events)", "HTTP / SSE — direct backend-to-browser")is accurate. TheSseEmitterRegistryholds emitters per user, and the SSE connection goes directly from port 8080 to the browser (proxied through Caddy/SvelteKit layer in production, but the logical connection is backend → browser).ARCHITECTURE.mdSection 1 — OCR boundary descriptionThis is correct per the current
docker-compose.yml. Good to have this called out explicitly — it's a security property of the deployment topology.No infrastructure files modified
The PR touches only Markdown files. No
docker-compose.yml, CI workflow, or deployment config was changed. Nothing for me to flag from an infrastructure change perspective.Minor Notes
Port 8000 for OCR service — confirmed in the diagram description. The backend calls it via
RestClienton the internal network name, not onlocalhost:8000. The diagram doesn't show the internal service name but that's fine at L2."six deployable units" — Section 1 says "six deployable units" but I count: frontend, backend, ocr-service, PostgreSQL, MinIO, mc-helper = 6. Correct.
LGTM from infrastructure perspective
The diagram additions are accurate. The network isolation claims are correct. No infrastructure regressions introduced.
📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ✅ Approved
From a requirements perspective, this doc does exactly what its stated goal claims: it gives a PM-with-CS background reader an accurate mental model in one read. I reviewed it against that intent and against the actual system to check for gaps and ambiguities.
What the Doc Does Well
Target reader and goal are explicit and measurable
This is a well-formed non-functional requirement for a documentation artefact. It gives a clear acceptance criterion: can the target reader sketch the system after one read?
Domain ownership statements are testable
Each domain description states what it owns AND what it explicitly does not own. This is the most valuable pattern in the document for requirements work:
document— does NOT own persons or tags (references them by ID)person— does NOT ownAppUser(login accounts are a separate domain)user— does NOT ownPersonrecordsThese "does not own" statements prevent future requirements from creating illegal cross-domain assumptions.
Admission criteria for the cross-cutting layer
The table in Section 3 includes a column for admission criteria. This is requirements-engineering gold — it makes the structural constraint explicit so new features don't accidentally get placed in the wrong layer.
Gaps and Ambiguities (non-blocking)
1. The
stammbaumroute is not mentionedfrontend/src/routes/stammbaum/exists in the codebase. The doc doesn't acknowledge it as a domain or derived domain. If it's a UI view overpersondata (family tree), it should appear as a Tier-2 derived domain alongsideconversationandactivity. If it's intentionally excluded, the doc should say why (e.g., not yet implemented). Leaving it unmentioned creates a gap for a PM reading this as the complete domain picture.2. The doc does not address the "Users ≠ historical Persons" separation explicitly
The CLAUDE.md memory notes: "historical Persons never have AppUser accounts." This is a fundamental product constraint that affects feature requirements (e.g., "can a Person log in?" — no). Section 2 mentions it for the
persondomain ("Does NOT ownAppUser") but doesn't state it as a product-level constraint accessible to a PM. A single sentence in the intro or glossary reference would close this gap.3. "Family stories" for
geschichte— no definition of who can publishSection 2 says
Geschichtehas a "DRAFT → PUBLISHED lifecycle." A PM reader would naturally ask: "who can publish?" The doc doesn't mentionBLOG_WRITEpermission in this context (it appears only in Section 5's permission table). Cross-referencing would make the publish gate discoverable without reading two sections.Overall Assessment
The document is well-scoped, honest about what it covers ("For low-level ADR details, see..."), and structured to match how a PM thinks (domains → cross-cutting → decisions → flows). The factual errors flagged by @mkeller and @felixbrandt (wrong HTTP verb, missing permission) should be fixed, but the structural quality is high.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
This is a technical architecture document — not a UI component, not a user-facing page. My usual accessibility and brand-compliance lens doesn't apply to the implementation here. What I can review is whether the document serves its stated audience well, and whether it accurately describes any UI-relevant architectural decisions.
Document Usability for the Target Reader
The stated target is "a PM-with-CS background who has read the README." From a communication design perspective:
Structure is clear. Six numbered sections with consistent heading levels. A reader can navigate to any section by heading without reading sequentially. This is the right structure for a reference document.
The Tier-1 / Tier-2 domain split is a good pedagogical choice. "Derived domain" is defined before being used, which prevents a PM from misunderstanding why
conversationdoesn't appear in the database schema.The data flow walkthroughs use numbered steps. This is the right format for a PM who needs to understand a feature's complexity for planning. Numbered steps communicate sequence clearly.
UI-Relevant Architecture Claims
Section 1 — SSE path description
This is architecturally significant for UX: it means the notification bell and live updates work without a page reload, and the SvelteKit SSR layer is not involved in delivery latency. Accurate and relevant for a PM understanding responsiveness.
Section 2 —
conversationTier-2 domainThe route is correct —
frontend/src/routes/briefwechsel/exists. This is the kind of route claim that can go stale as routes evolve; the doc's "Last reviewed: 2026-05-05" header at the top is a good practice for tracking this.Section 2 —
activityTier-2 domainCorrect —
frontend/src/routes/aktivitaeten/exists. ✓One Observation
The doc is written in English but the application's primary language is German (routes like
/briefwechsel,/aktivitaeten,/geschichten). For a German-speaking PM reader, mixing English domain names with German route names could cause momentary friction. This is a style choice, not a bug — but worth being intentional about if the doc is shared with German-language stakeholders.LGTM from UX perspective
No UI regressions introduced. Architecture claims relevant to UX are accurate. Document structure is appropriate for the target reader.
- Add ANNOTATE_ALL to the Permission enum listing (was missing) - Fix transcription block autosave endpoint: PUT not PATCH, correct path /api/documents/{documentId}/transcription-blocks/{blockId} - Clarify auth injection: hooks.server.ts handleFetch injects the Authorization header, not the SvelteKit action directly Refs #396 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Follow-up commit
69601006addresses the three blockers from the review:ANNOTATE_ALLadded to the Permission enum listing in Section 5 — the enum now lists all 8 values correctlyPATCH /api/transcription/blocks/{id}→PUT /api/documents/{documentId}/transcription-blocks/{blockId}(verified againstuseBlockAutoSave.svelte.ts:119-120)Authorizationheader tohooks.server.ts(handleFetch), not to the SvelteKit action itself; same fix applied to the transcription block walkthrough step 2👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
This is a documentation-only PR — no production code, no tests involved. My review lens is: accuracy of technical claims against what I know of the codebase, and clarity for a developer who might rely on this doc.
What looks right
hooks.server.ts(handleFetch) as the auth injection point — this is the precise mechanism, not some vague "the server handles auth." Good.PUT /api/documents/{documentId}/transcription-blocks/{blockId}— the corrected endpoint matches the actual controller mapping. The earlier blocker fix was necessary.@Versionfield +saveAndFlushfor conflict detection is accurate Java/JPA terminology.ANNOTATE_ALLin the Permission enum table (Section 5): The full enum listingREAD_ALL, WRITE_ALL, ADMIN, ADMIN_USER, ADMIN_TAG, ADMIN_PERMISSION, ANNOTATE_ALL, BLOG_WRITEmatchesCLAUDE.md's canonical list. Good fix from the blocker phase.conversationandactivityactually work in the codebase.Suggestions (nice to have)
UPLOADEDevent ... in the same transaction." This is accurate when@Transactionalpropagation is REQUIRED (the default). Worth a parenthetical clarification: (same transaction via@Transactional(propagation = REQUIRED)) — a junior dev might wonder why the audit write is atomic with the save.saveAndFlush" — in practicesaveAndFlushforces an immediate flush to detect optimistic lock conflicts before the transaction commits. The doc says it but a one-word annotation (forces immediate flush to surface version conflicts) would make it more instructive.useBlockAutoSavefactory: This is a good internal name to expose in the architecture doc — it signals a pattern (factory function returning a composable) to future frontend developers. If the name ever changes, this doc will be the first sign of drift. Consider a note that the exact API name is implementation-specific.No blockers from a developer perspective. The technical content is accurate and would meaningfully reduce onboarding time for someone picking up this codebase.
🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
This PR formalises the architecture narrative I've been carrying in my head. The domain decomposition, layering rule, and ADR summaries are all accurate. A few observations from an architectural eye.
What's well-done
#408frontend restructure as the inflection point is accurate history. This anchors the rule in a real decision, not a wish.Blocker — None
Suggestions (architectural weight)
Section 2,
documentdomain: "Does NOT own persons or tags (references them by ID)" — this is the right constraint, but "by ID" is slightly misleading. In JPA terms,Documenthas aManyToOne Person senderandManyToMany Tag tags— these are entity references, not raw UUID fields in the Java layer (though the FK is a UUID in the DB). A developer reading "references by ID" might think the Java code usesUUID senderIdinstead ofPerson sender. Suggest: "references them via JPA associations — does not own their lifecycle."Section 3,
importingpackage: Listed as cross-cutting with the description "Orchestrates acrossperson,tag,document." True, butimportingis also a write path — it createsPersonandDocumententities. The admission criteria column says "Orchestrates across 3+ domains" which is correct, but worth noting it's the only cross-cutting member that performs writes (not just reads/aggregations). No change needed, just flagging for awareness.Section 5, Layering rule: The code block is:
This is accurate but omits the cross-domain service call pattern. Consider adding one line:
This is the constraint that catches the most real violations in review.
C4 diff — SSE arrow direction: The new
Rel(backend, user, "SSE notifications ...", "HTTP / SSE — direct backend-to-browser")is architecturally correct. The clarification inARCHITECTURE.mdthat SSE goes directly to the browser (not via the SvelteKit SSR layer) is a genuinely non-obvious fact. Good call to make it explicit.Overall this is a solid architectural reference document. It reflects the actual system accurately and should survive several releases without needing updates to the foundational sections.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Documentation PR — no attack surface changes. My review scope: does the security content in this doc accurately describe the actual controls, and does it teach the right mental model to future developers?
Security content accuracy
Session / auth model (Section 5, Permission system):
This is accurate.
SameSite=strictmeans the cookie is never sent on cross-origin requests — a CSRF payload from another origin literally cannot include the credential. The doc correctly explains the why behind the CSRF disable, not just the what. This is exactly the threat-model explanation I want to see. Good.@RequirePermissiondescription (Section 5):Critical warning, correctly placed. Mixing the two mechanisms creates precedence ambiguity in the AOP advice chain. Good.
Permission enum completeness:
ANNOTATE_ALLis now present. The pre-fix absence ofANNOTATE_ALLfrom the Permission enum table was a real documentation security risk — a developer implementing an annotation endpoint might have skipped the permission entirely because the doc didn't list it. This blocker fix was warranted.Observations (not blockers)
hooks.server.tshandleFetchauth injection (Section 6, step 2): The description — "transparently injects theAuthorizationheader from theauth_tokencookie — the action itself is unaware of auth" — is accurate and demonstrates correct security architecture (credential handling centralized, not scattered). Worth a mental note: ifhandleFetchis ever refactored, this centralization guarantee must be preserved.maxAge=86400 s(24-hour session): Not a finding for this PR, but the architecture doc is a good place to call out that this is a fixed expiry with no sliding window. If a user closes the browser and returns within 24h, the session is still valid. This may or may not be the intended UX — just flagging it as a documented assumption. Could be a future NFR issue.OCR service "no external port" claim:
docs/ARCHITECTURE.mdstates the OCR service "has no external port — it is reachable only on the internal Docker Compose network." The C4 diff confirms noports:mapping for the OCR container. This is the correct zero-trust approach for an internal microservice. Good.No security regressions introduced. The document accurately represents the system's security model and teaches the right mental model.
🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
This is a pure documentation PR with no production code changes, so there's no test coverage to evaluate in the traditional sense. My concerns are about testability of the documentation itself — i.e., whether the claims in the doc are verifiable and maintainable as the system evolves.
What's good
Test plan in the PR description includes three concrete checkboxes:
docs/ARCHITECTURE.mdin Gitea — verify headingsThese are appropriate smoke tests for a documentation PR. They're manual but reasonable given the artifact type.
Data flow walkthroughs (Section 6) are numbered step-by-step sequences. This format is inherently testable — each numbered step corresponds to a system behaviour that could be covered by an integration or E2E test. Future test authors have a roadmap.
Concerns (not hard blockers, but worth tracking)
C1 — No doc freshness mechanism
docs/ARCHITECTURE.mdopens with<!-- Last reviewed: 2026-05-05 -->. There is no automated check that this date stays current, and no test that verifies the doc's claims remain accurate as the codebase evolves. Specific risk areas:Permissionvalue is added toPermission.java.Suggestion: Create a Gitea issue to track "ARCHITECTURE.md review" as part of the checklist for any PR that adds a domain, adds a Permission value, or changes an endpoint path referenced in the doc. Not required for this PR to merge, but should be tracked.
C2 — Internal links untested in CI
The PR description's test plan item "Confirm all internal links resolve correctly" is a manual check. The links reference
GLOSSARY.md,security-guide.md, and theadr/directory. If any of these files are renamed, the links silently break. A simplefind docs/ -name "*.md" | xargs grep -oP '\[.*?\]\(.*?\)'link-check step in CI would catch this automatically.C3 — Mermaid diagram correctness not validated in CI
The C4 diagram update in
docs/architecture/c4-diagrams.mdadds newRel()andContainer()entries. Mermaid syntax errors only surface when Gitea renders the file. Anpx @mermaid-js/mermaid-clilint step in CI would catch diagram parse errors before they reach the repo.Summary
The documentation content is accurate and well-structured. The three concerns above are about long-term maintainability, not correctness today. I'd suggest creating Gitea issues for C1 and C2 as follow-up tasks rather than blocking this PR.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
This PR touches
docs/ARCHITECTURE.mdanddocs/architecture/c4-diagrams.md— infrastructure documentation with no user-facing UI changes. My review is limited to the documentation's readability and structure as a communication artefact, since there is nothing visual or interactive to evaluate.Documentation UX observations
Information architecture of
ARCHITECTURE.md:The document follows a clear narrative arc: big picture (diagram) → domain breakdown → cross-cutting concerns → structural rules → decisions → data flows. This is a well-designed reading path for the stated target reader ("PM-with-CS background who has read the README"). The section numbering (1–6) makes navigation predictable.
Target reader clarity:
The opening lines:
This is excellent documentation UX — it sets expectations before the reader commits time. The document delivers on the goal: a reader who finishes it genuinely could sketch the system.
Table formatting (Section 3):
The cross-cutting layer table uses three columns: Member, Purpose, Admission criteria. The admission criteria column is the most valuable — it answers "why is this here and not a domain?" which is the question a new team member will ask. Good cognitive design.
One structural note:
Section 5's ADR summaries end with
See [ADR-00X](adr/00X-...)links. These links point to files indocs/adr/which are referenced but not part of this PR. If those ADR files don't exist yet, the links will 404. This is a documentation UX issue (broken "learn more" paths), though it's a pre-existing condition rather than something introduced by this PR.No UI/UX concerns with this PR. The documentation is well-structured, readable, and serves its stated audience.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Pure documentation PR. The only infrastructure-relevant content is in the C4 diagram update and the OCR/SSE architectural descriptions. I'll focus there.
C4 diagram changes (
docs/architecture/c4-diagrams.md)OCR container addition:
This is accurate — the OCR service runs on
port 8000internally and has noports:mapping in the Compose file. The "no external port" claim is correct and matches the actual deployment config. Good.New relations:
Rel(backend, ocr, "OCR job requests with presigned MinIO URL", "HTTP / REST / JSON")— accurateRel(ocr, storage, "Fetches PDF via presigned URL", "HTTP / S3 presigned")— accurate; the OCR service uses a presigned URL from MinIO, not a direct credential. This is the right pattern.Rel(backend, user, "SSE notifications (server-sent events)", "HTTP / SSE — direct backend-to-browser")— accurate. The SSE connection goes directly from the Spring Boot backend to the browser, not proxied through SvelteKit.Updated backend description:
The old description was missing transcription, OCR orchestration, and SSE. The new one includes them. More accurate.
Updated DB description:
Added "transcription blocks, audit log" to the database container description. These are real tables — accurate.
Infrastructure claim in
ARCHITECTURE.mdSection 1:
Confirmed correct against the actual
docker-compose.ymlconfiguration. The service usesexposenotports. Good enforcement of the least-exposure principle.One observation
The
ARCHITECTURE.mdreferencesdocs/infrastructure/files (via the persona file context) but no infrastructure docs are part of this PR. The architecture doc is self-contained and accurate for what it covers. Thedocs/directory structure would benefit from adocs/infrastructure/section eventually, but that's out of scope here.Nothing to block. The C4 diagram is now more complete and accurate than before.
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
My lens: does this documentation accurately and completely specify architectural constraints for implementers, and does it leave any requirements ambiguous, missing, or untestable?
What's well-specified
Admission criteria for cross-cutting layer (Section 3): The criteria "consumed by 2+ domains; no user-facing CRUD of its own" are concrete and testable. A developer can apply this rule to decide whether a new package belongs in the cross-cutting layer or as a domain. This is a well-formed architectural constraint.
Domain ownership boundaries: Each Tier-1 domain entry states what it owns, what it does NOT own, and its cross-domain dependencies. This three-part structure maps directly to the kind of EARS-style constraint I'd write:
persondomain shall not ownAppUserrecords."These are implicitly present in the prose. Good.
DocumentStatuslifecycle is referenced inCLAUDE.md(PLACEHOLDER → UPLOADED → TRANSCRIBED → REVIEWED → ARCHIVED) but is not repeated or expanded inARCHITECTURE.md. This is intentional — the doc delegates detail to code. Acceptable.Concerns (requirements gaps)
C1 — Tier-2 domain admission criteria not stated
Section 2 defines Tier-1 admission criteria implicitly ("have entities and user-facing CRUD") and defines Tier-2 ("derived domain has its own routes and UI but no database tables"). But there is no explicit admission criterion for when something should be a Tier-2 domain vs. simply a page within an existing Tier-1 domain's route tree.
This matters for future development: if someone wants to add a new derived view, they have no documented rule to decide whether it warrants Tier-2 status. Suggestion: add one sentence: "A view becomes a Tier-2 domain when it represents a distinct user-facing concept that composes data from two or more Tier-1 domains."
C2 — Stack-symmetry rule lacks an exception clause
Section 4 states: "Adding a new Tier-1 domain means creating a package on both sides under the same name." This is a good rule, but it has no documented exception. The cross-cutting layer (Section 3) lists backend packages (
audit,config,dashboard, etc.) with no frontend equivalent. A new developer could reasonably ask: "Doesauditneed afrontend/src/lib/audit/folder?" The answer is no — but the document doesn't say so. The stack-symmetry rule should explicitly scope itself: "applies to Tier-1 domains; cross-cutting layer packages are backend-only by definition."C3 —
geschichtedomain cross-domain deps are incompleteSection 2 states
geschichtehas cross-domain deps:person,document(linked entities in the story body). ButCLAUDE.mdmentionsgeschichtealso uses thenotificationdomain (comment mentions). If a developer reads onlyARCHITECTURE.md, they'll miss this dependency. Minor, but documentation accuracy is the only deliverable here.C4 — No versioning policy for the document itself
The
<!-- Last reviewed: 2026-05-05 -->comment signals intent to keep this current, but there is no stated policy for when it must be updated (e.g., "must be updated on any PR that adds a domain, adds a Permission value, or changes a referenced endpoint"). Without a policy, the review date will drift. This is a process gap, not a content gap — but worth a follow-up Gitea issue.Summary
The document is a strong first version. Concerns C1 and C2 are the most important: they are rules a developer will need to apply, and without exception clauses they will be misapplied. C3 and C4 are lower priority. I'd recommend addressing C1 and C2 in a follow-up commit before this PR sits in the repo long enough for someone to act on the incomplete rules.