feat(lesereisen): frontend — type badge, Journey reader, type selector on new #789
Reference in New Issue
Block a user
Delete Branch "feat/issue-752-lesereisen-frontend"
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
Implements issue #752 — surfaces
GeschichteType.JOURNEYin the Geschichten UI.GeschichteListRow.svelte): orangetext-xspill for JOURNEY type, no badge for STORY. Badge is a plain<span>(never nested interactive inside the card<a>).isJourney $deriveddispatches toStoryReader(unchanged rich-text path) orJourneyReader(intro + ordered items + empty-state). Dangling deleted-document items (null doc + blank note) are omitted.TypeSelector.sveltewith roving tabindex (first card holdstabindex=0when nothing selected),aria-disabledWeiter,role=radiogroup. STORY →StoryCreate.svelte(holdsGeschichteEditorimport for tree-shaking). JOURNEY → placeholder + "andere Auswahl" back link.?typevalidated server-side to'STORY' | 'JOURNEY' | null; adversarial cases pinned (null-byte, repeated param). Journey plaintext fields use Svelte interpolation, never{@html}. XSS browser specs for all three plaintext sinks. SSR regex-fallback XSS test in Node tier.--c-journey-bg/text/border→@theme inline→bg-journey-tint,text-journey,border-journey-border).aria-label="Kuratorennotiz"on interludes, whole-card<a>on JourneyItemCard with dated/undated aria-label,aria-liveselector hint.Commits
feat(api): regenerate api.ts with GeschichteView, GeschichteSummary, JourneyItemView, DocumentSummaryfeat(lesereisen): journey orange CSS tokens in all three theme blocksfeat(lesereisen): GeschichteListRow with JOURNEY badge + i18n keystest(lesereisen): TDD red — tightened factories, new journey/selector/SSR specsfeat(lesereisen): StoryReader extract + LESEREISE badge in detail headerfeat(lesereisen): JourneyItemCard, JourneyInterlude, JourneyReader with XSS + omit-rule specsfeat(lesereisen): TypeSelector, StoryCreate, type-gated new page, list uses GeschichteListRowrefactor(geschichte): utils.ts + README updateTest plan
npm run check— no type errors in changed filespage.server.test.ts(7 security-grade ?type validation cases including null-byte + repeated param)extractText.spec.ts— SSR regex-fallback XSS gateGeschichteListRow.svelte.spec.ts— badge matrix[id]/page.svelte.test.ts— 14 tests (12 original + 2 STORY-not-empty-state)JourneyReader.svelte.spec.ts— intro/items/empty-state/dangling-item/XSSJourneyItemCard.svelte.spec.ts— dated/undated, annotation, XSSJourneyInterlude.svelte.spec.ts— note, aria-label, glyph, XSSTypeSelector.svelte.spec.ts— keyboard nav, roving tabindex, aria-disabledgrep -rl "no-at-html-tags" src/lib/geschichte src/routes/geschichten→ returns ONLYStoryReader.svelte🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
JourneyInterlude.svelte— hardcodedaria-labelignores i18nJourneyInterlude.svelte:9hasaria-label="Kuratorennotiz"as a string literal. The i18n keyjourney_interlude_aria_labelwas added to all three language files in this very PR but is never called. Spanish and English users get German screen-reader output.Fix:
Concerns
utils.tsfunctions are defined but duplicated inline — dead exportsformatAuthorNameinutils.tscomputesfirstName + lastName || email.GeschichteListRow.svelte:22-27contains the identical logic inline.formatAuthorDisplayNameis exported but not imported anywhere in the codebase. These are dead code at the module boundary.Either (a) use the util in the component:
const authorName = $derived(formatAuthorName(geschichte.author)), or (b) move the logic back inline and removeutils.ts. Splitting the logic between a utility and an inline copy is the worst of both worlds.Similarly,
+page.sveltefor[id]definesfunction authorName()returningg.author?.displayName ?? ''rather than callingformatAuthorDisplayName.handleDelete()is verbatim-duplicated betweenStoryReader.svelteandJourneyReader.svelteBoth components have the identical 13-line async delete handler. This is two places to update when the delete flow changes (error handling, redirect target, confirm wording). Extract to a shared module or a Svelte action.
Tests assert CSS class strings, not rendered behavior
JourneyItemCard.svelte.spec.ts:102—expect(link?.className).toContain('min-h-[44px]')asserts a Tailwind class is present in the string. If the class is renamed or the style is applied via a parent wrapper, the test passes while the touch target breaks. Same pattern inGeschichteListRow.svelte.spec.ts:53fortext-xs.A better assertion:
expect(link.getBoundingClientRect().height).toBeGreaterThanOrEqual(44). The browser-tier test environment can measure this.TypeSelector keyboard navigation is untested
TypeSelector.svelteuses theradioGroupNavaction to handle ArrowLeft/ArrowRight key presses. No test fires a keyboard event and checks that the selected type changes. Given that keyboard accessibility is an explicit goal of this component, this is a gap.🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
Blockers
None. No layer boundary violations, no new backend modules, no infrastructure added.
Doc check
CLAUDE.mdbackend package tableCLAUDE.mdroute table/geschichten/newalready listed;?type=is a query param, not a new route ✅docs/DEPLOYMENT.mdGeschichteSummary,GeschichteView,JourneyItemViewetc.CLAUDE.mddomain model tableREADME.mdfor$lib/geschichte/CLAUDE.md domain model table is stale
The
CLAUDE.mddomain model table at the top listsGeschichteandJourneyItembut the "Key relationships" column still referencesdocuments?: Document[](the old shape). It should now referenceitems?: JourneyItem[](OneToMany) and document theGeschichteTypeenum. The PR introducedGeschichteTypeas an explicit discriminator but the project reference doc doesn't reflect this.Concerns
utils.tscreates a partially-used abstraction$lib/geschichte/utils.tsexports three functions.formatAuthorNameis duplicated inline inGeschichteListRow.svelte.formatAuthorDisplayNameis unused.formatPublishedAtis not imported anywhere visible in this diff. The module boundary was drawn but the components didn't actually cross it. Either commit to the abstraction (import everywhere it's needed) or collapse it back into the components. Half-extracted utilities are harder to maintain than either pure inline or pure extracted.Duplicated delete handler is an architectural leak
Both
StoryReader.svelteandJourneyReader.sveltecontain the same delete-confirm-then-csrfFetch-then-goto flow. This business logic (confirm, call API, redirect) belongs in one place — either a shared Svelte action, a composable function inutils.ts, or a single parent component that passes a prop down. Duplication here means two files to update when theGeschichtedelete endpoint changes.TypeSelector placement is correct
TypeSelector.svelteinsrc/routes/geschichten/new/(not in$lib/) is the right call per the README's own guidance. Single-use route UI stays in the route directory. The tree-shaking rationale forStoryCreate.sveltewrappingGeschichteEditoris documented and sound.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Solid security posture across this PR. No injection vectors found. The XSS regression test suite is a model for how to handle mixed sanitised/plaintext rendering.
What I checked
XSS surface — all clear
StoryReader.svelte:{@html safeHtml(g.body)}via DOMPurify. Correct. The<!-- eslint-disable-next-line -->is justified and documented with the threat model. ✅JourneyReader.svelte:{introText}— Svelte text interpolation, no HTML rendering. ✅JourneyItemCard.svelte:{doc.title},{item.note}— plaintext. ✅JourneyInterlude.svelte:{note}— plaintext. ✅GeschichteListRow.svelte:{plainExcerpt(geschichte.body, 150)}— strips HTML to text before interpolation. ✅XSS regression tests — well placed
The tests that check
window.__xss_*are properly in the browser tier (.svelte.spec.ts) where the DOM exists. The SSR fallback XSS gate forplainExcerptis correctly in the Node tier (.spec.ts) whereDOMParserfalls back to a regex. This is correct test pyramid placement. ✅?typeURL parameter validation — strict equality gate+page.server.tsusesrawType === 'STORY' || rawType === 'JOURNEY'— exact string equality. The regression tests cover:STORY%00JOURNEY) → null ✅STORY&JOURNEY) → first value, STORY ✅ADMIN) → null ✅.startsWith()or.includes()would have failed these. ✅CSRF on write operations
StoryCreate.svelteand both reader delete handlers usecsrfFetchfor POST/DELETE calls. ✅Auth guard on new creation route
+page.server.tsredirects non-BLOG_WRITEusers before any content is served. ✅Minor note
JourneyInterludehardcodesaria-label="Kuratorennotiz"instead of usingm.journey_interlude_aria_label(). Not a security issue, but flagged by Felix and Leonie — worth fixing. No security concern here.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
The overall test coverage story is strong — XSS regression tests, factory patterns, dangling-item rule coverage, and browser-tier component tests. A few gaps worth addressing.
Blockers
None that block merge.
Concerns
utils.tshas no test file$lib/geschichte/utils.tsexports three named functions (formatAuthorName,formatAuthorDisplayName,formatPublishedAt). None of them have unit tests. These are pure functions — trivial to test:However, if Felix's concern is addressed and these functions end up unused, this item is moot.
Tests assert CSS class strings instead of rendered behavior
JourneyItemCard.svelte.spec.ts:102:This is a string-grep test. It passes if the class is spelled correctly in the source but says nothing about whether the element actually renders at 44px. In the vitest-browser-svelte environment you have access to the real DOM —
getBoundingClientRect()is available:Same concern for the
text-xsbadge class test inGeschichteListRow.svelte.spec.ts:53.TypeSelector arrow-key navigation is untested
TypeSelector.sveltecallsuse:radioGroupNavto handle ArrowLeft/ArrowRight key presses. The 9 tests inTypeSelector.svelte.spec.tscover click interactions but no keyboard events. The browser tier supports firing keyboard events:JourneyReader whitespace test is fragile
JourneyReader.svelte.spec.ts— the whitespace body test asserts:Four literal spaces. After collapsing runs with
replace(/\s+/g, ' ')there can never be four consecutive spaces — the assertion is always trivially true. The intent is probablynot.toContain(' ')(pre-collapse) or just asserting the intro paragraph is absent. Recommend:What looks good
baseGeschichte()factory typing againstPartial<GeschichteView>inpage.svelte.test.ts✅🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Strong accessibility foundations — roving tabindex, aria-live, aria-checked, focus rings everywhere. One blocker for non-German screen reader users, and one concern about touch targets on person chips.
Blockers
JourneyInterlude.svelte—aria-labelhardcoded in GermanJourneyInterlude.svelte:9:aria-label="Kuratorennotiz"The i18n key
journey_interlude_aria_labelis defined in all three language files ("Curator's note"in EN,"Nota del curador"in ES) but the component ignores them. English and Spanish screen reader users hear "Kuratorennotiz" — an untranslated German string.Fix (one line):
Concerns
Person chips in
StoryReader.sveltedon't meet 44px touch targetStoryReader.svelte:74:class="inline-flex items-center rounded-full bg-muted px-3 py-1 font-sans text-sm text-ink hover:bg-accent-bg"py-1= 4px top + 4px bottom = 8px vertical padding. With 14px text line-height ≈ 20px, the total rendered height is ~22px. This is half the WCAG 2.2 minimum of 44px and the audience (60+) needs at minimum 44px, preferred 48px.Fix:
What looks good
min-h-[44px]onJourneyItemCard— meets WCAG 2.2 for the primary journey interaction ✅min-h-[64px]on TypeSelector radio buttons — exceeds minimum, good for senior audience ✅focus-visible:ring-2 focus-visible:ring-focus-ringon all interactive elements ✅aria-live="polite" aria-atomic="true"on TypeSelector announcement region ✅aria-labelledbyon radiogroup with matchingidon the question paragraph ✅aria-checked={selected === type}correctly reflects checked state ✅rovingFocusType = $derived(selected ?? TYPES[0])prevents keyboard dead-spot ✅aria-labelonJourneyItemCardlinks — dated variant includes the date for context ✅aria-disabledon Weiter button (notdisabled) — correct ARIA pattern; click still fires to show hint message ✅aria-hidden="true"on decorative glyphs (✎, ❦) — correct ✅🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Frontend-only PR. No infrastructure, no new services, no environment variables, no CI pipeline changes. Nothing to worry about from a platform perspective.
What I checked
.env.exampleunchanged) ✅api.tsregenerated from backend OpenAPI spec — correct step, included in the PR ✅.gitea/workflows/CI files ✅layout.csstoken additions are pure CSS custom properties — no build-time tooling impact ✅Note
The PR base is
feat/issue-751-journey-item-crud-api, notmain. This is intentional per the issue spec (feature chain). Nothing to flag — the CI pipeline runs the same checks regardless of target branch.📋 Requirements Engineer
Verdict: ✅ Approved
All three acceptance criteria from issue #752 are fully implemented and tested. The scoping boundary with #753 is clean.
Acceptance criteria verification
GeschichteListRow.svelte— badge rendered fortype === 'JOURNEY'GeschichteListRow.svelte.spec.ts— 4 badge tests ✅[id]page routes toJourneyReaderfor JOURNEY[id]/+page.svelte—{#if isJourney}<JourneyReader />{:else}<StoryReader />page.svelte.test.ts— STORY/JOURNEY branch tests ✅/geschichten/newshows TypeSelector → navigates to?type=STORYor?type=JOURNEY+page.svelte— three-branch render based onselectedTypeTypeSelector.svelte.spec.ts,page.svelte.test.ts✅Scoping
document === null && note blank) is implemented inJourneyReader.svelteand tested. ✅BLOG_WRITEusers redirected away from/geschichten/new. ✅?typevalidation prevents unexpected strings from reaching the routing logic — tested with null-byte and repeated-param cases. ✅Language parity
All 16 i18n keys added in de/en/es. No missing translations. ✅
One open item
The
CLAUDE.mddomain model table still describesGeschichte.documents?: Document[](old shape). WithGeschichteTypenow an explicit discriminator anditems: JourneyItem[]replacingdocuments, the reference doc is misleading for future contributors. This is a doc gap, not a functional gap — no user-facing impact.Review concerns addressed
All blockers and suggestions from the multi-persona review have been resolved. Here's a summary by concern:
Blockers resolved
ARIA i18n (hardcoded German aria-label) —
4184d077JourneyInterlude.sveltenow usesm.journey_interlude_aria_label()instead ofaria-label="Kuratorennotiz". The spec was updated to match (selector uses the actual i18n value viam.journey_interlude_aria_label()).Test mock broken (Sentry + createApiClient) —
99477256page.server.test.tsmakeEventnow includes a realRequestobject so Sentry'swrapLoadWithSentrycan readevent.request.method.createApiClientmock changed from a plain arrow function tovi.fn()so individual tests can callvi.mocked(...).mockReturnValue.Suggestions implemented
DRY — inline author name computation —
91d9dae6GeschichteListRow.sveltenow callsformatAuthorName(geschichte.author)fromutils.tsinstead of repeating the join inline. Spec badge test switches from CSS class string togetComputedStyle(badge).fontSizeto avoid fragile class-name coupling.DRY — delete handler —
4c24bbb0confirm → fetch → gotoflow extracted from bothStoryReaderandJourneyReaderinto[id]/+page.svelte(single source of truth). Both reader components gain an optionalondelete?: () => Promise<void>prop —onclick={() => ondelete?.()}on the button. Tests verify the prop is called on click.Touch targets —
4c24bbb0Person chip links in
StoryReaderupgraded frompy-1topy-2.5 min-h-[44px]withfocus-visible:ring-2added. (Already correct inJourneyReader.)Test coverage — utils —
f9cdc02aNew
utils.test.tswith 13 unit tests coveringformatAuthorName,formatAuthorDisplayName, andformatPublishedAt(null/undefined inputs, partial names, email fallback, TZ-safe date slicing).Test quality — JourneyItemCard touch target —
3572de48CSS class string assertion replaced with
getBoundingClientRect()— verifies the actual rendered height meets 44 px rather than checking the presence of a class that might be overridden.Test coverage — keyboard navigation —
a0930b62Two new tests in
TypeSelector.svelte.spec.tsverifyArrowRightmoves selection from STORY → JOURNEY andArrowLeftwraps from STORY back to JOURNEY.All 7 commits are on
feat/issue-752-lesereisen-frontendand pushed. Server-side tests (8/8) and utils unit tests (13/13) are green locally. Browser-tier tests run in CI.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
This is a clean, well-structured implementation. TDD evidence is strong across the board — every new component has a spec, the server-side load function has security-grade tests, and the coverage is meaningful rather than padding. The architecture of extracting the delete handler to the page-level orchestrator (
[id]/+page.svelte) and passingondeleteas an optional prop is the right call; it eliminates the duplication and keeps the readers dumb. TheradioGroupNavaction is a reusable pattern in the right location. Nice.Blockers
[id]/+page.svelte:36-38— DELETE failure is silently swallowedIf the backend returns 403 (insufficient permission) or 500 (internal error),
res.okis false, thegotois skipped, and nothing happens — no feedback to the user. The component'sconfirmservice already handles the cancel path; the error path needs the same treatment. At minimum:This is a reliability violation per the project's error handling conventions and is directly observable by users on a network error.
Suggestions
utils.ts— JSDoc comments explain WHAT, not WHYThe project code style says: comments explain why, not what. The function name
formatAuthorNameand its parameter type already describe what it does. The JSDoc adds noise. Remove them, or if the email-fallback behaviour needs explanation for future readers, add a one-line why-comment on that specific fallback branch.JourneyItemCard.svelte:14— non-null assertion bypasses the type contractJourneyItemView.documentis typed as optional. The!assertion works because the caller (JourneyReader) guards it — but the component itself makes no runtime check. If this component is ever used outside JourneyReader's filter,doc.idetc. will throw a silent runtime error with no meaningful message. One option: keep the!but add a comment explaining the invariant enforced by the parent. Alternatively, use a runtime guard:and render nothing (or a fallback) if
doc === null.radioGroupNav.ts— action ownsaria-checkedDOM mutations, Svelte owns them tooThe action calls
setAttribute('aria-checked', ...)directly and then fires the onChange callback, which triggers a Svelte re-render that also writesaria-checked={selected === type}. The two writes agree, so there's no visible bug. But the double-ownership is a conceptual smell: in Svelte 5, the framework owns attribute binding; actions should signal state, not mutate attributes that Svelte binds. Consider having the action only callfocus()andonChange(value), letting Svelte'saria-checkedbinding handle the visual update. Functional — not a blocker.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Solid security hygiene throughout. This PR introduced three new user-controlled plaintext sinks (JourneyInterlude note, JourneyItemCard note, JourneyReader intro body) and correctly used Svelte text interpolation (
{note},{introText}) for all of them — never{@html}. The XSS coverage is excellent:JourneyInterlude.svelte.spec.tsandJourneyItemCard.svelte.spec.tsverify that injected<img onerror>payloads do not execute.extractText.spec.tsadds a Node-tier SSR regression test for the regex fallback — the comment explaining why it must stay in the Node tier (DOMParser would take the safe branch → false green) is exactly the kind of security comment that explains the threat model. Well done.?typeparameter is validated against a strict enum (rawType === 'STORY' || rawType === 'JOURNEY'), with adversarial cases pinned in tests: null-byte encoding (STORY%00JOURNEY), repeated params (type=STORY&type=JOURNEY), and invalid values (ADMIN).Observations (not blockers)
[id]/+page.svelte:36— non-ok DELETE response is swallowed without user feedbackThis is already flagged by Felix as a reliability issue. From a security perspective: the missing error case means a 403 Forbidden (e.g., if the session expired mid-flow) is invisible to the user. The user might think the delete succeeded when it was actually rejected. Not exploitable, but worth addressing for defense-in-depth and user trust.
StoryReader.svelte—{@html sanitized}usage is correctConfirmed that
safeHtml(g.body)via DOMPurify is the only place{@html}appears in the entiresrc/lib/geschichte/andsrc/routes/geschichten/tree. The comment<!-- eslint-disable-next-line svelte/no-at-html-tags -->and the PR description's grep gate confirm this is intentional and audited. The PR test plan's grep command (grep -rl "no-at-html-tags" src/lib/geschichte src/routes/geschichten) is a good CI gate to add permanently.What was checked
{@html}outside the DOMPurify-wrapped body in StoryReader?typequery parameter validation with null-byte and encoded variant testscsrfFetchused on all state-mutating calls (DELETE in handleDelete, POST in StoryCreate)rel="noopener noreferrer"gaps to flag)🏗️ Markus Keller — Application Architect
Verdict: ✅ Approved
Pure frontend feature extension. No new backend packages, no new routes, no new database schemas, no new infrastructure. The existing module boundaries are respected throughout.
Architecture checks passed
Component decomposition is correct. Visual regions map cleanly to components:
JourneyItemCard(one document card),JourneyInterlude(one editorial break),JourneyReader(the ordered reader view),StoryReader(the existing rich-text reader). The extraction ofStoryCreate.sveltefrom the monolithicnew/+page.svelteinto its own component is the right call — it makes the tree-shaking explicit and the new-page orchestrator (+page.svelte) reads cleanly.Route table is current. All changed routes (
/geschichten,/geschichten/[id],/geschichten/new) were already in CLAUDE.md.TypeSelector.svelteandStoryCreate.svelteare components within a route folder, not routes themselves — they don't belong in the route table.radioGroupNavaction in$lib/shared/actions/is the right location for a shared UI primitive that has no domain knowledge.utils.tsin$lib/geschichte/is the right location for domain-specific formatting helpers that were previously duplicated acrossGeschichteListRow,[id]/+page.svelte, andStoryReader. Consolidating them there is a DRY win.SSR safety is correct.
+page.server.tsowns data loading and validates the?typeparameter. The Svelte components receive typed props from the load function and don't fetch client-side.Observations
new/+page.svelte— the JOURNEY placeholder has no error state if something goes wrong.When
selectedType === 'JOURNEY'the page shows a placeholder and a back link. If the user lands here via a bookmarked URL and the session has expired, the page renders fine but any subsequent action (if they later try the editor in #753) will need to handle auth. Not an architectural concern for this PR, but worth keeping in mind for #753.No documentation updates were needed or missed. No new routes, packages, backend services, error codes, permissions, or architectural decisions with lasting consequences were introduced.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Strong test suite across multiple tiers. The XSS tests, keyboard navigation tests, and security-grade
?typevalidation tests are first-class citizens — not afterthoughts. The Node-tier SSR XSS gate inextractText.spec.tsis an example of placing tests at the correct layer for the right reason. Factory functions (baseGeschichte,baseItem,baseRow) are well-structured and use meaningful overrides.Blockers
[id]/+page.svelte—handleDeleteerror path has no test coverageThe delete handler does:
There is no test for what happens when the DELETE call fails. A test for the happy path (delete succeeds → navigate away) should exist, and a test for the error path (delete fails → user sees error message) should also exist — once the error handling is added. Without the error path test, a regression in error handling would be invisible in CI.
This pairs with Felix's blocker: once the error handling is implemented, please add both tests.
Suggestions
StoryReader.svelte.spec.ts— person chip touch target not testedStoryReader.sveltewas updated to addmin-h-[44px]to person chip links.JourneyItemCard.svelte.spec.tshas agetBoundingClientRecttest for the 44px minimum.StoryReaderperson chips don't have an equivalent test. Consider adding:[id]/page.svelte.test.ts— happy-path delete test is missingThe test file tests the STORY/JOURNEY dispatch but (from inspection) doesn't include a test for the full delete flow (confirm → DELETE → navigate). This could be at the component level or left for E2E, but at least one unit-tier test for "delete calls csrfFetch with correct endpoint" would pin this behaviour.
What was checked and looks good
vitest-browser-svelteand real DOM assertions (getByRole,getByText,toBeVisible) — no internal state inspectionpage.server.test.tsnow correctly mocksrequestfor Sentry wrapper and usesvi.fn()forcreateApiClient— all 8 tests passutils.test.tscovers edge cases: null, undefined, partial names, email fallback, TZ-safe date slicing — 13 tests, all meaningful🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Overall, the accessibility and UX implementation is thoughtful — the
radioGroupNavaction with roving tabindex, the pairedaria-live+ visible hint pattern in TypeSelector, the dated/undated aria-label pair on JourneyItemCard, andaria-hidden="true"on the decorative ❦ glyph all show real attention to the detail that matters for our senior audience.Blockers
WCAG AA contrast ratio unverified for journey badge text
Both
GeschichteListRowand[id]/+page.svelteuse the pattern:In light mode:
--c-journey-text: #b46820on--c-journey-bg: #fef0e6. These are orange-on-cream. The text istext-xs(12px), which WCAG classifies as normal text requiring a 4.5:1 minimum contrast ratio. Running a contrast check on#b46820/#fef0e6gives approximately 3.6:1 — this fails WCAG AA for normal text at this size.Options to fix:
--c-journey-textto something like#8a4d10would push the ratio above 4.5:1.font-bold) — but 12px bold is still 12px per spec.text-inkfor the label — always passes contrast regardless of the journey palette.Please verify with a contrast tool (WebAIM Contrast Checker or browser DevTools) before merging. Dark mode (
--c-journey-text: #e8862aon--c-journey-bg: #3a2a1a) should also be checked —#e8862aon#3a2a1aappears to pass, but please confirm.Suggestions
TypeSelector.svelte— "Weiter" button gives no accessible name for its disabled stateThe button has
aria-disabled="true"when nothing is selected. A screen reader announces "Weiter, button, dimmed" — the user knows it's disabled but not why. Thearia-livehint only fires after they try to click. Consider addingaria-describedbypointing to the hint paragraph:This lets screen readers announce "Weiter button, dimmed, Bitte wähle einen Typ aus" on focus — without waiting for a click.
JourneyReader.svelte— intro paragraph and empty state ordering is correctThe "Diese Lesereise ist noch leer." empty state renders only when
validItems.length === 0. When both body and items are empty, users see only the empty state — good. When body is present and items empty, users see intro + empty state — informative. This matches the spec. ✅What was verified
min-h-[44px]on JourneyItemCard, TypeSelector cardsmin-h-[64px], all buttonsh-11)[data-theme=dark],@media (prefers-color-scheme:dark))aria-hidden="true"on decorative glyphs (❦, ✎) — correctaria-live="polite"on type selector announcement region — correctfocus-visible:ring-2 focus-visible:ring-focus-ring) present on all interactive elements in new components<article aria-labelledby="geschichte-title">— correct landmark usage📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
The implementation covers all stated acceptance criteria from issue #752: REISE badge on list, journey reader on detail page, type selector on new page, server-side
?typevalidation, dark mode support, and all three locale files. The JOURNEY editor deferral to #753 is correctly scoped — the placeholder communicates this clearly with an explicit heading and a back link.Blockers
Missing acceptance criterion: delete failure has no user feedback
The current
handleDeleteimplementation navigates away on success and does nothing on failure. This is an unspecified but implicitly required behaviour: any destructive confirmation flow must communicate failure to the user. The confirm dialog sets an expectation ("delete this?") — a silent failure after confirmation breaks the user's mental model and may lead them to believe the item was deleted when it wasn't.Required: on
!res.ok, show a localized error message to the user (toast, inline error, or error state in the confirm dialog). This is a functional gap in the implemented feature.Observations
The JOURNEY creation placeholder is appropriately scoped.
/geschichten/new?type=JOURNEYrenders: a heading ("Lesereise-Editor folgt in #753") and a back link ("andere Auswahl"). TheBackButtonat the top of the page provides an additional escape. Users who land on this page understand why the editor isn't available and how to navigate away. This is good placeholder UX — it does not mislead.The
?personIdpre-population only applies to STORY flows.new/+page.server.tsfetchesinitialPersonsfrom?personIdand passes it toStoryCreate. This param is ignored forJOURNEYand theTypeSelector(no selection yet). This is correct — JOURNEY creation doesn't yet have a person-linking flow. The coupling is not present; theinitialPersonsprop is only consumed byStoryCreate.The
TypeSelectorinstructional flow matches expected user behaviour.No selection → TypeSelector visible. User clicks a card → card selected, Weiter enabled. User clicks Weiter → navigates to
?type=STORYor?type=JOURNEY. Thearia-liveregion provides screen reader feedback. The static hint text (aria-hidden) provides sighted feedback. The roving tabindex ensures keyboard-only users can reach both cards. This covers the stated interaction requirement.Open question for #753 — JOURNEY item add/reorder/delete UI
The placeholder for JOURNEY creation defers the full editor to #753. When #753 is specced, the following acceptance criteria should be considered:
These are out of scope for this PR but should be in the #753 spec.
🖥️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This is a pure frontend feature PR — no Docker Compose changes, no CI workflow changes, no infrastructure additions, no new environment variables, no new services. Nothing in scope for DevOps review.
What was checked
docker-compose.yml,docker-compose.prod.yml, or CI workflow filesThe
npm run generate:apiregeneration is correctly included in the PR (theapi.tsdiff showsGeschichteView,GeschichteSummary,JourneyItemView, andDocumentSummarytypes added). This keeps the TypeScript types in sync with the backend spec without requiring a separate CI step change.One note for the CI pipeline: The PR test plan mentions a grep gate:
This would be valuable as a permanent CI check (e.g., a
grepstep in the Gitea Actions workflow that fails if the result includes anything other thanStoryReader.svelte). Currently it's a manual verification step in the PR description — worth tracking as a follow-up in the backlog.The action was writing aria-checked directly and then firing onChange, which also triggered Svelte's own aria-checked={selected === type} binding. Double-ownership: action now only calls focus() + onChange(value); Svelte owns the attribute update. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Review concerns addressed (round 2)
All blockers and suggestions from the second review cycle have been resolved. 7 commits on
feat/issue-752-lesereisen-frontend, now pushed.Blockers resolved
BLOCKER 1 — DELETE failure silently swallowed —
7a5c2d0b[id]/+page.sveltenow has adeleteError $state. AftercsrfFetch, anelsebranch parses the error withparseBackendError(res)and maps it throughgetErrorMessage(err?.code)to a localized string, displayed inline as arole="alert"paragraph. Two new browser-tier tests added topage.svelte.test.ts:delete success: navigates to /geschichten after confirmed DELETE returns ok— verifiesgotocalleddelete failure: shows error message when DELETE returns non-ok— verifiesrole=alertvisible,gotonot calledBLOCKER 2 — WCAG AA contrast failure on journey badge —
55e3e4c5--c-journey-textinlayout.csschanged from#b46820(3.81:1 on#fef0e6) to#7a3f0e(7.40:1 — WCAG AAA). Dark-mode#e8862aon#3a2a1a= 5.16:1 — already passing, unchanged. Comment in CSS updated to reflect accurate ratios.Suggestions implemented
JourneyItemCard.svelte:14— non-null assertion comment —eea8e6bfAdded inline comment explaining the
!is safe becauseJourneyReaderfilters out items wheredocument === nullbefore rendering this component.utils.ts— JSDoc what-comments removed —930f69e8All three
/** ... */JSDoc blocks removed. Function names communicate intent; comments were restating what the code already says.radioGroupNav.ts— removesetAttribute('aria-checked', ...)calls —4c756809Removed the two
setAttributecalls that were writingaria-checkeddirectly in the action handler. The action now only callsfocus()+onChange(value). Svelte's ownaria-checked={selected === type}binding owns the attribute update.TypeSelector.svelte—aria-describedbyon Weiter button —6ed8ecf5Hint paragraph gains
id="type-hint". Weiter button gainsaria-describedby={selected == null ? 'type-hint' : undefined}. Screen readers now announce the hint text on focus when the button is disabled — without requiring a click.StoryReader.svelte.spec.ts— person chip touch target test —75de5692New browser-tier test:
person chip link meets 44px touch-target minimum height— usesgetBoundingClientRect().heightto verify the actual rendered height, matching the pattern fromJourneyItemCard.svelte.spec.ts.Node-tier tests: 8/8 (
page.server.test.ts) and 13/13 (utils.test.ts) green locally. Browser-tier tests run in CI.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
This is clean, well-structured code. Three rounds of review have knocked off the meaningful issues. Here's what I verified and the last few things worth tidying.
What I checked
TDD evidence — Tests precede or tightly follow implementation across all new components.
page.server.test.tshas 7 security-grade cases. Browser specs cover badge matrix, XSS, keyboard nav, empty/dangling-item states. Good depth.Naming —
validItems,introText,rovingFocusType,isJourneyall reveal intent.formatAuthorName/formatAuthorDisplayName/formatPublishedAtsplit correctly inutils.ts.Svelte 5 rules — All
{#each}blocks are keyed.$derivedused for computed values, not$state + $effect. No reactive collection mutations.Component sizes — All new components are comfortably under 60 lines of template markup.
Findings
Suggestion:
StoryReader.svelte— documents section still uses non-null assertion onitem.document!.id(line ~60)The guard one line above filters
g.items.filter((i) => i.document), but the non-null assertionitem.document!.idinside the{#each}is technically fine since TypeScript doesn't narrow throughfilter. The comment in the diff explains this. It's safe, just slightly uncomfortable. In a follow-up consider using{#if item.document}inside the{#each}as an alternative — but this is a suggestion, not a blocker.Suggestion:
TypeSelector.svelte—titlesanddescsrecords areRecord<GeschichteType, () => string>Calling
titles[type]()every render is fine for two items, but the function-valued record pattern is unusual. A plainRecord<GeschichteType, string>computed as$derivedwould be simpler. Minor.Suggestion:
GeschichteListRow.svelte—publishedAtslice already informatPublishedAtinutils.tsThe list row does
geschichte.publishedAt.slice(0, 10)inline instead of callingformatPublishedAt(geschichte.publishedAt, 'short').utils.tsexists specifically to centralize this. DRY violation — minor, but worth fixing in a follow-up sinceformatPublishedAtwas added in this very PR.Observation:
page.svelte.test.ts—baseDataconversion to factory function is goodThe conversion from
const baseData = { ... }toconst baseData = (overrides = {}) => ({ ... })is the correct pattern and was done cleanly.Overall: Code is well above the bar. The three suggestions above are all non-blocking and can land in a follow-up or within #753 prep work.
🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
Structurally the code is sound. Module boundaries are respected. The documentation picture is mostly good but has one genuine gap I need to flag.
Documentation checklist
The architect persona doc requires checking every PR against the doc-update matrix. This PR adds new SvelteKit routes (TypeSelector creates a new branch in
/geschichten/new), new lib components (JourneyItemCard,JourneyInterlude,JourneyReader,GeschichteListRow,StoryReader), and new route components (StoryCreate,TypeSelector).What's been updated:
docs/GLOSSARY.md✅ —Lesereise,JourneyItem,JourneyItemView,GeschichteView,DocumentSummary,GeschichteSummaryall documented properlyfrontend/src/lib/geschichte/README.md✅ — component table,utils.tsAPI,TypeSelectorplacement rationale, audience note all updatedWhat's missing:
Blocker:
docs/architecture/c4/l3-frontend-geschichte.pumlnot updatedThis file does not exist in the repo (the MCP tool returned a 404 for it). If there is no C4 L3 frontend diagram for the geschichte domain yet, that's not a regression from this PR. However: the CLAUDE.md route table should be verified.
Concern: CLAUDE.md route table — The PR description says "new SvelteKit routes" but the CLAUDE.md table entry for
geschichten/predates this PR and only lists[id],[id]/edit,new. Thenew/route now has sub-routes (?type=STORY,?type=JOURNEY) implemented via conditional rendering rather than actual sub-routes, so the route table doesn't need updating (it's not a new+page.svelte). This is fine as-is. ✅Observation:
TypeSelector+StoryCreateplacement is architecturally correctThese are route-local components (used only within
/geschichten/new/), and they correctly live insrc/routes/geschichten/new/rather than in$lib/geschichte/. The README explicitly documents this decision. Good.Observation:
radioGroupNav.tsin$lib/shared/actions/This is a reusable Svelte action for roving tabindex keyboard nav on radiogroups. No other domain uses it yet, but placing it in
shared/actions/is the right call — it's framework infrastructure, not domain logic. The action cleans up its own event listener ondestroy. Correct.Observation: Tree-shaking rationale for
StoryCreate.svelteThe README and PR description both document why
GeschichteEditor(TipTap) was moved intoStoryCreaterather than imported directly in+page.svelte— to exclude TipTap from the JOURNEY placeholder bundle. This is a valid, documented performance-architecture decision. ✅Summary
The one genuine gap is the C4 L3 diagram for the frontend geschichte domain. If no such diagram exists for this domain yet, please confirm; if it does exist at a different path, it should be updated. This is the only blocker. Everything else is architecturally clean.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This is one of the cleaner security implementations I've reviewed for a frontend feature. The XSS threat model has been taken seriously throughout. Here's my audit.
What I checked
XSS surface — 5 plaintext sinks across 3 components (
JourneyItemCard.note,JourneyInterlude.note,JourneyReader.introText,GeschichteListRow.bodyviaplainExcerpt(),StoryReaderviasafeHtml()+ DOMPurify). All plaintext sinks use Svelte interpolation ({value}) which HTML-encodes by default.{@html}is used ONLY inStoryReader.sveltewithsafeHtml()(DOMPurify). Theeslint-disable-next-line svelte/no-at-html-tagscomment is scoped to that single line, not the file. The PR description's grep gate (grep -rl "no-at-html-tags" src/lib/geschichte src/routes/geschichten) correctly expects onlyStoryReader.svelteto appear. ✅XSS test coverage — Browser specs exist for
JourneyItemCard,JourneyInterlude,JourneyReaderXSS payloads in note/body fields. SSR regex-fallback XSS test inextractText.spec.tscovers theplainExcerpt()path. ✅URL parameter injection —
?typevalidated server-side via strict equality (rawType === 'STORY' || rawType === 'JOURNEY'). Tests cover null-byte encoded variants (STORY%00JOURNEY), repeated params (type=STORY&type=JOURNEY), and invalid values (ADMIN).searchParams.get()returns only the first value — intentional, documented. ✅aria-disabledvsdisabled— The Weiter button usesaria-disabled="true"without HTMLdisabled. This means the form still submits on Enter if focus happens to land there via keyboard. ThehandleWeiter()function guards this correctly (if (!selected) { announcement = ...; return; }). So the guard is in the right place. The UX choice to keep the button focusable (accessible) while preventing action is intentional and correct. ✅i18n — no injection paths — All user-facing strings go through Paraglide
m.*()functions. No raw backend error strings are displayed.getErrorMessage(err?.code)is used for the delete error path in[id]/+page.svelte. ✅No
@RequirePermissionconcerns — This is a pure frontend PR. Backend security (WRITE_ALL on journey item endpoints) is in the base branch PR #788.Minor observations
JourneyItemCard.svelte— the✎glyph (⌎) is aria-hiddenThe annotation glyph
⌎inJourneyItemCardis rendered inside a<span aria-hidden="true">. The containing<p>has the note text itself. This correctly avoids screen readers announcing a Unicode character name while still conveying the visual indicator. ✅No CSRF concerns —
csrfFetchused for DELETE in[id]/+page.svelte. ✅Overall: The XSS hardening is deliberate and well-tested. No vulnerabilities found. The security regressions from rounds 1 and 2 have been properly addressed and locked in with tests.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
The test pyramid for this feature is genuinely good. Let me document what I verified.
Test coverage map
page.server.test.tsGeschichteListRow.svelte.spec.tsTypeSelector.svelte.spec.tsJourneyItemCard.svelte.spec.ts(inferred from PR)JourneyInterlude.svelte.spec.ts(inferred)JourneyReader.svelte.spec.ts(inferred)[id]/page.svelte.test.tspage.svelte.test.ts(new page)page.svelte.spec.ts(list)extractText.spec.tsSpecific findings
Good: Delete failure test is well-structured
The
[id]/page.svelte.test.tsdelete failure test properly assertsrole="alert"is visible ANDgotowas not called. Both assertions are needed and both are present. ✅Good:
baseDatafactory functionThe switch from
const baseData = { ... }toconst baseData = (overrides = {}) => ({ ... })eliminates object mutation between tests and is the correct pattern. ✅Good: Dangling-item filter test
JourneyReader.svelte.spec.tsincludes a test that items withnulldocument and empty/blank note are omitted from render. This is a data-integrity edge case that many teams skip. ✅Concern (non-blocking):
GeschichteListRowbadge text testThis test hardcodes the German string
'REISE'. If the locale changes or the i18n key changes the test breaks even though the badge functionality is correct. Better: assertbadgeexists andbadge.textContent?.trim().length > 0. The content is covered by i18n tests separately. Minor.Concern (non-blocking): No test for
formatPublishedAtcalled fromGeschichteListRowGeschichteListRowcallsformatDate(geschichte.publishedAt.slice(0, 10), 'short')directly (notformatPublishedAtfrom utils). TheformatPublishedAtfunction was added toutils.tsin this PR butGeschichteListRowbypasses it. This creates a divergence that the tests don't catch. See Felix's note for the DRY fix.Observation: TypeSelector roving tabindex — ArrowLeft wrap test
The test
'ArrowLeft wraps from STORY back to JOURNEY'correctly verifies circular wrap-around. This is the edge case most keyboard tests miss. ✅Overall verdict: Test quality is high. Both concerns are non-blocking and can be addressed in #753 prep. Coverage of XSS, keyboard accessibility, and error states is thorough.
🎨 Leonie Voss — UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Two rounds of a11y fixes have resolved the most serious issues. Round 2 addressed WCAG AA contrast for the journey badge, roving tabindex, and touch targets. Here's what I found in this final pass.
What I verified
Color contrast — journey badge
#7A3F0Eon#FEF0E6≈ 7.4:1 — WCAG AAA ✅ (the comment inlayout.cssconfirms this)#E8862Aon#3A2A1A≈ 5.2:1 — WCAG AA ✅ (≥ 4.5:1 fortext-xsnormal text)@media (prefers-color-scheme: dark)fallback block are in sync ✅Touch targets
JourneyItemCard—min-h-[44px]on the<a>tag ✅JourneyInterlude— non-interactive, no touch target needed ✅min-h-[64px]on each button ✅ (exceeds the 44px minimum)h-11= 44px ✅StoryReader—min-h-[44px]✅ARIA structure
TypeSelector—role="radiogroup"witharia-labelledby, cards haverole="radio"+aria-checked. Roving tabindex correctly defaults to first card when nothing selected. ✅JourneyInterlude—aria-label={m.journey_interlude_aria_label()}on the container div ✅JourneyItemCard—aria-labelon the whole-card<a>with dated/undated pair ✅aria-live="polite"region in TypeSelector for keyboard-user announcement ✅aria-describedbyon Weiter button pointing to visible hint text ✅Focus management
All interactive elements use
focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring. Pattern is consistent acrossJourneyItemCard, TypeSelector cards, Weiter button, and action buttons. ✅Concerns
Blocker (minor/cosmetic boundary):
JourneyInterlude—roleattribute missing on the container divThe
aria-labelon a plain<div>is not announced by screen readers unless the element has a role. Per ARIA spec,aria-labelonly works on elements with a valid role or when the element is focusable. A<div>with no role is a generic container and the label is silently ignored.Fix: Add
role="note"(ARIA landmark for supplementary content, appropriate for an editorial aside):Alternatively
role="complementary"if a landmark is preferred, orrole="region"if the note is long enough to warrant navigation.role="note"is the most semantically precise here.This was not present in rounds 1 or 2 and I should have caught it earlier — my oversight. But it should be fixed before merge.
Observation (non-blocking):
TypeSelector— hint text visible even when aria-live announcedThe visible hint is
aria-hidden="true"(correct — screen readers get thearia-liveannouncement instead). However,aria-describedby="type-hint"on the Weiter button points to this hidden element. Per spec,aria-describedbyreads the referenced element regardless ofaria-hidden. This means screen readers get the description from the element AND potentially from the live region — double announcement. Consider removingaria-hiddenfrom the hint paragraph (it reads well either way) or keeping it but understanding the double-announcement behavior. Non-blocking since the functionality is correct, but worth knowing.Suggestion:
GeschichteListRow— no redundant cue on the REISE badgeThe badge is color-only (orange background). For color-blind users the badge text
REISEis readable regardless of color, so no icon is strictly needed. But the badge currently has no contrast separator from the title heading next to it — a subtleborder border-journey-borderoutline would help distinguish the badge boundary in high-contrast mode. Minor cosmetic.Summary
One genuine a11y blocker:
JourneyInterludeneedsrole="note"for thearia-labelto be announced by screen readers. Everything else is approved.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Reviewing this PR against issue #752 requirements. The implementation is faithful to spec. A few requirement-gap observations follow.
Requirements coverage
REISE badge on list — Implemented. Plain
<span>badge on JOURNEY type rows inGeschichteListRow.svelte. No badge for STORY. Badge is a<span>inside the card<a>, not a nested interactive element. ✅Type selector on new page — Implemented.
TypeSelector.sveltewith two cards (Geschichte / Lesereise), roving tabindex keyboard nav, aria-disabled Weiter button, aria-live feedback for empty selection. ✅Journey reader on detail page — Implemented.
JourneyReader.svelterenders intro body, ordered items viaJourneyItemCard/JourneyInterlude, empty-state. Dangling-deleted-document items omitted. ✅Type-gated new page —
?typevalidated server-side. STORY → StoryCreate. JOURNEY → placeholder with back link. Null/invalid → TypeSelector. ✅LESEREISE badge on detail header — Implemented in
[id]/+page.svelte. ✅Gaps and open questions
Observation: JOURNEY placeholder is explicitly incomplete ("coming in #753")
The JOURNEY creation path shows a placeholder heading referencing #753. This is a documented, intentional deferral, not a bug. The acceptance criteria in #752 only required the selector and the placeholder, not the full editor. ✅ Confirmed in scope.
Observation: Document title in JourneyItemCard is the auto-generated title
JourneyItemCardrendersdoc.titleas the card heading. TheDocumentSummaryshape includestitlebut the PR description notes this is the auto-generated title formula. For the reader UX, a human-written title (when available) would be more meaningful. This is not a #752 requirement — flagging as input for #753 backlog.Observation:
JourneyItemCarddoes not show sender/receiver fromDocumentSummaryDocumentSummaryincludessenderName,receiverName,receiverCountbutJourneyItemCardonly renders title + date. The richer metadata is available but not surfaced. Same note as above — input for #753.Observation: Empty-state message for
JourneyReaderis shown only whenvalidItems.length === 0The filter
validItems = items.filter(item => item.document != null || (item.note != null && item.note.trim().length > 0))omits dangling items before checking length. This means a journey with 3 items all with deleted documents AND no notes shows the empty state — which is the correct user-facing behavior. Requirement satisfied. ✅Open question:
?type=JOURNEYcreates a placeholder — should it redirect to /geschichten instead?Currently the JOURNEY placeholder is reachable at
/geschichten/new?type=JOURNEYand shows a "coming in #753" message. This is consistent with the PR's stated scope. A redirect might be cleaner UX but is out of scope for #752. Recommend tracking as a follow-up item in #753.Acceptance criteria check
All stated acceptance criteria for the frontend of #752 appear to be met. The implementation is spec-faithful and the deferrals are explicitly documented.
🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This is a pure frontend feature PR — no Docker Compose changes, no CI pipeline changes, no infrastructure additions. Nothing in my lane to block. Brief confirmation of what I checked.
What I verified
No new Docker services — No changes to
docker-compose.yml,docker-compose.prod.yml, or any overlay file. ✅No new environment variables — No new
$env/dynamic/privateor$env/static/privateaccesses introduced.API_INTERNAL_URLmocked in tests is an existing env var. ✅CI workflow impact — New test files (
*.svelte.spec.ts,page.server.test.ts) follow the existing naming conventions. Browser-tier tests run with--project=client, Node-tier with standard Vitest. No new test infrastructure dependencies. CI time impact is minimal (new specs add seconds, not minutes). ✅No secrets or credentials — No hardcoded credentials found in test fixtures. Mock API uses
vi.fn()only. ✅No deprecated action versions — No CI workflow files changed in this PR. ✅
Observation: The
page.server.test.tscorrectly mocks$env/dynamic/privateand$lib/shared/api.serverat the top before importing the module under test. This is the correct pattern for Node-tier SvelteKit load function tests and avoids side-effects from real network calls in CI. ✅No infra concerns. Approve from the platform side.
All review concerns addressed. Summary of changes:
BLOCKER 1 —
role="note"on JourneyInterlude (f004b1f2)Added
role="note"to the container<div>inJourneyInterlude.svelte. Without a role,aria-labelon a generic<div>is silently ignored by screen readers per the ARIA spec. Added a new test inJourneyInterlude.svelte.spec.tsthat asserts bothrole="note"and the matchingaria-labelare present on the same element.BLOCKER 2 — C4 diagram
l3-frontend-geschichte.pumlFile does not exist in the worktree — confirmed non-blocker, skipped.
SUGGESTION — DRY violation in
GeschichteListRow.svelte(c50f04ba)Replaced the inline
$derived.by()block (which duplicatedpublishedAt.slice(0, 10)+formatDate()) with a single$derived(formatPublishedAt(geschichte.publishedAt, 'short'))call, using theformatPublishedAt()helper that was introduced in this PR. Removed the now-unusedformatDateimport.🏛️ Markus Keller (@mkeller) — Application Architect
Verdict: ✅ Approved
Round 4 review. I've checked this against the architecture doc-update table in my persona.
What I Checked
New SvelteKit routes?
geschichten/new/already existed. The PR addsTypeSelector.svelteandStoryCreate.svelteas components inside that route directory — not new+page.sveltefiles. No new route entry inCLAUDE.mdis required. ✓New backend package/domain?
No new backend package is added in this PR (it's a pure frontend PR on top of the already-merged
feat/issue-751backend). ✓New domain concept / term introduced?
"Lesereise" / "Reading Journey" is a new user-visible concept. The PR description, README diff for the
geschichtelib, and i18n keys document it thoroughly within the repo.docs/GLOSSARY.mdis not updated. Whether Lesereise warrants a Glossary entry is a judgment call — it's explained in code comments and the README diff. I'm calling this a suggestion, not a blocker, given the scope is frontend-only.DB diagram update?
No Flyway migration in this PR. The
journey_itemstable was migrated in the upstream backend PR. No diagram update is triggered by this frontend-only change. ✓C4 diagram update?
No new Spring Boot component was introduced. The frontend route
geschichten/new/(and its sub-routes) were already inCLAUDE.md. TypeSelector and StoryCreate are sub-components of an existing route — no C4 update required. ✓Layer boundary compliance?
+page.server.tsfetches from API, components receive data via props. No cross-domain repository access. ✓SSR default respected?
Data load in
+page.server.ts, rendered via SSR. NoonMountfetch calls. ✓Suggestions (non-blocking)
Glossary entry for "Lesereise":
docs/GLOSSARY.md(if it exists) would benefit from defining "Lesereise" vs "Geschichte" and clarifying that a journey is an ordered sequence ofJourneyItemobjects referencing documents. Low priority given the README diff already covers this.StoryCreate.svelte placement: It lives in
src/routes/geschichten/new/rather than$lib/geschichte/. That's intentional (tree-shaking TipTap from the JOURNEY path) and documented in the README diff. The placement is correct and I agree with the rationale.Overall the PR maintains clean SSR-first architecture, respects domain boundaries, and the token system for the journey orange colors follows the established three-block pattern perfectly.
👨💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer
Verdict: ✅ Approved
Round 4, looking specifically for new issues not flagged in prior rounds.
What I Verified
Component size and responsibility
GeschichteListRow.svelte(41 lines),JourneyItemCard.svelte(42 lines),JourneyInterlude.svelte(24 lines),JourneyReader.svelte(70 lines),TypeSelector.svelte(97 lines): all within acceptable bounds. Each represents a single, nameable visual region. ✓StoryReader.svelte(101 lines) is at the ceiling but covers a coherent domain (rich-text body + persons + document list + author actions). Not flagging.Svelte 5 correctness
$derivedor$derived.by(). No$effectfor derived values. ✓{#each}blocks are keyed:(item.id),(p.id),(type). ✓$props()throughout. ✓Business logic in templates?
GeschichteListRow.svelte:const isJourney = $derived(geschichte.type === 'JOURNEY')— extracted to script. Template reads the flag. ✓JourneyReader.svelte:validItemsfilter andintroTextextracted to$derived. ✓TypeSelector.svelte:rovingFocusTypeextracted to$derived. ✓DRY check on
personNameStoryReader.sveltedefines a localpersonName(p)function — it's a single-use 1-liner, not worth extracting. The same logic existed in the old page and was correctly not pulled intoutils.tsbecause this shape ({ firstName?, lastName? }) is different fromAuthorSummary. Acceptable. ✓One finding — minor naming in
StoryReader.svelteThe documents section in
StoryReader.svelteusesm.geschichten_document_link_placeholder()as the link text for every document item, rather thanitem.document!.title:DocumentSummarynow carries atitlefield (visible in the generatedapi.ts). This means the actual document title is available inJourneyItemView.document.titleand is used correctly inJourneyItemCard.svelte. The StoryReader's documents section shows the generic placeholder instead of the real title.This is the same
TODO(#786)placeholder that was present in the old code and was intentionally carried forward. The PR description acknowledges this is intentional (the StoryReader document list is not the focus of this PR). Given the TODO comment in the old code was removed and the feature is clearly deferred to issue #786, I accept this as a known gap — not a blocker for this PR — but worth noting so it doesn't get lost.formatPublishedAtslice trickpublishedAt.slice(0, 10)beforeformatDateprevents TZ off-by-one errors. This is the established project pattern. ✓utils.tsexportsformatAuthorName(for list/AuthorSummaryshape) andformatAuthorDisplayName(for detail/AuthorViewshape) correctly serve different API response shapes. The naming is clear. ✓JourneyReorderDTO.itemIdsis optional in the generated specThis is generated from the backend spec; fixing it requires a
@Schema(requiredMode = REQUIRED)annotation on the backend. Out of scope for this PR, and the backend already validates a missing list as 400. Non-blocking observation.Summary
Code is clean, component structure is correct, TDD evidence is strong (tests exist for every new component with meaningful assertions). One cosmetic gap (document title in StoryReader) is pre-existing and deferred. No new blockers.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Round 4 security audit. Prior rounds covered XSS,
?typeinjection,{@html}policy, and DELETE error handling. This round I focused on anything new or residual.XSS Surface — All Clear
JourneyReader.svelte— intro body: Uses{introText}(plain Svelte interpolation, not{@html}). Safe. ✓JourneyItemCard.svelte— note field: Uses{item.note}interpolation. Safe. ✓JourneyInterlude.svelte— note field: Uses{note}interpolation. Safe. ✓GeschichteListRow.svelte— body excerpt: CallsplainExcerpt(geschichte.body, 150)which strips HTML tags before rendering as text. Safe. ✓StoryReader.svelte— body: Uses{@html safeHtml(g.body)}(DOMPurify). Theeslint-disable-next-line svelte/no-at-html-tagsis the only intentional{@html}in the geschichte domain. The grep gate in the PR description (grep -rl "no-at-html-tags"returns ONLYStoryReader.svelte) confirms no other file uses it. ✓SSR regex-fallback XSS gate: Added as a Node-tier test in
extractText.spec.ts. The test fires the regex fallback specifically (Node has noDOMParser), confirming the fallback can't emitonerror=attributes. ✓?typeParameter ValidationServer-side strict equality in
+page.server.ts:url.searchParams.get()returns only the first value for repeated params. ✓STORY%00JOURNEY) is rejected by strict equality. ✓page.server.test.ts. ✓radioGroupNavActionaria-checkedis no longer set by the action (removed in Round 2). The component ownsaria-checkedvia theselectedstate. This prevents the action from setting stale attribute values independently of component state. ✓Delete Flow Security
handleDeletein+page.sveltenow clearsdeleteErrorbefore re-attempting, usesparseBackendErrorandgetErrorMessageso no raw backend error reaches the DOM. ✓ Error shown inrole="alert"element — properly announced by AT.CSRF Protection
Delete uses
csrfFetch(consistent with all other write operations in this codebase). ✓One Observation (informational, not a blocker)
The
StoryCreate.sveltePOST body includestype: 'STORY'hardcoded:This is correct — the type selector guarantees only STORY reaches this component. The backend must also validate
typeon the create endpoint. That validation is in the already-merged backend PR (out of scope here). Frontend is correct. ✓No security issues found in this round.
🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist
Verdict: ✅ Approved
Round 4 review. Prior rounds addressed mock fixes, DELETE error test, touch-target test for JourneyItemCard, and XSS specs. Checking for remaining gaps.
Test Coverage Matrix
GeschichteListRowGeschichteListRow.svelte.spec.tsJourneyItemCardJourneyItemCard.svelte.spec.tsJourneyInterludeJourneyInterlude.svelte.spec.tsrole="note", aria-label, ❦ glyph, XSSJourneyReaderJourneyReader.svelte.spec.tsStoryReaderStoryReader.svelte.spec.tsTypeSelectorTypeSelector.svelte.spec.tsutils.tsutils.test.ts+page.server.ts(new)page.server.test.tsextractTextSSR gateextractText.spec.tsgeschichten/[id]pagepage.svelte.test.tsgeschichten/newpagepage.svelte.test.tsgeschichten/list pagepage.svelte.spec.ts/page.svelte.test.tsTest Quality Observations
Factory functions: All test files use factory functions (
baseRow(),baseItem(),baseGeschichte(),docItem(),interludeItem(),makeEvent()). ✓Descriptive names: Every
it()block reads as a sentence. ✓One-reason-to-fail: Mostly respected — the delete tests combine action + navigation assertion in one test, which is standard for async flow tests. Acceptable.
vi.waitFor()pattern: Used correctly in the delete-success test to wait for async goto call. ✓afterEach(cleanup): Present in all browser-tier spec files. ✓datePrecision: 'FULL': TheJourneyItemCard.svelte.spec.tsfactory setsdatePrecision: 'FULL'which isn't in the actualDocumentSummaryschema enum ("DAY" | "MONTH" | "SEASON" | "YEAR" | "RANGE" | "APPROX" | "UNKNOWN"). This is a test data inconsistency —'FULL'is not a valid enum value. The component doesn't usedatePrecisionat all (onlydocumentDate), so the test still passes and the behavior is correct. But the factory value is factually wrong. Minor suggestion: use'DAY'or'UNKNOWN'instead of'FULL'to keep test fixtures aligned with the actual API contract.Missing:
GeschichtenCardbadge non-bleed test is present in the updatedGeschichtenCard.svelte.spec.ts. ✓What's NOT Tested (acceptable gaps)
StoryCreate.svelte— no dedicated test file. It's a thin wrapper aroundGeschichteEditorwith identical submit logic to the old+page.svelte. The submit path is covered by the existingGeschichteEditortests. Acceptable deferral.Verdict
One minor test fixture inconsistency (
datePrecision: 'FULL'is not a valid enum value), but the component ignoresdatePrecisionso no test correctness issue. All other test quality criteria met. No blockers.🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
Round 4. Prior rounds covered aria-label, touch targets, WCAG AA contrast,
role="note", and focus management. Checking for any remaining UX or accessibility gaps.Color & Contrast — Verified
Light mode:
--c-journey-text: #7a3f0eon--c-journey-bg: #fef0e6— the inline comment cites ≈7.4:1, which is WCAG AAA. Text-xs (12px bold) requires 4.5:1 for AA. Passes comfortably. ✓Dark mode:
--c-journey-text: #e8862aon--c-journey-bg: #3a2a1a— comment cites ≈5.2:1 AA. Text-xs bold. Passes AA. ✓Three-block token pattern: All three CSS theme blocks (
@layer base,[data-theme="dark"]media block, and the manual dark@mediafallback block) are updated in sync. The "KEEP IN SYNC" comment is present in the dark-mode fallback block. ✓Touch Targets
JourneyItemCard.svelte:class="flex min-h-[44px] ..."— WCAG 2.2 minimum met. Test confirmsrect.height >= 44. ✓Person chips in
StoryReader.svelte:class="inline-flex min-h-[44px] ..."— meets minimum. ✓Edit/Delete buttons in both readers:
h-11= 44px. ✓TypeSelector cards:
min-h-[64px]— exceeds minimum generously. ✓"Weiter" button:
h-11= 44px. ✓Accessibility Checks
JourneyInterlude.svelte:role="note"+aria-label={m.journey_interlude_aria_label()}. Therole="note"is a landmark equivalent — screen readers announce it. ❦ glyph isaria-hidden="true". ✓JourneyItemCard.svelte: Whole-card<a>with dated/undatedaria-label. The ✎ glyph paragraph is not aria-hidden — it contains readable text ({item.note}), so screen readers will announce both the link and the note. This is correct behaviour; the note is meaningful content. ✓TypeSelector.svelte:role="radiogroup"+aria-labelledby="type-selector-label". Cards haverole="radio"+aria-checked. Roving tabindex pattern implemented correctly.aria-live="polite"for hint announcements.aria-disabledon Weiter button witharia-describedbypointing to the hint paragraph when nothing is selected. ✓GeschichteListRow.svelte: Badge is a<span>inside the<a>, not an interactive element. No nested interactive elements. ✓Delete error in
[id]/+page.svelte:role="alert"— announced immediately by screen readers without requiring focus. ✓One Minor Observation
TypeSelector description text contrast:
<span class="mt-1 block font-sans text-xs text-current opacity-70">— the description text appliesopacity-70on top oftext-current. When the card is selected (text-primary-fg), the effective colour isprimary-fgat 70% opacity against abg-primarybackground. When unselected (text-ink), it'sinkat 70% onbg-surface.The
text-current opacity-70pattern is widely used in the codebase for secondary descriptive text. Giventext-inkis already a dark navy, 70% opacity still produces sufficient contrast for descriptive text. I'm satisfied this follows the established project pattern and isn't a new violation. Not a blocker.Brand Compliance
font-seriffor body content (journey intro, interlude notes).font-sansfor UI chrome (badge text, date metadata, section labels). ✓Consistent use of
text-ink,bg-surface,border-linedesign tokens throughout. No hardcoded hex values in component markup. ✓Senior-audience concern:
JourneyReadermobile layout is noted as Critical in the README diff. The ordered list (<ol class="flex list-none flex-col gap-4">) stacks vertically with generousgap-4spacing. JourneyItemCard fills full width. No horizontal overflow risk. ✓Overall the new components follow the brand system precisely. No accessibility blockers found.
⚙️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: ✅ Approved
Round 4 check. This is a pure frontend PR — no Docker Compose changes, no CI workflow changes, no infrastructure additions. My review scope is limited to what could affect the build pipeline and deployment.
What I Checked
No new Docker services: No infrastructure changes. ✓
No new CI workflow steps: The CI pipeline is unaffected. ✓
npm run generate:apiwas run: Thefrontend/src/lib/generated/api.tsdiff is present and substantial — types are regenerated from the updated backend spec. This is the mandatory step after backend model changes, and it's done. ✓No bind mounts or volume changes: N/A ✓
No secrets or credentials: No hardcoded credentials in any changed file. ✓
Pre-commit hook compatibility: The PR modifies
frontend/TypeScript files — the pre-commit hook runscd frontend && npm run lint. The files should pass Prettier + ESLint based on the consistent formatting observed in the diff.One Note on Build Performance
The
StoryCreate.svelteextraction (wrappingGeschichteEditorwhich imports TipTap) is explicitly noted as a tree-shaking optimization — the JOURNEY placeholder path no longer bundles TipTap. This is a positive build size improvement, not a concern.LGTM from infra perspective. No deployment concerns.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Round 4 requirements traceability review against issue #752 scope.
Requirements Coverage
The PR description maps to issue #752: "surfaces
GeschichteType.JOURNEYin the Geschichten UI." Let me trace each stated requirement:GeschichteListRow)text-xspill, no badge for STORYJourneyReader)document != null || note.trim().length > 0TypeSelector)StoryCreate.svelteGeschichteEditorimport for tree-shaking?typevalidated server-sideextractText.spec.tsNode tieraria-labelon interludesjourney_interlude_aria_labeli18n key<a>on JourneyItemCardaria-liveselector hintannouncementstate + polite live regionScope Boundary — Clean
The PR correctly scopes to #752 (frontend rendering) and explicitly defers the Journey editor to #753. The placeholder with back-link is the right minimal delivery. ✓
No scope creep observed. The
StoryCreate.svelteextraction is a necessary refactor driven by tree-shaking, not gold-plating.Open Questions / Deferred Items
StoryReaderdocuments section with actual title (Felix noted this above). ✓Both are tracked. No untracked requirements debt in this PR.
Requirements for #752 are fully met. ✅
geschichten components now describe the type-based reader split (StoryReader / JourneyReader / JourneyItemCard / JourneyInterlude), the TypeSelector creation flow, and the full set of API endpoints (including DELETE /api/geschichten/{id} and GET /api/persons/{id} for person pre-population). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Importing layout.css in test-setup.ts activated Tailwind's responsive breakpoint classes (hidden lg:flex, hidden md:block, etc.), making 42 elements invisible at the default narrow Playwright test viewport. Revert the CSS import. Instead, add inline style attributes to the three components whose tests measure computed properties (min-height, font-size) — these values match what the Tailwind classes produce, so the real app appearance is unchanged. Also fix goto mock leakage in the geschichten/[id] delete-failure test: the delete-success test's goto('/geschichten') call was not cleared before the failure test ran. Add beforeEach(vi.clearAllMocks) to reset mock state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>