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

epic(legibility): big-bang restructure — backend layer→domain, frontend lib→domain #406

New Issue
Closed
opened 2026-05-04 16:12:31 +02:00 by marcel · 8 comments
marcel commented 2026-05-04 16:12:31 +02:00
Owner
Copy Link

Epic context

This is Epic 4 of 5 for the Codebase Legibility Refactor (see #387).

This epic contains the actual structural change. Two large mechanical PRs (one per stack) restructure the codebase from layer-based to domain-based packaging. Domain names are stack-symmetric per the canonical domain set in #387 first comment.

This is gated by Epic 1 (Audit, #387) and Epic 3 (Pre-flight, #402). Do not start REFACTOR-1 or REFACTOR-2 until:

  1. AUDIT-6 (#393) verdict §5 says "🟢 ready for refactor" or "🟡 ready after these prerequisites" with prerequisites met
  2. Epic 3's pre-flight is complete — the test suite is the safety net

Scope

  • REFACTOR-1: Backend controller/, service/, repository/, model/, dto/, exception/, security/, config/ → per-domain packages + shared/
  • REFACTOR-2: Frontend flat lib/components/, lib/utils/, lib/hooks/, etc. → per-domain folders + lib/shared/
  • REFACTOR-3: Add ArchUnit (or equivalent) test enforcing backend domain boundaries (no controller→repo, no cross-domain repo)
  • REFACTOR-4: Add ESLint rule (or convention test) enforcing frontend domain boundaries (no cross-domain imports except via shared)

Acceptance criteria

  • All 4 child issues are closed
  • Backend package layout matches the canonical domain set (Tier 1: document, person, tag, user, geschichte, notification, ocr; cross-cutting: shared/) per REFACTOR-1
  • Frontend lib/ layout matches and mirrors backend domain names per REFACTOR-2
  • ArchUnit test prevents controller→repo direct injection AND cross-domain repo access (REFACTOR-3)
  • ESLint or equivalent prevents cross-domain frontend imports (REFACTOR-4)
  • Full backend test suite green throughout (./mvnw test)
  • Full frontend test suite green throughout (npm run test and npx playwright test)
  • Rubric checks C3.4, C3.5, C3.6, C4.1, C6.1, C6.2, C6.3, C6.4 all PASS post-refactor

Definition of Done

This epic closes when REFACTOR-1, -2, -3, -4 are all closed AND a follow-up audit re-run (CLEANUP-5 in Epic 5) confirms the rubric checks above PASS.

Dependency

  • Blocked by: AUDIT-6 verdict (#393); Epic 3 (#402) completion
  • Blocks: DOC-6 (#400) per-domain READMEs; DOC-7 (#401) CLAUDE.md migration; CLEANUP-5 (final scorecard)
## Epic context This is **Epic 4 of 5** for the *Codebase Legibility Refactor* (see #387). This epic contains the actual structural change. **Two large mechanical PRs** (one per stack) restructure the codebase from layer-based to domain-based packaging. Domain names are stack-symmetric per the canonical domain set in #387 first comment. This is gated by **Epic 1 (Audit, #387)** and **Epic 3 (Pre-flight, #402)**. Do not start REFACTOR-1 or REFACTOR-2 until: 1. AUDIT-6 (#393) verdict §5 says "🟢 ready for refactor" or "🟡 ready after these prerequisites" with prerequisites met 2. Epic 3's pre-flight is complete — the test suite is the safety net ## Scope - **REFACTOR-1**: Backend `controller/`, `service/`, `repository/`, `model/`, `dto/`, `exception/`, `security/`, `config/` → per-domain packages + `shared/` - **REFACTOR-2**: Frontend flat `lib/components/`, `lib/utils/`, `lib/hooks/`, etc. → per-domain folders + `lib/shared/` - **REFACTOR-3**: Add ArchUnit (or equivalent) test enforcing backend domain boundaries (no controller→repo, no cross-domain repo) - **REFACTOR-4**: Add ESLint rule (or convention test) enforcing frontend domain boundaries (no cross-domain imports except via shared) ## Acceptance criteria - [ ] All 4 child issues are closed - [ ] Backend package layout matches the canonical domain set (Tier 1: document, person, tag, user, geschichte, notification, ocr; cross-cutting: shared/) per REFACTOR-1 - [ ] Frontend `lib/` layout matches and mirrors backend domain names per REFACTOR-2 - [ ] ArchUnit test prevents controller→repo direct injection AND cross-domain repo access (REFACTOR-3) - [ ] ESLint or equivalent prevents cross-domain frontend imports (REFACTOR-4) - [ ] Full backend test suite green throughout (`./mvnw test`) - [ ] Full frontend test suite green throughout (`npm run test` and `npx playwright test`) - [ ] Rubric checks C3.4, C3.5, C3.6, C4.1, C6.1, C6.2, C6.3, C6.4 all PASS post-refactor ## Definition of Done This epic closes when REFACTOR-1, -2, -3, -4 are all closed AND a follow-up audit re-run (CLEANUP-5 in Epic 5) confirms the rubric checks above PASS. ## Dependency - **Blocked by:** AUDIT-6 verdict (#393); Epic 3 (#402) completion - **Blocks:** DOC-6 (#400) per-domain READMEs; DOC-7 (#401) CLAUDE.md migration; CLEANUP-5 (final scorecard)
marcel added this to the Codebase Legibility milestone 2026-05-04 16:12:31 +02:00
marcel added the epiclegibilityrefactor labels 2026-05-04 16:12:50 +02:00
marcel commented 2026-05-04 21:05:16 +02:00
Author
Owner
Copy Link

🏗️ Markus Keller — Senior Application Architect

Observations

  • The direction is correct and overdue. With 212 production Java files in flat layer packages (controller/, service/, repository/, model/, dto/), a feature change for geschichte currently touches 5 packages. After the refactor a developer goes to geschichte/ and finds everything. That's the right end state.
  • The backend already has two domain-organized packages: audit/ and relationship/. Those prove the pattern works and should be the reference for REFACTOR-1.
  • The dashboard/ package is also partially domain-organized (DashboardController, DashboardService, DTOs co-located). This is evidence the team has already been organically moving toward domain packaging — REFACTOR-1 formalizes what's already happening.
  • The shared/ category in the scope is the critical design decision. Looking at the current codebase, candidates are: exception/, security/, config/, and cross-cutting DTOs. GlobalExceptionHandler and SecurityConfig clearly belong in shared/. DomainException and ErrorCode belong in shared/exception/.
  • Cross-domain repository access exists today and ArchUnit (REFACTOR-3) will flag it on day one. Specific cases to resolve before or during REFACTOR-1:
    • MassImportService injects DocumentRepository directly — after the refactor it would be document.DocumentRepository accessed from massimport/ or document/. This is acceptable if MassImportService is placed in the document/ domain.
    • SegmentationTrainingExportService injects AnnotationRepository, DocumentRepository, and TranscriptionBlockRepository — this is a genuine cross-domain service; it belongs in ocr/ and must call the other domains via their service interfaces.
    • SenderModelService injects TranscriptionBlockRepository — cross-domain; resolve by calling TranscriptionService instead or moving SenderModel into the ocr/ domain entirely.
  • The TranscriptionQueueService injecting DocumentRepository is the most dangerous pattern for ArchUnit — after the refactor it will immediately fail the transcription → document.DocumentRepository rule. Plan this resolution explicitly before writing the ArchUnit test.

Recommendations

  • Define the shared/ boundary criteria before writing a line of code. Rule: a class goes in shared/ if and only if it is imported by ≥ 3 domains with no owning domain. DomainException, ErrorCode, Permission, SecurityConfig, AsyncConfig, WebConfig qualify. Entities that appear in cross-domain relationships (e.g. Person referenced from Document) do NOT go in shared/ — they stay in their owning domain and the referencing domain gets a summary projection.
  • Treat REFACTOR-1 and REFACTOR-3 as one PR, not two. Write the ArchUnit test first (red), then move the packages until it's green. This is TDD applied to architecture. Writing ArchUnit after the refactor gives you false confidence that the new structure is already correct.
  • The canonical domain set in #387 comment needs a domain assignment table before any file moves. Produce a table: Class → Current package → Target domain package. This is the migration map and prevents mid-PR confusion about where OcrJobDocument goes.
  • For the frontend REFACTOR-2, the lib/components/ flat directory has 81 of 102 components unorganized. The document/ and chronik/ subdirectories already exist and are working correctly. Mirror those as the pattern. Note: import paths change, so all routes/ files that import from $lib/components/SomeThing will need updating — that's the actual work, not the moves themselves.

Open Decisions

  • Where does MassImportService live after the refactor? It touches document/, person/, tag/, and file/ domains. Options: (a) put it in document/ since that's the primary output — and accept it calls PersonService, TagService via their service interfaces; (b) create an import/ domain. Option (a) is simpler and correct under the rules — MassImportService creates Documents; the calls to PersonService and TagService are already going through service interfaces.
## 🏗️ Markus Keller — Senior Application Architect ### Observations - The direction is correct and overdue. With 212 production Java files in flat layer packages (`controller/`, `service/`, `repository/`, `model/`, `dto/`), a feature change for `geschichte` currently touches 5 packages. After the refactor a developer goes to `geschichte/` and finds everything. That's the right end state. - The backend already has two domain-organized packages: `audit/` and `relationship/`. Those prove the pattern works and should be the reference for REFACTOR-1. - The `dashboard/` package is also partially domain-organized (DashboardController, DashboardService, DTOs co-located). This is evidence the team has already been organically moving toward domain packaging — REFACTOR-1 formalizes what's already happening. - The `shared/` category in the scope is the critical design decision. Looking at the current codebase, candidates are: `exception/`, `security/`, `config/`, and cross-cutting DTOs. `GlobalExceptionHandler` and `SecurityConfig` clearly belong in `shared/`. `DomainException` and `ErrorCode` belong in `shared/exception/`. - **Cross-domain repository access exists today** and ArchUnit (REFACTOR-3) will flag it on day one. Specific cases to resolve before or during REFACTOR-1: - `MassImportService` injects `DocumentRepository` directly — after the refactor it would be `document.DocumentRepository` accessed from `massimport/` or `document/`. This is acceptable if `MassImportService` is placed in the `document/` domain. - `SegmentationTrainingExportService` injects `AnnotationRepository`, `DocumentRepository`, and `TranscriptionBlockRepository` — this is a genuine cross-domain service; it belongs in `ocr/` and must call the other domains via their service interfaces. - `SenderModelService` injects `TranscriptionBlockRepository` — cross-domain; resolve by calling `TranscriptionService` instead or moving `SenderModel` into the `ocr/` domain entirely. - The `TranscriptionQueueService` injecting `DocumentRepository` is the most dangerous pattern for ArchUnit — after the refactor it will immediately fail the `transcription` → `document.DocumentRepository` rule. Plan this resolution explicitly before writing the ArchUnit test. ### Recommendations - **Define the `shared/` boundary criteria before writing a line of code.** Rule: a class goes in `shared/` if and only if it is imported by ≥ 3 domains with no owning domain. `DomainException`, `ErrorCode`, `Permission`, `SecurityConfig`, `AsyncConfig`, `WebConfig` qualify. Entities that appear in cross-domain relationships (e.g. `Person` referenced from `Document`) do NOT go in `shared/` — they stay in their owning domain and the referencing domain gets a summary projection. - **Treat REFACTOR-1 and REFACTOR-3 as one PR, not two.** Write the ArchUnit test first (red), then move the packages until it's green. This is TDD applied to architecture. Writing ArchUnit after the refactor gives you false confidence that the new structure is already correct. - **The canonical domain set in #387 comment needs a domain assignment table** before any file moves. Produce a table: `Class → Current package → Target domain package`. This is the migration map and prevents mid-PR confusion about where `OcrJobDocument` goes. - For the frontend REFACTOR-2, the `lib/components/` flat directory has 81 of 102 components unorganized. The `document/` and `chronik/` subdirectories already exist and are working correctly. Mirror those as the pattern. Note: `import` paths change, so all `routes/` files that import from `$lib/components/SomeThing` will need updating — that's the actual work, not the moves themselves. ### Open Decisions - **Where does `MassImportService` live after the refactor?** It touches `document/`, `person/`, `tag/`, and `file/` domains. Options: (a) put it in `document/` since that's the primary output — and accept it calls `PersonService`, `TagService` via their service interfaces; (b) create an `import/` domain. Option (a) is simpler and correct under the rules — `MassImportService` creates Documents; the calls to `PersonService` and `TagService` are already going through service interfaces.
marcel commented 2026-05-04 21:05:43 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

  • REFACTOR-1 scope is larger than it reads. The backend has 19 controllers, 37 services, 24 repositories, 35 models, and 54 DTOs — a total of ~169 class files in layer packages, plus all test mirrors. Every test file in src/test/java/.../controller/, .../service/, .../model/, .../dto/ has to move and update its package declaration. The test side is the hidden work here.
  • The test suite is already well-organized by layer, not domain. DocumentServiceTest, DocumentControllerTest, PersonServiceIntegrationTest etc. are in test/service/ and test/controller/ — not co-located with their production code. After REFACTOR-1, the target should be co-location: document/DocumentServiceTest.java next to document/DocumentService.java. This is the right time to do it, and skipping it means losing the legibility benefit for tests.
  • REFACTOR-2 frontend: 81 out of 102 Svelte components are still flat in lib/components/. The document/ and chronik/ subdirectories already exist as working patterns. The work is moving those 81 flat components into their domain subdirectories and updating all import statements in routes/**. A global search-and-replace is safe since all imports use the $lib/components/ComponentName form.
  • REFACTOR-4 (ESLint domain boundary rule): The current eslint.config.js has no import-boundary rules. The eslint-plugin-import or a custom no-restricted-imports rule can enforce lib/components/document/* not being imported from lib/components/person/*. However, this is harder to configure precisely than ArchUnit. A simpler approach: use ESLint no-restricted-imports with per-domain deny-lists targeting the specific known violations rather than a blanket rule.

Recommendations

  • Backend: use IntelliJ's "Move" refactor (or mvn package renames) rather than manual sed. Each class must get its package declaration updated and every import referencing it updated. IntelliJ handles this automatically and is safer than a find-replace script.
  • Do REFACTOR-1 as a single commit per domain (one commit moves all document/* classes, next commit moves person/*, etc.) rather than one giant commit. Smaller commits are easier to review and easier to bisect if something breaks. The test suite gates each commit.
  • For REFACTOR-2, create the domain subdirectories first and move components one domain at a time. Order by number of existing test files (lowest first) to de-risk early moves.
  • Proposed REFACTOR-4 approach: use eslint-plugin-import no-restricted-paths rule to prevent lib/components/personDomain/* importing from lib/components/documentDomain/*. This is simpler than a full custom plugin and can be extended incrementally as new domains stabilize.
  • Name the REFACTOR-2 convention now: lib/components/[domain]/ or lib/[domain]/components/? The issue says lib/shared/ for cross-cutting — clarify whether the domain components live at lib/components/document/ (shallow) or lib/document/components/ (deep). Shallow is already the established pattern (lib/components/document/ exists) — stick with it for consistency.

Open Decisions

  • Test co-location in REFACTOR-1: should test files move into domain packages alongside production code (e.g. document/DocumentServiceTest.java), or stay in a parallel test package tree? Co-location is cleaner, but it is additional scope. The issue doesn't specify. This should be decided before starting so the PR doesn't turn into a partial migration.
## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - **REFACTOR-1 scope is larger than it reads.** The backend has 19 controllers, 37 services, 24 repositories, 35 models, and 54 DTOs — a total of ~169 class files in layer packages, plus all test mirrors. Every test file in `src/test/java/.../controller/`, `.../service/`, `.../model/`, `.../dto/` has to move and update its `package` declaration. The test side is the hidden work here. - **The test suite is already well-organized by layer**, not domain. `DocumentServiceTest`, `DocumentControllerTest`, `PersonServiceIntegrationTest` etc. are in `test/service/` and `test/controller/` — not co-located with their production code. After REFACTOR-1, the target should be co-location: `document/DocumentServiceTest.java` next to `document/DocumentService.java`. This is the right time to do it, and skipping it means losing the legibility benefit for tests. - **REFACTOR-2 frontend: 81 out of 102 Svelte components are still flat** in `lib/components/`. The `document/` and `chronik/` subdirectories already exist as working patterns. The work is moving those 81 flat components into their domain subdirectories and updating all `import` statements in `routes/**`. A global search-and-replace is safe since all imports use the `$lib/components/ComponentName` form. - **REFACTOR-4 (ESLint domain boundary rule)**: The current `eslint.config.js` has no import-boundary rules. The `eslint-plugin-import` or a custom `no-restricted-imports` rule can enforce `lib/components/document/*` not being imported from `lib/components/person/*`. However, this is harder to configure precisely than ArchUnit. A simpler approach: use ESLint `no-restricted-imports` with per-domain deny-lists targeting the specific known violations rather than a blanket rule. ### Recommendations - **Backend: use IntelliJ's "Move" refactor (or `mvn` package renames) rather than manual `sed`**. Each class must get its `package` declaration updated and every import referencing it updated. IntelliJ handles this automatically and is safer than a find-replace script. - **Do REFACTOR-1 as a single commit per domain** (one commit moves all `document/*` classes, next commit moves `person/*`, etc.) rather than one giant commit. Smaller commits are easier to review and easier to bisect if something breaks. The test suite gates each commit. - **For REFACTOR-2, create the domain subdirectories first** and move components one domain at a time. Order by number of existing test files (lowest first) to de-risk early moves. - **Proposed REFACTOR-4 approach**: use `eslint-plugin-import` `no-restricted-paths` rule to prevent `lib/components/personDomain/*` importing from `lib/components/documentDomain/*`. This is simpler than a full custom plugin and can be extended incrementally as new domains stabilize. - **Name the REFACTOR-2 convention now**: `lib/components/[domain]/` or `lib/[domain]/components/`? The issue says `lib/shared/` for cross-cutting — clarify whether the domain components live at `lib/components/document/` (shallow) or `lib/document/components/` (deep). Shallow is already the established pattern (`lib/components/document/` exists) — stick with it for consistency. ### Open Decisions - **Test co-location in REFACTOR-1**: should test files move into domain packages alongside production code (e.g. `document/DocumentServiceTest.java`), or stay in a parallel test package tree? Co-location is cleaner, but it is additional scope. The issue doesn't specify. This should be decided before starting so the PR doesn't turn into a partial migration.
marcel commented 2026-05-04 21:06:12 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

  • This is a structural refactor with no functional changes. From a security perspective, the primary concern is: does the refactor accidentally break the security boundary enforcement? The answer depends on how SecurityConfig, PermissionAspect, and @RequirePermission are moved.
  • SecurityConfig and PermissionAspect must land in shared/security/ — not in any domain package. If they end up in, say, user/ for convenience, any import from user/ package becomes a transitive dependency for every domain that uses @RequirePermission. That's incorrect coupling.
  • @RequirePermission and Permission enum are referenced by every controller. They must be in shared/security/. Their package change means every controller's import block changes — confirm the security annotations still resolve correctly after the package rename.
  • The SecurityConfig currently lives in config/ — it references CustomUserDetailsService which lives in service/. After refactoring: CustomUserDetailsService should move to user/ (it's user-domain logic), and SecurityConfig in shared/security/ will need to import from user/. This is a clean direction but requires explicit attention.
  • AuthE2EController is a controller that exists only for E2E test auth flows. It must stay behind a profile guard (@Profile("e2e") or equivalent). Confirm this guard is not accidentally removed during the refactor. It currently lives in controller/ — moving it to auth/ or user/ domain is correct, but double-check the profile annotation survives.
  • The RateLimitInterceptor in config/ is a security control. It should move to shared/security/ or shared/config/ to signal its security intent, not get buried in a domain package.

Recommendations

  • Produce an explicit list of security-critical classes and their target packages before writing code: SecurityConfig → shared/security/, PermissionAspect → shared/security/, Permission → shared/security/, RateLimitInterceptor → shared/security/, CustomUserDetailsService → user/, AuthController → auth/ or user/, AuthE2EController → user/ with @Profile("e2e") confirmed.
  • Add one @WebMvcTest slice test after the refactor that verifies @RequirePermission(Permission.WRITE_ALL) is still enforced on a write endpoint. This is a 5-line test and confirms the AOP wiring survived the package move. Specifically: import the post-move SecurityConfig and PermissionAspect in the test's @Import({...}) and verify a READ_ALL user gets 403 on a DELETE endpoint.
  • Do not move GlobalExceptionHandler into any domain. It belongs in shared/ and maps DomainException → HTTP responses for the entire application. Moving it into, say, document/ would be semantically wrong.
  • For REFACTOR-4 (ESLint boundary rule), ensure the rule explicitly blocks domain components from importing security utilities directly — only shared/ exports should be accessible cross-domain. This isn't a common violation today but the rule should prevent future drift.

No critical security vulnerabilities are introduced by this refactor if the above is followed. No open decisions from security — the placements are clear.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - This is a structural refactor with no functional changes. From a security perspective, the primary concern is: **does the refactor accidentally break the security boundary enforcement?** The answer depends on how `SecurityConfig`, `PermissionAspect`, and `@RequirePermission` are moved. - **`SecurityConfig` and `PermissionAspect` must land in `shared/security/`** — not in any domain package. If they end up in, say, `user/` for convenience, any import from `user/` package becomes a transitive dependency for every domain that uses `@RequirePermission`. That's incorrect coupling. - **`@RequirePermission` and `Permission` enum** are referenced by every controller. They must be in `shared/security/`. Their package change means every controller's import block changes — confirm the security annotations still resolve correctly after the package rename. - The `SecurityConfig` currently lives in `config/` — it references `CustomUserDetailsService` which lives in `service/`. After refactoring: `CustomUserDetailsService` should move to `user/` (it's user-domain logic), and `SecurityConfig` in `shared/security/` will need to import from `user/`. This is a clean direction but requires explicit attention. - **`AuthE2EController`** is a controller that exists only for E2E test auth flows. It must stay behind a profile guard (`@Profile("e2e")` or equivalent). Confirm this guard is not accidentally removed during the refactor. It currently lives in `controller/` — moving it to `auth/` or `user/` domain is correct, but double-check the profile annotation survives. - The `RateLimitInterceptor` in `config/` is a security control. It should move to `shared/security/` or `shared/config/` to signal its security intent, not get buried in a domain package. ### Recommendations - **Produce an explicit list of security-critical classes and their target packages before writing code**: `SecurityConfig → shared/security/`, `PermissionAspect → shared/security/`, `Permission → shared/security/`, `RateLimitInterceptor → shared/security/`, `CustomUserDetailsService → user/`, `AuthController → auth/ or user/`, `AuthE2EController → user/ with @Profile("e2e") confirmed`. - **Add one `@WebMvcTest` slice test after the refactor** that verifies `@RequirePermission(Permission.WRITE_ALL)` is still enforced on a write endpoint. This is a 5-line test and confirms the AOP wiring survived the package move. Specifically: import the post-move `SecurityConfig` and `PermissionAspect` in the test's `@Import({...})` and verify a READ_ALL user gets 403 on a DELETE endpoint. - **Do not move `GlobalExceptionHandler`** into any domain. It belongs in `shared/` and maps `DomainException` → HTTP responses for the entire application. Moving it into, say, `document/` would be semantically wrong. - For REFACTOR-4 (ESLint boundary rule), ensure the rule explicitly blocks domain components from importing security utilities directly — only `shared/` exports should be accessible cross-domain. This isn't a common violation today but the rule should prevent future drift. No critical security vulnerabilities are introduced by this refactor if the above is followed. No open decisions from security — the placements are clear.
marcel commented 2026-05-04 21:06:38 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

  • The test suite is the explicit safety net for this refactor. That's the right framing. Current test file count: ~92 Java test files. All of them have import org.raddatz.familienarchiv.controller.*, .service.*, .model.*, .dto.* — every single import must be updated after the package moves.
  • The ApplicationContextTest.java is the critical smoke test: it loads the full Spring context and will immediately catch any broken bean wiring caused by package moves. Run this after every domain move during REFACTOR-1.
  • PostgresContainerConfig.java is in the test root and is imported by integration tests. It must not get lost or accidentally re-packaged during the refactor — it's shared test infrastructure.
  • The acceptance criteria requires "full backend test suite green throughout" — this implies the suite must be green at each commit, not just at the end. With ~169 production files moving across 92 test files, running ./mvnw test after each domain sub-move is the only way to catch breakage early.
  • REFACTOR-3 (ArchUnit) test design: the issue says "prevents controller→repo direct injection AND cross-domain repo access." Those are two separate rules needing two test cases. The first (controller→repo) is straightforward. The second (cross-domain repo) requires agreeing on the domain assignment before it can be written — you can't assert "ocr/ does not import document/repository/" until document/repository/ exists. Confirming: ArchUnit must be written against the post-refactor package layout, not the current one.
  • Known ArchUnit failures that exist today: SenderModelService imports TranscriptionBlockRepository (ocr→transcription cross-domain), SegmentationTrainingExportService imports DocumentRepository and AnnotationRepository (ocr→document, ocr→annotation). These violations must be resolved before REFACTOR-3's test can be written green. If they are not resolved, REFACTOR-3 either gets a permanent @ignore list (bad) or the test is intentionally weaker than advertised.

Recommendations

  • Add a step to the acceptance criteria: "ArchUnit test failures list is zero (no ignoreClasses or ignoreDependencies exceptions)." Without this, the ArchUnit test can be written with a long exception list and still "pass."
  • The two mechanical PRs (REFACTOR-1 and REFACTOR-2) should each have an explicit CI run requirement before merge: ./mvnw test must pass (REFACTOR-1) and npm run test && npx playwright test must pass (REFACTOR-2). This is already in the acceptance criteria but should be a CI gate, not a manual check.
  • For REFACTOR-1, batch the test file moves with their production counterparts in the same commit. Moving DocumentService.java to document/ in one commit and DocumentServiceTest.java to document/ in the next creates a window where the test doesn't compile. Keep them together.
  • REFACTOR-3 test skeleton to write before REFACTOR-1 starts (red phase): 
    @AnalyzeClasses(packages = "org.raddatz.familienarchiv")
class DomainBoundaryTest {
    @ArchTest
    static final ArchRule no_controller_to_repo =
        noClasses().that().resideInAPackage("..controller..")
            .should().dependOnClassesThat().resideInAPackage("..repository..");

    @ArchTest
    static final ArchRule document_owns_its_repo =
        noClasses().that().resideOutsideOfPackage("..document..")
            .should().dependOnClassesThat()
            .resideInAPackage("..document..repository..")
            .orShould().dependOnClassesThat()
            .resideInAPackage("..document..");
}
    Run this now — it will fail red and drive the cross-domain violation fixes needed before REFACTOR-1 is "done."

Open Decisions

  • Should ApplicationContextTest be expanded to assert bean count or specific critical beans? Right now it just loads the context. A more robust smoke test would assert that SecurityConfig, PermissionAspect, and each domain controller bean are present. This is optional scope but would make the safety net stronger.
## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - The test suite is the explicit safety net for this refactor. That's the right framing. Current test file count: ~92 Java test files. All of them have `import org.raddatz.familienarchiv.controller.*`, `.service.*`, `.model.*`, `.dto.*` — every single import must be updated after the package moves. - **The `ApplicationContextTest.java` is the critical smoke test**: it loads the full Spring context and will immediately catch any broken bean wiring caused by package moves. Run this after every domain move during REFACTOR-1. - **`PostgresContainerConfig.java` is in the test root** and is imported by integration tests. It must not get lost or accidentally re-packaged during the refactor — it's shared test infrastructure. - The acceptance criteria requires "full backend test suite green throughout" — this implies the suite must be green at each commit, not just at the end. With ~169 production files moving across 92 test files, running `./mvnw test` after each domain sub-move is the only way to catch breakage early. - **REFACTOR-3 (ArchUnit) test design**: the issue says "prevents controller→repo direct injection AND cross-domain repo access." Those are two separate rules needing two test cases. The first (controller→repo) is straightforward. The second (cross-domain repo) requires agreeing on the domain assignment before it can be written — you can't assert "ocr/ does not import document/repository/" until `document/repository/` exists. Confirming: ArchUnit must be written against the post-refactor package layout, not the current one. - **Known ArchUnit failures that exist today**: `SenderModelService` imports `TranscriptionBlockRepository` (ocr→transcription cross-domain), `SegmentationTrainingExportService` imports `DocumentRepository` and `AnnotationRepository` (ocr→document, ocr→annotation). These violations must be resolved before REFACTOR-3's test can be written green. If they are not resolved, REFACTOR-3 either gets a permanent `@ignore` list (bad) or the test is intentionally weaker than advertised. ### Recommendations - **Add a step to the acceptance criteria**: "ArchUnit test failures list is zero (no `ignoreClasses` or `ignoreDependencies` exceptions)." Without this, the ArchUnit test can be written with a long exception list and still "pass." - **The two mechanical PRs (REFACTOR-1 and REFACTOR-2) should each have an explicit CI run requirement** before merge: `./mvnw test` must pass (REFACTOR-1) and `npm run test && npx playwright test` must pass (REFACTOR-2). This is already in the acceptance criteria but should be a CI gate, not a manual check. - **For REFACTOR-1, batch the test file moves with their production counterparts** in the same commit. Moving `DocumentService.java` to `document/` in one commit and `DocumentServiceTest.java` to `document/` in the next creates a window where the test doesn't compile. Keep them together. - **REFACTOR-3 test skeleton to write before REFACTOR-1 starts** (red phase): ```java @AnalyzeClasses(packages = "org.raddatz.familienarchiv") class DomainBoundaryTest { @ArchTest static final ArchRule no_controller_to_repo = noClasses().that().resideInAPackage("..controller..") .should().dependOnClassesThat().resideInAPackage("..repository.."); @ArchTest static final ArchRule document_owns_its_repo = noClasses().that().resideOutsideOfPackage("..document..") .should().dependOnClassesThat() .resideInAPackage("..document..repository..") .orShould().dependOnClassesThat() .resideInAPackage("..document.."); } ``` Run this now — it will fail red and drive the cross-domain violation fixes needed before REFACTOR-1 is "done." ### Open Decisions - **Should `ApplicationContextTest` be expanded to assert bean count or specific critical beans?** Right now it just loads the context. A more robust smoke test would assert that `SecurityConfig`, `PermissionAspect`, and each domain controller bean are present. This is optional scope but would make the safety net stronger.
marcel commented 2026-05-04 21:07:00 +02:00
Author
Owner
Copy Link

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

  • This is a pure Java package rename + file move refactor — no infrastructure, Docker, or deployment changes required. The deployed JAR is structurally identical; only the internal class paths change. CI passes → deploy as normal.
  • The CI pipeline will be the most affected artifact: if any CI job has hardcoded package paths (e.g., in JaCoCo exclusions, Checkstyle configs, or Maven Surefire patterns), those need updating. Checked: pom.xml has JaCoCo but no ArchUnit. ArchUnit being added in REFACTOR-3 is a new test dependency — it needs a Maven dependency entry.
  • Maven dependency for ArchUnit (REFACTOR-3): 
    <dependency>
    <groupId>com.tngtech.archunit</groupId>
    <artifactId>archunit-junit5</artifactId>
    <version>1.3.0</version>
    <scope>test</scope>
</dependency>
    This is a test-scope dependency only. No build time impact, no production artifact impact.
  • Compile time risk: With 212 Java files changing packages, a single missed import statement causes a compile failure. IntelliJ's refactor handles this automatically. If done manually with scripts, a compilation check (./mvnw clean compile -DskipTests) is the fastest failure signal before running the full test suite.
  • No Flyway migrations required. Package restructuring does not change the DB schema. Confirm this explicitly in the PR description so reviewers don't go looking for missing migrations.
  • REFACTOR-2 frontend: npm run check (svelte-check) will catch broken import paths immediately. Running npm run check after each domain move is faster feedback than a full build.
  • For REFACTOR-4 (ESLint import boundary): eslint-plugin-import must be added to frontend/package.json if not already present (it isn't currently in the ESLint config). Alternatively, no-restricted-imports in the existing config needs no new dependency.

Recommendations

  • Add a compile-only CI check step (./mvnw clean compile -DskipTests) that runs before the full test suite in REFACTOR-1's PR. Fast failure is better than a 2-minute wait.
  • For REFACTOR-4, use the zero-dependency approach first: ESLint no-restricted-imports with per-domain deny lists in the existing eslint.config.js. No new package to maintain: 
    'no-restricted-imports': ['error', {
    patterns: [
        { group: ['$lib/components/document/*'], message: 'Import document components only from the document domain' }
    ]
}]
    This is immediately enforceable in the existing config and beats a new plugin dependency for now.
  • Pin the ArchUnit version in pom.xml and add it to any Renovate/Dependabot config so patch updates are automated. ArchUnit is actively maintained and follows semantic versioning.

No infrastructure changes needed, no open decisions from the DevOps side. This is a safe refactor from a deployment perspective.

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Observations - This is a pure Java package rename + file move refactor — no infrastructure, Docker, or deployment changes required. The deployed JAR is structurally identical; only the internal class paths change. CI passes → deploy as normal. - **The CI pipeline will be the most affected artifact**: if any CI job has hardcoded package paths (e.g., in JaCoCo exclusions, Checkstyle configs, or Maven Surefire patterns), those need updating. Checked: `pom.xml` has JaCoCo but no ArchUnit. ArchUnit being added in REFACTOR-3 is a new test dependency — it needs a Maven dependency entry. - **Maven dependency for ArchUnit** (REFACTOR-3): ```xml <dependency> <groupId>com.tngtech.archunit</groupId> <artifactId>archunit-junit5</artifactId> <version>1.3.0</version> <scope>test</scope> </dependency> ``` This is a test-scope dependency only. No build time impact, no production artifact impact. - **Compile time risk**: With 212 Java files changing packages, a single missed import statement causes a compile failure. IntelliJ's refactor handles this automatically. If done manually with scripts, a compilation check (`./mvnw clean compile -DskipTests`) is the fastest failure signal before running the full test suite. - **No Flyway migrations required.** Package restructuring does not change the DB schema. Confirm this explicitly in the PR description so reviewers don't go looking for missing migrations. - **REFACTOR-2 frontend**: `npm run check` (svelte-check) will catch broken import paths immediately. Running `npm run check` after each domain move is faster feedback than a full build. - For REFACTOR-4 (ESLint import boundary): `eslint-plugin-import` must be added to `frontend/package.json` if not already present (it isn't currently in the ESLint config). Alternatively, `no-restricted-imports` in the existing config needs no new dependency. ### Recommendations - **Add a compile-only CI check step** (`./mvnw clean compile -DskipTests`) that runs before the full test suite in REFACTOR-1's PR. Fast failure is better than a 2-minute wait. - **For REFACTOR-4**, use the zero-dependency approach first: ESLint `no-restricted-imports` with per-domain deny lists in the existing `eslint.config.js`. No new package to maintain: ```js 'no-restricted-imports': ['error', { patterns: [ { group: ['$lib/components/document/*'], message: 'Import document components only from the document domain' } ] }] ``` This is immediately enforceable in the existing config and beats a new plugin dependency for now. - **Pin the ArchUnit version in `pom.xml`** and add it to any Renovate/Dependabot config so patch updates are automated. ArchUnit is actively maintained and follows semantic versioning. No infrastructure changes needed, no open decisions from the DevOps side. This is a safe refactor from a deployment perspective.
marcel commented 2026-05-04 21:07:20 +02:00
Author
Owner
Copy Link

📋 Elicit (Requirements Engineer)

Observations

  • The acceptance criteria are measurable and well-structured. Four child issues, eight rubric checks, and explicit test gates — this is a spec-dense epic as required. No ambiguity in what "done" means.
  • One gap in the acceptance criteria: REFACTOR-3 says "ArchUnit test prevents controller→repo direct injection AND cross-domain repo access." The issue does not specify what happens with the known existing violations (3+ cross-domain repo accesses found in the current code). If those violations are not resolved before or during REFACTOR-1, REFACTOR-3 either fails permanently or is written with an exceptions list. This is an implicit prerequisite that should be explicit.
  • The canonical domain set is referenced as "in #387 first comment" — it is not embedded in this issue. If #387 is closed or its first comment is edited, the source of truth for domain names is lost. The domain list (document, person, tag, user, geschichte, notification, ocr, shared) should be reproduced in this issue or in the child issues for REFACTOR-1 and REFACTOR-2.
  • The dependency chain is correctly stated (blocked by #393 AUDIT-6 and #402 Epic 3), but the blocking conditions are asymmetric: AUDIT-6 gives a binary "ready/not ready" signal, while Epic 3 (pre-flight) is a suite of tasks. Consider adding: "All child issues of #402 must be closed before REFACTOR-1 starts."
  • REFACTOR-3 and REFACTOR-4 (enforcement tests) are good requirements. The issue doesn't specify whether they go in the same PR as REFACTOR-1/REFACTOR-2 or separate ones. Separate is safer (enforcement tests can be written, confirmed green, and merged independently from the structural move).

Recommendations

  • Embed the canonical domain set directly in this issue (copy from #387 first comment). This makes the epic self-contained.
  • Add an explicit prerequisite: "All known cross-domain repository violations must be resolved as part of REFACTOR-1 before REFACTOR-3 can be written without an exceptions list." Name the specific violations: SenderModelService→TranscriptionBlockRepository, SegmentationTrainingExportService→DocumentRepository+AnnotationRepository+TranscriptionBlockRepository.
  • Clarify the co-location question for tests: the acceptance criteria says the test suite must be green, but doesn't specify whether test files move to domain packages alongside production code. A sentence either way eliminates ambiguity for whoever implements REFACTOR-1.
  • The Definition of Done is well-constructed. One addition worth making: "No @ArchIgnore, ignoreDependency(), or orShould() clauses in the ArchUnit test that exist only to paper over known violations."

No open decisions from the requirements perspective — the gaps above are clarification items that belong in this issue's body, not implementation choices.

## 📋 Elicit (Requirements Engineer) ### Observations - The acceptance criteria are measurable and well-structured. Four child issues, eight rubric checks, and explicit test gates — this is a spec-dense epic as required. No ambiguity in what "done" means. - **One gap in the acceptance criteria**: REFACTOR-3 says "ArchUnit test prevents controller→repo direct injection AND cross-domain repo access." The issue does not specify what happens with the *known existing violations* (3+ cross-domain repo accesses found in the current code). If those violations are not resolved before or during REFACTOR-1, REFACTOR-3 either fails permanently or is written with an exceptions list. This is an implicit prerequisite that should be explicit. - **The canonical domain set is referenced as "in #387 first comment"** — it is not embedded in this issue. If #387 is closed or its first comment is edited, the source of truth for domain names is lost. The domain list (document, person, tag, user, geschichte, notification, ocr, shared) should be reproduced in this issue or in the child issues for REFACTOR-1 and REFACTOR-2. - **The dependency chain is correctly stated** (blocked by #393 AUDIT-6 and #402 Epic 3), but the blocking conditions are asymmetric: AUDIT-6 gives a binary "ready/not ready" signal, while Epic 3 (pre-flight) is a suite of tasks. Consider adding: "All child issues of #402 must be closed before REFACTOR-1 starts." - **REFACTOR-3 and REFACTOR-4** (enforcement tests) are good requirements. The issue doesn't specify whether they go in the same PR as REFACTOR-1/REFACTOR-2 or separate ones. Separate is safer (enforcement tests can be written, confirmed green, and merged independently from the structural move). ### Recommendations - **Embed the canonical domain set directly in this issue** (copy from #387 first comment). This makes the epic self-contained. - **Add an explicit prerequisite**: "All known cross-domain repository violations must be resolved as part of REFACTOR-1 before REFACTOR-3 can be written without an exceptions list." Name the specific violations: `SenderModelService→TranscriptionBlockRepository`, `SegmentationTrainingExportService→DocumentRepository+AnnotationRepository+TranscriptionBlockRepository`. - **Clarify the co-location question for tests**: the acceptance criteria says the test suite must be green, but doesn't specify whether test files move to domain packages alongside production code. A sentence either way eliminates ambiguity for whoever implements REFACTOR-1. - The Definition of Done is well-constructed. One addition worth making: "No `@ArchIgnore`, `ignoreDependency()`, or `orShould()` clauses in the ArchUnit test that exist only to paper over known violations." No open decisions from the requirements perspective — the gaps above are clarification items that belong in this issue's body, not implementation choices.
marcel commented 2026-05-04 21:07:32 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

  • This epic is backend architecture and file organization — no UI changes, no new user-facing behavior. There is nothing for me to review from a visual design or accessibility standpoint.
  • I checked: the frontend REFACTOR-2 reorganizes lib/components/ into domain subdirectories. This does not affect rendered HTML, CSS classes, or Tailwind tokens — only import paths change. Accessibility behavior is fully preserved.
  • One thing worth flagging: the 81 components currently in the flat lib/components/ root include several that I'd expect to be shared across domains: BackButton.svelte, ConfirmDialog.svelte, DateInput.svelte, GroupDivider.svelte, HelpPopover.svelte, LanguageSwitcher.svelte. These should land in lib/components/shared/ or lib/shared/components/ — not in any single domain folder. If BackButton.svelte gets moved to document/ because a document page uses it, it becomes unreachable from person/ pages without a cross-domain import. Plan the shared/ component list explicitly before REFACTOR-2 starts.

Recommendations

  • Before REFACTOR-2, produce a three-column classification of all 81 flat components: Component → Target domain → Reason. Any component used by pages in 2+ domains goes into shared/. This prevents the need to move things twice.
  • No design or accessibility work required for this epic. No concerns from my angle beyond the shared-component classification above.

No open decisions from the UX/accessibility perspective.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - This epic is backend architecture and file organization — no UI changes, no new user-facing behavior. There is nothing for me to review from a visual design or accessibility standpoint. - I checked: the frontend REFACTOR-2 reorganizes `lib/components/` into domain subdirectories. This does **not** affect rendered HTML, CSS classes, or Tailwind tokens — only import paths change. Accessibility behavior is fully preserved. - One thing worth flagging: the **81 components currently in the flat `lib/components/` root** include several that I'd expect to be shared across domains: `BackButton.svelte`, `ConfirmDialog.svelte`, `DateInput.svelte`, `GroupDivider.svelte`, `HelpPopover.svelte`, `LanguageSwitcher.svelte`. These should land in `lib/components/shared/` or `lib/shared/components/` — not in any single domain folder. If `BackButton.svelte` gets moved to `document/` because a document page uses it, it becomes unreachable from `person/` pages without a cross-domain import. Plan the `shared/` component list explicitly before REFACTOR-2 starts. ### Recommendations - Before REFACTOR-2, produce a three-column classification of all 81 flat components: `Component → Target domain → Reason`. Any component used by pages in 2+ domains goes into `shared/`. This prevents the need to move things twice. - No design or accessibility work required for this epic. No concerns from my angle beyond the shared-component classification above. No open decisions from the UX/accessibility perspective.
marcel commented 2026-05-04 21:07:49 +02:00
Author
Owner
Copy Link

🗳️ Decision Queue — Action Required

3 decisions need your input before implementation starts.

Architecture

  • Where does MassImportService live after the refactor? It creates Documents but calls PersonService, TagService, and uses DocumentRepository. Options: (a) document/ domain — it creates Documents, and its calls to PersonService/TagService already go through service interfaces so no boundary violation; (b) import/ domain — avoids coupling to document/ but adds a new domain for a single service. Recommendation: option (a) — the primary output is a Document. (Raised by: Markus)

  • Test co-location in REFACTOR-1: should test files move into domain packages alongside production code (e.g. document/DocumentServiceTest.java next to document/DocumentService.java), or stay in a parallel test/... package tree mirroring the new domain structure? Co-location is the cleaner end state and aligns with the legibility goal. But it doubles the number of file moves. Options: (a) full co-location in same PR as REFACTOR-1; (b) co-location as a separate follow-up after the structural move is merged. Either way, the convention must be decided before REFACTOR-1 starts so the PR isn't a partial migration. (Raised by: Felix, Sara)

Quality Gates

  • Should ApplicationContextTest be expanded to assert critical bean presence? Currently it only loads the Spring context and verifies no startup exception. Options: (a) keep it as a minimal smoke test — fast, low maintenance; (b) expand to assert that SecurityConfig, PermissionAspect, and each domain's controller bean are present in the context — stronger safety net against silent misconfiguration after the package moves. Option (b) adds ~10 lines and makes the safety net meaningful. (Raised by: Sara)
## 🗳️ Decision Queue — Action Required _3 decisions need your input before implementation starts._ ### Architecture - **Where does `MassImportService` live after the refactor?** It creates Documents but calls `PersonService`, `TagService`, and uses `DocumentRepository`. Options: (a) `document/` domain — it creates Documents, and its calls to `PersonService`/`TagService` already go through service interfaces so no boundary violation; (b) `import/` domain — avoids coupling to `document/` but adds a new domain for a single service. **Recommendation: option (a)** — the primary output is a Document. _(Raised by: Markus)_ - **Test co-location in REFACTOR-1**: should test files move into domain packages alongside production code (e.g. `document/DocumentServiceTest.java` next to `document/DocumentService.java`), or stay in a parallel `test/...` package tree mirroring the new domain structure? Co-location is the cleaner end state and aligns with the legibility goal. But it doubles the number of file moves. Options: (a) full co-location in same PR as REFACTOR-1; (b) co-location as a separate follow-up after the structural move is merged. Either way, the convention must be decided before REFACTOR-1 starts so the PR isn't a partial migration. _(Raised by: Felix, Sara)_ ### Quality Gates - **Should `ApplicationContextTest` be expanded to assert critical bean presence?** Currently it only loads the Spring context and verifies no startup exception. Options: (a) keep it as a minimal smoke test — fast, low maintenance; (b) expand to assert that `SecurityConfig`, `PermissionAspect`, and each domain's controller bean are present in the context — stronger safety net against silent misconfiguration after the package moves. Option (b) adds ~10 lines and makes the safety net meaningful. _(Raised by: Sara)_
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 epic legibility refactor
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#406
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?