fix(test): resolve pre-existing TranscriptionEditView and Richtlinien test failures #423
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Context
Two test failures exist on
mainand were carried into PR #422 (domain restructuring). They are pre-existing and unrelated to the restructuring, but need a tracked home so CI failures can be diagnosed quickly.Failing tests
1.
TranscriptionEditView.svelte.spec.tsTwo tests fail:
auto-save debounce — passes the block mentionedPersons array as the 3rd save argument—onSaveBlockis called with an empty[]instead of the expected[{ personId: 'p-aug', displayName: 'Auguste Raddatz' }]. Likely a reactivity issue with how mention state is captured in the debounced save closure (state_referenced_locallySvelte warning is present in the file).flush on blur — flushes pending save immediately on textarea blur before debounce expires—TypeError: Cannot read properties of null (reading 'dispatchEvent'). The textarea element is not found at the time.dispatchEventis called.2.
src/routes/hilfe/transkription/page.svelte.spec.tsrenders Wikipedia external link with security attributes and new-tab annotation— expectslink.textContentto contain'öffnet in neuem Tab', but the component renders that text only in thearia-labelattribute, not as visible text content. The component usesaria-label="{m.richtlinien_wiki_link()} — {m.common_opens_new_tab()}"while the visible text is just{m.richtlinien_wiki_link()}. Either the test needs to checkaria-labelinstead oftextContent, or the component needs to add a visible (orsr-only) span with the annotation.Acceptance criteria
mainstate_referenced_locallyinTranscriptionEditViewRelated
mainbefore PR refactor(frontend): restructure lib/ from flat-by-type to domain-based (#408) (#422)👨💻 Felix Brandt — Fullstack Developer
Observations
Failure 1 —
mentionedPersonscapture in debounced save closure (TranscriptionBlock+useBlockAutoSave)The root cause is subtle. In
TranscriptionBlock.svelte, theemitChange()function captureslocalMentionsby reference from the outer$statevariable at call time. TheuseBlockAutoSave.handleTextChange()stores that reference inpendingMentions. However,localMentionsis a plainPersonMention[]array replaced on each update (not mutated), so the value stored inpendingMentionsat the momenthandleTextChangeis called is correct — but then the test issuesuserEvent.type()which fires theonUpdateTiptap callback and the suggestioncommand()callback via separate code paths. The Svelte warningstate_referenced_locallyis the compiler surfacing thatlocalMentionsinside the getter lambda() => localMentionsofbind:mentionedPersonscloses over the localletbinding — meaning the setter receives the updated array, but the getter thatuseBlockAutoSave.handleRetryreads from still sees the closed-over snapshot from binding time if anything intercepts. The practical effect: whenuserEvent.typeinserts characters before the existing@Auguste Raddatzmention node without triggeringonUpdatefor the mention itself,pendingMentionsmay be re-set to[]from a priorhandleTextChangecall that saw empty mentions before the editor finished deserializing. The test then waits 3 s for the debounced save to fire with[{ personId: 'p-aug', displayName: 'Auguste Raddatz' }]but receives[].The
state_referenced_locallywarning onbind:mentionedPersonsinTranscriptionBlock.svelte(line 164) should be taken seriously — the Svelte compiler is flagging that the getter closes over a local$statevariable whose identity changes on reassignment. The fix is to make the getter capture a stable reference or use$derivedinstead.Failure 2 —
flush on blurtest:TypeError: Cannot read properties of nullThe test does
document.querySelector('[role="textbox"]')and then calls.dispatchEvent(). The element isnullat query time. This is a timing issue: TipTap's editor is created inonMount, which runs asynchronously after the initial render frame.vitest-browser-svelte'srender()completes synchronously (from the test's perspective), but the editor'seditorEldiv withrole="textbox"is only attached after the async mount lifecycle completes. The test doesn'tawaitanything betweentextarea.fill()and thedispatchEventcall on the raw DOM element — the[role="textbox"]selector hits before Tiptap'seditorProps.attributesare applied. Apage.getByRole('textbox').element()(which the other tests use) would wait for the element to exist via Playwright locators; the rawdocument.querySelectorcall does not.Failure 3 — Richtlinien page:
textContentvsaria-labelThe test (line 29-30) queries
document.querySelector('a[href*="wikipedia"]')and then callslink.textContent. The component renders the visible link text as{m.richtlinien_wiki_link()}and puts the new-tab annotation only inaria-label="{m.richtlinien_wiki_link()} — {m.common_opens_new_tab()}". The test comment on line 28 (// icon communicates "opens new tab" visually; aria-label carries the text for a11y) correctly describes the intent — this is actually a well-implemented a11y pattern. The fix is to update the assertion to checkaria-labelrather thantextContent.Recommendations
Fix the Richtlinien test assertion: Change
expect(link.textContent).toContain(...)toexpect(link.getAttribute('aria-label')).toContain('öffnet in neuem Tab'). The component's pattern (visible icon + aria-label) is correct; only the assertion is wrong. The existing line 30 already does this correctly — but the issue description says the test checkslink.textContent. Check if there is a second assertion earlier in the test that checkstextContent; if so, remove it and keep only thearia-labelcheck.Fix the blur-flush test: Replace
document.querySelector('[role="textbox"]')withawait page.getByRole('textbox').first().element(). This uses the Playwright locator which waits for the element to be present (after Tiptap'sonMountcompletes), then returns the DOM element fordispatchEvent. The pattern is already established in the delete-block tests.Fix the
mentionedPersonsdebounce test: The safest structural fix inTranscriptionBlock.svelteis to replace thebind:mentionedPersonsgetter-setter pair with a two-way binding that avoids the local closure: lift thelocalMentionstracking into the setter and makeemitChangealways read the latest value synchronously. Alternatively, rename the setter to avoid capturing theletbinding. Thestate_referenced_locallywarning should be silenced by design, not by suppression — fix the reactivity model.Never suppress
state_referenced_locallywith a comment without understanding the failure mode. The warning inPersonMentionEditor.test-host.svelteshows the pattern of intentionally suppressing it. TheTranscriptionBlock.sveltecase is not intentional and needs the underlying reactivity fixed.🧪 Sara Holt — QA Engineer
Observations
Test quality: these are meaningful tests, not noise. All three failures describe real defects — not flaky timing, not implementation-detail assertions. They should be fixed, not disabled or worked around.
Failure 1 —
mentionedPersonsauto-save test: The test usesuserEvent.type()withvi.waitForat a 3 s timeout, which is the correct approach for a Tiptap editor where fake timers interfere with CDP keyboard events (as documented in the existing test comments). The test setup is sound. The bug is in production code — thependingMentionsstored byuseBlockAutoSave.handleTextChangeis not the same array that gets serialized at save time.Failure 2 — blur test:
nullelement:document.querySelector('[role="textbox"]')without awaiting is a classic vitest-browser anti-pattern. The test comment acknowledges thatlocator.blur()is not available; the correct substitute is(await page.getByRole('textbox').first().element()).dispatchEvent(...). The existing delete-block tests in this same file already demonstrate this pattern correctly (usingawait page.getByRole('button'...').element()). The blur test was not updated to follow the same pattern, which is why it's the only one broken in this group.Failure 3 — Richtlinien link annotation: The test intention is correct (verify the a11y pattern). The implementation of the assertion is slightly behind the component implementation. The test comment on line 28 already states the correct expectation (
aria-label carries the text for a11y) — the assertion on line 30 just needs to be the only check, and it already is. Based on the issue description, the failing assertion is somewhere in the test that checkslink.textContent. Reading the spec file confirms:link.textContentwill return just{m.richtlinien_wiki_link()}(the visible link text), not thearia-labelcontent.CI impact: Three failing tests on
mainmean CI is not fully green. While these are labelledP3-later, they create noise that can mask future real failures. Any team that ignores CI failures over time loses trust in the suite.Test architecture observation: The
TranscriptionEditView.svelte.spec.tsfile is 373 lines and tests 9 distinct describe blocks — that is well-organized. Factory functions (renderView) and per-describe isolation work correctly. The blur test's raw DOM query is the only structural inconsistency.Recommendations
Fix the blur test's DOM query by adding
awaitand using the locator API:const el = await page.getByRole('textbox').first().element() as HTMLElement. This makes it consistent with the delete-block test pattern already in the same file. Do not usedocument.querySelectorinvitest-browsertests — use locators.Add a regression guard for the
mentionedPersonsbug: Once the production code fix lands, confirm the existing test is actually red before the fix and green after. If it was never truly red, add aconsole.warnor test comment explaining why it was passing incorrectly so it cannot silently revert.Keep the Richtlinien test as-is in intent — just fix the assertion. The aria-label check on line 30 is already correct; just confirm there is no earlier
textContentassertion hiding the failure. The test titlerenders Wikipedia external link with security attributes and new-tab annotationaccurately describes what should be verified.Do not add
@Disabledor skip markers — these tests are close to passing and can be made green quickly. Disabling them would hide regressions in the exact behaviors they guard.After fixing, run
npm run testlocally to confirm all three describe blocks are green. The test suite for this component is otherwise solid and should not require changes beyond the three targeted fixes.🏗️ Markus Keller — Application Architect
Observations
Scope is correctly bounded. All three failures are in the
frontend/domain (transcription/andhilfe/). No cross-domain or backend concerns are implicated. This is a focused fix.useBlockAutoSaveas a separated module is correct design. Extracting the save/debounce logic intouseBlockAutoSave.svelte.tskeeps it independently testable and separate from the rendering concern inTranscriptionBlock.svelte. The failure in the mention-capture test points to a subtle reactivity contract violation at the boundary betweenTranscriptionBlock(which owns thelocalMentionsstate) anduseBlockAutoSave(which stores a snapshot of mentions athandleTextChangetime). The module boundary is correct; the binding contract needs tightening.The
data-block-wrapper+onblurlistener pattern: The blur handler is attached to thedata-block-wrapperdiv inTranscriptionEditView.svelte, relying on event bubbling from the TipTap contenteditable. This is architecturally reasonable — TipTap fires native blur events that bubble. The test failure is not an architecture problem; it's a test-side timing issue.No documentation update required for this fix. This issue touches no backend packages, routes, entities, migrations, or infrastructure. The standard doc-update checklist is not triggered.
state_referenced_locallySvelte compiler warning: The Svelte compiler emits this when a getter in abind:expression closes over a locally scoped$statevariable. The compiler warning is inTranscriptionBlock.svelteon thebind:mentionedPersonsbinding. This is not a style issue — it is the compiler's signal that the binding contract is fragile. It should be treated as a structural defect, not a warning to suppress.Recommendations
Resolve the
state_referenced_locallywarning by design, not suppression. The fix should make the binding's getter always read the current reactive value without capturing a stale closure. One clean approach: extract the getter–setter pair into a$derivedor use a stable$stateobject (instead of reassigning the array reference) so the getter always reflects the current value.Do not change the
useBlockAutoSavemodule interface. ThehandleTextChange(blockId, text, mentions)signature is correct — it takes a snapshot at call time. The contract is thatTranscriptionBlockmust callhandleTextChangewith the current mentions at the moment of each text change. If the binding is fixed inTranscriptionBlock, the module requires no changes.The Richtlinien fix requires no architectural review — it is purely a test assertion correction. The component's
aria-labelpattern (visible text + accessible label with new-tab annotation) is the correct implementation per WCAG 2.1 SC 2.4.4.Document the intentional client-side fetch exception in
PersonMentionEditor.svelte(line 127–139) as an ADR. The inline comment references "Markus #5616: an ADR will formalise this" — this issue is an appropriate reminder that the follow-up ADR is still open.Open Decisions
PersonMentionEditor.sveltealready acknowledges this. Should this be formalized before the issue closes, or tracked as a separate work item? The suggestion plugin'sitemsfunction fires on every keystroke — routing through SSR is impractical, so the exception is justified. The ADR should record this reasoning permanently.🔒 Nora "NullX" Steiner — Security Engineer
Observations
No security vulnerabilities in scope. These are test failures in frontend component tests — no backend endpoint changes, no auth flow changes, no new input surfaces. My scan focused on the code touched by this issue.
PersonMentionEditor.svelteclient-side fetch (line 144):fetch('/api/persons?q=...')withencodeURIComponent(query)is correct practice. The query is properly encoded before use in the URL. The response is cast toPerson[]after anokcheck and sliced to 5 items. No XSS surface — the returned data flows into TipTap's Mention extensionrenderHTMLwhich serializes to a<span>withdata-*attributes, notinnerHTML. This is safe.External link in Richtlinien page (
href="https://de.wikipedia.org/wiki/Kurrent"): Hastarget="_blank" rel="noopener noreferrer" referrerpolicy="no-referrer". All three required attributes are present. Thearia-labelcarrying the "öffnet in neuem Tab" string is also correct (users with screen readers are told the link opens a new tab — preventing surprise). This is a textbook-correct implementation.flushOnUnloadbeacon inuseBlockAutoSave.svelte.ts(line 119): Usesfetchwithkeepalive: trueto send aPUTrequest on page unload. The payload is JSON-serialized{ text, mentionedPersons }. This does not introduce a security concern — it uses the same endpoint as the normal save path, authenticated via cookie, with the same content-type header. Thekeepaliveflag is the correct choice for unload beacons.No hardcoded secrets, no logging of user input without sanitization, no JPQL concerns — this is a pure frontend component fix.
The
state_referenced_locallywarning is not a security issue, but it is worth noting that a stale closure capturing mention state could theoretically cause a different user's mention to be associated with a save payload (if blocks were reused across documents without a component teardown). In this codebase, blocks are unmounted and remounted per document, so the practical risk is zero — but the fix still eliminates the theoretical edge case.Recommendations
Verify the
data-editor-innerselector in the placeholder$effectdoes not introduce a selector-injection risk. It does not —editorEl.querySelector('[data-editor-inner]')is a static string, not user-controlled. This is clean.The aria-label pattern on the Wikipedia link is correct and should be preserved exactly as-is. Do not move the new-tab annotation to
textContent(a visiblesr-onlyspan) as a "fix" — the currentaria-labelapproach is preferable because it avoids redundant announcements for sighted users.No action required on the security surface of the code changed by this issue. The fix is purely a reactivity and test-assertion correction.
🚀 Tobias Wendt — DevOps & Platform Engineer
Observations
CI impact is the primary concern here. Three failing tests on
mainmean CI is persistently non-green. Even with aP3-laterlabel, a non-green CI baseline is operationally problematic: new PRs cannot distinguish genuine regressions from pre-existing failures without reading historical context. This erodes confidence in the pipeline and slows diagnosis.No infrastructure files are changed by this fix. No Docker Compose changes, no CI workflow changes, no image tag updates. My review is scoped to the CI/pipeline implications.
The test runner is
vitest-browserwith Playwright/Chromium, run vianpm run testin thefrontend/project. The failing tests are in the browser-mode project (component tests against real DOM), not the server-mode project. This means they require a Chromium browser in the CI environment. If the Gitea Actions runner does not have Chromium/Playwright pre-installed, these tests may fail for environmental reasons unrelated to the code — worth confirming.The
P3-laterlabel is reasonable for product priority, but from an operational standpoint, a persistent CI failure isP1for the pipeline. A broken test that's been onmainsince PR #422 means every CI run for every subsequent PR shows failures that are not caused by that PR's changes. This creates diagnostic noise.Recommendations
Merge this fix promptly despite the
P3-laterlabel — the CI noise cost accumulates with every PR that passes throughmain. A 30-minute fix now saves hours of context-switching over the next month.Confirm Playwright/Chromium is installed on the Gitea Actions runner before attributing the blur test failure to code. If the runner environment changed between when these tests were written and when they started failing, the root cause might be environmental. Run
npx playwright install chromium --with-depsin the CI setup step if it is missing.Add a CI step that fails fast on pre-existing test failures after this fix lands — something like a
test:browsertarget that is distinct from the server-mode tests. This makes it immediately obvious when a browser-mode test breaks, rather than discovering it during a PR review weeks later.No changes to
docker-compose.ymlare needed for this fix. The modifieddocker-compose.ymlin the working tree (shown in git status) is unrelated to this issue — ensure those changes don't accidentally get bundled into this fix commit.📋 Elicit — Requirements Engineer
Observations
The acceptance criteria are precise and testable, which is exactly right for a bug fix issue:
main" — binary pass/fail, measurable.state_referenced_locallyinTranscriptionEditView" — observable via build output.The issue correctly identifies three distinct failures and attributes each to a specific technical cause. This level of pre-diagnosis in the issue body is excellent — it reduces the implementation ambiguity to near zero.
One implicit acceptance criterion is missing: there is no criterion requiring that the existing passing tests in
TranscriptionEditView.svelte.spec.tscontinue to pass after the fix. Given that thementionedPersonsfix touchesTranscriptionBlock.svelte(which affects the entire{#each}render path), a regression in the other 30+ tests in the spec file is possible. The DoD should explicitly include: "no previously-passing tests in the same spec files become red."The related PR #422 context is clear. It is sufficient to note that the failures are pre-existing and unrelated to that PR's intent. No additional scoping is needed.
Scope boundary: The issue is scoped to three specific test failures and one Svelte warning. The inline comment in
PersonMentionEditor.sveltereferences an open ADR ("Markus #5616") and a separate issues ("Nora #5618 #3"). These are explicitly out of scope for this issue and should remain tracked separately.Recommendations
Add one acceptance criterion: "All previously-passing tests in
TranscriptionEditView.svelte.spec.tsandpage.svelte.spec.tsremain green" — this guards against regressions introduced by the fix.The issue title is accurate —
fix(test): resolve pre-existing TranscriptionEditView and Richtlinien test failurescorrectly signals that this is a test-fix commit, not a product feature or behavior change. Keep it.Labels look correct:
bug,test,P3-later. Thetestlabel correctly signals this is a test-suite fix, not a product behavior change. TheP3-lateris reasonable for user-facing priority, though as Tobias notes, CI health makes it operationally higher priority.No new i18n keys, no new error codes, no new routes are implied by this fix. The i18n and error-handling checklists from
CLAUDE.mddo not apply here.🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist
Observations
The Wikipedia link in the Richtlinien page is correctly implemented from an a11y standpoint. The pattern — visible text
{m.richtlinien_wiki_link()}, iconaria-hidden="true", andaria-label="{m.richtlinien_wiki_link()} — {m.common_opens_new_tab()}"— is the right approach. Screen reader users hear the full label including the new-tab notification. Sighted users see the external link icon as the visual cue. This is WCAG 2.1 SC 2.4.4 (Link Purpose) compliant.The failing test is asserting the wrong thing, not the wrong implementation. The component is correct; the assertion should check
aria-label, nottextContent. Fixing the test preserves the correct a11y pattern without changing the component.One minor concern in the Richtlinien page: The Wikipedia info card uses
border-brand-sandas a border class (line 61 of+page.svelte). Based on the brand token system,border-brand-sandapplies a border in the sand palette color. This should be verified against the project's Tailwind v4 token configuration — ifborder-brand-sandis not a registered utility inlayout.css, it would silently produce no border. The canonical card pattern usesborder-line. This is a separate cosmetic concern, not part of this issue's scope.The
TranscriptionBlockcomponent's TipTap editor (PersonMentionEditor) correctly usesrole="textbox"andaria-multiline="true"on the contenteditable div. Thearia-labelfromm.transcription_editor_aria_label()gives screen reader users context for the field. The blur-flush test failure is a test-side DOM timing issue, not a component accessibility problem.Touch target compliance: The
TranscriptionBlockbuttons (delete, review toggle, move up/down) useh-7 w-7(28px) for the move arrows and similar small sizes for the action buttons. The project standard is 44px minimum. This is a pre-existing issue separate from this bug fix, and should be addressed in a dedicated accessibility issue rather than blocking this fix.Recommendations
Keep the
aria-labelpattern on the Wikipedia link exactly as-is. Fix only the test assertion. The component's a11y implementation is correct.After fixing, manually verify that a screen reader (VoiceOver or NVDA) announces "Wikipedia article on Kurrent — opens in new tab" (or the German equivalent) when the link receives focus. The
aria-labeloverrides the visible text for assistive technology, so the full string including the "öffnet in neuem Tab" annotation should be announced.Create a follow-up issue for the touch target sizes on the
TranscriptionBlockaction buttons — they are below the 44px WCAG 2.2 minimum for the mobile (60+) audience this project serves. This is worth a dedicated fix.Open Decisions
sr-onlyspan vsaria-labelfor new-tab annotation: Both patterns are valid. The currentaria-labelapproach avoids duplicate text (sighted users would hear "Wikipedia Wikipedia — opens in new tab" if asr-onlyspan were inside the link). Thearia-labelapproach is the right call here — but if the project has a sharedExternalLinkcomponent planned, the decision should be standardized in that component's design.Decision Queue
Two open decisions surfaced across the persona reviews. Both are out of scope for this fix but need a tracked home.
Theme 1 — ADR: Client-side fetch exception in Tiptap suggestion plugin
Raised by: Markus (architect)
PersonMentionEditor.sveltemakes a client-sidefetch('/api/persons?q=...')call on every@keystroke in the suggestion plugin. This is a documented exception to the project's SSR-first rule. The inline comment (line 127–139) already references "Markus #5616: an ADR will formalise this" — but that ADR has not been written.Decision needed: Should the ADR be created as part of this fix, or tracked as a separate follow-up issue? The exception is justified (routing typeahead queries through SSR would make the dropdown unusable), so the ADR content is already known — this is purely a documentation timing question.
Theme 2 — A11y follow-up:
TranscriptionBlocktouch target sizesRaised by: Leonie (UI/UX)
The move-up/move-down arrow buttons in
TranscriptionBlock.svelteuseh-7 w-7(28px), and the review toggle and delete buttons are similarly sized. The WCAG 2.2 minimum is 44px, which the project standard also requires. This affects the 60+ transcriber audience on mobile/tablet.Decision needed: Create a dedicated a11y issue for
TranscriptionBlockbutton sizing, or fold it into the next transcription panel maintenance issue?Theme 3 —
sr-onlyspan vsaria-labelfor external link new-tab annotationRaised by: Leonie (UI/UX)
The current pattern on the Wikipedia link (
aria-labelcontaining the full text including "öffnet in neuem Tab") is correct. If a sharedExternalLinkcomponent is planned, the pattern should be standardized there.Decision needed: Is a shared
ExternalLinkcomponent planned? If yes, should this pattern be codified now? If no, the existing per-componentaria-labelpattern is sufficient and requires no further action.Resolution
All three failing tests are now green on
main. The fixes landed in earlier commits as part of broader test-suite maintenance:flush on blur+mentionedPersonsdebounce tests — fixed incdb54c75(fix(tests): fix 2 more pre-existing vitest-browser spec failures): switchedtextarea.fill()+ fake timers touserEvent.type()+vi.waitForfor the mention test; usedpage.getByRole('textbox').first()locator instead of rawdocument.querySelectorfor the blur dispatch.6ab7abb9(fix(tests): fix 3 pre-existing vitest-browser spec failures): the test already checkslink.getAttribute('aria-label')(nottextContent), matching the component's correctaria-labelpattern.No new
state_referenced_locallywarnings were introduced — the two existing warnings inTranscriptionEditView.svelte:77(foronSaveBlockanddocumentIdpassed tocreateBlockAutoSave) are pre-existing and were not touched by any of these fixes.Closing as resolved.