Timeline: TimelineEvent CRUD API #775
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Milestone: Zeitstrahl — Family Timeline
Spec:
docs/superpowers/specs/2026-06-07-family-timeline-design.md§ "Assembly & API"Depends on: TimelineEvent entity issue (#2) — must land first (entity,
timeline/package, Flyway migration fortimeline_events+ join tables, ADR for the new domain,CLAUDE.mdpackage-table entry, DB diagram). This issue must not introduce its own migration.Scope
Curator CRUD for timeline events. Backend-only — no rendered surface (curator forms live in spec issue #9). Clean layered architecture:
TimelineEventController → TimelineService → TimelineEventRepository; persons/documents resolved through their services, never their repositories.Endpoints
POST /api/timeline/events— create.@RequirePermission(Permission.WRITE_ALL)PUT /api/timeline/events/{id}— update.@RequirePermission(Permission.WRITE_ALL)DELETE /api/timeline/events/{id}— delete.@RequirePermission(Permission.WRITE_ALL)GET /api/timeline/events/{id}— single event for the edit form.READ_ALLThe
GETresponse must expose the linkedpersonIds/documentIds(and full linked person/document entities as the model serializes) so the curator edit form (#9) can pre-populate its pickers without a second round-trip. The entity is returned directly (no response DTO), consistent with the project pattern. At family-archive scale, linked collections will typically contain 0–5 items; a payload with 50 full entities is acceptable and expected behavior, not a bug.Data / request DTO
TimelineEventRequest— flat input DTO intimeline/:titletypeeventDate{year}-01-01whenprecision == YEAR(see D5).precisionYEARwhen omitted.eventDateEndprecision == RANGE. See RANGE invariant below.description@Size(max = 5000)on the DTO. Persisted with@Column(length = 5000)on the entity (see D6).personIdsnull⇒ no links (no NPE).@Size(max = 50)(see D7). On update, replaces the link set.documentIdsnull⇒ no links (no NPE).@Size(max = 50)(see D7). On update, replaces the link set.Bean Validation on
@Sizeconstraints produces 400 automatically — no service-layer code needed for the size caps.RANGE invariant
eventDateEnd != null && precision != RANGE→ 400 (end only meaningful for ranges).precision == RANGE && eventDateEnd == null→ 400 (a RANGE event requires a non-null end — see decision D2).Link resolution (fail-closed)
personIdsviaPersonService.getAllById(List<UUID>). Important:getAllByIdcallspersonRepository.findAllById(ids)which silently returns a shorter list for unknown IDs — it does not throw.TimelineServicemust check the returned list size against the input size and throwDomainException.notFound(ErrorCode.PERSON_NOT_FOUND, "One or more person IDs not found")on mismatch. Do not modifyPersonService.getAllById.documentIdsby loopingDocumentService.getDocumentById(UUID)per id — there is no batch fetch onDocumentService; per-id gives freeDOCUMENT_NOT_FOUND404s and avoids a speculative batch method. Do not injectDocumentRepositoryintoTimelineService.repository.savenever called).Service behavior
create(TimelineEventRequest request, UUID actorId)andupdate(UUID id, TimelineEventRequest request, UUID actorId).actorIdthrough controller → service. The project has no JPA auditing (@EnableJpaAuditing/AuditorAwareabsent) —createdBy/updatedByare set manually, mirroringDocumentService.updateDocument(..., UUID actorId)andAnnotationService. Controller obtains it viaUUID actorId = requireUserId(authentication);(asDocumentControllerdoes) and passes it in.createdBy/updatedByareUUIDfields on the entity (consistent withDocumentAnnotationand other entities — notString, not anAppUserrelation).createsetscreatedBy;updatesetsupdatedBy.@Transactionalon write methods only (create/update/delete). No class-level@Transactional.getEvent(UUID id)is a read →@Transactional(readOnly = true)— this is load-bearing architecture, not optional. Omitting it causesLazyInitializationExceptionin production when thepersons/documentslazy collections are accessed during serialization (ADR-022 /getDocumentDetailprecedent).DomainException.notFound(ErrorCode.TIMELINE_EVENT_NOT_FOUND, ...).persons/documentscollections from the request — not merged.Method structure in
create/updateOrder the method as named private steps to keep it under 20 lines:
validateRequiredFields(request)— title non-blank, type/eventDate non-null.validateRangeInvariant(request)— both RANGE directions.normalizeEventDate(request)— if precision is YEAR, set day/month to 1 (D5).resolvePersons(personIds)— getAllById + size check.resolveDocuments(documentIds)— per-id loop.repository.save(...).Tasks
TimelineEventController(POST/PUT/DELETE/GET) with@RequirePermission(Permission.WRITE_ALL)on the three writes,READ_ALLon GET; threadactorId = requireUserId(authentication)into create/update.TimelineServicewrite methods (create,update,delete) +getEventread (with@Transactional(readOnly = true)); ownsTimelineEventRepository.TimelineEventRequestinput DTO (flat intimeline/): title, type, eventDate, precision, eventDateEnd, description (@Size(max=5000)), personIds (@Size(max=50)), documentIds (@Size(max=50)).PersonService.getAllById+ explicit size-comparison check → throwPERSON_NOT_FOUNDon mismatch. Resolve documents by loopingDocumentService.getDocumentById— no repo injection, fail-closed.createdBy(UUID) on create /updatedBy(UUID) on update fromactorId.title/type/eventDate; defaultprecisiontoYEARwhen omitted; RANGE invariant (both directions); server-normalizeeventDateto{year}-01-01when precision is YEAR.ErrorCode.TIMELINE_EVENT_NOT_FOUNDunder a// --- Timeline ---section comment inErrorCode.java. Add tofrontend/src/lib/shared/errors.ts, add acaseingetErrorMessage(), add i18n keyserror_timeline_event_not_foundinmessages/{de,en,es}.json(e.g. German: "Zeitleistenereignis nicht gefunden."). Reuse existingPERSON_NOT_FOUND/DOCUMENT_NOT_FOUNDfor link resolution — no new codes for those.@Column(length = 5000)fordescription. No@Versionfield (D4 — omit optimistic locking; add later if concurrent-edit scenarios arise).npm run generate:apiafter the model/endpoint changes.docs/architecture/c4/l3-backend-timeline.puml; addTimelineEvent/EventTypetodocs/GLOSSARY.md. (Flyway/DB-diagram/ADR belong to entity issue #2.)@DataJpaTestintegration test requires #2's Flyway migration to be present in the classpath. It will fail until #2 merges. Mark with@Disabled("Depends on #2 migration")or accept CI red on this branch until #2 lands — decide before opening the PR.Acceptance criteria
READ_ALL-only user calling a write endpoint → 403;WRITE_ALL→ 2xx.TIMELINE_EVENT_NOT_FOUND.title, or nulltype/eventDate→ 400.precisionin the request defaults it toYEAR.precision = YEARand anyeventDatestores{year}-01-01(not{year}-07-15or similar).eventDateEndwithprecision != RANGE→ 400;precision == RANGEwith nulleventDateEnd→ 400.personId/documentId→ 404, nothing persisted.createdBy(UUID) set on create,updatedBy(UUID) set on update, from the authenticated actor.description> 5000 chars → 400; more than 50personIdsordocumentIds→ 400 (Bean Validation).Tests
Branch-coverage gate is 88% (backend) — the negative tests below are required, not optional. Use
@Builderfactories (makeTimelineEvent(...)); no setter chains, no 20-line@BeforeEach. Run targeted single files locally; full sweep to CI.@WebMvcTest(TimelineEventController.class)+@Import(SecurityConfig, PermissionAspect), mockTimelineService): for each of POST/PUT/DELETE — 401 unauthenticated, 403 withREAD_ALLonly, 2xx withWRITE_ALL. For GET — 200 authorized, 404 missing. Include a test wherepersonIdscontains a valid and an invalid ID → assert 404 andrepository.savenever invoked.@ExtendWith(MockitoExtension.class), mockPersonService/DocumentService):notFound(TIMELINE_EVENT_NOT_FOUND).personId→getAllByIdreturns empty list → size mismatch → throwsPERSON_NOT_FOUND,repository.savenever called.documentId→ per-id loop throwsDOCUMENT_NOT_FOUND,repository.savenever called.eventDateEndwithprecision != RANGE→ 400 (separate@Test).precision == RANGEwith nulleventDateEnd→ 400 (separate@Test).personIdsand nulldocumentIds→ no NPE, empty collections (separate@Test).YEAR(explicit test to cover the default branch).precision = YEARwith non-01-01 date → stored as{year}-01-01(normalization test).createdBy/updatedByset from the passedactorId.@DataJpaTest+ Testcontainerspostgres:16-alpine, never H2): round-trip — create with 2 persons + 1 doc, reload, assert thetimeline_event_persons/timeline_event_documentsjoin rows persisted and reload correctly. Uses@DataJpaTest's built-in@Transactionalrollback (default; no extra annotation needed, but document in a comment). Note: requires #2's migration — mark@Disabled("Depends on #2 migration")until that issue lands.Decisions resolved (round 1)
TIMELINE_EVENT_NOT_FOUND(over reusing a generic code). A precise 404 message and the project's structured-error discipline outweigh the cheap 4-step frontend chore; the original "no new ErrorCode" note was aspirational and is superseded.RANGEevent requires a non-nulleventDateEnd. A range with no end is incoherent for a timeline marker; requiring it makes the guard and its test unambiguous.YEARwhen omitted. Matches the entity default and keeps the curator form forgiving.@VersiononTimelineEvent: Omit. Timeline events have low concurrency risk (family archive, few curators). Keeps the migration minimal; add later via a new migration if concurrent-edit scenarios arise.eventDatenormalization forYEAR-precision: Server normalizes. Whenprecision == YEAR, the service setseventDate = LocalDate.of(date.getYear(), 1, 1). This prevents silent inconsistency between events entered through different curator form implementations and makes stored data unambiguous regardless of client.descriptionmax length: 5000 characters. MatchesPersonUpdateDTO.bioand is ample for event narrative descriptions. Applied as@Size(max = 5000)on the DTO and@Column(length = 5000)on the entity.personIds/documentIdslist size cap: 50 per list. More than enough for family events (realistic max ~10 persons). Applied as@Size(max = 50)on both DTO fields; Bean Validation returns 400 automatically.Cross-cutting recommendations from review (actorId threading, readOnly transaction on GET, fail-closed link resolution with explicit size check, per-id document loop, named private validation methods, i18n key pattern, integration test dependency note, no @Version, server-side date normalization, concrete size caps) have been folded into the relevant sections above.
🏛️ Markus Keller — Application Architect
Observations
The spec is now very precise and the Round 1 decisions have been folded in cleanly. From an architecture standpoint the remaining risks are documentation completeness and one structural ambiguity in the
getEventread method.1.
getEventtransaction annotation contradicts the project's read conventionThe issue says
getEventmust be@Transactional(readOnly = true)because lazy collections need the session open during serialisation. That is correct for this case. But the issue also references the rule "@Transactionalon write methods only" as if it's a global project rule. Clarification for the implementer:getDocumentByIdis also@Transactional(readOnly = true)— the real rule is "write methods get@Transactional; reads that traverse lazy associations get@Transactional(readOnly = true); simple reads with no lazy traversal get no annotation." The issue body is clear on this point but the "No annotation — reads do not need transaction overhead" sidebar in CLAUDE.md is slightly misleading for this case. Not a blocker — but worth noting in the ADR or a comment on the method.2. Missing ADR — the spec says one "may" be warranted; the issue body doesn't require it
The design spec says: "An ADR may be warranted for the new
timeline/domain + entity." The issue task list asks for a C4 diagram but is silent on the ADR. Given this is the first concrete backend issue in a new package (timeline/), writing ADR-035 before implementation starts is consistent with the project's "write the ADR before writing the code" discipline. The highest existing ADR is ADR-034. The C4 diagram requirement (l3-backend-timeline.puml) is already in the task list — good.3. DB diagram update requirement is not in the task list
Per Markus's review table: any new Flyway migration adding tables (which this issue depends on, though it doesn't introduce the migration itself) must update
docs/architecture/db/db-orm.pumlanddocs/architecture/db/db-relationships.puml. Those updates belong to issue #2 (the entity migration), not here. Confirmed: the task list correctly defers that to #2. Nothing to fix — just ensuring the implementer doesn't accidentally think those are out-of-scope entirely.4.
CLAUDE.mdpackage table entry is requiredThe task list calls for a
CLAUDE.mdpackage-table entry under issue #2. This issue createsTimelineEventController,TimelineService, andTimelineEventRepository— which means the package entry is needed at or before this PR. Cross-check: the task list says "Note: this issue must not introduce its own migration" — but it does introduce the controller/service/repository in thetimeline/package. TheCLAUDE.mdentry belongs to whichever PR first creates code in that package.Recommendations
timeline/domain package, why it is separate fromgeschichte/, and the layering rule (ownsTimelineEventRepository, reaches Person/Document through their services). Two paragraphs is enough.CLAUDE.mdpackage-table entry lands in this PR (or #2's PR, whichever comes first), not deferred past merge.getEventexplaining why it is@Transactional(readOnly = true)— mirrors the pattern ongetDocumentById(seeDocumentService:1001) and prevents future refactorers from stripping it.@Disablednote handles the dependency cleanly.Open Decisions (omit this section entirely if none)
None — all architectural decisions are resolved.
👨💻 Felix Brandt — Senior Fullstack Developer
Observations
The spec is tight and the Round 1 recommendations (actorId threading, readOnly transaction, fail-closed resolution, named private methods,
@Disablednote) have all been folded in. From a clean-code and implementation standpoint, a few concrete gaps remain before this is ready to code.1.
validateRequiredFieldsduplicates Bean Validation workThe spec mandates Bean Validation (
@NotBlankon title,@NotNullon type/eventDate) and also a named privatevalidateRequiredFields(request)step. If Bean Validation is wired correctly with@Validon the controller parameter, the service-layervalidateRequiredFieldsstep is dead code — Bean Validation fires first at the controller boundary and the service never sees an invalid request. The spec should clarify: either (a) the service trusts Bean Validation and thevalidateRequiredFieldsprivate method is a fallback guard (in which case it's belt-and-suspenders), or (b) the service must guard even for non-HTTP callers. In the current architecture, services are always called from controllers via HTTP, so (a) is fine but it needs to be explicit.2. The
resolveDocumentsloop creates a semantically different failure mode fromresolvePersonsFor persons: the spec uses
getAllById(batch) + explicit size check — one round trip, one failure point. For documents: the spec mandates a per-id loop callingDocumentService.getDocumentById(UUID). This is asymmetric: a request with 50 documentIds makes 50 separate DB round trips. At family-archive scale (50 docs max) this is acceptable, but the implementer should know thatgetDocumentByIdis@Transactional(readOnly = true)internally — calling it 50 times inside the parent@Transactionalwrite method will participate in the outer transaction correctly (Spring's defaultREQUIREDpropagation). No bug, but worth a brief comment in the implementation.3. The
methodStructureorder exposes an edge case in theupdatepathThe spec's 6-step order for
create/updateis: validate → range-invariant → normalize → resolvePersons → resolveDocuments → save. In theupdatepath, step 6 (save) replaces the existingpersons/documentssets. The spec says "replaces (set semantics)." The implementer must clear and re-add collection members rather than reassigning a newSetto the field — Hibernate doesn't track a replaced collection reference. Concrete code:event.getPersons().clear(); event.getPersons().addAll(resolvedPersons);notevent.setPersons(new HashSet<>(resolvedPersons)). This is a common Hibernate pitfall that can cause orphan rows or constraint violations depending on the join-table cascade config on the entity (which comes from issue #2).4. Missing
@NotBlank,@NotNullannotations on the DTOThe task list says
@Size(max=5000)and@Size(max=50)are needed. It does not explicitly list@NotBlank(title) and@NotNull(type, eventDate). These must be in the DTO for Bean Validation to fire. The issue's acceptance criteria require 400 on blank/null — Bean Validation is how that happens. Add them to the DTO spec.5.
npm run generate:apiis listed but the spec is backend-onlyThe issue correctly calls for regenerating types after model/endpoint changes. Since this is a backend-only issue, the only frontend-touching step is regeneration — no components. Confirm in the PR description that the developer ran
npm run generate:apiand committed the updated types.Recommendations
validateRequiredFieldsintent: if Bean Validation is the primary guard (correct choice), rename the private method toassertRequiredFieldsPresentand document it as a defensive guard for non-HTTP call paths, or remove it entirely and rely on@Valid.resolveDocumentsloop noting thatgetDocumentByIdparticipates in the outer@Transactionalcontext — prevents future "why are we making 50 queries?" confusion.clear()+addAll()on Hibernate collections in the update path — never replace the collection reference.@NotBlanktotitleand@NotNulltotype/eventDatein the DTO. These are required for the 400 acceptance criterion to pass via Bean Validation.validateRangeInvariant,normalizeEventDate,resolvePersons,resolveDocuments) are clean — stick to them. Keep each under 10 lines.Open Decisions (omit this section entirely if none)
None — all code-style decisions are resolved.
🔒 Nora "NullX" Steiner — Security Engineer
Observations
Round 1 folded in the key security-relevant decisions (permission gating on all three writes, actorId threading from the authenticated principal, fail-closed link resolution). The spec is now security-sound at the model level. My Round 2 checks focus on what the spec doesn't say explicitly but the implementation must get right.
1.
READ_ALLonGET /api/timeline/events/{id}— confirmed correct, but annotate explicitlyThe issue says GET requires
READ_ALL. Checking the existing controller patterns:DocumentControllerdoes not annotate its GET endpoints with@RequirePermissionbecause the project'sSecurityConfigrequires authentication globally (.anyRequest().authenticated()).READ_ALLis effectively the baseline authenticated permission. The spec is correct to call it out, but the implementation question is: doesTimelineEventController.getEventneed an explicit@RequirePermission(Permission.READ_ALL)annotation, or does global authentication (anyRequest().authenticated()) cover it? Looking atDocumentController.getDocument— no annotation, relies on global auth. The issue spec saysREAD_ALLis required but doesn't say whether that means an explicit annotation or relies on the global rule. For consistency with existing controllers, rely on the global auth rule (no@RequirePermissionon GET). Adding@RequirePermission(READ_ALL)would be redundant but harmless. Decide one way; either is defensible.2.
personIdsanddocumentIdsare user-controlled UUID lists — no authorization check on the linked entitiesThe spec resolves persons/documents by ID and fails closed if any ID is unknown. However, it does not check whether the authenticated user has read access to the linked person or document. In this family archive, all authenticated users have
READ_ALL(by design — global auth requirement +READ_ALLbaseline), so linking a person/document you can read is always allowed. This is correct for the current permission model. If the permission model ever gains per-document access control, link resolution would need authorization. Not a current risk — noting for future-proofing.3.
descriptionfield: 5000 chars, no HTML sanitization — correct for this contextThe
descriptionis stored and returned as plain text. The frontend rendering should usetextContent(or Svelte's default text interpolation{description}) not{@html description}. The spec doesn't mention this but the frontend issue (#9 cursor form) should be reminded. Not a backend concern for this issue.4.
createdBy/updatedByare set fromrequireUserId(authentication)— correct and consistentThis matches
DocumentController.updateDocumentexactly:UUID actorId = requireUserId(authentication)→ passed to service → set on entity. The only risk is forgetting to passactorIdon thecreatecall. The controller test suite (spec:@WebMvcTestwith mockedTimelineService) should assert that the servicecreate/updatemethods are called with the correctactorId— not just that the endpoint returns 2xx.5. No
@Version(optimistic locking omitted — D4)Decision D4 is correct for this domain. The only security nuance: without optimistic locking, concurrent
PUTrequests on the same event id will silently last-write-wins. For a family archive with few curators editing the same event simultaneously, this is acceptable. The spec correctly calls it out.Recommendations
@RequirePermission(READ_ALL)or relies on the global auth filter. Document the choice with a one-line comment. Either is correct; consistency withDocumentController(no annotation on GET) is the tie-breaker.actorIdis passed through to the service — not just that the endpoint returns 200. UseArgumentCaptoron the mockedTimelineService.descriptionfield must be rendered with text interpolation, not{@html}, since no sanitization is applied at the backend.TIMELINE_EVENT_NOT_FOUNDis correctly scoped.Open Decisions (omit this section entirely if none)
None — all security-relevant decisions are resolved.
🧪 Sara Holt — QA Engineer
Observations
The test section in this issue is the most prescriptive I've seen in this project — every test is named with its scenario, layered correctly across the three test types, and the
@Disableddependency caveat is explicit. Round 1 recommendations have been incorporated. Remaining gaps are about coverage completeness and a possible testing pattern trap.1. The
@DataJpaTestintegration test has a critical dependency issue — the@Disablednote is insufficient by itselfThe spec says: "Mark with
@Disabled(\"Depends on #2 migration\")until #2 lands." This is correct. But@DataJpaTestby default runs Flyway and expects the migration history to be consistent. Without #2's migration, the@DataJpaTestwill not just fail — it may fail at Spring context loading (Flyway validation error) before the test even runs. The@Disabledannotation won't prevent context load. The safe pattern is@DataJpaTest+@AutoConfigureTestDatabase(replace = NONE)+ Testcontainers, with Flyway disabled or the migration absent. Recommend: add a comment in the test class noting that until #2's migration file is present insrc/main/resources/db/migration/, the test class will fail at Flyway startup even when@Disabledis on individual tests. The whole class may need to be@Disabledor wrapped in a conditional.2. Controller test: the spec says "include a test where
personIdscontains a valid and an invalid ID → assert 404 andrepository.savenever invoked"This is good. But "repository.save never invoked" in a
@WebMvcTesttests the service mock, not the repository. The correct assertion isverify(timelineService, never()).create(...)(or equivalently verify the mock method was not called). In a@WebMvcTestwith mockedTimelineService, the repository is not in context at all. Clarify in the implementation: the controller test verifies that when the service throwsPERSON_NOT_FOUND, the controller returns 404 — the "save never invoked" guarantee is in the service unit test, not the controller test.3. Missing test: update with empty
personIds/ emptydocumentIds(not null, but empty list)The spec tests
null→ no NPE. ButpersonIds = [](empty list, not null) is a distinct case: it means "clear all links." TheresolvePersonssize-check logic:getAllById([])returns[]; input size is 0; returned size is 0; size check passes; result is empty set. This should work correctly but it's not explicitly tested. Add:update with empty personIds replaces link set with empty set.4. Test ordering in service test — the "RANGE invariant direction 1 and 2" tests are listed as separate
@TestmethodsGood — the spec correctly says "separate
@Test" for each direction. This is the right approach (one behavior per test). Confirm that each test uses a minimalTimelineEventRequestthat only varies the failing field — not a mega-request that happens to also trigger the other invariant.5. 88% branch coverage gate — the test list is comprehensive but may fall short on the
normalizeEventDatebranchesThe normalization logic has at least two branches:
precision == YEAR(normalize) andprecision != YEAR(don't normalize). The spec explicitly includes "precision = YEAR with non-01-01 date → stored as{year}-01-01" test. The spec also has "precision omitted → defaults to YEAR." Confirm: does "defaults to YEAR then normalizes" get covered by combining those two tests? Yes — the default-to-YEAR test plus the normalization test together cover both branches. Good.Recommendations
@Disabledscope: if the@DataJpaTestclass fails at context load (not at test run),@Disabledon individual tests won't help. Consider@Disabledat the class level, or suppress Flyway migration validation for the class with@AutoConfigureTestDatabase(replace = NONE)+ aspring.flyway.enabled=falseoverride until #2 lands.update with empty personIds list clears all linked persons— distinct from the null case.makeTimelineEvent(...)should be the primary setup vehicle. Confirm all 3 test classes share the same factory pattern (or a shared test util class) rather than each class having its own builder chain.Open Decisions (omit this section entirely if none)
None.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Observations
This issue is a pure backend application issue with no infrastructure changes. No new Docker services, no new compose entries, no CI workflow changes. My review is correspondingly brief.
1. CI impact of the
@Disabledintegration testThe issue notes that the
@DataJpaTestintegration test requires #2's Flyway migration and should be marked@Disabled. The coverage gate is 88% branch coverage. A disabled test class contributes 0% to coverage — if this class is fully disabled, the branches it would have covered are unexercised. This means the 88% gate may require the remaining controller + service tests to cover those branches instead. The spec's test list is comprehensive enough that coverage should still be met from the service unit tests and controller@WebMvcTesttests, but the implementer should run./mvnw clean verifyon this branch (without #2 present) to confirm the gate passes before opening the PR.2. No new env vars or secrets
This issue introduces no new configuration values, environment variables, or secrets. Nothing to add to
.env.exampleor thedocker-compose.yml.3.
npm run generate:apiin CIThe issue correctly calls for regenerating types. In CI, the TypeScript type regeneration step requires the backend to be running (
--spring.profiles.active=dev). This is not a new concern — it's an existing pattern — but the PR checklist should confirm types were regenerated locally and committed. The CI pipeline doesn't regenerate types automatically (it runs pre-committed types).Recommendations
./mvnw clean verifylocally before opening the PR to confirm the 88% branch gate passes with the integration test@Disabled.No concerns from my angle beyond the coverage gate note above.
📋 Elicit — Requirements Engineer
Observations
The spec is now exceptionally precise for a backend-only issue — all D1–D7 decisions are resolved, acceptance criteria are testable, edge cases are enumerated, and the dependency chain is explicit. This is a well-structured issue. My role in Round 2 is to look for any remaining untestable, ambiguous, or missing requirements that the implementation team could interpret differently.
1. The
createdBy/updatedByupdate semantics are partially underspecifiedThe spec says: "
createsetscreatedBy;updatesetsupdatedBy." It does not say whetherupdatealso preservescreatedBy(i.e., the original creator's UUID is never overwritten). This is almost certainly the intent (standard audit pattern), but the acceptance criteria don't include: "after an update,createdBystill holds the original actor." Add this as an acceptance criterion or test, otherwise the implementer could set bothcreatedByandupdatedByon every update.2. The acceptance criterion for "Precision default" is testable but the test scenario is too narrow
"Omitting
precisionin the request defaults it toYEAR." This is verified by the service unit test. However, the acceptance criterion doesn't specify what happens onGET /api/timeline/events/{id}— does the response reflectprecision = YEAR(i.e., the default is persisted, not just applied transiently)? The entity defaultYEARshould be stored, not just computed. Confirm: the DTO hasprecisionasOptional/nullable, and the service sets the entity'sprecisionfield toYEARbefore callingsave. The round-trip integration test should verify this.3. Missing acceptance criterion:
eventDateEndisnullafter create withprecision != RANGEThe spec guards against
eventDateEnd != null && precision != RANGE→ 400. But there's no criterion stating that after a successful create withprecision = YEAR,eventDateEndisnullin the stored entity and returned in GET. This is implied but not explicit. Minor — add one sentence.4. The
@Size(max=50)onpersonIds/documentIds— what does the API return when violated?Bean Validation on
@Sizereturns a generic 400 with a validation error body. The issue says "Bean Validation on@Sizeconstraints produces 400 automatically — no service-layer code needed." Confirmed. But the acceptance criterion saysmore than 50 personIds → 400. The error response body will be Spring's default validation error format (not aDomainExceptionstructured error withErrorCode). This is the existing project behavior (consistent with@Size(max=5000)on description). Fine — just noting it's a different error shape thanDomainExceptionerrors.5. No acceptance criterion for the response body shape of
GET /api/timeline/events/{id}The spec says the entity is returned directly (no response DTO), consistent with the project pattern, and that
personIds/documentIds(full linked entities) must be exposed. The acceptance criteria don't include: "GET response includes the linked person entities" or "GET response includes the linked document entities." Add this, otherwise the implementer might return a minimal entity without initializing lazy collections (which would surface as empty arrays or a LazyInitializationException if@Transactional(readOnly = true)is missing).Recommendations
createdByretains the original creator UUID; onlyupdatedBychanges."/api/timeline/events/{id}response includes the full linked person and document entities (not just their IDs), enabling the curator edit form to pre-populate pickers without a second round-trip."precisiondefaulted toYEARis persisted and returned in the GET response."eventDateEndis null in the GET response whenprecision != RANGE."Open Decisions (omit this section entirely if none)
None — all requirements decisions are resolved. These are documentation clarifications, not new decisions.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
This issue is backend-only (no rendered surface — curator forms live in spec issue #9). No UI, no accessibility, no responsive design decisions to review here.
What I checked: the spec says
GET /api/timeline/events/{id}must expose full linked person/document entities so the curator edit form (#9) can pre-populate its pickers without a second round-trip. That is the right design — pre-populated pickers are a usability baseline, not a nice-to-have. If the GET response returns empty lazy collections, the form will silently show no linked persons/documents when re-opened for editing. The@Transactional(readOnly = true)ongetEvent(required in the spec) is what keeps those collections accessible during serialisation. No changes needed from my perspective — this concern is already captured in the spec.No concerns from my angle for this backend-only issue. I'll engage fully when issue #9 (curator event forms) arrives.