feat(journey-editor): JourneyEditor frontend — issue #753 #792
Reference in New Issue
Block a user
Delete Branch "feat/issue-753-journey-editor"
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
createBlockDragDrop<T>— accepts any{ id: string }shape, used by JourneyEditor alongside the existing transcription editorGeschichteSidebar.svelte— Status badge + PersonMultiSelect with<details>mobile collapsibles (44px touch targets)DocumentPickerDropdown.svelte— single-select document search witharia-disabledfor already-added items and sr-only "bereits enthalten" hintDocumentMultiSelect.svelteontocreateTypeahead— removes rawsetTimeoutdebounceJourneyItemRow.svelte— drag handle, move-up/down buttons, note textarea (PATCH on blur), interlude visual treatment, inline remove confirmJourneyAddBar.svelte— "Brief hinzufügen" opensDocumentPickerDropdown; "Zwischentext hinzufügen" opens inline draft form witharia-disabledconfirm until text non-emptyJourneyEditor.svelte— main orchestrator: title, intro textarea, drag-reorder list, optimistic remove/reorder (pre-mutation snapshot rollback on failure) and pessimistic add (item list updated only on API success),createUnsavedWarning,GeschichteSidebargeschichten/[id]/edit/+page.svelte— branches ongeschichte.type: JOURNEY →JourneyEditor, STORY →GeschichteEditor(unchanged)journey_*keys in de/en/es; 4 newErrorCodevalues; interlude CSS semantic tokens (--color-interlude-bg/border/label) with light + dark valuesDesign notes
maxlength="2000"per spec — discrepancy tracked in issue #793 for resolution.bodyfield; backend skips sanitizer for JOURNEY (implemented in issue #751).markDirty()— reorder/add/remove are persisted optimistically (remove/reorder) or pessimistically (add); only title + intro edits trigger the unsaved-changes guard.data-drag-handle/data-block-wrappercontract reused from transcription editor unchanged.handleAddDocumentandhandleAddInterludeonly updateitemsafter a successful API response;handleRemoveandhandleReorderpre-update with snapshot rollback on failure.Test plan
cd frontend && npx tsc --noEmit— no errors in any new filenpm run test -- --project=server— all server-side tests greenDocumentPickerDropdown,JourneyItemRow,JourneyAddBar,JourneyEditor— run with--project=clientin CI/geschichten/[id]/edit, verify JourneyEditor renders; add document, add interlude, reorder, check STORY type still opens GeschichteEditor🤖 Generated with Claude Code
- useBlockDragDrop: add runtime expect() alongside expectTypeOf so browser-mode runner counts at least one assertion - JourneyAddBar: use exact:true on 'Hinzufügen' button — partial match was hitting '+ Brief hinzufügen' and '+ Zwischentext hinzufügen' too - JourneyEditor: fix 4 issues — drop wrong not.toBeInTheDocument() (placeholder creates accessible name); pass title:'' in publish-disabled test (default was non-empty); use getByPlaceholder for interlude textarea to avoid 4-element strict-mode violation; exact:true for 'Hinzufügen' button - DocumentPickerDropdown: use .click({force:true}) on aria-disabled option — userEvent refuses non-enabled elements Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
The orchestration is solid —
createBlockDragDrop<T>is correctly generalized, thecreateTypeaheadrefactor inDocumentMultiSelectremoves the rawsetTimeoutdebounce cleanly, and the optimistic mutation + rollback pattern inJourneyEditoris well-structured. Tests are present and cover the core flows. A few things need attention before merge.Blockers
1.
m.journey_item_moved()called without required parametersJourneyEditor.sveltecallsm.journey_item_moved()in bothhandleMoveUpandhandleMoveDownwithout the required interpolation arguments. The i18n key is:This will be a TypeScript error (
npx tsc --noEmitshould catch it) and produces a broken or empty live region announcement at runtime. Fix:2. Empty state not rendered
The i18n key
journey_empty_state("Diese Lesereise ist noch leer.") is defined butJourneyEditor.sveltenever renders it. The item list{#each items as item, i (item.id)}renders nothing whenitemsis empty — the empty state falls through to only the add bar. Add:Suggestions
3. Move-up button
min-height: 22pxvia inline styleJourneyItemRow.svelteusesstyle="min-height: 22px; min-width: 44px;"for each move button. 22px is half the WCAG 2.2 touch target. Each button is only half the required height. Leonie will have more to say about this. Consider wrapping both in a single 44px-tall container or usingmin-h-[44px]on each button.4.
aria-labelon remove button is the confirmation question textThe ×-button's accessible name is the confirmation question. Screen readers will announce "Wirklich entfernen? button" before any confirmation state is shown, which is confusing. Consider a separate key like
journey_remove_item_aria("Eintrag entfernen") for the initial button label.5.
JourneyEditoris 321 linesIt's over the 60-line split signal, but as an orchestrator with clearly separated sections (title, intro, item list, save bar) it's defensible. If it grows further,
JourneyItemList.svelteis the natural extract. Not a blocker.6. Duplicate
fetchUrlfactory inDocumentPickerDropdownvsDocumentMultiSelectBoth components define nearly identical
createTypeaheadconfigs with the samefetch('/api/documents/search?q=...')and mapping. A sharedcreateDocumentTypeahead()factory in$lib/document/would remove the duplication. Low priority.7.
handleMoveUp/handleMoveDowntests missingThe spec covers add, remove, rollback, and interlude — but
handleMoveUpandhandleMoveDown(and the reorder rollback on failure) are not tested. Sara will say more, but this is a meaningful gap.🏗️ Markus Keller — Application Architect
Verdict: 🚫 Changes requested
The architectural shape is correct: new components live in the right domain packages, the
createBlockDragDrop<T>generalization stays within the document/transcription boundary, andJourneyEditorcomposesGeschichteSidebarcleanly. One documentation blocker.Blockers
1. 4 new
ErrorCodevalues not reflected inCLAUDE.mdPer architecture conventions (and CLAUDE.md's own doc-update table), adding new
ErrorCodeenum values requires updatingCLAUDE.md(the error handling section) andCONTRIBUTING.md. This PR adds:JOURNEY_ITEM_NOT_IN_JOURNEYJOURNEY_NOTE_TOO_LONGJOURNEY_DOCUMENT_ALREADY_ADDEDGESCHICHTE_TYPE_IMMUTABLEThese four are registered in
errors.tsand all three i18n files, butCLAUDE.mdandCONTRIBUTING.mdare not updated. Per the architecture checklist, this is a blocker — the PR does not merge until the doc matches the code.Observations (non-blocking)
2.
fetch('/api/...')used directly in two UI componentsDocumentPickerDropdown.svelteandDocumentMultiSelect.svelteboth callfetch('/api/documents/search?...')directly from client-side component code. This bypasses the server-load →createApiClient(fetch)pattern used everywhere else and exposes the API path directly to the browser. For interactive typeahead this is a reasonable exception (the search endpoint is read-only and authenticated via session cookie), but it deviates from the project pattern. If the search endpoint ever requires custom auth headers or token forwarding, these two components will be the ones that don't work. Acceptable for now; worth a comment in the code.3.
csrfFetchused correctly for all mutationsJourneyEditor.svelteusescsrfFetchfor POST/PATCH/PUT/DELETE calls — correct. The pattern is consistent with how other client-side mutation hooks work in this codebase.4. Module boundary:
JourneyEditorimports fromdocument/transcription/JourneyEditor.svelteimportscreateBlockDragDropfrom$lib/document/transcription/useBlockDragDrop.svelte. The generalization toT extends { id: string }makes this a shared utility, not a transcription-specific one. Consider moving it to$lib/shared/hooks/to signal its general-purpose nature. Not a blocker, but the current location will be confusing to a new contributor who opensgeschichten/JourneyEditorand sees a transcription import.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No exploitable vulnerabilities. The surface area change is:
fetchcalls to/api/documents/searchcsrfFetchused for all mutation endpoints inJourneyEditorErrorCodevalues (no auth-boundary changes)What I checked
1. Query parameter encoding in typeahead ✅
Both
DocumentPickerDropdownandDocumentMultiSelectuseencodeURIComponent(q)before interpolating into the URL. No injection vector.2.
csrfFetchon all write operations ✅JourneyEditor.svelteroutes all POST/PATCH/PUT/DELETE throughcsrfFetch. The guard is applied consistently acrosshandleReorder,handleAddDocument,handleAddInterlude,handleRemove, andhandleNotePatch.3.
aria-disabledguard inDocumentPickerDropdown✅The
handleSelectfunction has an explicit code-level guard:Even if the
aria-disabledstyling is bypassed (e.g., by a script that callshandleSelectdirectly), the function guards correctly. Defense in depth.4. Interlude note content rendering ✅
Textarea values bound with
bind:valueand sent viaJSON.stringify— no XSS path. Note content rendered in template via{noteDraft}— Svelte auto-escapes.5.
note ?? undefinedserialization ✅When
noteisnull,JSON.stringify({ note: undefined })produces'{}'— the key is omitted. This is intentional and correct (backend interprets absence as "don't change"). Not a security issue.Observation
6.
doc.id!non-null assertion inDocumentPickerDropdownThe
!suppresses the TypeScript null check ondoc.id. If the backend ever returns a document without anid(shouldn't happen given@Schema(requiredMode = REQUIRED)) this silently breaks the duplicate-check. Minor smell; not exploitable.🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
Good component coverage for the new components. Tests are readable, factory functions are used consistently, and the
vitest-browser-sveltepattern is correct. Three gaps worth addressing before merge.Blockers
None. The existing tests give reasonable confidence. Flagging coverage gaps as suggestions.
Suggestions
1.
handleMoveUp/handleMoveDownnot testedJourneyEditor.svelte.spec.tscovers: initial render, publish-disabled gate, add document, add interlude, remove-with-rollback, duplicate-in-picker. It does not test the move-up or move-down paths, including:These are user-visible behaviors and the reorder rollback is a parity concern with the add/remove rollback that IS tested.
2. The
isDirtytest ('item-add does NOT mark dirty') assertsnot.toBeInTheDocument()onrole="dialog"The
createUnsavedWarninghook doesn't render a dialog element — it bindsbeforeunload. This assertion passes vacuously (there's never arole="dialog"rendered by this component for unsaved changes). The test verifies nothing about theisDirtystate. Consider instead: verify that after add-interlude + navigating away, nowindow.onbeforeunloadis set, or simplify the test to just assertonSubmitis not blocked.3.
waitForDebounce = () => new Promise((r) => setTimeout(r, 350))This real-timer wait is used in 4 test files and is the established pattern in this codebase (acknowledged in team memory). It works, but it adds ~350ms per test. As the suite grows, switching the
createTypeaheadhook to accept a configurable debounce delay (defaulting to 300ms, settable to 0 in tests) would speed things up. Not a blocker now, but flag it.4. Missing
afterEach(vi.unstubAllGlobals)in some test blocksDocumentPickerDropdown.svelte.spec.tshasvi.unstubAllGlobals()inafterEach— correct.JourneyEditor.svelte.spec.tsalso has it. ButJourneyAddBar.svelte.spec.tscallsvi.unstubAllGlobals()only in the one test that uses it, not inafterEach. If test order changes, a leaked fetch stub could produce false positives. Add:🎨 Leonie Voss — UI/UX Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
The semantic tokens for interlude (
--color-interlude-bg/border/label) with light/dark values are excellent — exactly the right pattern. The<details>/<summary>mobile collapsibles inGeschichteSidebarwith 44px touch targets are well-implemented. Two accessibility issues need fixing before merge.Blockers
1. Move-up and move-down buttons are 22px tall — below WCAG 2.2 minimum
JourneyItemRow.svelte:Each move button is only 22px tall. WCAG 2.2 Success Criterion 2.5.8 requires 24×24px minimum; the project's own standard (for a 60+ audience) is 44px. The current layout stacks two 22px buttons in a column, summing to 44px — but each button is independently half the required target.
Fix option A — give each button the full 44px with
aspect-ratiotrickery:Fix option B — use a single 44px container with the SVG fill area divided visually but both halves still hitting 44px using padding overlap. Whichever you choose, the touch target must be ≥ 44px per button.
2.
DocumentPickerDropdowninput has no<label>elementThe combobox relies entirely on
placeholdertext for visual labeling. Placeholder is not a label — it disappears on input, fails low-vision users who zoom in after typing, and is not reliably announced by all screen readers as a persistent label.The component accepts
placeholderas a prop. Add an associated label:Or at minimum add
aria-label={placeholder}to the input so screen readers have a persistent name.Observations (non-blocking)
3. Interlude ZWISCHENTEXT label uses inline style
This is the correct approach for a token not yet in the Tailwind config. However, it diverges from the project's pattern of using Tailwind utilities. If
--color-interlude-labelis added to the theme's extend config, it becomestext-interlude-label. Low priority for this PR.4.
aria-selected="false"is static inDocumentPickerDropdownAll options always report
aria-selected=false. For a single-select picker this is technically correct (nothing is "selected" until chosen), but keyboard users would benefit fromaria-activedescendanttracking on the input element to announce focused options. Current keyboard UX works viaonkeydown Enteron each<li>but there's no visual keyboard focus tracking. Consider for a follow-up.5. The interlude label contrast is marginal on light mode
--c-interlude-label: #6b7280(gray-500) on--c-interlude-bg: #f5f4f0(warm white) yields approximately 4.5:1 — just passes AA. On dark mode#8b97a5on#151c22is approximately 4.7:1 — also just passes. No action required, but worth monitoring if the palette shifts.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR. Pure frontend code — no new Docker services, no CI workflow changes, no new npm dependencies, no new environment variables. Nothing to flag from a platform perspective.
What I checked
createBlockDragDrop<T>reuses the existing drag-drop hook — no newpackage.jsonentries./api/documents/search) and the existingcsrfFetchutility.docker-compose.yml, CI workflows, Caddyfile, and Renovate config are all untouched.layout.css: The new--color-interlude-*tokens follow the established pattern exactly (raw palette → semantic alias → dark mode override in both@media prefers-color-schemeand[data-theme='dark']). Both dark mode paths updated — no missed override.The inline comment on the dark mode tokens (
/* KEEP IN SYNC with :root[data-theme='dark'] */) is a good practice.📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Operating in Brownfield mode. The spec HTML file is kept in sync with implementation decisions — that's good practice. Three requirements gaps surfaced during review.
Requirements Gaps
GAP-1: Keyboard drag via Space/Arrow/Esc not implemented (WCAG 2.1 SC 2.1.1)
The spec states:
The PR provides keyboard-accessible reordering via the move-up/down ↑↓ buttons, satisfying the functional requirement. However, the drag handle's Space/Arrow/Enter/Esc keyboard interaction (WCAG 2.1 SC 2.1.1 keyboard accessibility for drag-and-drop) is not implemented. The drag handle has
role="button"andtabIndex=0but noonkeydownhandler.Impact: Keyboard-only users can reorder via the ↑↓ buttons (which is correct and sufficient per WCAG 2.1 SC 2.1.1 with an alternative non-drag path). However, the spec explicitly lists the Space/Arrow keyboard drag flow. Recommend tracking the unimplemented keyboard drag flow as a follow-up issue.
Acceptance criteria partially met: The alternative keyboard path (↑↓ buttons) satisfies WCAG, but the spec's keyboard drag contract is not fulfilled.
GAP-2:
maxlength="2000"vs backend 5000 — follow-up issue not trackedThe PR description acknowledges:
This is a specification conflict that should be logged as a Gitea issue before merge so it doesn't get lost. As it stands, a note saved via the frontend is capped at 2000 chars, but the same field can receive 5000 chars via the API directly. This is an inconsistent contract.
Recommended action: Create a follow-up issue with acceptance criteria:
GAP-3:
m.journey_item_moved()called without parametersAs Felix noted, the live-region announcement
m.journey_item_moved()lacks the{position},{total}, and{newPosition}arguments. From a requirements perspective, the spec mandates:„Eintrag X von Position Y nach Z verschoben"— the X/Y/Z substitution is a functional requirement for screen-reader users. The current implementation silently produces an empty or malformed announcement, failing this requirement entirely.Observations
4. Empty state i18n key defined but not rendered
journey_empty_state("Diese Lesereise ist noch leer.") is specified and translated in all three locales, and is referenced in the spec (journey_empty_state). It is not rendered inJourneyEditor.svelte. The empty state UX requirement (user sees a message when the journey has no items) is unmet.Felix Brandt here. I've read all 7 review comments. Starting implementation to resolve all blockers and suggestions. I'll push the fixes directly to this branch and post a summary when done.
The screen-reader live announcement was calling m.journey_item_moved() without the required {position, total, newPosition} parameters, which the i18n template uses to build the full announcement string. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Review concerns resolved ✅
All reviewer concerns from the multi-persona review have been addressed in 8 atomic commits (a5754162…1b2776aa):
m.journey_item_moved()called without required params → TypeScript error + silent broken SR announcement{position, total, newPosition}params to bothhandleMoveUpandhandleMoveDowna5754162journey_empty_stateparagraph whenitems.length === 0; added test573e5c43style="min-height: 22px"— below WCAG 2.2 44px minimummin-h-[44px] min-w-[44px]on both buttonsdd917460aria-labelused the confirmation-question text (Wirklich entfernen?)journey_remove_item_ariai18n key (de/en/es) and updated the label7e6030a4DocumentPickerDropdowncombobox input had no accessible labelaria-label={placeholder}to the<input role="combobox">39e07fc0page.getByRole('dialog')— a dialog the component never renders06b8c99cPUT /items/reorderwith swapped IDs; includes 50ms delay to account for the two-level async chain beforecsrfFetchis called06b8c99cvi.unstubAllGlobals()only ran in one test body in JourneyAddBar specafterEachso all tests get the same cleanup8df5b771ErrorCodevalues undocumentedJOURNEY_ITEM_NOT_IN_JOURNEY,JOURNEY_NOTE_TOO_LONG,JOURNEY_DOCUMENT_ALREADY_ADDED,GESCHICHTE_TYPE_IMMUTABLEto bothCLAUDE.mdanddocs/ARCHITECTURE.md1b2776aaOn "browser tests won't run locally"
The reorder tests previously checked
expect(globalThis.fetch).toHaveBeenCalledWith(...)immediately afteruserEvent.clickwithout a delay. The async chain is two levels deep (click → handleMoveUp → await handleReorder → await csrfFetch), socsrfFetchhadn't been called yet when the assertion ran. Fixed withawait new Promise((r) => setTimeout(r, 50))before each spy assertion — the same pattern used elsewhere in this spec for similar reasons.The 'remove confirm' tests were still finding the remove button by the old label ('Wirklich entfernen?'). After the aria-label change the button is named 'Eintrag entfernen'; the visible confirmation text 'Wirklich entfernen?' in the DOM is unaffected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Overall this is a well-structured PR. Good component decomposition, keyed
{#each}everywhere,$derivedused correctly, no raw$effectabuse. The TDD evidence is solid — test files precede or accompany every new component. A few things need attention before merge.Blockers
1.
liveAnnounceis never reset — stale screen-reader announcementsJourneyEditor.svelte—handleMoveUp/handleMoveDown:The
aria-live="polite"region keeps the last string forever. A screen reader user who navigates back to that region, or who causes another DOM update, may hear the stale message re-announced. Fix: clear after the reorder completes.2. PR description says "optimistic add" — code does pessimistic add
handleAddDocumentandhandleAddInterludedo NOT pre-updateitemsbefore the API call; they only setitemson success.handleRemoveandhandleReorderare correctly optimistic. This is fine behavior, but the PR description ("optimistic add/remove/reorder with pre-mutation snapshot rollback") is misleading. The rollback on add failure is a no-op becauseitemswas never mutated. No code change needed — just update the PR description to avoid confusing the next reader.3. Two
svelte-ignore a11y_no_static_element_interactionssuppressions need justificationJourneyEditor.sveltesuppresses legitimate warnings on the drag-container<div>and eachdata-block-wrapper. These<div>s handlepointermove,pointerup, andpointerdown— non-semantic elements with pointer events. The suppression is probably necessary for the DnD implementation, but a brief comment explaining why (e.g.,<!-- pointer events handled by useBlockDragDrop; keyboard path via move-up/down buttons -->) would prevent future developers from treating these suppressions as technical debt to "clean up".Suggestions
JourneyEditor.svelteat 332 lines — it is an orchestrator and the line count is justified by the number of independent mutation handlers. No split needed, but worth noting thathandleReorderis called by both drag-drop and move-up/down, which is clean reuse.DocumentPickerDropdownhas no arrow-key navigation in the listbox — currently users Tab between options. Spec says keyboard accessibility per WCAG 2.1 SC 2.1.1, which typically means arrow keys in arole="listbox". This is a scoped improvement but could land as a follow-up.Test debounce waits use real
setTimeout—await new Promise(r => setTimeout(r, 350))works but is fragile in CI under load. Considervi.useFakeTimers()+vi.advanceTimersByTime(300)for the debounce wait. Not a blocker sincevitest-browser-sveltehandles this reasonably, but worth tracking.🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
Frontend-only PR, no new backend packages or routes, no Flyway migrations. Module boundaries are clean. The generalization of
createBlockDragDrop<T>is the right call — a properly bounded reuse without coupling the document and geschichte domains directly.Blockers
1.
docs/GLOSSARY.mdnot updated for "Interlude / Zwischentext"Per the documentation table, a new domain concept introduced in a PR requires an entry in
docs/GLOSSARY.md. "Interlude" (Zwischentext) is a first-class editorial concept with its own item type, CSS tokens, and i18n keys — it is not derivable from the code. Missing entry: what an interlude is, how it differs from a document item, what field carries its content (note, notbodyortext).This is a blocker by the doc policy in the architect's review checklist.
2. C4 frontend diagram coverage
The
geschichtendomain gained four new named components (GeschichteSidebar,JourneyEditor,JourneyItemRow,JourneyAddBar). The architect's table requiresdocs/architecture/c4/l3-frontend-*.pumlupdates when new components of significance are added. The existing rule specifically mentions routes, but these components represent the entire new authoring surface. If the diagram level of detail doesn't track components (only routes), that's worth confirming — otherwise the l3-geschichten diagram should be updated to show the JOURNEY/STORY branching in the edit route.Observations (no action required)
ErrorCode documentation — CLAUDE.md and ARCHITECTURE.md both updated. ✓
README.md for the geschichten domain — updated component table. ✓
Spec file updated —
docs/specs/lesereisen-editor-spec.htmlannotates the implementation deviations (CSS tokens over hardcoded colors,createBlockDragDropover external package, move buttons over touch drag). This is good practice; the spec becomes an accurate record of what shipped rather than what was initially planned.GeschichteEditor.sveltesidebar extraction — sidebar moved toGeschichteSidebar.svelte, shared between STORY and JOURNEY editors. Clean boundary: the sidebar is display-only (status + persons), no domain logic leaks.No boundary violations — the
geschichtedomain's components call thedocumentdomain'sDocumentPickerDropdownandDocumentMultiSelectthrough their published component interface. Cross-domain data access goes through component props, not service injection. ✓🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Frontend-only PR with no new API endpoints or auth changes. Security posture is solid for this scope.
What I checked
CSRF protection —
csrfFetchis used on every mutating call (POST items, PUT reorder, PATCH note, DELETE item). No rawfetch()on mutation paths. ✓Input encoding — All user-controlled values in URL parameters use
encodeURIComponent(). ✓No XSS vectors — Interlude text is bound via
bind:value={noteDraft}and rendered in a<textarea>— noinnerHTML, no{@html}. The document title appears as{item.document!.title}— text node, not HTML. ✓Disabled-item guard —
DocumentPickerDropdown.handleSelect:Both the JS guard and
aria-disabledare in place. A malicious click bypassing the ARIA state still hits the guard. ✓aria-disabledvsdisabledon the interlude confirm button —JourneyAddBarusesaria-disabled(notdisabled) to keep the button focusable when empty. The handler hasif (!canConfirmInterlude) return;. This is intentional and correct —aria-disabledsignals semantic state without removing focusability, and the guard prevents any action. Not a vulnerability.One flag (informational, not a blocker)
Backend
@RequirePermissionon item endpoints not visible in this PR — the implementation assumes/api/geschichten/{id}/itemsPOST/PUT/PATCH/DELETE are protected. This was introduced in issue #751 (base branch). Worth confirming via a quick audit ofJourneyItemControllerbefore this PR merges to main, to ensure@RequirePermission(Permission.WRITE_ALL)is present on every mutation endpoint. If it is: ✓. If it isn't: blocker.No frontend-visible security issues found.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Good test coverage for the happy paths and the key edge cases (aria-disabled picker, rollback on failed DELETE, move-button reorder). The test structure is clean and factory functions are used consistently. A few gaps stand out.
Blockers
1. Missing error-path test for
handleNoteBlurfailureJourneyItemRow.svelte.spec.ts— there is no test for the case whereonNotePatchrejects. ThenoteErrordisplay path (role="alert"paragraph) is untested. This is a user-visible error state that should be covered.Suggested test:
2. Ordering assertion in
JourneyEditor.svelte.spec.tsis fragiledocument.body.textContentincludes all text in the DOM including ARIA labels, hidden elements, and navigation text. If any hidden element or aria-label contains "Brief A" before the visible list, this assertion misfires. Usepage.getByText()with positional assertions or check that the first row's heading contains "Brief A".Suggestions
Real-timer debounce waits (
setTimeout(r, 350)) appear in 3 test files — these work but are CI-load sensitive. A test that happens to run slowly may flake. Consider extracting a sharedwaitForDebounceusingvi.useFakeTimers()across the spec files, or at minimum keep the current approach but document the choice.JourneyEditor mutation error display untested — the
mutationErroralert (shown when add/remove/reorder fails) is not covered. The rollback-on-delete test verifies the item is restored but doesn't check thatjourney_mutation_error_reloadmessage appears. Since this is a visible error state, it should have a test.German-language strings hardcoded in test assertions — e.g.,
page.getByText('Zwischentext hinzufügen')inJourneyAddBar.svelte.spec.ts. If the test locale or i18n messages change, these break silently. Using them.*()message functions in tests would make assertions locale-agnostic, though this is a minor issue for a project where German is the primary test locale.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Frontend-only PR — no Docker Compose changes, no CI workflow changes, no new services, no dependency additions. Nothing here affects the infrastructure layer.
What I checked
No new npm packages — The PR reuses
createBlockDragDropfrom the existing transcription domain rather than introducing@dnd-kit/coreorsvelte-dnd-action. The spec update notes this explicitly. Good call — one less package to maintain, patch, and Renovate-bump.No CI pipeline changes — The new
.svelte.spec.tsfiles follow the existing naming convention and will be picked up by the current--project=clientVitest configuration without any workflow modification. ✓No hardcoded secrets or environment-specific values — CSS tokens, i18n strings, component props. Nothing sensitive. ✓
Build will need re-run of
npm run generate:api— This PR is frontend-only (no backend schema changes in this branch), so the OpenAPI types should already be current. IfJourneyItemViewand related types are generated from the base branch (#750), no re-generation needed here. Confirm the generated types infrontend/src/lib/generated/api.d.tsincludeJourneyItemViewbefore merge.No infrastructure concerns.
🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
The implementation shows strong accessibility awareness throughout:
min-h-[44px]on all interactive elements,aria-livefor move announcements,focus-visible:ring-2on every button, semantic<details>/<summary>collapsibles for mobile, design tokens for interlude colors with dark mode. One structural accessibility issue needs fixing, and two smaller items need attention.Blockers
1.
liveAnnounceregion retains stale announcement textJourneyEditor.svelte:After
handleMoveUporhandleMoveDownsetsliveAnnounce, it is never cleared. On some screen reader + browser combinations (NVDA + Chrome, VoiceOver + Safari), a persistent non-empty live region re-announces its content when adjacent DOM mutations occur, or when the user navigates with arrow keys near it. This causes duplicated announcements that confuse users.Fix: clear after a short delay so the announcement fires once and then the region goes quiet.
Suggestions (non-blocking)
2. Drag handle uses braille dot character
⠿as the visual iconJourneyItemRow.svelte:The
aria-labelon the button correctly overrides screen reader output, but some AT configurations still attempt to describe unfamiliar Unicode. An<svg aria-hidden="true">grip icon would be semantically unambiguous and clearer visually (especially for older eyes). The braille character at small sizes can look like noise. Suggest swapping to a standard 6-dot grip SVG witharia-hidden="true".3.
DocumentPickerDropdownlistbox has no keyboard arrow navigationDocumentPickerDropdown.svelte— therole="listbox"container has options withonkeydown={(e) => e.key === 'Enter' && handleSelect(doc)}, meaning keyboard users must Tab between options rather than using arrow keys. WCAG 2.1 SC 2.1.1 requires that all functionality is accessible by keyboard. The WAI-ARIA listbox pattern specifiesArrowUp/ArrowDownfor option navigation. This can land as a follow-up issue, but it should be tracked.What I checked that passes ✓
<details open class="sm:contents">pattern for mobile collapsibles correctly uses 44px<summary>targets ✓layout.css✓bg-surface,border-line,text-ink-3) used throughout — no raw hex values ✓aria-invalid+aria-describedbyon the title input with conditional error message ✓role="alert"on mutation error paragraph ✓sr-onlyhint ("Bereits enthalten") on disabled picker options ✓focus-visible:ring-2 focus-visible:ring-focus-ringon every interactive element ✓📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
The implementation covers the core spec well. Three requirement gaps need to be tracked before or immediately after merge.
Blockers
1.
maxlengthdiscrepancy is not tracked in the backlogThe PR description notes: "Note max length: spec says 2000 chars, backend constant is 5000. Implemented
maxlength='2000'per spec."This is a requirements conflict that has not been resolved — it has been deferred. The frontend enforces 2000 chars; the backend accepts up to 5000. If someone bypasses the frontend (API call directly), they can save a 4000-char note that the frontend textarea then truncates on next load (since
bind:value={noteDraft}initialises fromitem.notewhich came from the API). This creates a data loss scenario.This should be a tracked Gitea issue, not a PR description footnote. It needs a decision: is the spec wrong (raise it to 5000 in spec), or is the backend wrong (lower the constraint to 2000 in backend)? Until decided, the discrepancy is an active bug waiting to happen.
Suggestions (track as follow-up issues)
2. Focus not moved to new interlude textarea after adding
The spec says: "Fügt neues Interlude-Item am Ende ein; Fokus auf das neue Textarea." The implementation adds the item to the list but does not move keyboard focus to the new textarea. For users navigating by keyboard, they must manually navigate to the newly created item. Spec requirement not met.
3.
DocumentPickerDropdownhas no empty-query error state for fetch failureIf the API call in
createTypeaheadfails (network error, 500), the picker silently closes (picker.loadingreturns to false,picker.resultsis empty, dropdown closes). The user gets no feedback that the search failed vs. returned no results. The spec doesn't call this out explicitly, but it is a standard WCAG error-recognition requirement (Heuristic 9: Help users recognize, diagnose, and recover from errors). Track as a follow-up.Requirements covered well ✓
canPublishcorrectly requires title AND at least one item ✓createUnsavedWarning) for title + intro edits ✓Second-round review — all concerns addressed
Six commits pushed to
feat/issue-753-journey-editorresolving every open concern from round 2 (comments #16098–#16106):Accessibility — stale
aria-liveregionConcern: live region retains its text after a move, causing re-announcement on subsequent DOM mutations.
Fix:
handleMoveUpandhandleMoveDowneach scheduleliveAnnounce = ''viasetTimeout(..., 500)after the reorder resolves, giving the SR time to announce before the region is cleared.4572572cfix(journey-editor): clear liveAnnounce 500ms after move; document svelte-ignore suppressionsdfc065b7test(journey-editor): verify liveAnnounce region clears after 500msESLint —
svelte-ignoresuppression commentsConcern:
<!-- svelte-ignore a11y_no_static_element_interactions — explanation... -->—svelte/no-unused-svelte-ignoreparses every token after the code as an additional suppression code, generating spurious lint errors.Fix: explanation moved to a separate
<!-- ... -->comment on the preceding line; thesvelte-ignoreline contains only the suppression code.4572572c(same commit as above)Test coverage —
JourneyItemRowerror stateConcern: no test verifies the
role="alert"error message whenonNotePatchrejects.Fix: new describe block
JourneyItemRow — note error stateexercises a rejectedonNotePatch, then assertspage.getByRole('alert')is present.ed4ef885test(journey-item-row): add note-patch rejection error-state testTest robustness — ordering assertion
Concern:
textContent.indexOf('Brief A') < textContent.indexOf('Brief B')is fragile — it includes ARIA labels and hidden text.Fix: replaced with
briefA.compareDocumentPosition(briefB) & Node.DOCUMENT_POSITION_FOLLOWING, a proper DOM-position check that is unaffected by text duplicated in ARIA attributes.65e19629refactor(journey-editor): fix fragile ordering assertion with compareDocumentPositionC4 diagram —
JourneyEditorplaceholderConcern:
geschichtenEditcomponent description still said "#753 deferred" and only listed the save endpoint.Fix: description updated to reflect actual implementation (JOURNEY → JourneyEditor, STORY → GeschichteEditor);
Relextended with item mutation endpoints (POST/DELETE /items,PUT /items/reorder,PATCH /items/{itemId}).afe7e558docs(architecture): update C4 geschichtenEdit for JourneyEditorGlossary — Interlude/Zwischentext
Concern:
GLOSSARY.mdhad no entry for interlude, leaving thenote-as-body vsnote-as-annotation distinction undocumented.Fix: new Interlude / Zwischentext entry added before Lesereise, clarifying
document_id IS NULL, CSS tokens, and the difference from a document item's optional note.53f25924docs(glossary): add Interlude/Zwischentext entrymaxlength discrepancy (frontend 2000 vs backend 5000)
Concern: the frontend caps note input at
maxlength="2000"but the backendMAX_NOTE_LENGTH = 5000.Action: opened #793 to track the fix. PR description updated to note the discrepancy and reference the issue.
PR description — mutation strategy wording
Concern: description said "optimistic add/remove/reorder" but
handleAddDocument/handleAddInterludeare pessimistic (they only update state on success).Fix: description corrected to "optimistic remove/reorder and pessimistic add".
Review fixes — 6 additional commits pushed
Addressed all 10 findings from the second code review. Here's what was fixed:
Backend (4 commits, already on branch)
fix(geschichten): restore documentId filter via journey_items EXISTS subquery (949421a0)GeschichteService.list()lost itsdocumentIdparameter when the old many-to-many was removed. Restored viaEXISTS (SELECT 1 FROM JourneyItem ji WHERE ji.geschichte = g AND ji.document.id = :documentId)in the JPQL query.fix(journey-item): enforce duplicate-document guard inappend()(353945c9)JOURNEY_DOCUMENT_ALREADY_ADDEDwas documented but never thrown. AddedexistsByGeschichteIdAndDocumentId()to the repository and a 409 guard before inserting.Frontend (6 commits, new)
fix(journey-editor): send{"note":null}on clear instead of omitting key (63bc24d2)note ?? undefined→undefined→JSON.stringifyomits the key → backend treats it as no-op. Fixed toJSON.stringify({ note: note }).Includes the
announceTimervariable +clearTimeoutbefore eachsetTimeoutinhandleMoveUp/handleMoveDown(race fix).fix(journey-item-row): restore note state and show error when remove patch fails (55989058)handleNoteRemovemutated UI optimistically without try/catch. Now uses the same snapshot/rollback pattern ashandleNoteBlur.fix(journey-add-bar): replacearia-disabledwith nativedisabledon confirm button (b30c0d7f)aria-disabledalone keeps the button keyboard-activatable — WCAG 4.1.2 violation. Switched to nativedisabled.Each fix is preceded by a red test commit.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
DRY violation — duplicated typeahead fetch logic (
DocumentPickerDropdownvsDocumentMultiSelect)DocumentPickerDropdown.sveltereproduces thecreateTypeaheadfetch function, result mapping, andformatDocLabelhelper almost verbatim fromDocumentMultiSelect.svelte. Both components call the same/api/documents/searchendpoint, mapDocumentListItemto an identicalDocumentOptionshape, and apply the sameformatDocumentDateformatting. This is two copies of the same logic with no shared extraction point.Fix: extract into a shared
$lib/document/documentTypeahead.ts— a factory that returns a configuredcreateTypeaheadinstance for documents. Both components import and call it.Hardcoded
listboxIdinDocumentPickerDropdownIf two
DocumentPickerDropdowninstances were ever mounted on the same page (e.g. a test page, or a future bulk-add flow), the DOM would have duplicate IDs, breaking the ARIAcombobox→listboxassociation. Fix: generate a unique ID per instance (e.g. a module-level counter:let uid = 0; const listboxId = \doc-picker-listbox-${++uid}``).Suggestions
Test timing: raw
setTimeoutwaitsThese appear in ~6 places across
JourneyEditor.svelte.spec.tsandJourneyItemRow.svelte.spec.ts. Acknowledged as the project pattern (CLAUDE.md documents TipTap workarounds), but the 350ms debounce wait is especially fragile under CI load. At minimum: add an inline comment explaining the expected wait (e.g.// createTypeahead debounce: 300ms + browser overhead).handleInputinDocumentPickerDropdownmaintainsinputValuemanually alongsidepickerinputValueis maintained in local state separately frompicker's internal query. Ifpicker.close()clears the query inside the hook,inputValuemay drift. Either exposepicker.queryas a readable state and dropinputValue, or document why both are needed.Non-null assertion on
doc.idIf the OpenAPI-generated type already makes
idnon-optional (as it should with@Schema(requiredMode = REQUIRED)on the backend), drop the!. If it's still optional in the generated types, the assertion is hiding a type gap — fix the schema annotation instead.What's solid ✅
append_returns409_when_document_already_in_journey,list_passes_documentId_to_repository_as_journey_item_filter) are proper red/green tests withassertThatThrownByandverify. No implementation exists before the test in any added case.createTypeaheadrefactor ofDocumentMultiSelectis clean — removes rawsetTimeoutdebounce,loadingstate, and manualresultsarray in favour of the hook. 40+ lines deleted.{#each items as item, i (item.id)}— keyed iteration throughout ✅JourneyEditorfollows the established project pattern correctly.JourneyItemRow:handleNoteRemovesavesprevDraftandprevShowNotebefore mutation and correctly restores both on failure.JOURNEY_DOCUMENT_ALREADY_ADDEDguard inJourneyItemService.append()runs before the document lookup — correct ordering (avoids a redundant DB read on the happy path).$derivedused correctly throughout:isInterlude,itemTitle,needsConfirmOnRemove,alreadyAddedIds,canPublish. No$effectfor derived values.🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
Ran the documentation currency check per my review checklist:
CLAUDE.mdpackage table update, no C4 L3 backend diagraml3-frontend-3c-people-stories.pumlupdatedErrorCodevaluesCLAUDE.md+docs/ARCHITECTURE.mddocs/GLOSSARY.mddocs/specs/lesereisen-editor-spec.html<del>annotationsgeschichte/domainAll required documentation updates are present. No blocker from my perspective.
Architecture observations (non-blocking)
The
documentIdfilter onGET /api/geschichtenis undocumented scopeThis is real infrastructure: a new filter capability allowing callers to ask "which journeys contain this document?" The PR description doesn't mention it; the JourneyEditor itself doesn't use it (it computes
alreadyAddedIdsfrom localitemsstate). This looks like forward infrastructure for a future "document detail → shows containing journeys" view.I'm not blocking on this — the implementation is correct and the JPQL EXISTS subquery is within the Geschichte domain boundary (JourneyItem is a Geschichte sub-entity, not a cross-domain access). But it should be tracked in an issue so it doesn't become orphaned infrastructure.
Layering is clean:
JourneyEditornever calls repositories directly; it goes through/api/geschichten/{id}/itemsendpoints which delegate throughJourneyItemService. The separation of concerns betweenGeschichteController(type-level metadata) andJourneyItemController(item mutations) is maintained.The
DocumentPickerDropdownfetch pattern: Calling/api/documents/searchdirectly from the browser is consistent with the existingDocumentMultiSelectcomponent. For interactive typeahead, SSR server-load is impractical; this is the right call. The endpoint requires auth via session cookie and is read-only.No new infrastructure, no new packages — reusing
createBlockDragDropfrom the transcription editor for drag-drop is architecturally sound. It avoids pulling in@dnd-kitorsvelte-dnd-action(both external deps that would require version pinning, security auditing, and Renovate tracking).🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Full OWASP Top 10 pass on the diff. Findings below.
Checked ✅ — no issues
1. All write mutations use
csrfFetchEvery state-changing operation in
JourneyEditor.sveltegoes throughcsrfFetch:handleAddDocument→csrfFetch(POST /items)✅handleAddInterlude→csrfFetch(POST /items)✅handleRemove→csrfFetch(DELETE /items/${id})✅handleNotePatch→csrfFetch(PATCH /items/${id})✅handleReorder→csrfFetch(PUT /items/reorder)✅csrfFetch(PUT /api/geschichten/${id})✅2. JPQL parameterization — injection-proof
Named parameter, UUID type. No string concatenation. ✅
3. Structured error codes — no implementation details leaked
The error message goes to server logs; the frontend sees only the
codeenum value, mapped to a user-friendly i18n string viagetErrorMessage(). ✅4. New
ErrorCodevalues inerrors.tsare properly mapped ingetErrorMessage()JOURNEY_ITEM_NOT_IN_JOURNEY,JOURNEY_NOTE_TOO_LONG,JOURNEY_DOCUMENT_ALREADY_ADDED,GESCHICHTE_TYPE_IMMUTABLE— all four havecasehandlers and i18n translations in all three locales. ✅5. Document search uses authenticated session
DocumentPickerDropdownandDocumentMultiSelectcallfetch('/api/documents/search?...')without an explicit auth header — relying on the session cookie forwarded automatically by the browser. This is consistent with the existingDocumentMultiSelectpattern and acceptable for read-only typeahead. The endpoint requiresREAD_ALLon the server side. ✅Observation (not a vulnerability, robustness smell)
Non-null assertion
doc.id!inhandleSelectIf the OpenAPI-generated
DocumentOption.idis typed asstring | undefined(because the backend@Schema(requiredMode)annotation is missing or thegenerate:apiscript wasn't run after adding it), this assertion silently skips the duplicate check for documents without an id. This is not a security issue (the backend enforces the dedup guard), but it could mask a type generation gap. Suggest: verifyidisstring(notstring | undefined) in the generated types, or use a defensiveif (!doc.id) return;that fails loudly.Attack surface: the
documentIdfilter onGET /api/geschichtenConfirming this is safe: the endpoint requires
READ_ALLauthentication. ThedocumentIdparameter reveals only "which journeys contain this document" — information the authenticated user can derive by reading the journeys themselves. No privilege escalation or data leakage. ✅🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Blocker
Missing integration test for
documentIdfilter inGeschichteListProjectionTestThe PR adds a
documentIdfilter toGeschichteRepository.findSummaries(). The existing projection test addsnullas the new parameter to seven existing test calls — but introduces no test case that verifies the filter actually works.Concretely: the JPQL EXISTS subquery
has zero coverage in the integration test suite. A query bug (e.g., wrong join condition, wrong entity reference) would pass all tests and surface only at runtime.
Required test case (Testcontainers, real Postgres):
The
GeschichteServiceTest.list_passes_documentId_to_repository_as_journey_item_filterverifies the param is threaded through the service layer correctly — but that's a unit test with a mock repo. It says nothing about whether the JPQL is correct.Suggestions
Raw
setTimeouttiming in component testsThese appear in 6+ places. 350ms for the debounce wait is the most fragile — under CI load (especially on the shared runner), 350ms may not be enough for the debounce + fetch mock + DOM update chain. At minimum, add inline comments explaining the intent. Better: factor into a named constant
const DEBOUNCE_WAIT = 400with a buffer above the 300ms debounce.GeschichteListProjectionTest— 7 mechanicalnulladditions with no new behaviourThe pattern of adding
, null)to seven existingfindSummaries()calls without adding a covering test for the new param is a red flag. Tests exist to prove the new behaviour works. The param pass-through is tested at the service unit-test layer; the query correctness is not.What's solid ✅
Backend TDD evidence is strong:
append_returns409_when_document_already_in_journey— properassertThatThrownBy+DomainException.getCode()assertion ✅list_passes_documentId_to_repository_as_journey_item_filter— useseq(documentId)verify ✅GeschichteControllerTestupdated with new param stubs ✅GeschichteServiceIntegrationTestupdated ✅Frontend component tests are well-structured:
JourneyEditor.svelte.spec.ts(392 lines): 10 describe blocks covering empty state, item render order, publish gate, add document, add interlude, remove rollback, reorder via move buttons, live announce, note patch body, duplicate aria-disabledJourneyItemRow.svelte.spec.ts(146 lines): covers note open, note patch, error state, note remove error with rollback, interlude rules (no remove link), interlude empty-text block, remove confirm, cancel restoresmakeGeschichte,defaultProps,docItem,interludeItem) ✅getByRole,getByText,toBeDisabled) ✅SearchFilterBar.svelte.spec.tsfix — waiting fortoBeVisible()before interacting with the undated toggle is the correct fix for the race with the slide transition. ✅🎨 Leonie Voss — UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Blocker
WCAG AA contrast failure on interlude label text
Gray-500 (#6b7280) on warm-white (#f5f4f0) produces approximately 4.0:1 contrast ratio. WCAG 2.1 AA requires 4.5:1 for normal text. The ZWISCHENTEXT label uses
text-xs(12px rendered), which is treated as normal text under the standard — the 3:1 large-text exception does not apply below 18px regular or 14px bold.Fix: Replace
#6b7280with#4b5563(gray-600, ~6.9:1 on white), or use the project's existing semantic token--color-ink-2which is already WCAG-verified for this background.The dark mode token is fine:
#8b97a5 on #151c22 is approximately 6.3:1 — passes AA and is close to AAA. ✅
High concern (not a hard blocker, but affects senior audience)
Confirm/Cancel buttons in inline remove confirmation are undersized
py-1gives approximately 8px top+bottom padding. Attext-xs(12px line height) that's ~28px total height — well below the 44px WCAG 2.2 minimum for the 60+ audience. The main row buttons correctly usemin-h-[44px], but the confirm row doesn't inherit this.Fix: Add
min-h-[44px] px-3 py-2ormin-h-[44px]to both confirm/cancel buttons, or usemin-h-[36px]as a pragmatic minimum for inline inline confirmations.Suggestions
Drag handle uses Braille pattern ⠿ as visual affordance
Screen readers handle this correctly via
aria-label✅. But sighted users unfamiliar with this affordance (especially seniors) may not recognise it as a drag handle. Consider a 6-dot gripper SVG (⋮⋮or a proper 2×3 dot grid) which is a widely recognised drag indicator pattern. Minor cosmetic issue, not a blocker.listboxId = 'doc-picker-listbox'inDocumentPickerDropdown— duplicate ID riskIf two
DocumentPickerDropdowninstances were mounted simultaneously, thearia-controlswould point to the wrong listbox, breaking keyboard navigation for screen readers. Generate a unique ID per instance.What's working well ✅
min-h-[44px] min-w-[44px]), drag handle (min-height: 44px; min-width: 44pxinline), add bar buttons (h-11= 44px) ✅--color-interlude-bg/border/label) defined for both light and dark mode with the explicit sync comment ✅aria-live="polite"region inJourneyEditorfor move announcements — screen readers will announce position changes without interrupting user flow ✅aria-labelon all icon-only buttons — drag handle, move up, move down, remove — every interactive element is identifiable ✅focus-visible:ring-2 focus-visible:ring-focus-ringon every focusable element throughout all new components ✅<details>mobile collapsibles inGeschichteSidebar— native progressive disclosure, no JavaScript dependency, accessible by default ✅aria-disabled="true"on already-added documents in the picker — screen readers can announce the unavailability of an option without removing it from the list ✅🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR. Checking through my review list:
package.jsonpom.xmlStrong call: reusing
createBlockDragDrop<JourneyItemView>from the transcription editor instead of introducing@dnd-kit/coreorsvelte-dnd-actionkeeps the dependency tree clean. Both of those libraries are >40KB minified, would need Renovate rules, security audits, and version pinning. Zero packages added = zero packages to maintain.The frontend build is otherwise unchanged — no new chunk splits, no new static assets beyond the CSS tokens and i18n strings, which are already included in the main bundle.
Nothing to flag from the ops side. LGTM.
📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ⚠️ Approved with concerns
Operating in Brownfield mode, reviewing against the stated requirements and tracing scope back to the issue.
Concern (non-blocking but important for traceability)
documentIdfilter onGET /api/geschichtenhas no issue or user storyThe PR adds a new filter parameter to the Geschichte list endpoint:
This is a real capability addition — backend controller, service, repository JPQL, and test coverage. But:
lesereisen-editor-spec.html) does not mention it.alreadyAddedIdsis derived from the journey's localitemsstate, not via a filter API call.This looks like forward infrastructure, possibly for a "document detail shows containing journeys" feature. That's a legitimate feature idea, but undocumented scope added inside a feature branch creates orphan capabilities: nobody knows it exists, nobody tests it end-to-end, and it doesn't close any issue.
Recommendation: Create a follow-up issue (
feat: filter GET /api/geschichten by documentId) explaining the intended user-facing use case, and link it from the PR description. Alternatively, if this was added speculatively and has no concrete user story yet, remove it and add it when the consuming UI feature is actually built — following the "pessimistic by default" principle stated in the PR description for adds.Tracked items ✅
maxlength="2000"per spec pending resolution. Correct handling of a discovered constraint ambiguity. ✅lesereisen-editor-spec.htmluses<del>to mark superseded spec values and<code>to record the actual implementation. This is an excellent living-spec pattern. ✅ErrorCodevalues implemented with i18n in all 3 locales,errors.tsupdated,getErrorMessage()updated, CLAUDE.md updated. The full 4-step checklist from CLAUDE.md was followed. ✅Acceptance criteria check
From the spec and PR description:
renders title input and intro textareapublish button disabled when no itemsitem-add enables publish buttonpublish stays disabled until title is non-emptycalls POST with documentId when document selectedcalls POST with note on interlude confirmrestores item on failed DELETEmove-up calls PUT reorder,move-down calls PUT reorderappend_returns409_when_document_already_in_journeyalready-added document appears as aria-disabled in pickersends {"note":null} when note textarea is cleareddoes not show "Notiz entfernen" for interlude itemsblocks saving empty text on interlude note blurCoverage is thorough. The one gap is the missing
GeschichteListProjectionTestcase for the newdocumentIdfilter (raised by Sara).role=listbox + role=option without arrow-key navigation is misleading — the WAI-ARIA combobox pattern requires aria-activedescendant handling that isn't implemented. Downgraded to plain <ul>/<li>; input keeps role=combobox + aria-controls pointing to the list id. listboxId was a module-level constant so two simultaneous instances would share the same DOM id. Fixed with a <script module> counter. Updated spec queries from getByRole('option') to getByText() — tests behaviour, not the ARIA implementation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>All reviewer concerns addressed. Here's a summary of every change made:
Medium — resolved
items = prevno-op (0130c4fa): Added comment to bothhandleAddDocumentandhandleAddInterludecatch blocks explaining thatprev === itemshere (add is pessimistic) and the assignment is kept for symmetry with optimistic handlers.listbox without arrow-key nav (
d35a1658): Downgradedrole="listbox"/role="option"to plain<ul>/<li>inDocumentPickerDropdown. The<input>retainsrole="combobox"+aria-controls. Also fixed duplicatelistboxIdby adding a<script module>counter (let _uid = 0).JPQL integration test (
8adc39e2): Added two tests toGeschichteListProjectionTest— one asserting the EXISTS subquery returns only the journey containing the target document, one asserting an unknowndocumentIdreturns empty.Minor — resolved
DocumentOptionduplication (7df64020): Extracted sharedDocumentOptiontype,createDocumentTypeahead()factory, andformatDocumentOption()helper intofrontend/src/lib/document/documentTypeahead.ts. Replaced all four local copies inDocumentPickerDropdown,DocumentMultiSelect,JourneyAddBar, andJourneyEditor.Unused
journey_item_pending_add/removekeys (ba394e11): Deleted both keys frommessages/de.json,en.json, andes.json(confirmed zero usages by grep).announceTimerduplication (0130c4fa): ExtractedscheduleAnnounceReset()helper; replaced identical 4-line clear+setTimeout blocks inhandleMoveUpandhandleMoveDown.Hardcoded "Status" in
GeschichteSidebar(d689a662): Addedgeschichte_sidebar_statuskey to all three message files (de/en/es) and replaced both hardcoded strings (<summary>and<h2>) with{m.geschichte_sidebar_status()}.Also from third-round persona reviews — resolved
WCAG AA contrast on interlude label (
33fe93a5): Raised--c-interlude-labelfrom gray-500 (#6b7280, ≈4.0:1) to gray-600 (#4b5563, ≈6.9:1).44px touch targets on confirm/cancel buttons (
401371fd): Appliedinline-flex min-h-[44px] items-centerto the inline remove confirm/cancel buttons inJourneyItemRow.Follow-up issue created
GET /api/geschichten?documentId=in the OpenAPI spec so it appears in the generated TypeScript types and Swagger UI.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.