feat(geschichte): restore document management for STORY-type Geschichten (#795) #804
Reference in New Issue
Block a user
Delete Branch "feat/issue-795-story-documents"
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?
Closes #795.
What
Restores the write path for documents on STORY-type Geschichten, closed since V72 dropped
geschichten_documents(#753 deferred this to the "future Lesereisen editor" — this is it).Backend
JourneyItemService.append(): JOURNEY-only type guard deleted (per the issue's decision — an allowlist over a two-constant enum would be unreachable dead code under the JaCoCo branch gate). Capacity cap, dedup guard, and note validation unchanged.append()/reorder()— error strings no longer claim a check the code doesn't perform.GESCHICHTE_TYPE_MISMATCHremoved end to end:ErrorCode.java,errors.tsunion +getErrorMessage()case, i18n keys in de/en/es. Verified zero mentions remain repo-wide;GESCHICHTE_TYPE_IMMUTABLEuntouched.Frontend
StoryDocumentPanel.sveltein the story editor sidebar: position-sorted item list, pessimistic add viaPOST /items, optimistic remove with snapshot-rollback viaDELETE /items/{id}, both throughcsrfFetch. Deleted-document items render as removable placeholder rows. 409 capacity/duplicate map to story-worded messages; everything else throughgetErrorMessage(). Polite live region + focus management on remove. Sidebarp-4section style, mobile<details>accordion, no inner scroll clamp.DocumentPickerDropdown: optionalinputIdprop so the panel can wire a visible<label for>; generated default id;JourneyAddBarbyte-for-byte untouched.GeschichteSidebargains optionalgeschichteId/itemsprops;GeschichteEditorderives both from itsGeschichteViewprop — panel absent on/geschichten/newand for journeys.Tests
story()factory, 3 new integration tests (STORY e2e, V72-style position-gap rows incl. removal, dangling deleted-document item). 67/67 green across the three journeyitem classes.StoryDocumentPanelspec, sidebar contract spec, editor + StoryCreate guards, 2 pickerinputIdtests. 79/79 across the six touched spec files. Lint clean, new files svelte-check clean.Notes for reviewers
ON DELETE SET NULLcollides withchk_journey_item_not_emptyfor note-less items — deleting such a linked document 500s at the DB. Pre-existing, not touched here.🤖 Generated with Claude Code
The input always carries an id (generated doc-picker-input-{uid} default, mirroring the listbox id scheme). When a caller passes inputId it wires a visible <label for> — the aria-label fallback is dropped then so the visible label wins. JourneyAddBar stays untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Sidebar-section-styled panel (p-4 card, mobile <details> accordion, no inner scroll clamp) that lists a story's journey items in position order. Add is pessimistic via POST /items; remove is optimistic with snapshot rollback via DELETE /items/{id}; both through csrfFetch. Already-linked documents are unselectable in the reused DocumentPickerDropdown (visible label wired via inputId). Document-less items (ON DELETE SET NULL) render as removable placeholder rows. 409 capacity/duplicate map to story-worded messages, everything else through getErrorMessage(). Add/ remove are announced in a polite live region and focus moves to the previous row's remove button (picker input when the list empties). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>⚙️ Tobias Wendt — DevOps & Platform Engineer
⚠️ Approved with concerns
Nothing in this PR touches the parts I operate — and I verified that, rather than taking the description's word for it.
What I checked across all 19 changed files:
db/migration, no schema change. The feature rides entirely on the existing V72journey_itemstable. Nothing to sequence at deploy time, no migration to test against a clean database beyond what CI already runs.docker-compose*.yml, noapplication*.yml, no CI workflow edits, no new dependencies inpom.xmlorpackage-lock.json. Deploy is a plain image swap.GESCHICHTE_TYPE_MISMATCHcode is symmetric front/back, so neither version emits a code the other can't map. Clean.ON DELETE SET NULLscenarios — that's the kind of test that would have caught a migration regression.Blockers
None.
Suggestions
chk_journey_item_not_empty/ON DELETE SET NULLcollision before merging — not after. The PR notes it as "pre-existing", which is true, but this PR materially widens its blast radius: until now only journey curators could create note-less items; after merge, every STORY editor adding documents viaStoryDocumentPanel.sveltecreates exactly the note-less rows that make a later document delete 500 at the DB constraint. The integration test (JourneyItemIntegrationTest.java,story_item_with_deleted_document_survives_and_remains_deletable) even has to work around it with a note — the comment in that test documents the bug. That's a known, reproducible production 500 that will land in GlitchTip with no tracked issue behind it. An alert (or error report) without a runbook is noise; same goes for a known 500 without an issue number.console.erroron failed mutations (StoryDocumentPanel.svelte,handleAdd/handleRemovecatch blocks) — fine for now, but ifVITE_SENTRY_DSNis set these network failures vanish into the browser console instead of GlitchTip. Consider routing through the existing Sentry capture path in a follow-up; not worth blocking a feature PR over.Good, boring deploy. That's a compliment.
🏛️ Markus Keller — Application Architect (SvelteKit · Spring Boot · PostgreSQL)
🚫 Changes requested — the code is architecturally sound, but two architecture documents now contradict the code. Per our rule, a doc omission is a blocker, not a concern.
What I checked
JourneyItemServicestill reaches Geschichte data only throughGeschichteQueryService— boundary intact after the guard removal.StoryDocumentPanelconsumesDocumentPickerDropdownthrough a clean published-prop extension (inputId);JourneyAddBaruntouched. Good.uq_journey_items_geschichte_document, positions by V73's DEFERRABLE unique. Deleting the unreachable type guard rather than keeping dead-branch code is the right call; the schema never encoded a JOURNEY-only restriction, so the application check was the only place the rule lived, and the product decision (#795) removed the rule.GESCHICHTE_TYPE_MISMATCHremoved in all four mandated places (ErrorCode.java, errors.ts union,getErrorMessage()case, de/en/es keys). I grepped the branch — zero mentions remain. The duplicate-key cleanup in the three locale files is welcome.l3-frontend-3c-people-stories.puml),frontend/src/lib/geschichte/README.mdupdated. Backend docs were not — see blockers.Blockers
docs/architecture/c4/l3-backend-3g-supporting.puml:22—JourneyItemServicecomponent still says "Enforces JOURNEY-type guard on append." This PR deletes exactly that guard. Same file, line 44:Rel(journeyItemSvc, geschQuerySvc, "Checks Geschichte existence and type")— the type check is gone; it now checks existence only. The backend diagram must match the code before merge ("new/changed service behavior in an existing backend domain → matchingl3-backend-*.puml").docs/GLOSSARY.md:154anddocs/ARCHITECTURE.md:41— both still defineJourneyItemas belonging exclusively to JOURNEY-type Geschichten ("a single stop in a Lesereise (Geschichtewithtype=JOURNEY)" / "ordered stops in a JOURNEY-type Geschichte"). After this PR,journey_itemsis the document-attachment mechanism for both subtypes — that is the central domain-model change of the PR and the glossary is the canonical place it must be recorded. Update both entries (andGLOSSARY.md:152's STORY description, which currently implies stories link only persons).Suggestions
Capture the "journey_items serves both subtypes" decision in the repo, not only in issue #795. A future developer reading
JourneyItem,JOURNEY_AT_CAPACITY, andJOURNEY_DOCUMENT_ALREADY_ADDEDwill reasonably conclude stories should not have items and may re-add the guard. A short ADR (docs/adr/) — context: V72 droppedgeschichten_documents; decision: one shared item table, no type guard; alternative rejected: separate story-documents join table — is cheap insurance. The blocker-2 glossary fix covers the what; the ADR covers the why.Naming drift is contained but real:
StoryDocumentPanel.failureMessage()re-words two journey-flavored 409 codes for story context. That presentation-layer mapping is the pragmatic choice today (renaming enum constants would ripple through ErrorCode.java, errors.ts, and three locales for zero behavior change), but note it in the ADR from suggestion 1 so the asymmetry is a documented decision, not an accident.File the follow-up issue for the
ON DELETE SET NULL×chk_journey_item_not_emptycollision now, before it leaves working memory. A DB-level constraint conflict that 500s a document delete is exactly the class of integrity bug we push to the schema to prevent — the V72 FK action and the V72 CHECK constraint encode contradictory rules for note-less items. Likely fix direction: delete the row instead of nulling the FK (trigger or application-side cascade), or relax the CHECK. Pre-existing and correctly out of scope here, but it needs a tracked issue, not a PR footnote.Fix the two doc blockers and this is an approve — the service-layer changes, DB-constraint posture, and frontend component ownership are all clean.
🎨 Leonie Voss — UI/UX Design Lead, Brand Specialist, Accessibility Advocate
✅ Approved
This is what I want every sidebar panel PR to look like. I checked brand compliance, a11y (labels, live regions, focus management, touch targets), 320px behavior, and the de/en/es copy.
Blockers
None.
What I verified
StoryDocumentPanel.sveltereproduces the sibling sidebar sections byte-for-byte:<details open class="sm:contents">mobile accordion with a 44px<summary>,sectioncardrounded border border-line bg-surface p-4 shadow-sm, hidden-below-smh2withfont-sans text-xs font-bold tracking-widest text-ink-3 uppercase. Side-by-side with the Status and Personen sections inGeschichteSidebar.svelte:30-76it is indistinguishable. Serif for document titles, sans for chrome — correct per the type system.inputIdprop onDocumentPickerDropdown.svelte:14plusaria-label={inputId ? undefined : placeholder}is exactly right: a visible<label for>wins when wired, the aria-label fallback stays forJourneyAddBar. Both paths have spec coverage. This is the pattern I'd ask other pickers to adopt.moveFocusAfterRemove()(StoryDocumentPanel.svelte:~95) prevents the classic focus-drops-to-<body>teleport: previous row's remove button, picker input when the list empties. Both covered by browser specs assertingdocument.activeElement.aria-live="polite" aria-atomic="true"sr-only region with add/remove announcements, plusrole="alert"for errors. Errors are text (not color-alone), useborder-danger bg-danger/10 text-dangersemantic tokens that remap in dark mode.h-11 min-w-[44px]= 44px, WCAG 2.5.8 pass;focus:outline-noneis paired withfocus-visible:ring-2 focus-visible:ring-focus-ring(a real token with a dark-mode remap inlayout.css:137/242), so keyboard users keep a visible indicator.min-w-0 flex-1 truncateon title spans prevents row overflow; no fixed widths anywhere; no inner scroll clamp so the page scrolls naturally.Suggestions
handleRemove()moves focus before the DELETE; if the request fails and the row is restored, focus stays on the neighbor's remove button rather than returning to the restored row's. Therole="alert"covers the announcement, so this is Low priority — but if you touch this file again, consider re-focusing[data-item-id="${item.id}"] [data-remove-btn]in the rollback branch.announceTimercleanup — the 500ms timer isn't cleared on destroy (StoryDocumentPanel.svelte:~37). Harmless today since it only writes$state, but anonDestroy(() => announceTimer && clearTimeout(announceTimer))is a one-liner. Low.<p role="alert">is text + color; adding the small warning icon used elsewhere would give the third redundant cue for low-vision users scanning the sidebar. Low.Nothing here warrants holding the PR. Nice work — the senior transcriber on a tablet will be able to use this panel without a mouse, and that's the bar.
👨💻 Felix Brandt — Fullstack Developer
✅ Approved
What I checked: TDD evidence, naming, function size, guard clauses, Svelte 5 idioms, component boundaries, dead code. The diff plus full reads of
JourneyItemService.java,StoryDocumentPanel.svelte,JourneyEditor.svelte, and an i18n-key usage sweep.What's good here:
append_to_STORY_type_creates_journey_item, capacity cap, dedup). Frontend coverage hits all four states — populated, empty, error (role="alert"+ rollback), and the deleted-document placeholder — plus the two focus-management paths. That's the full matrix, not just the happy path.append()after the guard removal still reads top-down: not-found → capacity → note validation → dedup → persist. The error-string fix ("Journey not found" → "Geschichte not found") is exactly right — the message no longer claims a check the code doesn't perform.GESCHICHTE_TYPE_MISMATCHremoval is complete. I grepped backend, frontend, and all three locale files independently — zero mentions remain,GESCHICHTE_TYPE_IMMUTABLEuntouched. Deleting an unreachable guard instead of keeping ceremonial dead code is the right call.{#each items as item (item.id)},$derivedforalreadyAddedIds(recreated Set, so noSvelteSetneeded),$props.id()for the label wiring, snapshot-and-rollback on optimistic remove. Thesvelte-ignore state_referenced_locallyis justified and documented with the re-mount contract.DocumentPickerDropdownchange is minimal and contract-tested both ways (generated default id + aria-label fallback; external id drops the aria-label so the visible<label for>wins).JourneyAddBaruntouched as promised.Blockers
None.
Suggestions
Dead i18n keys adjacent to your new ones —
geschichte_editor_dokumente_heading/geschichte_editor_dokumente_hint(de/en/es, e.g.frontend/messages/de.json:1080-1081) have zero usages infrontend/src. They sit directly below the newgeschichte_documents_*block and say nearly the same thing ("Welche Briefe oder Dokumente sind Teil dieser Geschichte?" vs. your new hint). Since this PR already collapsed three duplicate keys per locale, deleting these two while you're in the file would finish the cleanup — fine as a follow-up if you'd rather keep this commit scoped.failureMessage()now exists twice —JourneyEditor.svelte:93-96andStoryDocumentPanel.svelte:52-56share the same core (res.json().catch → code → getErrorMessage(code) : journey_mutation_error_reload()); the panel version adds two story-worded 409 cases on top. Two occurrences is below my extract threshold, so leaving it is fine — but if a third consumer shows up, pull the base mapper into$lib/geschichte/utils.tsand let callers override the 409 wording.StoryDocumentPanel.svelteis 193 lines — over my 60-line splitting signal. It's arguably one visual region, but the item row (title/placeholder span + remove button + inline SVG, lines ~134-163) is a nameable boundary:StoryDocumentRow.sveltewithitem,onRemoveprops would bring the panel under ~130 and make the row independently testable. Question, not a demand — the JourneyItemRow/JourneyEditor split next door is the precedent.announceTimernever cleared on unmount (StoryDocumentPanel.svelte:28-34) — a remove immediately before navigating away leaves a 500ms timeout firing into a destroyed component. Harmless in Svelte 5 (it just writes a$statenobody reads), but a one-line$effect(() => () => { if (announceTimer) clearTimeout(announceTimer); })closes it.Clean work — the truthful error messages and the end-to-end dead-code removal are exactly how I'd want this done.
🔐 Nora "NullX" Steiner — Application Security Engineer · Ethical Hacker
✅ Approved
I reviewed the full diff plus the surrounding controller, service, CSRF helper, and SecurityConfig on
feat/issue-795-story-documents. No vulnerability found. What I checked, and why each area passes:Blockers
None.
Authorization (the headline risk of this PR — guard removal). The deleted JOURNEY-type check in
JourneyItemService.append()was a domain-shape guard, not a security boundary. Every mutating endpoint that the new panel calls remains declaratively protected:POST /{id}/items,PATCH /{id}/items/{itemId},DELETE /{id}/items/{itemId},PUT /{id}/items/reorderall carry@RequirePermission(Permission.BLOG_WRITE)(GeschichteController.java:76-104). Removing the type guard widens what a BLOG_WRITE user can do within their own permission class — it grants nothing to anyone below that. No privilege escalation.IDOR.
delete()andupdateNote()scope the item to the URL's Geschichte viafindByIdAndGeschichteId(itemId, geschichteId)(JourneyItemService.java:111, 145) — an attacker cannot delete item X of story A through story B's URL.reorder()validates the requested ID set againstfindIdsByGeschichteIdexactly (JourneyItemService.java:170-173), so cross-Geschichte item IDs are rejected. Correct pattern.CSRF. Both panel mutations go through
csrfFetch(StoryDocumentPanel.svelte:62, 99), which attaches the token for POST/PUT/PATCH/DELETE (cookies.ts:53-59). No rawfetchfor writes.XSS.
item.document.titleand the deleted-document placeholder render via Svelte text interpolation only — auto-escaped. No{@html}, noinnerHTMLanywhere in the new component. The aria-labels and live-region announcements interpolate titles through Paraglide message functions into text nodes — same story.Information disclosure in errors. The reworded "Geschichte not found: {uuid}" messages echo only a Spring-parsed
UUIDpath variable — type-constrained, no injection or reflection surface.failureMessage()in the panel maps only known codes; unknown codes fall throughgetErrorMessage()whosedefault:returns a generic message (errors.ts:211-212) — no raw server detail reaches the DOM. Error-string honesty also improved: the message no longer claims a journey check the code doesn't perform — that matters during incident response.Audit trail. Add/remove/note/reorder all log
actorIdafter commit (JourneyItemService.java:102-104, 151-153). The new STORY write path inherits this for free.DoS posture. The 100-item cap and the DB-level dedup index backstop (
uq_journey_items_geschichte_document,saveAndFlushatJourneyItemService.java:90) survive the guard removal — the TOCTOU window between theexists()pre-check and insert stays closed at the constraint.Suggestions
Hardening (smell, not a vuln):
moveFocusAfterRemoveinterpolatestarget.idinto aquerySelectorattribute selector (StoryDocumentPanel.svelte:88). Today the value is a server-issued UUID, so it's inert — but if this pattern gets copy-pasted somewhere with user-influenced IDs, it becomes a selector-injection foothold. Cheap immunization:CSS.escape(target.id). Detection note: a Semgrep rule matchingquerySelector(\...${...}...`)withoutCSS.escape` would catch the whole class.Pre-existing, for awareness: the authorization model is coarse — any BLOG_WRITE holder can mutate any author's story items. That's the established design of this family-internal archive (consistent with
update/deleteon the Geschichte itself), not something this PR introduces or worsens. No action needed here; just flagging that per-author ownership, if ever wanted, belongs in a dedicated issue.The follow-up you noted (note-less item:
ON DELETE SET NULLvschk_journey_item_not_empty→ 500) is an availability bug, not exploitable beyond a self-inflicted error — fine to defer, but the eventual fix should land with the failing test first, per usual.Good work keeping the permission annotations declarative and the dedup enforcement at the constraint layer — that's exactly where they're auditable.
📋 Elicit — Senior Requirements Engineer / Business Analyst
✅ Approved — every acceptance criterion of #795 is implemented and test-covered; both documented deviations are justified and traceable; no undeclared scope creep.
Acceptance criteria traceability (issue #795 → implementation → test)
GeschichteEditor.svelte:237–242→GeschichteSidebar.svelte:83–85GeschichteEditor.svelte.spec.ts"shows the document panel…"/geschichten/new; attach after first savegeschichteIdoptional, panel gated on itStoryCreate.svelte.spec.ts+GeschichteEditor"hides the document panel"journey_item, reactive listStoryDocumentPanel.sveltehandleAdd(pessimistic POST)JourneyItemServiceTest.append_to_STORY_type_creates_journey_item;JourneyItemIntegrationTest.story_type_can_hold_journey_items_end_to_endhandleRemove(optimistic DELETE)const prev = [...items]alreadyAddedIdsderived;failureMessage()geschichte_documents_capacityappend_to_STORY_type_respects_capacity_captext-ink-3 italicJourneyItemIntegrationTest.story_item_with_deleted_document_survives_and_remains_deletable<body>liveAnnounce+moveFocusAfterRemoveORDER BY positionuntouchedJourneyEditor.svelte:355passes nogeschichteId→ panel structurally cannot render;JourneyAddBaruntouched (verified on branch)GeschichteSidebar.svelte.spec.ts"does not render the panel without geschichteId"GESCHICHTE_TYPE_MISMATCHfully removed;GESCHICHTE_TYPE_IMMUTABLEstaysGESCHICHTE_TYPE_IMMUTABLEintactDeviations — assessed as justified
GeschichteEditorinstead of route-level threading. The issue's prohibition rested on a stale-schema premise. I verified the premise is indeed outdated:GeschichteEditor.svelte:15already types its prop asGeschichteView | null, which carriesidanditems. The deviation honors the issue's intent (don't read fields the type doesn't have) while the letter no longer applied. Confirmed with the product owner per the completion comment. ✔hiddenbelowsm— focusing it on mobile would reproduce the exact body-drop failure the requirement guards against. The deviation better satisfies the requirement's underlying need. ✔Scope check
Blockers
None.
Suggestions
!res.ok→failureMessage()→getErrorMessage(code)), so coverage is substantively there — but a one-line 403 variant inStoryDocumentPanel.svelte.spec.tswould close the literal gap and guard the mid-session-revocation scenario the issue calls out.ON DELETE SET NULL×chk_journey_item_not_emptycollision (note-less linked document → DB-level 500 on document delete) is flagged in the completion comment but not yet tracked. Please open the follow-up Gitea issue before this knowledge evaporates — it directly affects criterion 8's real-world reachability: today the deleted-document placeholder can only ever arise for items that carry a note.JourneyEditor). That is adequate; ifJourneyEditor's sidebar call site ever gains props, nothing fails loudly. A cheap assertion in an existingJourneyEditorspec thatgeschichte_documents_headingis absent would make the STORY-only constraint regression-proof. Optional.Fine work — the issue's spec density paid off: 15/15 criteria verifiably met, both deviations argued from the requirement's intent rather than convenience.
🧪 Sara Holt — QA Engineer & Test Strategist
⚠️ Approved with concerns — every acceptance criterion in #795 has a test behind it and the layering is right (unit caps/dedup with Mockito, real-Postgres Testcontainers for the V72/dangling-document states, browser-mode specs for everything user-visible). My concerns are all about error-path coverage gaps and timing-based flake risk in the new browser specs — none blocks the merge.
What I verified
JourneyItemServiceTest.java:280–333) would have failed withGESCHICHTE_TYPE_MISMATCHbefore the guard came out; the two now-untestable rejection tests are deleted, and theverifyNoInteractionsimport is swept (:42). Thestory()factory sibling (:743) follows the issue's instruction instead of inlining builders. ✔JourneyItemIntegrationTest.java:289–376): STORY append+retrieve against real Postgres, V72-style position-gap rows (10/20/30) written through the repository to mirror migrated data with order + removability asserted, and theON DELETE SET NULLdangling-document case — including the well-commented note workaround forchk_journey_item_not_emptyand the follow-up issue flag for the note-less collision. That last find is exactly what integration tests against real constraints are for; an H2 suite would never have surfaced it. ✔StoryDocumentPanel.svelte.spec.ts(14 tests) covers all plan items: position sorting, empty state, deleted-document placeholder + removability, POST payload/URL assertion, dedup-disabled option, both story-worded 409s, DELETE rollback-on-failure, both live-region announcements, and both focus-management cases (previous row / picker input). Test names read as sentences throughout. ✔GeschichteSidebar.svelte.spec.ts,StoryCreate.svelte.spec.ts) plus the editor-level pass-through spec — the "no panel on /geschichten/new" criterion can't regress silently. ✔DocumentPickerDropdown.svelte.spec.ts:245–261): generated default id +aria-labelfallback (protects the untouchedJourneyAddBar), and external-id-wins. ✔GESCHICHTE_TYPE_MISMATCHremoval is complete acrossErrorCode.java,errors.ts(union + case), and all three locale files; the 11 new i18n keys exist in de/en/es;GESCHICHTE_TYPE_IMMUTABLEuntouched. ✔Blockers
None.
Suggestions
getErrorMessage(code)branch offailureMessage()is never exercised (StoryDocumentPanel.svelte:740in the diff / component line ~46). The specs cover the two panel-local 409s and the no-code fallback (500 with{}body), but the AC explicitly names 403 ("including 403 for users without BLOG_WRITE — no silently unresponsive buttons") and no test feeds a coded non-409 failure (e.g.{ code: 'FORBIDDEN' }, 403) through either mutation. One add-path test asserting the translated message for a 403 would close the last uncovered branch in the panel's error routing.catchpaths (network failure:fetchrejects) are untested in bothhandleAddandhandleRemove— the AC says "network error, 403". The removecatchcontains its own rollback (items = prev) that is a separate branch from the!res.okrollback you do test.stubFetchalready centralizes the mock; amutation: 'reject'mode plus one test (vi.fn().mockRejectedValue(...)on non-GET) would cover it.StoryDocumentPanel.svelte.spec.ts:7,waitForDebounce). I know this mirrors the pre-existingPersonTypeahead.svelte.spec.ts:6idiom, but a 50 ms margin on a loaded CI worker is exactly the kind of timing assumption I'd rather not propagate into new files. Prefer a retrying locator wait (await expect.element(page.getByText(...)).toBeInTheDocument()before clicking — vitest browser locators auto-retry) over the timer; the sleep then becomes unnecessary. Same class of risk, smaller: the live-region assertions readtextContentsynchronously whileannounce()auto-clears after 500 ms (StoryDocumentPanel.svelte,announce()); the click→assert gap is normally far below that, but a GC/screenshot pause in CI can eat 500 ms. A retryingawait expect.element(...)against the live region, or a longer clear timeout, removes the race.stubFetchchecks method/URL/body but not that the CSRF header actually rides along. Oneexpect(post?.[1]?.headers)assertion on theX-XSRF-TOKENheader would make the regression (someone swappingcsrfFetchfor plainfetch, which the backend rejects) fail in the suite instead of in production.story_type_can_hold_journey_items_end_to_end(JourneyItemIntegrationTest.java:290) goes through the service layer, not HTTP — "end_to_end" oversells it slightly.…_through_the_service_layeror just dropping the suffix keeps the name honest; the existing class style is service-level, so behavior is fine.Good, disciplined test work overall — the V72 seeding through the repository (not the service) and the dangling-document constraint discovery are exactly the right instincts.
Review concerns addressed — 9 commits pushed
All blockers and non-blocking suggestions from the review round have been resolved.
Blockers (Markus) — ✅
l3-backend-3g-supporting.puml—f43df608docs/GLOSSARY.md+docs/ARCHITECTURE.md—6b2dd2f2JourneyItementry: no longer claims JOURNEY-only; explains both subtypes share the table; links ADR-037.GeschichteSTORY entry: mentions document attachment viajourney_items.ARCHITECTURE.md:41: "ordered stops in a JOURNEY-type Geschichte" → "document attachments / editorial notes shared by both subtypes — no application-level type guard"Architectural decision record (Markus suggestion 1+2) — ✅
docs/adr/037-journey-items-serve-both-geschichte-subtypes.md—05652a18geschichten_documents; guard was the only STORY restriction.story_documentstable, allowlist guard, per-subtype validation.i18n cleanup (Felix suggestion 1) — ✅
chore(i18n): delete dead keys ...—8e4810d5geschichte_editor_dokumente_heading/geschichte_editor_dokumente_hintremoved from de/en/es.frontend/src/before deletion.Focus-after-rollback (Leonie suggestion 1) — ✅
fix(geschichte): restore focus to item remove button after failed DELETE rollback—d91bedba!res.okrollback branch and network-error catch branch.handleRemove: afteritems = prev,await tick()+querySelector CSS.escape(item.id) [data-remove-btn] .focus()in both rollback paths.Frontend hardening (Nora suggestion 1, Leonie suggestion 2+3, Felix suggestion 4) — ✅
refactor(geschichte): CSS.escape in focus queries, announceTimer cleanup, warning icon—23272121CSS.escape(target.id)applied inmoveFocusAfterRemove's success-path querySelector (rollback paths already had it from the previous commit).$effect(() => () => { if (announceTimer) clearTimeout(announceTimer); })— timer cleaned up on component destroy.aria-hidden="true") added inside<p role="alert">—flex items-start gap-2layout; same heroicons stroke style as the remove button.Missing test coverage (Sara suggestions 1–4, Elicit suggestion 1) — ✅
test(geschichte): add 403, catch-path, and CSRF header coverage for StoryDocumentPanel—a6184fa1handleAdd: routes throughgetErrorMessage, alert text non-empty and not raw "FORBIDDEN".handleAddcatch (network error): showsjourney_mutation_error_reload.handleRemovecatch (network error): showsjourney_mutation_error_reload.X-XSRF-TOKENfrom cookie: setsdocument.cookie = 'XSRF-TOKEN=test-csrf-token', assertsheaders.get('X-XSRF-TOKEN') === 'test-csrf-token'. Regression-proof against swappingcsrfFetchfor barefetch.X-XSRF-TOKENheader: same pattern.Debounce flake fix (Sara suggestion 3) — ✅
test(geschichte): replace waitForDebounce sleep with retrying locator waits—9ea21f60const waitForDebounce = () => new Promise(...)removed.addViaPicker:await expect.element(page.getByText(title)).toBeInTheDocument()waits for the dropdown result before clicking — auto-retries, no fixed sleep.await expect.element(page.getByRole('option')).toBeInTheDocument()replaces the 350 ms sleep.Backend test rename (Sara suggestion 5) — ✅
test(geschichte): rename journey items integration test—7ca6492fstory_type_can_hold_journey_items_end_to_end→story_type_can_hold_journey_items_through_serviceFollow-up issue (Tobias suggestion 1, Markus suggestion 3, Elicit suggestion 2)
Filed as #805 (
bug: deleting a document linked via a note-less journey_item 500s at DB constraint, P1-high) — describes root cause, blast-radius expansion from this PR, reproduction steps, and four candidate fix directions. Assigned before merge as requested.🤖 Generated with Claude Code
marcel referenced this pull request2026-06-11 17:10:08 +02:00
The call site passes a null payload and carries the id in the documentId column (matching FILE_UPLOADED), so the javadoc claiming Payload: {"documentId": "uuid"} misdescribed the audit schema. Audit javadocs are the contract forensic queries are written against. Addresses @felix, @nora and @elicit review on PR #806. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>