feat(lesereisen): data model + Flyway migration — GeschichteType, JourneyItem, migrate geschichten_documents #787
Reference in New Issue
Block a user
Delete Branch "feat/issue-750-lesereisen-data-model"
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
Closes #750
Introduces the foundational data model for Lesereisen (Reading Journeys):
GeschichteTypeenum (STORY/JOURNEY) — defaultSTORYfor all existing rowsJourneyItementity — ordered stops in a JOURNEY, each backed by an optionalDocumentreference and/or an editorial note;ON DELETE SET NULLon the document FK so archive deletions don't break journeystypecolumn togeschichten, createsjourney_itemstable, migrates existinggeschichten_documentsrows (ordered bymeta_date ASC NULLS LAST), drops old junction tableGeschichteSummaryinterface projection — replaces the old entity-basedlist()to avoidLazyInitializationExceptionwithopen-in-view=falseapi.tsupdated forJourneyItem/GeschichteSummary;geschichten/[id]renders document-backed items as links; document picker removed from editor (follow-on issue for full journey editor)Test plan
JourneyItemIntegrationTest— 9 tests:@OrderBy, cascade delete, orphan removal, type round-trip, type default, note-only, document-backed, ON DELETE SET NULL, CHECK constraintGeschichteListProjectionTest— 8 tests: interface projection fields, author nestingGeschichteHttpTest— 5 HTTP-layer tests (list, get, create, update, delete)GeschichteEditor.svelte.spec.ts,GeschichtenCard.svelte.spec.ts,GeschichtenCard.svelte.test.tsgeschichten_documentsrows🤖 Generated with Claude Code
- GeschichteType enum {STORY, JOURNEY} — default STORY - JourneyItem entity replaces geschichten_documents junction table; position-ordered, document_id nullable (note-only items allowed), CHECK(document_id IS NOT NULL OR note IS NOT NULL) - GeschichteSummary interface projection for list() queries (avoids lazy-init) - Geschichte entity gains `type` + `items` (LAZY, orphanRemoval, CascadeType.ALL) replacing the old `documents` ManyToMany bag - GeschichteUpdateDTO: remove documentIds (replaced by JourneyItem API) - V72 migration: adds `type` column, creates `journey_items` table with FK ON DELETE CASCADE (geschichte) / ON DELETE SET NULL (document), migrates geschichten_documents ordered by meta_date, drops junction table Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
This is a well-structured data-model PR. The layering is correct, the Flyway migration is careful, and the LazyInitializationException problem is addressed at the right layer. Two doc-update blockers remain per the architecture review rules.
Blockers
1.
db-orm.pumlnot updated — blocker per architecture rulesThe migration drops
geschichten_documentsand createsjourney_items. Thedb-relationships.pumlwas updated correctly. Butdocs/architecture/db/db-orm.pumlstill shows the oldgeschichten_documentsentity (lines 373, 439-440) and has nojourney_itemsentity with its columns (id,geschichte_id,position,document_id,note) or the newtypecolumn ongeschichten. Per the architect's doc-update table: "New Flyway migration adding/removing/renaming a table or column →db-orm.puml". This is a blocker — the PR does not merge until the diagram matches the code.2. C4 L3 backend diagram
l3-backend-3g-supporting.pumlnot updatedGeschichteServicenow owns a new sub-domain (journeyitem/JourneyItemRepository) andGeschichteSummaryis a new type in thegeschichtepackage. The existing diagram describes the service as managing "stories that link persons and documents" — that description is now stale. TheJourneyItemcomponent and theGeschichteSummaryprojection should appear. Per the rule: "New controller or service in an existing backend domain → matchingl3-backend-*.puml".Suggestions
Sentinel UUID pattern in
GeschichteService.list()The sentinel
UUID.fromString("00000000-0000-0000-0000-000000000000")is a workaround for empty-IN()invalidity. It works and is documented, but it's a leaky abstraction — the caller has to know that passing an empty list triggers a different code path than passing the sentinel. Consider extracting to a named constantPERSON_FILTER_SENTINELwith a comment explaining why, or making the JPQL branch explicit at the repository layer with aCASE WHEN :personCount = 0 THEN TRUE. The current approach is fine for now given the follow-on editor issue, but flag it for cleanup when the editor lands.GeschichteSummaryinterface projection —@Schemaannotations missingGeschichteSummaryis an interface projection and drives theGET /api/geschichtenOpenAPI type. The generatedapi.tshasid: stringandtitle: stringas required (correctly), but the interface itself has no@Schema(requiredMode = REQUIRED)annotations (it's an interface, not an entity, so Springdoc infers optionality differently for nested projections). Worth verifying the generated spec matches the contract expectations —id,title,status, andtypeshould always be present for the frontend to render correctly.GeschichteSpecificationsTODO commentThe
// TODO(lesereisen-editor): restore document filter via journey_items join when editor landscomment is fine as a placeholder, but the corresponding follow-on Gitea issue number should be cited so the TODO is traceable. A naked TODO without an issue number becomes orphaned after a few sprints.Hibernate.initialize(g.getItems())ingetById()The pattern works and the comment explaining why is excellent. However, calling
g.getItems().size()(as hinted in the entity comment) rather thanHibernate.initialize(...)would be slightly more idiomatic and avoids an explicit Hibernate API import at the service layer. Either is fine — this is a style note, not a blocker.What's Done Well
ON DELETE SET NULLFK onjourney_items.document_idis exactly right for this domain: an archive deletion should not break a Lesereise, just leave a placeholder stop.chk_journey_item_not_emptyCHECK constraint at the database layer is the correct place for this invariant — application code can never accidentally create an empty item.GeschichteSummaryprojection replacing entity serialization for the list endpoint is the right architectural move givenopen-in-view: false. ThePersonSummaryDTOprecedent is correctly cited.Geschichte.javaexplaining the LAZY +Hibernate.initializepattern are load-bearing and should stay.Overall: fix the two diagram blockers and this is mergeable.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Solid data-model work. The TDD evidence is excellent — 9+8+5 tests covering the new entities, projection, and HTTP layer. Two issues warrant attention before merge: one is a functional bug in the reader UI, one is a missing
@Transactional(readOnly = true)annotation.Blockers
1.
geschichten/[id]/+page.svelte— document-backed items displaydocumentIdUUID as link textIn
frontend/src/routes/geschichten/[id]/+page.svelte, the new template renders:item.noteisnullfor document-backed items, so readers see a raw UUID like3a7f2b12-...as the link label. The old code displayedd.titleandd.documentDate. This is a regression in display quality — a follow-on issue for the full reader UI is expected, but shipping a UUID-as-link-text to readers is not acceptable even as a stub. Either display a translated fallback (m.geschichten_document_link_fallback()) or filter these items out of the section entirely until the reader UI lands. The section guardg.items.some((i) => i.documentId)already hides note-only items correctly; apply the same discipline to the label.2.
GeschichteService.getById()— missing@Transactional(readOnly = true)before the PR's own commentLooking at the diff:
This is actually present — good. BUT the method previously had no
@Transactionalat all. The rule in this codebase (and Felix's own guidelines) is: read methods are not annotated (default non-transactional is fine). The comment in the entity explicitly calls out thatgetById()is@Transactional(readOnly=true)and callsgetItems().size()to force-init. This is the correct special case — the annotation is required here precisely because ofopen-in-view: false+ LAZY collection. The annotation is justified and documented. No change needed, but flag for other reviewers: this is an intentional deviation from the "no @Transactional on reads" default, and the comment explains why.Actually — this is not a blocker, just a note. Reclassifying.
Suggestions
[id]/+page.svelte— unkeyed{#each}guardThe key
(item.id)is correct. Good.GeschichteEditor.svelte—documentIdsremoved cleanlyThe
documentIdsprop removal is complete acrossGeschichteEditor.svelte, bothnew/+page.svelteand[id]/edit/+page.svelte, the DTO, and the service. No dangling references visible. Clean.GeschichteServiceTest—list_forces_PUBLISHED_status_for_reader_without_BLOG_WRITEtest is thinThe test now only verifies that
findSummaries()is called, without asserting theeffectiveStatusargument isPUBLISHED. The old test had the same weakness (it assertedfindAll(spec, sort)was called, not what was in the spec). This is acceptable for a unit test — the status-pinning behaviour is covered end-to-end inGeschichteServiceIntegrationTest. But the test name says "forces PUBLISHED" and the assertion doesn't prove it. Consider:This makes the test prove what its name claims.
GeschichteHttpTest—setUp()deletes all geschichten globallygeschichteRepository.deleteAll()in@BeforeEachis fine for an isolated test container, but it will silently corrupt any other tests that run in the same Spring context without@Transactionalrollback. SinceGeschichteHttpTestintentionally omits@Transactional(correct, per the comment explaining why), this is the right trade-off — just worth noting that test execution order matters if new tests are added to the class later.JourneyItem.java—@JsonIgnoreondocumentfieldThe
getDocumentId()synthetic getter correctly exposes only the UUID, and the@JsonIgnoreon thedocumentfield prevents the full entity from being serialized. This is clean and prevents the circular reference / N+1 problem. The CWE-79 tripwire comment onnoteis a useful reminder for future renderers.GeschichteListProjectionTest—@BeforeEachdoes not delete personsThe test creates
Personrows directly but only deletesgeschichteninsetUp(). Person rows accumulate across test runs in the same container session. This won't cause failures because persons are looked up by the IDs created within each test, but it's noise. Not a blocker.What's Done Well
summaryStub()anonymous class inGeschichteControllerTestwith the comment "Mockito interface mocks are not serialized reliably by Jackson" is exactly the right fix for a subtle test infrastructure trap.JourneyItemIntegrationTestare excellent — every name is a sentence describing a behaviour.GeschichteHttpTestis the canonical guard forLazyInitializationException— the comment explaining why@Transactionalis deliberately omitted at class level is load-bearing.personFactoryinGeschichteEditor.svelte.spec.tsnow includesfamilyMember: falseandprovisional: false— correct alignment with the currentPersonschema.The UUID-as-link-text regression in
[id]/+page.svelteis the only item I'd want addressed before merge.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No security vulnerabilities found. The PR introduces a new entity, a Flyway migration, and some frontend cleanup. I've audited the input paths, the FK strategy, the permission annotations, and the note field handling. Everything checks out with one observation worth noting.
Observations (not blockers)
JourneyItem.note— CWE-79 tripwire comment is correct but incompleteThe comment in
JourneyItem.javareads:This is the right approach and the comment correctly identifies the contract. Svelte's
{note}interpolation auto-escapes HTML, so the current frontend rendering is safe. The risk lives in future consumers — if the note is ever included in an email, PDF export, or RSS feed, the renderer must escape it. The comment is load-bearing; do not remove it during refactoring.One addition worth making: the
notefield has no length constraint at the database layer (columnDefinition = "TEXT"with no CHECK). For a production archive, an unbounded TEXT field accepts gigabyte payloads. ACHECK (char_length(note) <= 5000)or equivalent would be consistent with howtranscription_blockshandles length limits. Not a blocker for this PR, but log it as a follow-on.GeschichteService— permission annotations on write endpointsGeschichteControllerwrite endpoints (POST,PATCH,DELETE) carry@RequirePermission(Permission.BLOG_WRITE)/@RequirePermission(Permission.WRITE_ALL)as before. The newGET /api/geschichtenreturningGeschichteSummaryis read-only and only accessible to authenticated users (nopermitAll()). The status-clamping logic inGeschichteService.list()correctly hidesDRAFTstories from users withoutBLOG_WRITE. This is the application-layer tenant isolation pattern in use here, and it's correctly implemented.JPQL in
GeschichteRepository.findSummaries()All parameters are bound via
@Param— no string concatenation. TheIN :personIdsclause uses a named parameter collection. The sentinel UUID approach for empty-IN avoidance is a known safe pattern (it's a compile-time constant, not user input). No injection surface here.Flyway migration
V72—DROP TABLE geschichten_documentsThe migration is irreversible after commit. The pre-migration
pg_dumpinstruction in the header is the right mitigation. The reverse procedure is documented. From a security standpoint, there are no privilege escalation risks in the DDL — it drops a junction table and creates a new one with standard FK constraints.What's Done Well
@JsonIgnoreonJourneyItem.documentprevents the fullDocumententity (including all its fields) from being inadvertently serialized into the journey item payload. Only thedocumentIdUUID is exposed. This is the minimum-exposure principle applied correctly.@CrossOriginannotations orpermitAll()additions.ErrorCodevalues were introduced, so no i18n mapping gap is possible from this PR.GeschichteServicestill routes document resolution throughDocumentService.getDocumentById()(reserved comment preserved), which enforces existence and scope checks. Cross-domain boundary is respected.Clean PR from a security perspective. The note-length constraint is the one thing I'd track in the backlog.
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
The test suite for this PR is one of the best I've seen in this codebase. Three layers are properly covered, the tooling choices are correct, and the coverage hits all the important behavioural boundaries. A few gaps worth tracking.
What's Covered Well
Layer separation is correct:
JourneyItemIntegrationTest—@SpringBootTest+@Transactional+ Testcontainers (real Postgres). Correct for entity/cascade/constraint testing.GeschichteListProjectionTest—@DataJpaTest+@AutoConfigureTestDatabase(Replace.NONE)+ real Postgres. Correct for repository projection testing.GeschichteHttpTest—@SpringBootTest(RANDOM_PORT), no@Transactional(intentional, catches lazy-init regressions), RestTemplate. Correct for HTTP/serialization boundary testing.GeschichteControllerTest—@WebMvcTestslice. Correct for controller/security layer.GeschichteServiceTest—@ExtendWith(MockitoExtension.class). Correct for service logic unit tests.GeschichteHttpTest— the LazyInitializationException guardThe test
list_returns_200_and_does_not_500_when_stories_have_journey_items()is exactly the right regression guard. Seeding a JOURNEY with items, then hitting the list endpoint without a transaction wrapper, will catch any future code change that accidentally triggers lazy loading on the list path. This test is permanent — never@Disabled.JourneyItemIntegrationTest— CHECK constraint testThis is the correct place for this test. H2 would silently pass it; real Postgres enforces the CHECK. Testcontainers here is not optional.
GeschichteListProjectionTest— AND-semantics person filterThe two-person AND test (both must be associated) combined with the single-person test and the no-filter test covers the full truth table for the
personCountlogic. Solid.Gaps (suggestions, not blockers)
1. No test for
ON DELETE SET NULLpath via HTTPJourneyItemIntegrationTest.deleting_document_sets_item_document_to_null_not_delete_item()covers this at the persistence layer — good. But there's noGeschichteHttpTestverifying that after a document is deleted,GET /api/geschichten/{id}returns the item withdocumentId: nullrather than a 500. The persistence test proves the FK works; the HTTP test would prove the JSON serialization handles a nulldocumentreference gracefully after the transaction boundary. Low priority, but a good follow-on test.2.
GeschichteServiceTest.list_forces_PUBLISHED_status— assertion gapAs Felix noted: the test verifies
findSummaries()is called but not with what status argument. Addingeq(GeschichteStatus.PUBLISHED)as the first arg toverify()would make the test prove what its name claims. This is a test quality issue, not a functional bug.3. No test for
positionordering in the list endpoint responseJourneyItemIntegrationTest.items_are_returned_in_position_order_regardless_of_insertion_order()covers the JPA@OrderByat the persistence layer. But there's no HTTP-layer test verifying that theitemsarray in theGET /api/geschichten/{id}JSON response is ordered by position. If someone removes the@OrderByannotation, the persistence test still passes (items are returned in insertion order, which happens to be correct in the test), but the HTTP test would catch the ordering regression in the serialized output.4.
GeschichteHttpTest—loginAsWriter()uses manual XSRF cookie constructionThe XSRF token is manually constructed as a UUID and sent as both cookie and header. This is correct for Spring's double-submit CSRF protection pattern. However, if the CSRF cookie name or header name changes in a security config update, this helper breaks silently (returning an empty session string). The tests would then 403 without a clear error message. Consider adding an assertion on the login response status:
assertThat(resp.getStatusCode().value()).isEqualTo(200)afterpostForEntityto fail fast with a meaningful message if login breaks.5. Frontend component specs —
GeschichtenCard.svelte.spec.tsThe
type: 'STORY' as constaddition tomakeStory()is correct. No test was added fortype: 'JOURNEY'rendering — presumably because the Journey reader UI is a follow-on. That's fine; just ensure the follow-on issue includes a test fortype-conditional rendering when it lands.What's Done Well
postgres:16-alpinevia Testcontainers. No H2 anywhere. The bugs that matter (CHECK constraints, ON DELETE SET NULL, JPQL IN-clause edge cases) are only catchable on real Postgres.@BeforeEachinJourneyItemIntegrationTestusesem.flush(); em.clear()— correct pattern for ensuring the next operation hits the database, not the first-level cache.noThrowRestTemplate()helper inGeschichteHttpTestthat suppresses error-handler exceptions is the right approach for asserting on non-2xx responses without try/catch noise.Strong test suite. The gaps are follow-on items, not merge blockers.
🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
This is primarily a data-model PR, so the frontend changes are intentionally minimal. Most of the UI work is deferred to the follow-on reader issue. However there is one user-facing regression that needs addressing before merge, and a few observations about the stub state.
Blockers
1.
geschichten/[id]/+page.svelte— raw UUID displayed as link text for document-backed itemsThe document section now renders:
For a document-backed item where
item.noteis null, readers see a 36-character UUID string as the clickable link label. This fails multiple usability standards simultaneously:3a7f2b12-8c4d-4e5f-a6b7-c8d9e0f1a2b3refers to.Required fix: Replace
{item.note ?? item.documentId}with a localized fallback. The simplest correct approach for the stub window:Where
m.geschichten_document_link()translates to something like "Brief ansehen" / "View letter" / "Ver carta" across the three locales. This degrades gracefully until the reader UI can show actual document titles.Alternatively, hide document-backed items entirely in the stub window and only show note-only items — but that hides potentially useful navigation for readers who understand the UUIDs are links.
Suggestions
2. Document section heading — "Dokumente (JourneyItems)" comment in HTML
The template comment
<!-- Dokumente (JourneyItems) -->is a developer note inside the markup. Users with browser DevTools (including screen reader users inspecting the DOM) will see this. Use a code comment format that doesn't emit to the DOM if needed, or simply remove it. In Svelte,<!-- -->HTML comments are preserved in the output.3. Section is conditionally shown only when document-backed items exist
This means note-only items (editorial interludes) are invisible on the reader detail page. Whether this is intentional or a gap depends on the follow-on reader spec. Flag it in the follow-on issue: "note-only items are currently suppressed on the detail page."
4. No empty state for
g.itemsWhen
g.type === 'JOURNEY'butg.itemsis empty (a journey stub with no items yet), the document section is hidden (thesome((i) => i.documentId)guard returns false). For a reader expecting a journey, a silent empty state is confusing. Consider showing a brief message wheng.type === 'JOURNEY' && g.items.length === 0. Again, a follow-on item — just ensure the reader spec includes this state.5.
GeschichteEditor.svelte— document picker removed cleanlyThe removal of the
DocumentMultiSelectsection from the editor is clean — no orphaned state, no dangling references. The aside panel now only contains the person picker section. The layout change is acceptable; the person picker section was already self-contained with its own<section>border.What's Done Well
initialDocumentsfrom the editor props is a clean breaking change — the TS types catch any callers that still pass the old prop.GeschichtenCardcomponent test updates (type: 'STORY',items: []) correctly align the test fixtures with the new schema.hasFilterscomputed value simplification (data.personFilters.length > 0only) is correct — removingdocumentFilterfrom the filter chip logic is consistent with the feature removal.Summary: The UUID-as-link-text issue is the only user-facing item that must be fixed before merge. Everything else is either correctly deferred or cleanly done.
🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure, CI, or deployment changes in this PR. The migration is application-managed via Flyway — nothing to touch in the Compose file or CI pipeline. A few observations for the operator.
Observations
Migration V72 — operator runbook is embedded in the SQL file
The pre-requisite
pg_dumpcommand and the reverse procedure are documented directly in the migration header. From an operations perspective this is good practice — the runbook travels with the migration. When this runs in CI it's a no-op (no data ingeschichten_documentson a fresh container). When it runs in production the operator has the commands to run before applying the migration.One operational note: the migration header says
docker exec familienarchiv-db sh -c 'pg_dump ...'. The container namefamilienarchiv-dbshould match the actual service name in the production Compose file. Worth verifying before the production run — if the name is different, the command silently fails to produce the backup.DROP TABLE geschichten_documentsis irreversibleThe migration correctly documents this. The Flyway history prevents re-running it. The only risk is if V72 is applied to a production database that still has live
geschichten_documentsrows before a backup is taken. The header mitigation (take the dump first) is the correct procedure.No new Docker services, ports, or volumes
This PR adds no infrastructure changes. No Compose file updates needed.
CI impact
The new integration tests (
JourneyItemIntegrationTest,GeschichteListProjectionTest,GeschichteHttpTest) add real Postgres container tests. Testcontainers reuses the existing container across tests in the same JVM instance — the CI time impact should be minimal (one additional container startup amortized across the test suite). No concern here.No new environment variables
The
JourneyItem.notefield has no configuration. TheGeschichteTypeenum is application-managed. Nothing to add to.env.exampleor the production Compose.What's Done Well
SELECT DISTINCTguard in the migration INSERT protects against duplicate junction rows creating duplicate journey items during the data migration.Clean from an infrastructure standpoint. The only action item is verifying the container name in the production runbook before executing in prod.
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Reviewing this PR against the original issue #750 (Lesereisen data model) and the broader product context. The implementation is faithful to the data model scope. Three requirements-level concerns are worth tracking.
Concerns
1. ASSUMPTION AS-002 in the migration is a product decision, not a technical one
The migration documents:
This is a user-facing regression for published stories with linked documents. Readers who previously saw "Brief vom 12. März 1923" as a link label now see a UUID (or, after this PR's blocker is fixed, "Brief ansehen" with no date context). This assumption was made implicitly by the developer. It should have been explicitly accepted by the product owner before the PR landed.
As a requirements observation: this is the right trade-off if the follow-on reader issue is the immediate next priority. If it isn't, the regression window extends. The acceptance criteria in issue #750 should include: "Existing STORY pages with linked documents must not degrade below a minimum readable state until the reader UI lands."
2. The
documentIdfilter onGET /api/geschichtenis removed with no migration path for callersThe PR removes
?documentId=from the list endpoint. If any external caller (bookmarked URL, email link, other page in the app) uses this query parameter, it silently stops filtering. The frontend+page.server.tsand+page.svelteare cleaned up correctly. But the removal is not flagged as a breaking API change and there's no deprecation notice. For a family-internal app this is likely fine — but worth verifying no other frontend route or component passesdocumentIdto the geschichten list API.3.
GeschichteTypehas no user-facing validation or error messagingThe new
typefield defaults toSTORY. TheGeschichteUpdateDTOdoes not include atypefield — JOURNEY type cannot be set via the API at all (no setter, no editor). This is by design (follow-on editor issue), but creates an implicit requirement gap: the only way to create a JOURNEY type today is directly in the database or via the migration. This is acceptable as a stub, but the follow-on editor issue must include:typefield inGeschichteUpdateDTOSTORYandJOURNEYare valid values)typeThese should be captured as acceptance criteria in the follow-on issue before it's worked.
What's Well-Specified
GeschichteSummaryprojection fields (id,title,status,type,author,publishedAt,body) match what the grid card needs to render. No over-fetching, no under-fetching.JourneyItemdata model (position,document_id,note, CHECK constraint) is minimal and sufficient for the described feature. No gold-plating.GLOSSARY.mdadditions forGeschichte,JourneyItem, andLesereiseare accurate and use user-facing language correctly.ARCHITECTURE.mdupdate correctly describes the new domain relationship.meta_dateas migration default) is transparent and correctly framed as "best available approximation" rather than a requirement.Open Questions to Capture in Follow-on Issue
/geschichten/new?type=JOURNEY) or a type selector within the existing new/edit flow?itemslist (it would be empty)?These don't block this PR — they belong in the follow-on editor issue spec.
- db-orm.puml: replace geschichten_documents with journey_items, add type column to geschichten, bump schema version to V72 - l3-backend-3g-supporting.puml: update GeschichteController and GeschichteService descriptions to mention STORY/JOURNEY subtypes and JourneyItem - geschichten/[id]/+page.svelte: replace raw UUID fallback with m.geschichten_document_link_placeholder() i18n key - messages/{de,en,es}.json: add geschichten_document_link_placeholder translation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Review blockers addressed — commit
2c5f7ac1Three blockers from the multi-persona review have been fixed:
1.
db-orm.pumlnot updated (Markus Keller)geschichten_documentsentity withjourney_items(withid,position,notecolumns)type : VARCHAR(32) NOT NULLto thegeschichtenentity2.
l3-backend-3g-supporting.pumlstale description (Markus Keller)GeschichteControllerdescription now mentions JOURNEY type andGeschichteSummaryprojectionsGeschichteServicedescription now mentions the two subtypes (STORY/JOURNEY) and orderedJourneyItemsequence3. Raw UUID as link label in
geschichten/[id]/+page.svelte(Felix Brandt, Leonie Voss){item.note ?? item.documentId}with{item.note ?? m.geschichten_document_link_placeholder()}geschichten_document_link_placeholderto all three locale files (de: "Dokument öffnen", en: "Open document", es: "Abrir documento")🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
This PR is a well-executed data model migration. The
GeschichteTypediscriminator,JourneyItementity, V72 Flyway migration, andGeschichteSummaryprojection all follow the established patterns cleanly. The decision to useON DELETE SET NULLon the document FK rather thanCASCADEis architecturally correct — it preserves journey integrity when archive documents are deleted. The comment aboutMultipleBagFetchExceptionrisk onGeschichte(firstListcollection; a second EAGERListwould blow up at boot) is exactly the kind of ADR-level context that belongs in the codebase.Blockers
1. DB diagrams updated, but
CLAUDE.mddomain model table is not.The architect persona table in
CLAUDE.md(## Domain Model) still shows the oldDocumentManyToMany onGeschichteand has no entry forJourneyItemorGeschichteType. Per my own review rule: a Flyway migration that adds a new table/FK is a blocker for the domain model table update.Specifically,
CLAUDE.mdsays:…and the Geschichte row is absent from the table entirely. The
journey_itemstable and theGeschichteTypeenum need entries. This is a documentation debt that will mislead the next LLM or new developer reading the file.2.
GeschichteSummaryinterface projection missing@Schemaannotations.The interface projection fields are returned in the list endpoint and serialized to the OpenAPI spec. Without
@Schema(requiredMode = REQUIRED)ongetId(),getTitle(),getStatus(),getType(), the TypeScript type generator emits these as?optionals inGeschichteSummary— the generatedapi.tsconfirms this (id: stringis non-optional, buttitle: stringalso appears non-optional only because openapi-typescript infers it from usage, not from spec annotation). This is a latent type-safety gap.Suggestions
S1.
list()method applies limit via.stream().limit(safeLimit)in Java, not at the DB layer.The
findSummariesJPQL query fetches up toMAX_LIMIT=200rows and then trims in Java. For current data volumes this is fine, but if the table grows the query could return 200 rows that get trimmed to 10. Consider addingLIMIT :limitto the JPQL query and passingsafeLimitas a parameter to push the limit into SQL. Low priority until volume justifies it.S2.
journeyitemsub-package placement is clean —geschichte/journeyitem/follows the feature-package pattern correctly. No boundary leaks found.S3. V72 migration comments are excellent — the production pre-requisite
pg_dumpcommand and the rollback procedure documented inline are exactly the ADR-level operational context that saves future on-call pain. This is a model for how migrations should be documented.S4.
docs/architecture/c4/l3-backend-3g-supporting.pumlwas updated — confirmed. TheCLAUDE.mdpackage structure table was not. Minor omission: thejourneyitemsub-package should be listed undergeschichte/.Overall: solid foundational work. Fix the
CLAUDE.mddomain model table and the@Schemaannotations before merging.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Good clean work throughout. The TDD evidence is solid —
JourneyItemIntegrationTest(9 tests),GeschichteListProjectionTest(8 tests), andGeschichteHttpTest(5 HTTP-layer tests) all precede the implementation in commit order and test meaningful behaviors, not just happy paths. ThesummaryStub()pattern inGeschichteControllerTest(concrete inner class instead of Mockito interface mock) is pragmatic and correct — Jackson can't reliably serialize Mockito proxy objects.No Blockers
The code is clean. A few observations and suggestions below.
Suggestions
S1.
{#each g.items.filter((i) => i.documentId) as item (item.id)}ingeschichten/[id]/+page.svelteFiltering inside the template expression is business logic that should be a
$derived:Then the template reads
{#each documentItems as item (item.id)}. This aligns with the "business logic in$derived, not template markup" rule and also makes the empty-state check cleaner.S2.
getDocumentId()computed getter onJourneyItemis a clever Jackson trick, but worth a note in the entity javadoc.The comment "JPA uses field access — this getter is not persisted" is accurate and important. The
@JsonIgnoreon thedocumentfield + the computedgetDocumentId()getter pattern is not obvious at first glance. This is already commented well, so this is just an acknowledgement — no change needed.S3.
GeschichteService.list()—personCountcomputed frompersonIdscould silently diverge fromsafePersonIds.The sentinel logic:
When
personIdsisnull,personCount=0andsafePersonIdsis the sentinel — correct. WhenpersonIdsis empty,personCount=0(via.size()) butsafePersonIdsis the sentinel — also correct. The edge case to watch: if a caller passesList.of(),personIds.size()is 0 and the sentinel is used. This is correct. The logic is sound but the dual-condition is slightly fragile. Consider a named helper likepersonFilterArgs(personIds)returning a record(safeIds, count). Minor — not a blocker.S4.
GeschichteEditor.svelte—initialDocumentsprop removed butdocumentIdsalso removed from theonSubmitpayload type.The type signature change is clean and the test (
GeschichteEditor.svelte.spec.ts) was updated to match. Confirmed no danglingdocumentIdsreferences in the page components. ✅S5.
GeschichteControllerTest—list_returns200_forReaderonly asserts status 200 and$[0].title.Missing assertion: does the response contain the new
typefield? The test hasGeschichteType.STORYinsummaryStub(), so adding.andExpect(jsonPath("$[0].type").value("STORY"))would guard against accidentally dropping the type from the serialized response.What's done well
Hibernate.initialize(g.getItems())pattern inside@Transactional(readOnly = true)is the right solution foropen-in-view: false. It's explicit, testable, and documented.GeschichteSpecifications.hasDocument()with a clearTODOcomment is cleaner than leaving dead code.ON DELETE SET NULLtest (deleting_document_sets_item_document_to_null_not_delete_item) is exactly the kind of database-constraint test that catches real production bugs.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
From an infrastructure and CI perspective, this PR is clean. The Flyway migration is the only infrastructure-touching change, and it's handled correctly. No new Docker services, no compose file changes, no action version updates needed.
No Blockers
What I checked
Flyway migration (V72): The migration runs as a standard DDL transaction —
ALTER TABLE,CREATE TABLE,CREATE INDEX,INSERT INTO ... SELECT,DROP TABLE. TheDROP TABLEat the end is the only irreversible step, and the migration correctly documents this with the pre-requisitepg_dumpcommand. The migration will run cleanly in CI via Testcontainers (every@SpringBootTestruns Flyway from scratch). ✅No new infrastructure components: No new Docker services, no new volumes, no new environment variables. The
journey_itemstable is owned by the existing PostgreSQL instance. Monthly bill unchanged. ✅Production deployment note: The V72 migration's documented pre-requisite — take a
pg_dumpofgeschichten_documentsbefore applying — is a manual step that needs to happen beforedocker-compose pull && docker-compose up -d. This is documented in the migration file itself, which is the right place for it. A production deployment checklist entry would be nice, but the inline documentation is sufficient.No CI workflow changes needed: The Gitea Actions workflow doesn't need updating — the backend integration test suite already runs Flyway on every PR via Testcontainers. The V72 migration will be exercised on the CI run for this PR.
Suggestion
S1. Consider adding a
LIMITclause note in the migration comments. TheINSERT ... SELECTin Step 4 is unbounded — for an archive with thousands ofgeschichten_documentsrows, it runs as a single transaction. This is fine for the expected data volume (family archive), but worth a comment for future reference. The existing comment "no DDL rollback path exists after commit" implicitly covers this, but an explicit "this is safe for expected row counts ≤ 10,000" would close the loop.That's it from my end — no infrastructure concerns with this PR.
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
This PR delivers the foundational data model for Lesereisen (issue #750) and is clearly spec-driven. The scope is well-contained: data model, migration, and frontend stub rendering. The PR description calls out ASSUMPTION AS-001 and AS-002 explicitly — this is excellent requirements practice. I'm reviewing for requirements traceability, scope completeness, and open questions that could affect the follow-on issues.
Blockers
None that would block this specific PR (it explicitly scopes itself as "foundational data model only").
Concerns (not blockers for this PR, but should be tracked as issues)
C1. Removed
documentIdfilter from the list endpoint — no follow-on issue tracked.The PR removes
GeschichteSpecifications.hasDocument()and leaves a// TODO(lesereisen-editor): restore document filter via journey_items join when editor landscomment. This is a regression in observable API surface: callers who used?documentId=to find stories referencing a specific document can no longer do so. The PR description doesn't mention this as a follow-on issue. There should be a Gitea issue tracking the restoration of this filter viajourney_items. If the filter was only used internally (no current UI uses it), that should be confirmed — but either way, the TODO needs a ticket number.C2. ASSUMPTION AS-002 acknowledges a reader degradation that is unquantified.
The migration note says existing published Geschichten "render the related-letters block; this block visibly degrades to generic links (loss of per-document title AND date) for ALL current readers during the stub window." The PR is accepted on the basis that "the reader follow-on is the next-priority blocking dependency." This is a conscious product trade-off, but the acceptance criteria for the follow-on reader issue should include: "the document-backed item renders with the document title and date (not
note ?? 'Open document')." That AC should be explicitly recorded in the follow-on issue.C3.
notefield on a document-backedJourneyItem— what does it mean?The CHECK constraint is
document_id IS NOT NULL OR note IS NOT NULL. AJourneyItemcan have both adocument_idAND anote. In the current reader stub (geschichten/[id]/+page.svelte), document-backed items render as{item.note ?? m.geschichten_document_link_placeholder()}— meaning thenoteis shown as the link label instead of the document title. This conflates "editorial annotation for a document stop" with "link label text." The requirements for hownotebehaves on a document-backed item (annotation? label? tooltip?) need to be defined in the Lesereisen reader issue before the UI is built. Not a blocker here, but a required AC for the follow-on.C4. No
typefilter on the list endpoint.Currently
GET /api/geschichtenreturns bothSTORYandJOURNEYtype Geschichten mixed together. The reader UI may need to filter by type (e.g., show Lesereisen separately from Stories). This is a follow-on requirement that should be tracked.What's well-specified
positiongap strategy (multiples of 1000) is a known pattern for drag-reorder UX — good to have it codified in the schema from the start.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
The PR does not introduce new attack surface — there are no new endpoints, no new user-controlled inputs to API-accessible paths, and all write paths remain behind
@RequirePermission(Permission.WRITE_ALL)or@RequirePermission(Permission.BLOG_WRITE). The XSS mitigations are intact. I ran through the OWASP Top 10 relevant to this diff.What I checked and found clean
Injection (CWE-89/JPQL injection): The new
findSummariesJPQL query uses named parameters (@Param) exclusively —:effectiveStatus,:authorId,:personIds,:personCount. No string concatenation. The subquery for person count usesIN :personIdswhich is correctly parameterized. ✅Authorization boundary (
BLOG_WRITEgating):getById()andlist()correctly gate DRAFT visibility behindcurrentUserHasBlogWrite(). Critically,getById()returnsNOT_FOUND(notFORBIDDEN) for DRAFTs when the caller lacksBLOG_WRITE— this prevents information leakage (DRAFT existence is not revealed). ✅XSS (CWE-79): The
JourneyItem.notefield carries a comment// CWE-79 tripwire: plain text — store verbatim, no sanitization. Any HTML/feed/PDF/email renderer MUST escape this; only Svelte {note} is auto-safe.The frontend renders this with Svelte's{item.note}interpolation (not{@html}), which auto-escapes. This is the correct pattern. The comment documenting the trust boundary is exactly what I want to see. ✅Mass assignment:
GeschichteUpdateDTOcorrectly removeddocumentIds— there's no path for a caller to inject document IDs into the system via the update endpoint now. ✅Sentinel UUID in JPQL:
List.of(UUID.fromString("00000000-0000-0000-0000-000000000000"))is passed when thepersonCount = 0guard short-circuits theIN()predicate. This is not a security concern — the predicate is never evaluated whenpersonCount = 0. The UUID value doesn't match any real entity. ✅One security smell (not a vulnerability)
The
@JsonIgnore+ computed getter pattern onJourneyItem.documenthas an implicit contract dependency.This exposes
documentIdto the serialized response but hides the fullDocumentobject. The security assumption is: callers only get the UUID, not the document contents. This is correct given the currentgetById()transaction initializes items. However, ifJourneyItemis ever serialized outside a transaction (e.g., in a projection or a different endpoint),document.getId()could trigger a lazy-load outside a Hibernate session. The existing tests cover thegetById()path, but this is a contract worth documenting explicitly on the getter: "Only call when a Hibernate session is active, or when document is already initialized."This is a smell, not a vulnerability — the current code paths are safe. No fix required for this PR.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
The test coverage for this PR is genuinely good. Three distinct test classes were written for new behavior, all using real PostgreSQL via Testcontainers, and the
GeschichteHttpTestspecifically avoids@Transactionalat the class level to prevent maskingLazyInitializationException— that is the correct and thoughtful choice. Test names are descriptive sentences. Let me walk through the layers.No Blockers
Test pyramid assessment
Unit layer (
GeschichteServiceTest): Updated correctly. The migration fromfindAll(Specification, Sort)tofindSummaries(...)is verified withany()matchers, which is appropriate since the exact query behavior is covered at the integration layer. The newlist_caps_limit_at_max_when_caller_passes_huge_valuetest replaceslist_filters_by_documentIdcleanly. ✅Integration layer — repository (
GeschichteListProjectionTest):sentinel()helper is documented with a comment explaining why a dummy UUID is used. ✅findSummarieswitheffectiveStatus = DRAFTandauthorId = null(the case where aBLOG_WRITEuser requests all drafts). ThehasAuthorcondition in the JPQL is(:authorId IS NULL OR g.author.id = :authorId)— whenauthorIdis null, all drafts are returned. This is the "writer sees all drafts" path and should have a test.Integration layer — entity (
JourneyItemIntegrationTest):@OrderBy, cascade delete, orphan removal, type round-trip, type default, note-only, document-backed, ON DELETE SET NULL, CHECK constraint. This is comprehensive constraint coverage. ✅@Transactionalat class level withem.flush(); em.clear()between arrange and assert is the correct pattern for testing JPA ordering and lazy-loading. ✅HTTP layer (
GeschichteHttpTest):@Transactionalat class level — that would keep a session open and maskLazyInitializationException" is exactly the reasoning I want to see documented. ✅POST /api/geschichten(create) with the new API shape (nodocumentIds). TheGeschichteControllerTestunit tests cover the shape indirectly, but an HTTP-layer create test would close the regression loop. Minor — the existingGeschichteServiceIntegrationTestlikely covers creation.Frontend tests (
GeschichteEditor.svelte.spec.ts,GeschichtenCard.svelte.spec.ts,GeschichtenCard.svelte.test.ts):docFactoryremoval and replacement ofdocuments: []→items: []is clean.draftFactorynow includestype: 'STORY'— correct.GeschichtenCard.svelte.test.tsnow properly types the factory return asGeschichte(the full entity type) rather than usingas unknown. This is a meaningful improvement in type safety. ✅Suggestions
S1. Add a
GeschichteListProjectionTestcase forauthorId = nullwitheffectiveStatus = DRAFT— the "BLOG_WRITE user requests all drafts, not just their own" path.S2. The
GeschichteHttpTest.list_returns_200_and_does_not_500_when_stories_have_journey_itemstest name is accurate but long. Consider:list_does_not_500_for_journeys_with_lazy_items. Minor style note.S3.
GeschichteControllerTest.summaryStub()returnsnullforgetAuthor(). SinceGeschichteSummary.AuthorSummaryis a nested interface projection, a test wheregetAuthor()returns a non-null value would guard against NPEs in any future template code that accessesauthor.firstNamewithout null checking.🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
This PR is primarily a data model and migration PR — the frontend changes are intentionally minimal stubs to unblock subsequent reader UI work. I'm reviewing the frontend changes that do ship to users now, with the understanding that the full Lesereisen reader experience is a follow-on. My concerns are scoped accordingly.
Blockers
1. Document link renders title as
item.note ?? m.geschichten_document_link_placeholder()— stub text is misleading.In
geschichten/[id]/+page.svelte, document-backed journey items now render as:The
m.geschichten_document_link_placeholder()resolves to "Dokument öffnen" (de), "Open document" (en), "Abrir documento" (es).For existing migrated
geschichten_documentsrows that had real document titles, this renders as a generic "Open document" link instead of the document title — because migratedJourneyItemrows havedocument_idset butnote = NULL. Users who previously saw "Brief von Eugenie Raddatz, 12. März 1923" in a story now see "Open document." This is the ASSUMPTION AS-002 degradation the PR acknowledges.However, the link label "Open document" is not actionable for a reader who doesn't know which document it leads to. Given this ships to all users immediately on merge, I'd recommend one of:
+page.server.tsand pass it to the template (requires a follow-on API call per item — acceptable for detail page)[Dokument {position/1000}]label as the fallback instead of "Open document" — at least communicates sequence positionOption (c) is the PR's stated intent (ASSUMPTION AS-002). If that's the decision, I'd ask that the follow-on issue explicitly includes "restore document title display on journey item links" as a must-have acceptance criterion so it doesn't get deferred indefinitely.
This is a UX concern, not a code bug. Marking as a blocker because real users will see degraded content on existing stories starting from this merge — the product owner should explicitly accept this trade-off.
Suggestions
S1. The "Dokumente" section heading in the detail page is unchanged — it still says the i18n key
geschichten_documents_section("Erwähnte Dokumente" / "Referenced documents"). For a JOURNEY, these are sequenced stops, not unordered referenced documents. A follow-on might want a different heading like "Dokumente in dieser Lesereise." Not urgent, but register it for the reader issue.S2. The filter
g.items.filter((i) => i.documentId)in the template shows only document-backed items. Note-only items (editorial interludes) are invisible in the current stub. For a STORY (migrated), this is fine — there should be no note-only items. For a JOURNEY with intentional note-only items (created by the future editor), these would silently disappear from the reader. The stub is a data model PR, so this is acceptable for now — but the follow-on reader issue must render note-only items too.S3. Accessibility on the new link rendering: The link markup:
Uses
{item.note ?? placeholder}as both visual label and accessible name. Whenitem.noteis null and the placeholder "Open document" is used, screen reader users hear "Open document" for every link — multiple identical accessible names on the same page (WCAG 2.4.6, also a WCAG 2.4.9 AAA issue). The follow-on reader UI must include unique accessible names per document link. For the current stub this is acceptable (same degradation acknowledged in ASSUMPTION AS-002).What's clean
{#each ... as item (item.id)}with a stable key expression. ✅Second-pass review blockers addressed — commit
b18198671.
CLAUDE.mdDomain Model table missing Geschichte/JourneyItem (Markus Keller)Geschichterow:GeschichteType (STORY/JOURNEY), ManyToMany persons, OneToMany itemsJourneyItemrow: ManyToOne geschichte (ON DELETE CASCADE), ManyToOne document (ON DELETE SET NULL), position, optional note2.
GeschichteSummarylacking@Schema(requiredMode = REQUIRED)(Markus Keller)getId(),getTitle(),getStatus(),getType()— all always non-nullAuthorSummary.getEmail()— email is NOT NULL in app_usersgetAuthor(),getPublishedAt(),getBody()unannotated (nullable)3. Issue #786 updated (Leonie Voss)
🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Clean foundational model. The structural choices are all defensible and the documentation was updated correctly across every doc that the architecture rules require. Let me work through my checklist.
Architecture Compliance Check
DB diagrams — required for every Flyway migration adding/removing tables:
Both
docs/architecture/db/db-orm.pumlanddocs/architecture/db/db-relationships.pumlupdated.geschichten_documentsreplaced withjourney_items, FK annotations added. ✅New entity/package — CLAUDE.md + C4 L3:
JourneyItemis ingeschichte/journeyitem/— a logical sub-package, not a new domain. CLAUDE.md domain model table updated with bothGeschichteandJourneyItem. C4 L3 (l3-backend-3g-supporting.puml) updated forGeschichteControllerandGeschichteServicedescriptions. ✅New domain term — GLOSSARY.md:
JourneyItem,Lesereise, and the updatedGeschichteentry are all present. ✅ARCHITECTURE.md — cross-domain dependencies:
geschichtedomain description updated to reflectJourneyItem.document_idON DELETE SET NULL semantics. ✅No new SvelteKit route added (existing routes modified). No new Docker service. No new ErrorCode or Permission. ✅
Data Model
The
ON DELETE SET NULLchoice onfk_journey_items_documentis architecturally correct: archive deletions should not cascade-destroy a curator's Journey. The CHECK constraint(document_id IS NOT NULL OR note IS NOT NULL)pushes integrity to the database layer — exactly what I want. ✅Sentinel UUID pattern in
GeschichteService.list(): The00000000-0000-0000-0000-000000000000sentinel avoids an invalid emptyIN()clause. This is a code-level workaround for a JPQL limitation. It works and is documented inline. The alternative would be a native query or a@Querywith conditional logic — both are more complex. Acceptable. No blocker.GeschichteSummaryas a Spring Data interface projection avoids theLazyInitializationExceptionon the list path cleanly. ThePersonSummaryDTOprecedent is correctly cited. ✅ASSUMPTION AS-001 (ordering by
meta_date) is explicitly documented in the migration SQL with the right framing: "best available approximation, not a requirement." This is precisely what an ADR comment should say. ✅MultipleBagFetchException risk is correctly documented in
Geschichte.javawith an explicit comment. The firstListon this entity is tracked; the risk is called out for future developers. ✅Boundary / Layering
DocumentServiceis retained inGeschichteServicewith a comment explaining why it's reserved for the lesereisen-editor follow-on. The comment explicitly notes that document resolution must go throughDocumentService.getDocumentById()— not the repository directly. Boundary discipline preserved. ✅The new
JourneyItemRepository.findAllByGeschichteId()is only used by tests. Production access toJourneyItems goes through theGeschichteentity'sitemscollection. No service-to-repository boundary leak. ✅Suggestions (non-blocking)
TODO(lesereisen-editor)comment inGeschichteSpecifications.java— this is a good practice but it would be even better tracked as a Gitea issue reference. If the linked follow-on issue exists, add// TODO #xxxso it doesn't become orphaned tech debt.Limit applied after the DB query (
findSummaries(...).stream().limit(safeLimit).toList()) — the JPQL query fetches up toMAX_LIMIT(200) rows from Postgres and then trims in Java. For the current scale this is fine; at higher volumes aPageableparameter would push theLIMITto the database. Not a blocker for this PR.ADR cross-reference — the migration comment references
ADR-022for the LAZY fetch strategy. Worth confirming that ADR exists; if it doesn't, the comment should link to what does document this decision.Overall: clean data model PR, good documentation hygiene, appropriate database-layer integrity enforcement. The sentinel pattern and lazy-init guard are both well-commented.
Approved.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Solid TDD execution and clean domain model. The backend is well-structured. I have one blocker on the frontend rendering logic, plus several suggestions.
Blockers
[id]/+page.svelte— document link label usesitem.noteas titleitem.noteis the editorial annotation of a stop — it can be multi-paragraph prose. Using it as the visible link text on a document-backed item produces broken UX: a 300-word editorial note as a link label. The old code showedd.title+d.documentDate. The new code loses both.This is a conscious stub decision (follow-on issue for the full Journey reader), but the current render is actively wrong for the document-backed case: a
JourneyItemwithdocumentIdANDnotewill display the note as the clickable label instead of the document title, which the frontend no longer has access to (onlydocumentIdis in the API response).The fix options:
m.geschichten_document_link_placeholder()for all document-backed items in the stub window (removeitem.note ??), and render the note separately as a non-link annotation.<!-- TODO: fetch document title in follow-on -->comment so the next developer knows why it looks wrong.This is a blocker because it actively misleads users: they see editorial prose as a clickable link label rather than a document title. Either fix the rendering or at minimum separate the note from the link label.
Code Quality Findings
JourneyItem.java—@JsonIgnore+ virtual getter pattern is clever but fragileThe comment correctly explains the intent. The risk:
@JsonIgnoreworks here because the field is nameddocument. If Lombok's@Datais ever replaced with arecordor the field renamed, the serialization contract silently breaks. This is worth a brief test inJourneyItemIntegrationTestthat checks the JSON containsdocumentIdand NOTdocument.Currently
GeschichteHttpTest.getById_returns_200_with_items_and_does_not_500_open_in_view_falsechecks the HTTP 200 and body content but doesn't assert the JSON shape ofitems. Suggestion only.GeschichteService.list()— sentinel UUID leaks a magic constantThe zero-UUID is repeated in
GeschichteListProjectionTest.sentinel()too. Extract to a named constant:This reveals intent and prevents divergence if the sentinel value ever needs to change.
GeschichteSummary.AuthorSummary—getEmail()is marked@Schema(requiredMode = REQUIRED)butfirstName/lastNameare notThe AuthorSummary interface exposes
emailas required butfirstName/lastNameas optional. That's consistent withAppUserhaving email as the mandatory field. Fine as-is, but worth a note: the frontendauthorName()function silently falls back toemailif both name fields are empty — this is robust. ✅GeschichteRepository.findSummaries()—@Paramnaming is consistent ✅GeschichteControllerTest.summaryStub()comment is accurateGood rationale. The anonymous class is the right call for Jackson-serialized test stubs. ✅
GeschichteEditor.svelte— document picker removal is cleanThe removal of
DocumentMultiSelectfrom the editor is intentional and well-scoped. The comment in the issue body references the follow-on issue. No dead code left behind. ✅Svelte
{#each}keying — correct throughoutAll
{#each}blocks that changed use(item.id)keying. ✅Summary
The backend is well-structured: guard clauses,
@Transactional(readOnly=true)correctly placed ongetById(),DomainExceptionusage consistent. The TDD evidence is strong — 22+ new tests across 3 test classes.The one blocker is the
item.noteused as link label, which is a genuine UX regression from the oldd.titlerendering.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No new authentication or authorization attack surface introduced. Existing permission boundaries are preserved. The
notefield handling deserves attention.What I Checked
Permission gates — no regressions:
All write endpoints on
GeschichteControllerretain@RequirePermission. The@GetMappinglist endpoint still requires authentication (nopermitAll()regression). The removal ofdocumentIdfrom list params doesn't weaken access control — it was never a security gate, just a filter. ✅JPQL in
GeschichteRepository.findSummaries()— injection-free:All parameters use
@Paramnamed parameters:No string concatenation. JPA parameterization is correct. ✅
JourneyItem.note— CWE-79 tripwire comment is present and accurate:The Svelte template uses
{item.note}(text interpolation, not{@html}), so XSS is not possible in the current frontend render path. The comment correctly flags that other rendering contexts (PDF export, email digest, RSS feed) must escape this field explicitly. This is precisely the kind of threat-model comment I want to see. ✅GeschichteService.getById()—@Transactional(readOnly=true)added:The previously non-transactional read method is now properly marked. This is a correctness fix, not a security issue, but worth noting: read-only transactions prevent accidental writes during the session. ✅
No new
@CrossOriginannotations. No actuator exposure changes. No new auth bypass paths. ✅Findings
Low severity / smell —
GeschichteHttpTestuses hardcoded test credentials in source:This is in a test class with
@ActiveProfiles("test"), not production code. The password is never used outside the test suite and is seeded fresh in@BeforeEach. Not a real vulnerability — test credentials in test classes are acceptable practice. No action required.Observation — the sentinel UUID
00000000-0000-0000-0000-000000000000inGeschichteServiceis passed toIN :personIdsbut never evaluated whenpersonCount=0:The JPQL predicate is
(:personCount = 0 OR (SELECT COUNT...) = :personCount). WhenpersonCount=0, the OR short-circuits and theINclause is never reached. No security implication — just confirming the logic is sound. ✅No new file upload paths, no new S3 interactions, no new deserialization entry points. ✅
Summary
This PR is security-clean. The
notefield has an appropriate threat-model comment, all database access is parameterized, permissions are preserved, and no new attack surface is introduced. The follow-on journey editor will need a careful review of any future write paths that acceptJourneyItemcontent from the client — particularly thenotefield deserialization and any future reordering endpoint that acceptspositionvalues.Approved.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
Strong test coverage for a foundational model PR. TDD evidence is visible throughout — the test files are well-organized and cover the right behaviors at the right layers. A few gaps worth noting.
Test Coverage Assessment
JourneyItemIntegrationTest— 9 tests,@SpringBootTest+@Transactional:@OrderByposition ordering ✅GeschichteTyperound-trip ✅GeschichteTypedefault ✅The CHECK constraint test is particularly important: it uses
assertThatThrownBy()correctly and callsflush()to force the constraint check. ✅The
@Transactionalclass annotation is appropriate here — each test rolls back, preventing cross-test contamination. ✅Real PostgreSQL via Testcontainers (not H2). ✅ The CHECK constraint and
ON DELETE SET NULLwould be invisible in H2.GeschichteListProjectionTest— 8 tests,@DataJpaTest:Good coverage of the projection contract. The
sentinel()helper function is clearly named and its purpose is documented. ✅GeschichteHttpTest— 5 tests,@SpringBootTest(RANDOM_PORT)without@Transactional:The deliberate omission of
@Transactionalat the class level is correctly explained in the class Javadoc:This is the right call for this specific test class. The lazy-init guard test (
getById_returns_200_with_items_and_does_not_500_open_in_view_false) is the canonical regression test for the most production-risky behavior in this PR. ✅Gaps (suggestions, not blockers)
Missing JSON shape assertion in
GeschichteHttpTest.getById_returns_200_with_items_and_does_not_500_open_in_view_false:The test verifies the response body contains
"Prolog"and"Epilog"as strings, but doesn't assert that:items[0].documentIdis absent (null) for note-only itemsitemsis present as a JSON array keydocument(the full entity) is NOT present (regression guard for@JsonIgnore)This is a suggestion — the current test catches the most important failure (no 500). Adding
assertThat(response.getBody()).contains("\"items\"").doesNotContain("\"document\"")would add a cheap serialization contract guard.No test for
list()DRAFT visibility gate inGeschichteHttpTest:The
GeschichteServiceIntegrationTestcovers the draft gate, butGeschichteHttpTestdoesn't test that a user withoutBLOG_WRITEsees zero drafts via HTTP. The existinggetById_returns_404_for_draft_when_reader_lacks_BLOG_WRITEcoversgetByIdbut notlist. Low risk givenGeschichteServiceIntegrationTestcovers this path.GeschichteControllerTest— test for removeddocumentIdquery param:The old
list_filters_by_documentIdtest was replaced withlist_caps_limit_at_max_when_caller_passes_huge_value(a net new test). This is correct behavior — the documentId filter is removed. No missing coverage.Test Quality
Naming — all test method names read as sentences ✅
items_are_returned_in_position_order_regardless_of_insertion_orderis a good example.em.flush()+em.clear()pattern — used consistently before re-loading from DB, preventing first-level cache hits from masking persistence failures. ✅BeforeEachsetup — lean, uses builders, no giant setup methods. ✅No
@Disabledtests. ✅GeschichteControllerTest.summaryStub()anonymous class — correct choice given Jackson doesn't reliably serialize Mockito interface proxies. ✅Summary
22+ tests across 3 new test classes, all at the appropriate test pyramid layers. The
GeschichteHttpTestanti-@Transactionalpattern is the most important correctness choice in this PR's test suite. CI time impact is bounded by the existing Testcontainers setup (shared container across classes). No blockers.Approved.
🎨 Leonie Voss — UI/UX Design Lead & Accessibility Advocate
Verdict: ⚠️ Approved with concerns
The backend and data model changes are out of my scope. I'm reviewing only the frontend changes:
geschichten/[id]/+page.svelte,GeschichteEditor.svelte, and the i18n strings. One concern I'd like addressed before merge.Concern (not a hard blocker, but important for the senior audience)
Document link label regression in
[id]/+page.svelte:The old render showed
d.title(the document title) and optionallyd.documentDate. Both are meaningful to a 60+ reader who recognizes letters by date and correspondent. The new render shows:"Dokument öffnen"/"Open document"(when note is absent) — this tells the user nothing about what document they're about to open.For a senior reader navigating a Lesereise, both states are degraded from the previous experience. The note text alongside a link label would be the correct rendering: show the document title (or placeholder) as the link, then show the note as a separate element below.
I understand this is a conscious stub for the follow-on Journey reader, but users in production will see this degraded state in the interim. At minimum, show
m.geschichten_document_link_placeholder()as the link text for document-backed items (never the note), and hide the notes section entirely until the Journey reader is ready.Suggested stub rendering:
What Looks Good
Svelte text interpolation for
item.note— when it is eventually rendered,{item.note}is auto-escaped, so no XSS risk from the template. ✅GeschichteEditor.svelte— document picker removal — the section was removed cleanly without leaving an empty<aside>or orphaned heading. ✅i18n keys added in all three languages (de, en, es) —
geschichten_document_link_placeholderis present in all three message files. ✅Keyed
{#each}with(item.id)— correct list reconciliation. ✅<article aria-labelledby="geschichte-title">present in the existing template — semantic landmark structure is preserved. ✅Focus styles on the document link — the existing link class does not include focus-visible ring styles:
This is unchanged from the previous
d.titlerendering — so not a regression introduced by this PR. Worth a follow-on fix in the Journey reader implementation.Summary
The interim rendering of document links is degraded from the previous experience, particularly for the senior audience who rely on document titles and dates to orient themselves. The note-as-label pattern is the specific issue. This is addressable with a 2-line change (use the placeholder for all document-backed links, suppress notes until the Journey reader is ready).
I'm approving with this concern flagged — the backend model is correct and the degraded state is a known stub window.
Approved with concern.
🖥️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR — no Compose file, no CI workflow, no Dockerfile. My review is confined to migration safety and operational concerns around the
DROP TABLEstep.Migration Safety
V72 —
DROP TABLE geschichten_documents:The migration includes a production pre-requisite comment with the exact
pg_dumpcommand needed before running it:This is good practice. A few operational notes:
The dump path
/tmp/pre_v72_backup_YYYYMMDD.sqlwrites inside the container. Ondocker compose downthat disappears. Before running this migration in production, copy the dump out of the container:The migration comment doesn't mention this. Worth adding, or noting in the deploy runbook.
The
DROP TABLEis flagged as the only irreversible step. The reverse procedure is documented. ✅geschichten_documentsis likely empty in production (new feature table), but the comment correctly states the dump is valuable even for an empty table because it captures the table definition. ✅No multiple transactions in the migration — all DDL is in one Flyway migration, so Flyway wraps it in a single transaction. If
CREATE TABLE journey_itemssucceeds butDROP TABLE geschichten_documentsfails, Postgres rolls back the entire migration. Flyway will mark it as failed and not re-run it. This is correct behavior. ✅Index on
journey_items (geschichte_id, position ASC)— appropriate for the primary access pattern (fetch all items for a journey, ordered by position). ✅CI Impact
No new infrastructure dependencies. The Testcontainers setup (
postgres:16-alpine) already handles the migration test in CI — V72 will be run as part of the standard integration test suite. No CI configuration changes required. ✅No Concerns
No Docker Compose changes, no new services, no image tag changes, no CI action version changes, no secrets. Nothing infrastructure-related to flag.
Approved.
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Reviewing from a requirements completeness perspective. The data model is well-aligned with the Lesereisen concept. I have one concern about an implicit requirement that appears in the stub window behavior, and a few open questions for the follow-on issues.
Requirements Coverage
FR covered by this PR:
GeschichteTypediscriminator (STORY/JOURNEY) — allows the system to distinguish prose stories from reading journeys ✅JourneyItementity with ordered positions, optional document reference, optional editorial note ✅ON DELETE SET NULLsemantics — a journey survives document deletions (archive integrity) ✅(document_id IS NOT NULL OR note IS NOT NULL)— no empty stops ✅geschichten_documentsrows — backward compatibility ✅GeschichteSummary— no LazyInit 500 ✅Implicit requirement gap — stub window UX:
The PR description notes this is a foundational model PR with the reader UI in a follow-on issue. However, ASSUMPTION AS-002 in the migration SQL acknowledges:
This is an accepted degradation, but there is no linked follow-on issue number to anchor the commitment. From a requirements standpoint, the acceptance criteria for "the reader sees document links" is partially met (links exist) but the information richness requirement (title + date) is deferred without a tracked issue.
Recommendation: The
<!-- TODO: fetch document title in follow-on -->comment or a link to the follow-on issue should be present in[id]/+page.svelteso the degraded state is not silently forgotten. This is the same concern Felix and Leonie raised from their respective perspectives.Open Questions for Follow-on Issues
These are not blockers for this PR, but they should be captured in the follow-on Journey editor issue to prevent requirement drift:
Position management: The
positionfield uses gaps of 1000 for drag-reorder headroom. Who is responsible for assigning initial positions — the client or the server? What happens when two items end up with the same position after a concurrent edit? (The entity docs say "undefined relative order" — is that acceptable for the editor UX?)STORY vs JOURNEY rendering: The detail page (
[id]/+page.svelte) currently renders allGeschichtetypes identically (body + persons + document-backed items). Will JOURNEY types need a distinct layout (e.g., no prose body, items as the primary content)? Thetypefield is now available on the frontend viaGeschichteSummary, but the detail page receives the fullGeschichteentity which also hastype— is the detail page expected to branch ontype?Note-only items: The CHECK constraint allows items with
noteonly (nodocument_id). Does the follow-on reader UI render note-only items as editorial interludes? The current frontend only shows document-backed items (g.items.filter((i) => i.documentId)) — note-only items are silently hidden.documentIdinGeschichteSummary: The list projection deliberately omitsitems. When a stories-list card needs to indicate "this is a Journey with 5 stops," where does that count come from? A count field in the projection, or a follow-on endpoint?Positive Findings
The ASSUMPTION AS-001 and AS-002 documentation in the migration SQL is exactly the kind of explicit requirements documentation I want to see alongside schema changes. The naming (
JOURNEY,JourneyItem,Lesereise) is consistent across code, docs, and i18n. The Glossary entries are precise. ✅Approved — with the recommendation to link or create the follow-on issue before merge, so the stub window acceptance criteria are tracked.
Third-pass review blocker addressed — commit
45500cc5item.noteused as anchor label for document-backed JourneyItems (Felix, Leonie, Elicit)item.noteis editorial prose, not a document identifier. Using it as the<a>label meant a multi-paragraph note could render as the clickable link text.Fix: always use
m.geschichten_document_link_placeholder()as the link label; renderitem.noteas a<p class="text-sm text-ink-3">caption below the link when present. Added<!-- TODO(#786) -->so the stub is tracked back to the follow-on reader issue.🏛️ Markus Keller (@mkeller) — Architect Review
✅ Approved
This is solid foundational work. The data model decision to move from an unordered
Set<Document>to an orderedList<JourneyItem>with a discriminator column is correct — it's the only shape that can represent a curated reading sequence. TheON DELETE SET NULLdesign for the document FK is the right call; archive deletions shouldn't collapse journeys. TheGeschichteSummaryprojection pattern mirrorsPersonSummaryDTOand correctly solves theLazyInitializationExceptionproblem inherent inopen-in-view: false. All of this is boring, appropriate technology.Documentation compliance check (required by my review protocol)
journey_items, dropsgeschichten_documents, addstypecolumndb-orm.puml,db-relationships.puml@OneToMany/ FKdocs/GLOSSARY.mdJourneyItem,Lesereise,GeschichteupdatedgeschichtepackageCLAUDE.mddomain model tableGeschichteandJourneyItemrowsGeschichteController/GeschichteServicedescriptionl3-backend-3g-supporting.pumldocs/ARCHITECTURE.mdgeschichtedomain descriptionThe documentation coverage is complete. This is rare — flag it as a positive signal for the team.
Findings
Observation (not a blocker) —
GeschichteSpecifications.hasDocument()removal leaves a documented TODOThe TODO is well-placed and references the follow-on issue. The specification class now has dead import cleanup from the removed
hasDocument/documentSubquerymethods. Confirmed clean.Observation —
JourneyItemRepository.findAllByGeschichteId()is unused in this PRThe repository method exists but nothing in this PR calls it directly — items are accessed via the owning
Geschichte.getItems()collection. This is fine for the follow-on editor, but worth noting that the repository ships before its first call site.Observation — sentinel UUID approach in
GeschichteService.list()This is a pragmatic workaround for JPA's invalid-empty-IN() SQL generation. The
personCount = 0guard in the JPQL query short-circuits the predicate, so the sentinel is never evaluated against real data. The comment explains the contract. Acceptable — but if this pattern recurs, a named constantSENTINEL_UUIDwould make it self-documenting across future callers.Observation — No ADR for the STORY/JOURNEY discriminator pattern
The decision to use a single-table discriminator column (
type) rather than table-per-type inheritance or a separateLesereiseentity has lasting consequences. The@InheritanceType.SINGLE_TABLEapproach was implicitly chosen (plain column, not JPA@Inheritance). This is the right call for the current team size, but an ADR indocs/adr/would capture why single-table was chosen over@Inheritance(SINGLE_TABLE)or a separate entity. Not a blocker — this PR's scope is the data model, not the inheritance strategy documentation — but flag for the follow-on.Summary
Domain boundary is clean:
journey/sub-package undergeschichte/is the right location. TheDocumentServicereservation comment inGeschichteServiceis a good signal that the team is thinking about boundary enforcement for the editor. TheGeschichteSummaryinterface projection over a JPQL query is architecturally superior to the previous Specification+Sort approach for this read path.No blockers. Approved.
👨💻 Felix Brandt (@felixbrandt) — Developer Review
⚠️ Approved with concerns
The backend is clean and well-structured. A few code quality issues in the frontend and one reliability concern in the backend service worth flagging.
Backend
✅ Entity code style is correct —
JourneyItemuses the full@Data @NoArgsConstructor @AllArgsConstructor @BuilderLombok stack,@Schema(requiredMode = REQUIRED)on every non-null field. ThegetDocumentId()virtual getter is well-commented.✅
@Transactional(readOnly = true)added togetById()— this was missing before; now present. TheHibernate.initialize(g.getItems())call inside the transaction boundary is the correct fix for the lazy-init problem withopen-in-view: false.✅ Guard clauses and
DomainExceptionusage — consistent with project style.Concern (suggestion) —
GeschichteService.list()— business logic in a multi-responsibility methodThe sentinel construction and count calculation are two separate data-transformation steps embedded in the method. The method still reads clearly enough, so this is a suggestion not a blocker — but extracting
buildSafePersonIds()and keeping the limit logic in one place would make the intent crisper.Concern (suggestion) —
GeschichteServiceTest— verify method usesanyLong()but not the actual valueThe unit tests verify the repository is called but don't assert the correct
personCountvalue is passed. For the person-filter tests, assertingeq(1L)oreq(2L)would prove the counting logic is correct, not just that the method was reached. This is test coverage quality, not a blocker.Frontend
Blocker —
[id]/+page.svelte— note rendered without XSS guardSvelte's
{item.note}text interpolation is auto-escaped — no raw HTML injection is possible here. The entity comment inJourneyItem.javaacknowledges this (// only Svelte {note} is auto-safe). This is actually correct as-is.Clarification: this is safe. Confirmed — not a blocker.
Suggestion —
[id]/+page.svelte— double-iterate ong.itemsTwo iterations over
g.itemsfor the same condition check and filter. This is trivial for the sizes involved, but a$derivedwould be cleaner:Then the template becomes
{#if documentItems.length > 0}/{#each documentItems as item (item.id)}. This is a style suggestion, not a blocker.Suggestion —
GeschichteEditor.svelte—initialDocumentsprop removed but editor now has no document pathThe PR description explicitly calls this out as a stub pending follow-on issue #786. The TODO comment in the template confirms intent. Accepted.
Observation —
GeschichtenCard.svelte.test.ts—makeGeschichtefactory now returns fullGeschichtetype (notGeschichteSummary)The card likely receives
GeschichteSummaryfrom the list endpoint. Verify the component acceptsGeschichteSummarynotGeschichte— if the card prop type isGeschichteit will includeitemsandpersonswhich the list projection doesn't carry. This is a type correctness concern worth checking before merge.Summary
Backend is solid. Frontend note-rendering is correctly safe (confirmed). One type-correctness concern on the card component prop type worth a quick check. Approving with the card-type suggestion flagged.
🔒 Nora "NullX" Steiner — Security Review
✅ Approved
I went through this PR with an adversarial lens. The security-relevant surface is: the new
notefield (potential XSS vector), theJourneyItemFK behavior (data exposure risk), and endpoint authorization coverage. Here's what I found.XSS —
item.notefieldFinding: Confirmed safe — no action needed
The backend stores the note verbatim (correct — no sanitization on write). The frontend renders it with Svelte text interpolation:
Svelte's
{...}syntax HTML-encodes all values. An attacker who stores<script>alert(1)</script>in a note will see it rendered as literal text. The comment inJourneyItem.javacorrectly documents this contract and the CWE-79 risk. No vulnerability.The comment also correctly warns that other renderers (email, PDF, RSS) must escape this field. That warning is load-bearing for the follow-on reader issue.
ON DELETE SET NULLbehavior — data exposureFinding: Design is correct — no exposure
When a document is deleted,
journey_items.document_idis set toNULLbut the item survives. ThegetDocumentId()accessor returnsnull. The frontend correctly checksi.documentIdbefore rendering a link:Items with
document_id = nullare filtered out — readers won't see dead links or null UUIDs. The item itself (its note/position) is preserved. No orphaned reference exposure.Authorization —
@RequirePermissioncoverageChecked:
GeschichteController. All write endpoints (POST,PATCH,DELETE) retain@RequirePermission(Permission.WRITE_ALL)orPermission.BLOG_WRITE. The list endpoint is read-only and unannotated — consistent with other list endpoints in the codebase. ThegetById()visibility gate (publishing check) is enforced in the service layer, not the controller, which is consistent with the pre-existing pattern.Note: The
GeschichteHttpTest.getById_returns_404_for_draft_when_reader_lacks_BLOG_WRITE()test validates the 404-visibility-as-security pattern explicitly. This is the right test to have.JPQL query — injection risk
All parameters use named
@Parambinding. No string concatenation. JPQL injection is not possible. Clean.Sentinel UUID
The
UUID.fromString("00000000-0000-0000-0000-000000000000")sentinel is passed as a parameter but the predicate is short-circuited before the IN() clause is evaluated. An attacker cannot influence this value (it is constructed server-side). No exposure.Summary
No security vulnerabilities found. The CWE-79 comment on
noteis exemplary — it documents the threat model for future renderers. The authorization coverage is complete. The 404-visibility pattern for drafts is tested.No blockers. Clean from a security perspective.
🧪 Sara Holt (@saraholt) — QA / Test Review
✅ Approved
This PR has one of the best test plans I've seen on this codebase. 22 new tests across three test classes, plus targeted updates to existing test classes. Let me go through each layer.
Test pyramid coverage
GeschichteServiceTest— 7 updated/newJourneyItemIntegrationTest— 9 tests@SpringBootTest+ TestcontainersGeschichteListProjectionTest— 8 tests@DataJpaTest+ TestcontainersGeschichteHttpTest— 5 tests@SpringBootTestRANDOM_PORTGeschichteControllerTest— updated@WebMvcTestGeschichteEditor.svelte.spec.ts,GeschichtenCard.svelte.spec.ts,GeschichtenCard.svelte.test.tsReal PostgreSQL via Testcontainers everywhere — no H2 in sight. The
@DataJpaTestuses@AutoConfigureTestDatabase(replace = NONE)+PostgresContainerConfig. The@SpringBootTestclasses use@ActiveProfiles("test")+@Import(PostgresContainerConfig.class). Exactly right.GeschichteHttpTest— specific praiseThis is the correct call. A class-level
@Transactionalwould give false confidence that lazy init is handled, because the test's own transaction would keep the session alive through serialization. By NOT annotating at class level, the test exercises the real production code path: service opens transaction → initializes items → closes transaction → Jackson serializes. Thelist_returns_200_and_does_not_500_when_stories_have_journey_itemstest name is a sentence that tells a developer exactly what regression it guards.JourneyItemIntegrationTest— 9 tests checked@OrderByround-trip: items inserted in position order 3000/1000/2000 and retrieved in 1000/2000/3000 order ✅getItems()list deletes the DB row ✅GeschichteTypeJOURNEY round-trip ✅GeschichteTypedefaults to STORY ✅ON DELETE SET NULL: deleting the referenced Document nullifiesdocument_id, item survives ✅All 9 behaviors tested. The
em.flush(); em.clear()pattern is used correctly to force SQL to execute and clear the first-level cache, making the assertions test actual database state. Factory methods are concise.GeschichteListProjectionTest— concernsSuggestion — test names for person-filter tests are adequate but could be sharper
This is a good name. The person-filter tests cover:
personCount = 0→ no filter (disabling path)One missing case: what happens when a story has more than the two requested persons? e.g., a story tagged A+B+C should appear in results for
personIds=[A,B]since it contains both. Not tested. Low priority since the JPQL logic isCOUNT(DISTINCT p.id) = personCountwhich handles this correctly, but a test would prove it.GeschichteServiceTest— concernThe tests verify
findSummariesis called but useanyLong()forpersonCount. This means the sentinel/count logic inlist()is not unit-tested — only the integration tests cover it indirectly. Not a blocker, but aneq(0L)oreq(1L)assertion on the personCount argument would catch a regression if someone changes the counting logic.Frontend component tests
GeschichtenCard.svelte.spec.ts— factory updated to includetype: 'STORY'anditems: []instead ofdocuments: []. Usesvitest-browser-svelterender. ✅GeschichtenCard.svelte.test.ts— factory now fully typed asGeschichtewith all required fields populated. Theas Geschichtecast at the end is needed because theGeschichtetype from the generated API includes all fields. ✅GeschichteEditor.svelte.spec.ts— document-related test removed, person-only test updated. Retained test coverage for the remaining behavior. ✅Migration test coverage
The
JourneyItemIntegrationTestandGeschichteListProjectionTestboth use Flyway viaPostgresContainerConfig. V72 runs as part of the migration stack, so the CHECK constraint, FK behaviors, and@OrderByare all tested against the real schema. ✅Summary
No blockers. The test suite for this PR is thorough and correct. The one gap (AND-semantics with a superset of persons) is low risk given the JPQL logic is correct. The
personCountunit-test gap is noted as a future improvement.Approved.
🎨 Leonie Voss (@leonievoss) — UX / Accessibility Review
⚠️ Approved with concerns
This is primarily a data model PR — the UI changes are minimal and intentionally stub-level. I'll focus on what shipped and flag what the follow-on reader issue must address.
Changes in scope:
geschichten/[id]/+page.svelteWhat changed: The document list section now renders
JourneyItemlinks instead of fullDocumentobjects. The title and date are replaced by a generic placeholder string.Finding 1 — Concern (not a blocker for this PR): Placeholder link is meaningless to users
The i18n string
geschichten_document_link_placeholderrenders as "Dokument öffnen" / "Open document" / "Abrir documento". This is the expected stub state while the journey reader follow-on (#786) is pending.For this PR: Acceptable as a temporary stub. The TODO comment clearly marks it.
For the follow-on: The link label must be the document title. "Open document" repeated N times violates Nielsen heuristic 2 (match between system and the real world) and makes keyboard navigation non-functional — screen readers would announce the same label for every link. The follow-on issue must resolve document titles before the feature is considered production-ready for our senior audience.
Finding 2 — Minor: Note text has no minimum width / overflow guard
text-smis 14px. The project minimum for body text is 16px (per design spec), with 18px preferred for the senior audience. For a supplementary note this may be acceptable, but flag it for the reader UI redesign: notes should be at least 14px with line-height that accommodates wrapping.At 320px viewport,
text-smwith nomax-worbreak-wordscould produce overflow if a note has no whitespace. Suggest addingbreak-wordsoroverflow-wrap-anywherefor the production version.Finding 3 — Minor: Link does not have a visible focus ring
The existing person-link pattern in the same file includes explicit focus styles:
The new document link is missing
focus-visible:ring-2 focus-visible:ring-focus-ring. For a keyboard user or screen reader user, navigating the document list would produce no visible focus indicator on this link.This is an accessibility gap. WCAG 2.1 criterion 2.4.7 (Focus Visible). For a PR that is a stub, I'll call this a concern rather than a blocker — but the follow-on reader must fix this.
Finding 4 — Positive: Keyed
{#each}with(item.id)Correct key expression. No reconciliation bugs when items are reordered.
Finding 5 — Positive: Section structure
Semantic
<section>with<h2>— matches the persons section pattern. Screen readers can navigate between sections. ✅Summary for the follow-on reader issue
The follow-on must address:
focus-visible:ring-2 focus-visible:ring-focus-ringto document linksbreak-wordsat 320pxFor this PR as a data model stub: Approved with the focus ring concern noted. The placeholder is intentional and documented.
Approved (stub state accepted, follow-on issues flagged).
🚀 Tobias Wendt (@tobiwendt) — DevOps / Infrastructure Review
✅ Approved
This PR has no Docker Compose changes, no CI workflow changes, and no new infrastructure components. From a DevOps lens, the only surfaces I care about are:
DROP TABLEin production)Flyway V72 migration — production safety
The migration includes a pre-requisite comment block with explicit
docker execcommands for taking a pre-migration backup:This is correct practice. The migration comment also includes a reverse procedure if V72 must be rolled back. The
DROP TABLE geschichten_documentsis the only irreversible step and it is clearly marked as such.Concern (suggestion): The pg_dump output path is
/tmp/pre_v72_backup_YYYYMMDD.sqlinside the container./tmpis ephemeral — it is wiped on container restart. Before running the migration in production, the dump should be copied out of the container immediately:Consider adding this copy step to the migration comment. Not a blocker — the nightly backup layer would also cover this, but belt-and-suspenders for an irreversible DDL step is worth documenting.
Test infrastructure impact
Three new
@SpringBootTest/@DataJpaTestclasses use Testcontainers (PostgresContainerConfig). Testcontainers with container reuse (if configured) will share the same Postgres instance across test classes in a single JVM run — no additional container cold-start cost per class.The
GeschichteHttpTestusesSpringBootTest.WebEnvironment.RANDOM_PORT, which starts the full application on a random port. This is correct — no port conflicts in parallel CI runs.No new Docker services, no new CI steps needed.
Credentials check
GeschichteHttpTestuses test-scoped credentials:These are test-only credentials created and deleted in
@BeforeEach. They are not hardcoded production credentials — the password is set viapasswordEncoder.encode(WRITER_PASSWORD). Clean.Summary
No infrastructure changes. Migration safety documentation is thorough. One suggestion: add a
docker cpstep to the pre-migration comment to ensure the backup survives container restarts before the irreversible DROP.No blockers. Approved.
📋 Elicit — Requirements Engineering Review
⚠️ Approved with concerns
I'm reviewing this against the original issue #750 and the PR description to check for requirements completeness, unresolved ambiguities, and scope compliance.
Requirements traceability
The PR description states it "Closes #750" and lists the following deliverables:
GeschichteTypeenum (STORY/JOURNEY)STORYfor existing rowsJourneyItementityposition, optionaldocument_id, optionalnotetype, createsjourney_items, migrates, drops junction tableGeschichteSummaryprojectionlist()api.tsupdated,[id]renders document links with placeholderAll stated deliverables are present.
Ambiguities and open questions surfaced by the diff
OQ-001: What happens to existing
STORY-type Geschichten that had documents via the oldgeschichten_documentstable?The migration migrates existing
geschichten_documentsrows intojourney_itemsordered bymeta_date ASC NULLS LAST. The assumption is documented:However, existing Geschichten had
type = STORY(default). These STORY-type Geschichten now haveJourneyItementries injourney_items. The frontend renders document-backed items for ALL Geschichte types (not just JOURNEY):Concern: A STORY with previously linked documents will now render its documents as journey items with the "Dokument öffnen" placeholder instead of the full document title + date it showed before. This is a regression in the existing STORY experience, acknowledged in:
This is accepted by the author. From a requirements perspective, this is a visible regression with a documented rationale and a follow-on dependency. I'd recommend the follow-on issue (#786) explicitly lists "restore document title display for STORY-type Geschichten" as a must-have acceptance criterion to prevent it from being forgotten.
OQ-002:
documentIdparameter removed from the GET/api/geschichtenendpointThe old endpoint supported
?documentId=<uuid>for filtering stories by a specific document. This has been removed with a TODO comment to restore it viajourney_items. Any callers of this parameter (internal links, external tools, browser history) will silently receive unfiltered results.Not a blocker for the data model PR, but the removal is a breaking change to the public API surface. The TODO should reference the follow-on issue number.
OQ-003: The
GeschichteSummaryprojection does not includetype-specific itemsThe list endpoint returns
GeschichteSummary, which has noitems. Card renderers cannot distinguish a JOURNEY from a STORY at the list level (only thetypefield is available). This is intentional for this PR, but the UX implication is that the list view cannot show a "reading journey" affordance until the follow-on adds it.Scope compliance
The PR correctly defers the journey editor, drag-reorder, and full reader experience to follow-on issues. The stub state is clearly marked with TODO comments referencing issue numbers. This is good issue-driven development practice.
Summary
The PR delivers what issue #750 requires. The two known regressions (STORY document display degradation,
documentIdfilter removal) are acknowledged in migration comments and follow a documented assumption. The main requirements concern is that ASSUMPTION AS-002's impact should be explicitly captured in the follow-on reader issue as a must-fix acceptance criterion.No blockers for the data model deliverable. Approved with the recommendation to update issue #786 to capture the STORY display regression as a must-have.
DocumentSummary: lean document projection for journey item embedding — skips tag-color resolution (getSummaryById), includes receiverCount (0 when no receivers, non-null). JourneyItemView: response record for item CRUD and GET. GeschichteView: detail response with summarised author {id, displayName} to prevent AppUser email/group leak. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>doesNotExist() asserts the key is absent from the JSON object, but Jackson serializes a null Optional<String> as {"note": null} — the key is present with a null value. nullValue() correctly matches that case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>The action was writing aria-checked directly and then firing onChange, which also triggered Svelte's own aria-checked={selected === type} binding. Double-ownership: action now only calls focus() + onChange(value); Svelte owns the attribute update. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>geschichten components now describe the type-based reader split (StoryReader / JourneyReader / JourneyItemCard / JourneyInterlude), the TypeSelector creation flow, and the full set of API endpoints (including DELETE /api/geschichten/{id} and GET /api/persons/{id} for person pre-population). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Importing layout.css in test-setup.ts activated Tailwind's responsive breakpoint classes (hidden lg:flex, hidden md:block, etc.), making 42 elements invisible at the default narrow Playwright test viewport. Revert the CSS import. Instead, add inline style attributes to the three components whose tests measure computed properties (min-height, font-size) — these values match what the Tailwind classes produce, so the real app appearance is unchanged. Also fix goto mock leakage in the geschichten/[id] delete-failure test: the delete-success test's goto('/geschichten') call was not cleared before the failure test ran. Add beforeEach(vi.clearAllMocks) to reset mock state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.