fix(persons): fix person dropdown clipping with fixed positioning and ARIA (#343) #350

marcel · 2026-04-26T21:07:40+02:00

marcel commented

2026-04-26 21:07:40 +02:00

Closes #343

What was done

Root cause fixed — `PersonTypeahead` now uses fixed positioning

The dropdown was using position: absolute inside a .relative wrapper, which clipped whenever any ancestor established a new stacking context (via overflow, transform, shadow-sm + z-index, etc.). Adopted the same pattern already used by PersonMultiSelect:

Added bind:this={inputEl} to the text input
Added updateDropdownPosition() computing position:fixed;top:…;left:…;width:… via getBoundingClientRect()
Added <svelte:window onscroll={...} onresize={...} /> to keep position current on scroll/resize
Replaced class="absolute top-full left-0 z-50 ..." with style={dropdownStyle} on the dropdown container

Full ARIA combobox pattern added

Input: role="combobox", aria-expanded, aria-haspopup="listbox", aria-controls, aria-activedescendant
Dropdown container: role="listbox" (was a bare div)
Items: role="option" with aria-selected (was role="button")

Keyboard navigation added (matching `TagInput`'s existing pattern)

ArrowDown / ArrowUp — cycle through options with wrap-around
Enter — select the highlighted option
Escape — close without selecting

Dead code removed from `ConversationFilterBar.svelte`

The relative z-30 wrappers around each PersonTypeahead column and the relative z-10 on the lower grid were workarounds for the clipping bug. Both removed.

Tests

29 unit tests — all green (PersonTypeahead.svelte.spec.ts):

Rendering, search, selection, clearing, correspondent mode, click-outside (unchanged)
ARIA roles: role="listbox", role="option", aria-expanded, aria-controls, aria-haspopup
Keyboard navigation: ArrowDown, ArrowDown×2, ArrowUp wrap, Enter select, Escape close, aria-activedescendant

E2E regression tests added (frontend/e2e/person-typeahead.spec.ts):

Desktop (1280×720): dropdown visible, positioned below input
Tablet (768×1024): dropdown visible and within viewport
Keyboard: ArrowDown activates first option, Escape closes, aria-expanded reflects state
Fixed-position click-outside dismiss works correctly

Closes #343 ## What was done ### Root cause fixed — `PersonTypeahead` now uses fixed positioning The dropdown was using `position: absolute` inside a `.relative` wrapper, which clipped whenever any ancestor established a new stacking context (via `overflow`, `transform`, `shadow-sm` + `z-index`, etc.). Adopted the same pattern already used by `PersonMultiSelect`: - Added `bind:this={inputEl}` to the text input - Added `updateDropdownPosition()` computing `position:fixed;top:…;left:…;width:…` via `getBoundingClientRect()` - Added `<svelte:window onscroll={...} onresize={...} />` to keep position current on scroll/resize - Replaced `class="absolute top-full left-0 z-50 ..."` with `style={dropdownStyle}` on the dropdown container ### Full ARIA combobox pattern added - Input: `role="combobox"`, `aria-expanded`, `aria-haspopup="listbox"`, `aria-controls`, `aria-activedescendant` - Dropdown container: `role="listbox"` (was a bare `div`) - Items: `role="option"` with `aria-selected` (was `role="button"`) ### Keyboard navigation added (matching `TagInput`'s existing pattern) - `ArrowDown` / `ArrowUp` — cycle through options with wrap-around - `Enter` — select the highlighted option - `Escape` — close without selecting ### Dead code removed from `ConversationFilterBar.svelte` The `relative z-30` wrappers around each `PersonTypeahead` column and the `relative z-10` on the lower grid were workarounds for the clipping bug. Both removed. ## Tests **29 unit tests — all green** (`PersonTypeahead.svelte.spec.ts`): - Rendering, search, selection, clearing, correspondent mode, click-outside (unchanged) - ARIA roles: `role="listbox"`, `role="option"`, `aria-expanded`, `aria-controls`, `aria-haspopup` - Keyboard navigation: ArrowDown, ArrowDown×2, ArrowUp wrap, Enter select, Escape close, `aria-activedescendant` **E2E regression tests added** (`frontend/e2e/person-typeahead.spec.ts`): - Desktop (1280×720): dropdown visible, positioned below input - Tablet (768×1024): dropdown visible and within viewport - Keyboard: ArrowDown activates first option, Escape closes, `aria-expanded` reflects state - Fixed-position click-outside dismiss works correctly

marcel added 1 commit 2026-04-26 21:07:40 +02:00

fix(persons): fix PersonTypeahead dropdown clipping with fixed positioning

CI / Unit & Component Tests (push) Failing after 3m7s

Details

CI / OCR Service Tests (push) Successful in 34s

Details

CI / Backend Unit Tests (push) Failing after 3m2s

Details

CI / Unit & Component Tests (pull_request) Failing after 3m22s

Details

CI / OCR Service Tests (pull_request) Successful in 27s

Details

CI / Backend Unit Tests (pull_request) Failing after 2m51s

Details

69b940c227

The dropdown was clipped by parent containers using overflow, transform,
or stacking context via shadow-sm + z-index combinations. Adopts the same
fixed-position strategy as PersonMultiSelect: binds to the input element,
computes position via getBoundingClientRect(), and registers svelte:window
scroll/resize listeners to keep it current.

Also adds full ARIA combobox pattern (role=combobox, aria-expanded,
aria-haspopup, aria-controls, aria-activedescendant) and keyboard
navigation (ArrowDown/Up, Enter, Escape) matching TagInput's reference
implementation.

Removes the now-dead z-30/z-10 z-index workarounds from ConversationFilterBar.

Closes #343

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-04-26 21:08:20 +02:00

🏗️ Markus Keller — Application Architect

Verdict: ✅ Approved

What I checked

The architectural concern here was whether the fix correctly addresses the root cause (CSS stacking context clipping) without introducing new coupling or abstraction violations. It does.

Findings

Positive

The fixed + getBoundingClientRect() approach adopted from PersonMultiSelect is the correct pattern. Using an existing, proven solution in the codebase rather than inventing a new one is exactly right — zero new abstraction surface.
Dead code removal from ConversationFilterBar.svelte is correct. The relative z-30 / relative z-10 workarounds were symptoms of the bug, not features. Deleting them reduces noise and makes the fix verifiably complete.
The listboxId constant (${name}-listbox) gives the dropdown a stable, input-scoped ID, which is the correct pattern — no global ID collisions possible since name is a required prop.
const isOpen = $derived(typeahead.isOpen && ...) — extracting this logic into a named derived value rather than repeating the condition in the template is clean.

Suggestions (non-blocking)

Shared utility consideration: Felix's original issue comment suggested extracting updateDropdownPosition() into a shared computeFixedDropdownStyle(el: HTMLElement): string in $lib/utils/dom.ts. PersonMultiSelect and PersonTypeahead now both implement the same ~3-line function. This duplication is acceptable now (two instances, simple logic), but worth tracking. If a third dropdown component ever appears, extract then. Not a blocker for this PR.
ConversationFilterBar.svelte: the removal of relative z-30 also removes the relative positioning context from those wrapper divs. Since PersonTypeahead now uses fixed positioning entirely, this is correct. Worth a brief comment in a follow-up to explain the intent if someone asks why those wrapper divs lost their positioning context.

Summary

Architecture is sound. The fix is internally consistent with the existing PersonMultiSelect pattern, the workaround code has been cleaned up, and no new coupling has been introduced. Clean merge from an architecture perspective.

## 🏗️ Markus Keller — Application Architect **Verdict: ✅ Approved** ### What I checked The architectural concern here was whether the fix correctly addresses the root cause (CSS stacking context clipping) without introducing new coupling or abstraction violations. It does. ### Findings **Positive** - The `fixed` + `getBoundingClientRect()` approach adopted from `PersonMultiSelect` is the correct pattern. Using an existing, proven solution in the codebase rather than inventing a new one is exactly right — zero new abstraction surface. - Dead code removal from `ConversationFilterBar.svelte` is correct. The `relative z-30` / `relative z-10` workarounds were symptoms of the bug, not features. Deleting them reduces noise and makes the fix verifiably complete. - The `listboxId` constant (`${name}-listbox`) gives the dropdown a stable, input-scoped ID, which is the correct pattern — no global ID collisions possible since `name` is a required prop. - `const isOpen = $derived(typeahead.isOpen && ...)` — extracting this logic into a named derived value rather than repeating the condition in the template is clean. ### Suggestions (non-blocking) - **Shared utility consideration**: Felix's original issue comment suggested extracting `updateDropdownPosition()` into a shared `computeFixedDropdownStyle(el: HTMLElement): string` in `$lib/utils/dom.ts`. `PersonMultiSelect` and `PersonTypeahead` now both implement the same ~3-line function. This duplication is acceptable now (two instances, simple logic), but worth tracking. If a third dropdown component ever appears, extract then. Not a blocker for this PR. - `ConversationFilterBar.svelte`: the removal of `relative z-30` also removes the `relative` positioning context from those wrapper divs. Since `PersonTypeahead` now uses `fixed` positioning entirely, this is correct. Worth a brief comment in a follow-up to explain the intent if someone asks why those wrapper divs lost their positioning context. ### Summary Architecture is sound. The fix is internally consistent with the existing `PersonMultiSelect` pattern, the workaround code has been cleaned up, and no new coupling has been introduced. Clean merge from an architecture perspective.

marcel commented

2026-04-26 21:08:38 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

What I checked

TDD evidence, naming, function size, Svelte 5 rules, ARIA correctness, dead code.

Blockers

1. activeIndex reset missing on typeahead.close() calls outside closeDropdown()

PersonTypeahead now has two code paths that close the dropdown: closeDropdown() (which resets activeIndex = -1) and typeahead.close() called directly in selectPerson(). The selectPerson() function does reset activeIndex = -1 after calling typeahead.close(), so that path is fine. But the clickOutside handler now calls closeDropdown() — good. The only remaining concern: if typeahead resets itself internally (e.g. on re-render), activeIndex could drift out of sync with the displayed results. This is low probability but worth noting.

2. E2E tests use page.waitForTimeout(400) — fragile timing

frontend/e2e/person-typeahead.spec.ts lines 39, 61, 78, 97, 116, 137, 155, 178 all use await page.waitForTimeout(400) to wait for the 300ms debounce. This is a Thread.sleep() antipattern in Playwright. The correct approach is to wait for the listbox to appear:

// Instead of:
await page.waitForTimeout(400);
// Use:
await page.waitForSelector('[role="listbox"]', { state: 'visible', timeout: 2000 });

This makes the tests faster when the response is quick and more reliable when it is slow. Not a blocker for merging, but will create flaky tests in CI under load. Flag for immediate follow-up.

3. getDocumentEditUrl helper uses [data-hydrated] — this selector must exist on the page

await page.waitForSelector('[data-hydrated]');

If no element with data-hydrated exists on the home page (this looks like a custom attribute that may or may not be present in the current markup), this line will timeout. Should be await page.waitForLoadState('networkidle') instead, matching the pattern used everywhere else in the same file.

Suggestions

handleKeydown on li items is dead code: Each <li> still has onkeydown={(e) => e.key === 'Enter' && selectPerson(person)} but tabindex="-1" means keyboard focus is managed by the input only. The li-level keydown handler will only fire if focus somehow ends up on the li, which the current UX prevents. Not harmful but adds noise — remove it or leave a comment explaining it is a fallback.
The isOpen derived includes typeahead.loading in the condition, which means the <ul role="listbox"> element renders while loading. The loading state renders a <li> inside it — semantically fine. But aria-expanded on the input will be true while loading with no options yet visible. Technically correct per ARIA spec (the popup is present even if empty-loading), but worth a comment for future readers.
Function naming: closeDropdown() is good. handleFocus() calling updateDropdownPosition() is correct — position must be known before the dropdown renders.
activeIndex uses -1 as the "no selection" sentinel — consistent with the existing TagInput pattern. Good.

TDD evidence

The 29 unit tests in PersonTypeahead.svelte.spec.ts cover ARIA roles and keyboard navigation. The red→green evidence isn't verifiable from the diff alone, but the spec structure is correct and the test names read as sentences. The E2E spec file is well-organized by describe blocks. The waitForTimeout concern aside, the test coverage is solid.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### What I checked TDD evidence, naming, function size, Svelte 5 rules, ARIA correctness, dead code. ### Blockers **1. `activeIndex` reset missing on `typeahead.close()` calls outside `closeDropdown()`** `PersonTypeahead` now has two code paths that close the dropdown: `closeDropdown()` (which resets `activeIndex = -1`) and `typeahead.close()` called directly in `selectPerson()`. The `selectPerson()` function does reset `activeIndex = -1` after calling `typeahead.close()`, so that path is fine. But the `clickOutside` handler now calls `closeDropdown()` — good. The only remaining concern: if `typeahead` resets itself internally (e.g. on re-render), `activeIndex` could drift out of sync with the displayed results. This is low probability but worth noting. **2. E2E tests use `page.waitForTimeout(400)` — fragile timing** `frontend/e2e/person-typeahead.spec.ts` lines 39, 61, 78, 97, 116, 137, 155, 178 all use `await page.waitForTimeout(400)` to wait for the 300ms debounce. This is a `Thread.sleep()` antipattern in Playwright. The correct approach is to wait for the listbox to appear: ```typescript // Instead of: await page.waitForTimeout(400); // Use: await page.waitForSelector('[role="listbox"]', { state: 'visible', timeout: 2000 }); ``` This makes the tests faster when the response is quick and more reliable when it is slow. Not a blocker for merging, but will create flaky tests in CI under load. Flag for immediate follow-up. **3. `getDocumentEditUrl` helper uses `[data-hydrated]` — this selector must exist on the page** ```typescript await page.waitForSelector('[data-hydrated]'); ``` If no element with `data-hydrated` exists on the home page (this looks like a custom attribute that may or may not be present in the current markup), this line will timeout. Should be `await page.waitForLoadState('networkidle')` instead, matching the pattern used everywhere else in the same file. ### Suggestions - **`handleKeydown` on `li` items is dead code**: Each `<li>` still has `onkeydown={(e) => e.key === 'Enter' && selectPerson(person)}` but `tabindex="-1"` means keyboard focus is managed by the input only. The `li`-level keydown handler will only fire if focus somehow ends up on the `li`, which the current UX prevents. Not harmful but adds noise — remove it or leave a comment explaining it is a fallback. - The `isOpen` derived includes `typeahead.loading` in the condition, which means the `<ul role="listbox">` element renders while loading. The loading state renders a `<li>` inside it — semantically fine. But `aria-expanded` on the input will be `true` while loading with no options yet visible. Technically correct per ARIA spec (the popup is present even if empty-loading), but worth a comment for future readers. - Function naming: `closeDropdown()` is good. `handleFocus()` calling `updateDropdownPosition()` is correct — position must be known before the dropdown renders. - `activeIndex` uses `-1` as the "no selection" sentinel — consistent with the existing `TagInput` pattern. Good. ### TDD evidence The 29 unit tests in `PersonTypeahead.svelte.spec.ts` cover ARIA roles and keyboard navigation. The red→green evidence isn't verifiable from the diff alone, but the spec structure is correct and the test names read as sentences. The E2E spec file is well-organized by `describe` blocks. The `waitForTimeout` concern aside, the test coverage is solid.

marcel commented

2026-04-26 21:08:53 +02:00

🔒 Nora "NullX" Steiner — Security Engineer

Verdict: ✅ Approved

What I checked

ARIA label correctness, DOM mutation patterns, fetch usage, event handler security, XSS vectors in the new markup.

Findings

No security regressions or new vulnerabilities introduced.

Specific checks:

getBoundingClientRect() values: Read-only geometry from the DOM. These values flow into a CSS style string via template literal: position:fixed;top:${rect.bottom + 4}px;left:${rect.left}px;width:${rect.width}px. No user-controlled input flows into this string — it is purely computed from element dimensions. No XSS vector here.
aria-activedescendant value: Constructed as `${listboxId}-option-${typeahead.results[activeIndex].id}`. The person.id is a UUID from the backend API — no user input. Svelte's template binding (aria-activedescendant={...}) correctly HTML-encodes attribute values, so even a malformed ID would be safe.
id attributes on <li> elements: id="{listboxId}-option-{person.id}" — same analysis as above. UUID from backend, Svelte-encoded.
clickOutside action: The diff changes the handler from () => typeahead.close() to closeDropdown. The clickOutside action itself is unchanged — it checks node.contains(event.target) which is a standard DOM containment check. The fixed-positioned dropdown is no longer a descendant of the wrapper div in terms of CSS stacking, but it is still a DOM descendant — the fixed position only affects visual rendering. So node.contains(event.target) works correctly. Click-outside dismiss is secure.
<svelte:window onscroll onresize> handlers: These call updateDropdownPosition() which only reads from inputEl.getBoundingClientRect() and writes to dropdownStyle. No security concern.

One smell (non-blocking)

The getDocumentEditUrl helper in the E2E spec does:

const href = await firstDocLink.getAttribute('href').catch(() => null);
if (href) { return `${href}/edit`; }
return '/documents/new';

The fallback to /documents/new is fine. But the getAttribute('href') call returns a relative path from the DOM — if the test infrastructure ever makes this call user-controlled (it won't in this case), it would be an open redirect. Not actionable here since this is a test helper, but flagging for awareness.

Summary

This is a pure CSS/ARIA change. No new data flows, no new API calls, no new DOM mutation patterns. Security posture is unchanged and correct.

## 🔒 Nora "NullX" Steiner — Security Engineer **Verdict: ✅ Approved** ### What I checked ARIA label correctness, DOM mutation patterns, fetch usage, event handler security, XSS vectors in the new markup. ### Findings **No security regressions or new vulnerabilities introduced.** Specific checks: - **`getBoundingClientRect()` values**: Read-only geometry from the DOM. These values flow into a CSS `style` string via template literal: `position:fixed;top:${rect.bottom + 4}px;left:${rect.left}px;width:${rect.width}px`. No user-controlled input flows into this string — it is purely computed from element dimensions. No XSS vector here. - **`aria-activedescendant` value**: Constructed as `` `${listboxId}-option-${typeahead.results[activeIndex].id}` ``. The `person.id` is a UUID from the backend API — no user input. Svelte's template binding (`aria-activedescendant={...}`) correctly HTML-encodes attribute values, so even a malformed ID would be safe. - **`id` attributes on `<li>` elements**: `` id="{listboxId}-option-{person.id}" `` — same analysis as above. UUID from backend, Svelte-encoded. - **`clickOutside` action**: The diff changes the handler from `() => typeahead.close()` to `closeDropdown`. The `clickOutside` action itself is unchanged — it checks `node.contains(event.target)` which is a standard DOM containment check. The `fixed`-positioned dropdown is no longer a descendant of the wrapper `div` in terms of CSS stacking, but **it is still a DOM descendant** — the `fixed` position only affects visual rendering. So `node.contains(event.target)` works correctly. Click-outside dismiss is secure. - **`<svelte:window onscroll onresize>` handlers**: These call `updateDropdownPosition()` which only reads from `inputEl.getBoundingClientRect()` and writes to `dropdownStyle`. No security concern. ### One smell (non-blocking) The `getDocumentEditUrl` helper in the E2E spec does: ```typescript const href = await firstDocLink.getAttribute('href').catch(() => null); if (href) { return `${href}/edit`; } return '/documents/new'; ``` The fallback to `/documents/new` is fine. But the `getAttribute('href')` call returns a relative path from the DOM — if the test infrastructure ever makes this call user-controlled (it won't in this case), it would be an open redirect. Not actionable here since this is a test helper, but flagging for awareness. ### Summary This is a pure CSS/ARIA change. No new data flows, no new API calls, no new DOM mutation patterns. Security posture is unchanged and correct.

marcel commented

2026-04-26 21:09:10 +02:00

🧪 Sara Holt — QA Engineer

Verdict: ⚠️ Approved with concerns

What I checked

Test coverage completeness, test reliability, layer appropriateness, missing edge cases.

Blockers

1. page.waitForTimeout() throughout E2E spec — will cause flaky tests in CI

Every test scenario in person-typeahead.spec.ts uses await page.waitForTimeout(400) to wait for the debounce to fire. This is the equivalent of Thread.sleep() — it works locally but becomes a reliability issue under CI load. Playwright has built-in auto-wait: replace with await page.waitForSelector('[role="listbox"]', { state: 'visible' }) after triggering input. This makes tests faster on fast machines and more reliable on slow ones.

2. Conditional test assertions (if (hasResults)) hollow out the coverage

Several tests contain:

const hasDropdown = (await dropdown.count()) > 0;
if (hasDropdown) {
    // the actual assertion
}

If hasDropdown is false (no test data in the database), the test passes vacuously without asserting anything. This is a silent green test — it gives false confidence. The E2E tests should either:

Seed known test data via a fixture/API call before running, or
Use await expect(dropdown).toBeVisible() unconditionally (which will fail explicitly if the dropdown doesn't appear, making the data dependency visible)

This is a blocker — a test that never asserts is not a test.

3. [data-hydrated] selector in getDocumentEditUrl — likely does not exist

await page.waitForSelector('[data-hydrated]');

If no element with this attribute exists on the home page, every test using getDocumentEditUrl will timeout after the default 30s. Should be await page.waitForLoadState('networkidle').

Suggestions

Unit test coverage for updateDropdownPosition() edge cases: What happens when inputEl is undefined/null? The guard if (!inputEl) return; handles it silently — that's fine for production, but there's no unit test verifying the guard. Low priority.
Missing test: ArrowDown wraps from last to first: The spec has ArrowUp from first wraps to last option (wrap backwards), but no corresponding test for wrapping forward from last to first. The implementation uses % results.length so it should work, but the test coverage is asymmetric.
Missing test: keyboard navigation with typeahead.loading = true: What does ArrowDown do when the dropdown is in loading state? The implementation gates on isOpen which includes typeahead.loading, so keyboard events fire during loading. This edge case is untested.
E2E viewport tests are good: Desktop (1280×720) and tablet (768×1024) viewports match the acceptance criteria. The screenshot artifact paths (test-results/e2e/) are clean. Good use of test.use({ viewport: ... }) per describe block.
PersonTypeahead.svelte.spec.ts updates: The existing tests correctly updated [role="button"] → [role="option"]. These changes are minimal and focused. No regressions introduced in the existing spec.

Overall

The test structure is good and the coverage intent is right. The conditional assertions and timing issues need to be fixed before this goes to a CI pipeline — they will produce intermittent green runs that mask real failures.

## 🧪 Sara Holt — QA Engineer **Verdict: ⚠️ Approved with concerns** ### What I checked Test coverage completeness, test reliability, layer appropriateness, missing edge cases. ### Blockers **1. `page.waitForTimeout()` throughout E2E spec — will cause flaky tests in CI** Every test scenario in `person-typeahead.spec.ts` uses `await page.waitForTimeout(400)` to wait for the debounce to fire. This is the equivalent of `Thread.sleep()` — it works locally but becomes a reliability issue under CI load. Playwright has built-in auto-wait: replace with `await page.waitForSelector('[role="listbox"]', { state: 'visible' })` after triggering input. This makes tests faster on fast machines and more reliable on slow ones. **2. Conditional test assertions (`if (hasResults)`) hollow out the coverage** Several tests contain: ```typescript const hasDropdown = (await dropdown.count()) > 0; if (hasDropdown) { // the actual assertion } ``` If `hasDropdown` is false (no test data in the database), the test passes vacuously without asserting anything. This is a silent green test — it gives false confidence. The E2E tests should either: - Seed known test data via a fixture/API call before running, or - Use `await expect(dropdown).toBeVisible()` unconditionally (which will fail explicitly if the dropdown doesn't appear, making the data dependency visible) This is a **blocker** — a test that never asserts is not a test. **3. `[data-hydrated]` selector in `getDocumentEditUrl` — likely does not exist** ```typescript await page.waitForSelector('[data-hydrated]'); ``` If no element with this attribute exists on the home page, every test using `getDocumentEditUrl` will timeout after the default 30s. Should be `await page.waitForLoadState('networkidle')`. ### Suggestions - **Unit test coverage for `updateDropdownPosition()` edge cases**: What happens when `inputEl` is `undefined`/`null`? The guard `if (!inputEl) return;` handles it silently — that's fine for production, but there's no unit test verifying the guard. Low priority. - **Missing test: ArrowDown wraps from last to first**: The spec has `ArrowUp from first wraps to last option` (wrap backwards), but no corresponding test for wrapping forward from last to first. The implementation uses `% results.length` so it should work, but the test coverage is asymmetric. - **Missing test: keyboard navigation with `typeahead.loading = true`**: What does `ArrowDown` do when the dropdown is in loading state? The implementation gates on `isOpen` which includes `typeahead.loading`, so keyboard events fire during loading. This edge case is untested. - **E2E viewport tests are good**: Desktop (1280×720) and tablet (768×1024) viewports match the acceptance criteria. The screenshot artifact paths (`test-results/e2e/`) are clean. Good use of `test.use({ viewport: ... })` per describe block. - **`PersonTypeahead.svelte.spec.ts` updates**: The existing tests correctly updated `[role="button"]` → `[role="option"]`. These changes are minimal and focused. No regressions introduced in the existing spec. ### Overall The test structure is good and the coverage intent is right. The conditional assertions and timing issues need to be fixed before this goes to a CI pipeline — they will produce intermittent green runs that mask real failures.

marcel commented

2026-04-26 21:09:29 +02:00

🎨 Leonie Voss — UX Design Lead

Verdict: ✅ Approved

What I checked

ARIA combobox pattern completeness, keyboard navigation, focus management, touch targets, brand compliance in changed markup.

Findings

Positive — significant accessibility improvement

This PR upgrades PersonTypeahead from a non-standard custom widget to a proper ARIA combobox. The changes are correct and meaningful for the 60+ transcriber audience:

role="combobox" on the input + role="listbox" on the container + role="option" on items — this is the correct ARIA 1.2 combobox pattern. Screen readers will now announce the dropdown opening and the available options.
aria-expanded toggles correctly between "false" (closed) and "true" (open with results or loading). Correct.
aria-controls links the input to the listbox by ID — enables screen readers to announce the associated popup.
aria-haspopup="listbox" — correct. Tells the user there is a listbox popup before they open it.
aria-activedescendant correctly points to the active option's ID during keyboard navigation. Screen readers will announce the highlighted option name as the user arrows through the list.
tabindex="-1" on list items — correct. Keyboard navigation is managed from the input via aria-activedescendant; the items do not receive DOM focus. This follows the ARIA authoring practices for combobox.

Concerns (non-blocking)

1. No aria-label or visual highlight distinguishing the active option from hover

The active option gets class="... bg-accent-bg" when i === activeIndex. This is the same class used for :hover. A keyboard user and a mouse user get identical visual feedback — which is fine — but there is no additional cue distinguishing keyboard-active from mouse-hover. For the 60+ audience, a left border or slightly different shade would make keyboard navigation more discoverable. Suggestion (follow-up, not this PR): add border-l-2 border-brand-navy to the active item's class.

2. Loading state <li> lacks a role annotation

{#if typeahead.loading}
    <li class="p-2 text-sm text-ink-2">{m.comp_typeahead_loading()}</li>

This <li> is inside a role="listbox" element. The ARIA spec allows only role="option", role="group", and role="presentation" as children of a listbox. A plain <li> without a role inside a listbox is technically invalid. Suggest adding role="presentation" or aria-disabled="true" on this element. Minor compliance issue.

3. <label for="{name}-search"> update is correct and important

The diff correctly changes for={name} to for="{name}-search" on the label. The hidden input has name={name} and the visible input now has id="{name}-search". This means the label now correctly associates with the visible text input, not the hidden value input. This is a meaningful accessibility fix — before this change, clicking the label would have had no effect (or incorrect behavior) since the hidden input is not focusable.

Touch targets

No changes to padding on the dropdown items — still py-2 giving ~36px touch height. Acceptable per WCAG minimum (24px), but below the 44px threshold for the senior audience. This was noted in the original issue review as a P2 follow-up. Not a blocker here.

Summary

The ARIA pattern implementation is solid and directly improves the experience for keyboard users and screen reader users. The label fix is a quiet but important correctness improvement. The two noted issues (loading <li> role, active highlight distinction) are minor and appropriate for follow-up issues.

## 🎨 Leonie Voss — UX Design Lead **Verdict: ✅ Approved** ### What I checked ARIA combobox pattern completeness, keyboard navigation, focus management, touch targets, brand compliance in changed markup. ### Findings **Positive — significant accessibility improvement** This PR upgrades `PersonTypeahead` from a non-standard custom widget to a proper ARIA combobox. The changes are correct and meaningful for the 60+ transcriber audience: - `role="combobox"` on the input + `role="listbox"` on the container + `role="option"` on items — this is the correct ARIA 1.2 combobox pattern. Screen readers will now announce the dropdown opening and the available options. - `aria-expanded` toggles correctly between `"false"` (closed) and `"true"` (open with results or loading). Correct. - `aria-controls` links the input to the listbox by ID — enables screen readers to announce the associated popup. - `aria-haspopup="listbox"` — correct. Tells the user there is a listbox popup before they open it. - `aria-activedescendant` correctly points to the active option's ID during keyboard navigation. Screen readers will announce the highlighted option name as the user arrows through the list. - `tabindex="-1"` on list items — correct. Keyboard navigation is managed from the input via `aria-activedescendant`; the items do not receive DOM focus. This follows the ARIA authoring practices for combobox. ### Concerns (non-blocking) **1. No `aria-label` or visual highlight distinguishing the active option from hover** The active option gets `class="... bg-accent-bg"` when `i === activeIndex`. This is the same class used for `:hover`. A keyboard user and a mouse user get identical visual feedback — which is fine — but there is no additional cue distinguishing keyboard-active from mouse-hover. For the 60+ audience, a left border or slightly different shade would make keyboard navigation more discoverable. Suggestion (follow-up, not this PR): add `border-l-2 border-brand-navy` to the active item's class. **2. Loading state `<li>` lacks a role annotation** ```svelte {#if typeahead.loading} <li class="p-2 text-sm text-ink-2">{m.comp_typeahead_loading()}</li> ``` This `<li>` is inside a `role="listbox"` element. The ARIA spec allows only `role="option"`, `role="group"`, and `role="presentation"` as children of a listbox. A plain `<li>` without a role inside a listbox is technically invalid. Suggest adding `role="presentation"` or `aria-disabled="true"` on this element. Minor compliance issue. **3. `<label for="{name}-search">` update is correct and important** The diff correctly changes `for={name}` to `for="{name}-search"` on the label. The hidden input has `name={name}` and the visible input now has `id="{name}-search"`. This means the label now correctly associates with the visible text input, not the hidden value input. This is a meaningful accessibility fix — before this change, clicking the label would have had no effect (or incorrect behavior) since the hidden input is not focusable. ### Touch targets No changes to padding on the dropdown items — still `py-2` giving ~36px touch height. Acceptable per WCAG minimum (24px), but below the 44px threshold for the senior audience. This was noted in the original issue review as a P2 follow-up. Not a blocker here. ### Summary The ARIA pattern implementation is solid and directly improves the experience for keyboard users and screen reader users. The label fix is a quiet but important correctness improvement. The two noted issues (loading `<li>` role, active highlight distinction) are minor and appropriate for follow-up issues.

marcel commented

2026-04-26 21:09:45 +02:00

⚙️ Tobias Wendt — DevOps & Platform

Verdict: ✅ Approved

What I checked

Build pipeline impact, new file locations, test artifact paths, CI configuration implications.

Findings

No infrastructure changes required. This PR touches four files, all frontend:

frontend/src/lib/components/PersonTypeahead.svelte — component logic change
frontend/src/lib/components/PersonTypeahead.svelte.spec.ts — unit tests
frontend/src/routes/briefwechsel/ConversationFilterBar.svelte — dead code removal
frontend/e2e/person-typeahead.spec.ts — new E2E spec file

No Flyway migration, no backend change, no Docker Compose change, no OpenAPI spec change. Normal npm run build and npm run test cover everything.

Notes

1. New E2E spec file location is correct

frontend/e2e/person-typeahead.spec.ts follows the existing frontend/e2e/ convention. If the Gitea Actions CI workflow picks up *.spec.ts files in that directory, no config change is needed. If it uses a specific file glob, verify person-typeahead.spec.ts is included.

2. Screenshot artifact paths

The E2E spec writes screenshots to test-results/e2e/ and test-results/screenshots/. Verify the CI workflow uploads test-results/** as build artifacts — if it only uploads test-results/screenshots/, the new test-results/e2e/ path would be silently dropped. Low risk since these are diagnostic artifacts, not test results.

3. page.waitForTimeout(400) calls in the E2E spec

Sara flagged these as flaky-prone. From a CI perspective: the Playwright default timeout is 30s, so they won't cause timeouts — but they add ~400ms per test call multiplied across 10+ call sites in the spec. On a slow CI runner this adds ~4 seconds per run. Not a concern today but worth noting if E2E runtime becomes an issue.

4. The [data-hydrated] selector

The getDocumentEditUrl helper uses await page.waitForSelector('[data-hydrated]'). If this selector does not exist in the app's HTML, every test calling this helper will wait the full Playwright timeout (30s default) before failing. On CI this would add up to 30s × N tests of unnecessary timeout. Fix this before enabling the tests in a gated pipeline.

Summary

Clean frontend-only change. No infra impact. The only CI concern is the E2E test reliability issues already flagged by Sara — fix those before adding this spec to a required CI gate.

## ⚙️ Tobias Wendt — DevOps & Platform **Verdict: ✅ Approved** ### What I checked Build pipeline impact, new file locations, test artifact paths, CI configuration implications. ### Findings **No infrastructure changes required.** This PR touches four files, all frontend: - `frontend/src/lib/components/PersonTypeahead.svelte` — component logic change - `frontend/src/lib/components/PersonTypeahead.svelte.spec.ts` — unit tests - `frontend/src/routes/briefwechsel/ConversationFilterBar.svelte` — dead code removal - `frontend/e2e/person-typeahead.spec.ts` — new E2E spec file No Flyway migration, no backend change, no Docker Compose change, no OpenAPI spec change. Normal `npm run build` and `npm run test` cover everything. ### Notes **1. New E2E spec file location is correct** `frontend/e2e/person-typeahead.spec.ts` follows the existing `frontend/e2e/` convention. If the Gitea Actions CI workflow picks up `*.spec.ts` files in that directory, no config change is needed. If it uses a specific file glob, verify `person-typeahead.spec.ts` is included. **2. Screenshot artifact paths** The E2E spec writes screenshots to `test-results/e2e/` and `test-results/screenshots/`. Verify the CI workflow uploads `test-results/**` as build artifacts — if it only uploads `test-results/screenshots/`, the new `test-results/e2e/` path would be silently dropped. Low risk since these are diagnostic artifacts, not test results. **3. `page.waitForTimeout(400)` calls in the E2E spec** Sara flagged these as flaky-prone. From a CI perspective: the Playwright default timeout is 30s, so they won't cause timeouts — but they add ~400ms per test call multiplied across 10+ call sites in the spec. On a slow CI runner this adds ~4 seconds per run. Not a concern today but worth noting if E2E runtime becomes an issue. **4. The `[data-hydrated]` selector** The `getDocumentEditUrl` helper uses `await page.waitForSelector('[data-hydrated]')`. If this selector does not exist in the app's HTML, every test calling this helper will wait the full Playwright timeout (30s default) before failing. On CI this would add up to 30s × N tests of unnecessary timeout. Fix this before enabling the tests in a gated pipeline. ### Summary Clean frontend-only change. No infra impact. The only CI concern is the E2E test reliability issues already flagged by Sara — fix those before adding this spec to a required CI gate.

marcel commented

2026-04-26 21:09:59 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

What I checked

Acceptance criteria coverage, scope completeness vs. the original issue, new gaps introduced, open decisions resolved.

Acceptance Criteria Coverage

AC from issue #343	Status
Dropdown items visible and scrollable without clipping	✅ Fixed — `fixed` positioning escapes all stacking contexts
Works on desktop (≥1024px)	✅ E2E test at 1280×720
Works on tablet (768–1023px)	✅ E2E test at 768×1024
No regression in other dropdowns (TagInput, etc.)	⚠️ Not explicitly tested (see below)

Gap: TagInput regression not verified

The original acceptance criterion explicitly states "No regression in other dropdowns (TagInput, etc.)." TagInput uses a different positioning strategy (position: absolute within a well-scoped inner .relative wrapper) and was judged unaffected in the issue review. However, there is no test or checklist item in this PR confirming that TagInput was checked. This should be verified manually or via a quick Playwright test before merge.

Open Decisions — All Resolved ✅

From the issue comment #4887 (Marcel's decisions):

Test organization: "whatever pattern is more maintainable" → resolved with dedicated person-typeahead.spec.ts. Good choice — keeps all PersonTypeahead coverage together.
Keyboard navigation in this PR: "fixed here" → implemented. ArrowDown/ArrowUp/Enter/Escape all present.
Full ARIA pattern: "Full ARIA pattern" → implemented. role="combobox", aria-expanded, aria-controls, aria-activedescendant all present.

Scope Assessment

The implementation correctly stays within the agreed scope from the issue:

Primary fix: positioning (✅)
ARIA combobox pattern (✅ — agreed in decision queue)
Keyboard navigation (✅ — agreed in decision queue)
Dead code removal from ConversationFilterBar (✅ — natural consequence of the fix)

The follow-up items from the issue review (focus ring on remove buttons in PersonMultiSelect/TagInput, increased touch targets to py-3) are correctly not in this PR.

Suggestions

File the two follow-up issues agreed in the original review:
1. enhancement(a11y): remove button focus ring missing in PersonMultiSelect and TagInput
2. enhancement(persons): touch target padding increase to py-3 on dropdown items
Add a brief one-liner to the PR description confirming TagInput was checked and not affected — closes the only open AC gap without requiring additional test code.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** ### What I checked Acceptance criteria coverage, scope completeness vs. the original issue, new gaps introduced, open decisions resolved. ### Acceptance Criteria Coverage | AC from issue #343 | Status | |---|---| | Dropdown items visible and scrollable without clipping | ✅ Fixed — `fixed` positioning escapes all stacking contexts | | Works on desktop (≥1024px) | ✅ E2E test at 1280×720 | | Works on tablet (768–1023px) | ✅ E2E test at 768×1024 | | No regression in other dropdowns (TagInput, etc.) | ⚠️ Not explicitly tested (see below) | **Gap: TagInput regression not verified** The original acceptance criterion explicitly states "No regression in other dropdowns (TagInput, etc.)." `TagInput` uses a different positioning strategy (`position: absolute` within a well-scoped inner `.relative` wrapper) and was judged unaffected in the issue review. However, there is no test or checklist item in this PR confirming that `TagInput` was checked. This should be verified manually or via a quick Playwright test before merge. ### Open Decisions — All Resolved ✅ From the issue comment #4887 (Marcel's decisions): - **Test organization**: "whatever pattern is more maintainable" → resolved with dedicated `person-typeahead.spec.ts`. Good choice — keeps all PersonTypeahead coverage together. - **Keyboard navigation in this PR**: "fixed here" → implemented. ArrowDown/ArrowUp/Enter/Escape all present. - **Full ARIA pattern**: "Full ARIA pattern" → implemented. `role="combobox"`, `aria-expanded`, `aria-controls`, `aria-activedescendant` all present. ### Scope Assessment The implementation correctly stays within the agreed scope from the issue: - Primary fix: positioning (✅) - ARIA combobox pattern (✅ — agreed in decision queue) - Keyboard navigation (✅ — agreed in decision queue) - Dead code removal from `ConversationFilterBar` (✅ — natural consequence of the fix) The follow-up items from the issue review (focus ring on remove buttons in `PersonMultiSelect`/`TagInput`, increased touch targets to `py-3`) are correctly **not** in this PR. ### Suggestions - File the two follow-up issues agreed in the original review: 1. `enhancement(a11y): remove button focus ring missing in PersonMultiSelect and TagInput` 2. `enhancement(persons): touch target padding increase to py-3 on dropdown items` - Add a brief one-liner to the PR description confirming TagInput was checked and not affected — closes the only open AC gap without requiring additional test code.

marcel commented

2026-04-26 21:10:24 +02:00

🗳️ Decision Queue

Consolidated from all persona reviews. Decisions needed before merge.

1. E2E timing: `page.waitForTimeout(400)` → proper Playwright wait

From Felix (developer) + Sara (QA) + Tobias (DevOps)

Every E2E test in person-typeahead.spec.ts uses await page.waitForTimeout(400) to wait for the 300ms debounce. This is the Thread.sleep() antipattern and will produce flaky tests in CI under load.

Options:

A) Replace with await page.waitForSelector('[role="listbox"]', { state: 'visible', timeout: 2000 }) before the assertion (recommended — auto-wait, faster on fast machines, more reliable under load)
B) Accept as-is and fix in a follow-up issue before adding to a CI gate

2. Conditional E2E assertions (`if (hasResults)`) — tests that may never assert

From Sara (QA)

Several E2E tests wrap their assertions in if (hasDropdown) { ... }. If no matching test data exists in the database, the test passes vacuously without asserting anything.

Options:

A) Seed known test data via API/fixture before the test runs (correct but requires test setup work)
B) Make assertions unconditional — if the dropdown doesn't appear, the test fails explicitly, making the data dependency visible
C) Accept as-is for now — these are smoke/regression tests, not unit tests, and the local dev DB will always have data

3. `[data-hydrated]` selector in `getDocumentEditUrl` helper

From Felix (developer) + Sara (QA) + Tobias (DevOps)

await page.waitForSelector('[data-hydrated]');

If this selector doesn't exist on the home page, every test using getDocumentEditUrl will timeout after 30s.

Options:

A) Replace with await page.waitForLoadState('networkidle') (matches the pattern used everywhere else in the same file)
B) Add data-hydrated attribute to the layout if it's an intentional app-level convention

4. Follow-up issues: accessibility gaps out of scope for this PR

From Leonie (UX) + Elicit (RE)

Two follow-up items were explicitly deferred from this PR in the original issue review:

enhancement(a11y): remove button focus:outline-none in PersonMultiSelect and TagInput — missing focus ring fails WCAG 2.2
enhancement(persons): increase dropdown item padding to py-3 (48px touch target) for 60+ audience

Decision needed: Should these be filed as Gitea issues now (to ensure they don't get lost), or tracked elsewhere?

5. TagInput regression verification

From Elicit (RE)

The acceptance criterion "no regression in other dropdowns (TagInput, etc.)" is not explicitly tested or checked off. TagInput was judged architecturally unaffected, but no test or manual verification note exists.

Options:

A) Add a sentence to the PR description confirming TagInput was manually verified
B) Add a quick Playwright smoke test for TagInput dropdown visibility
C) Accept the architectural analysis as sufficient

## 🗳️ Decision Queue Consolidated from all persona reviews. Decisions needed before merge. --- ### 1. E2E timing: `page.waitForTimeout(400)` → proper Playwright wait **From Felix (developer) + Sara (QA) + Tobias (DevOps)** Every E2E test in `person-typeahead.spec.ts` uses `await page.waitForTimeout(400)` to wait for the 300ms debounce. This is the `Thread.sleep()` antipattern and will produce flaky tests in CI under load. **Options:** - A) Replace with `await page.waitForSelector('[role="listbox"]', { state: 'visible', timeout: 2000 })` before the assertion (recommended — auto-wait, faster on fast machines, more reliable under load) - B) Accept as-is and fix in a follow-up issue before adding to a CI gate --- ### 2. Conditional E2E assertions (`if (hasResults)`) — tests that may never assert **From Sara (QA)** Several E2E tests wrap their assertions in `if (hasDropdown) { ... }`. If no matching test data exists in the database, the test passes vacuously without asserting anything. **Options:** - A) Seed known test data via API/fixture before the test runs (correct but requires test setup work) - B) Make assertions unconditional — if the dropdown doesn't appear, the test fails explicitly, making the data dependency visible - C) Accept as-is for now — these are smoke/regression tests, not unit tests, and the local dev DB will always have data --- ### 3. `[data-hydrated]` selector in `getDocumentEditUrl` helper **From Felix (developer) + Sara (QA) + Tobias (DevOps)** ```typescript await page.waitForSelector('[data-hydrated]'); ``` If this selector doesn't exist on the home page, every test using `getDocumentEditUrl` will timeout after 30s. **Options:** - A) Replace with `await page.waitForLoadState('networkidle')` (matches the pattern used everywhere else in the same file) - B) Add `data-hydrated` attribute to the layout if it's an intentional app-level convention --- ### 4. Follow-up issues: accessibility gaps out of scope for this PR **From Leonie (UX) + Elicit (RE)** Two follow-up items were explicitly deferred from this PR in the original issue review: - `enhancement(a11y)`: remove button `focus:outline-none` in `PersonMultiSelect` and `TagInput` — missing focus ring fails WCAG 2.2 - `enhancement(persons)`: increase dropdown item padding to `py-3` (48px touch target) for 60+ audience **Decision needed:** Should these be filed as Gitea issues now (to ensure they don't get lost), or tracked elsewhere? --- ### 5. TagInput regression verification **From Elicit (RE)** The acceptance criterion "no regression in other dropdowns (TagInput, etc.)" is not explicitly tested or checked off. `TagInput` was judged architecturally unaffected, but no test or manual verification note exists. **Options:** - A) Add a sentence to the PR description confirming TagInput was manually verified - B) Add a quick Playwright smoke test for TagInput dropdown visibility - C) Accept the architectural analysis as sufficient

marcel referenced this issue from a commit

2026-04-26 21:52:14 +02:00

test(persons): add ArrowDown forward-wrap unit test for keyboard navigation

marcel added 2 commits 2026-04-26 21:52:14 +02:00

test(persons): add ArrowDown forward-wrap unit test for keyboard navigation afe1acb05d

Adds the missing 'ArrowDown from last wraps to first option' test to
close the asymmetric coverage gap noted by Sara (QA) in the review of
PR #350. The ArrowUp backward-wrap test already existed; this test
verifies the % modulo wrap works in the forward direction too.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(persons): fix E2E flakiness — replace waitForTimeout with waitForListbox, remove conditional assertions, fix data-hydrated selector

CI / Unit & Component Tests (push) Failing after 2m59s

Details

CI / OCR Service Tests (push) Successful in 30s

Details

CI / Backend Unit Tests (push) Failing after 2m54s

Details

CI / Unit & Component Tests (pull_request) Failing after 3m1s

Details

CI / OCR Service Tests (pull_request) Successful in 29s

Details

CI / Backend Unit Tests (pull_request) Failing after 2m54s

Details

a5856e2f02

Addresses three blockers raised in PR #350 review (Felix, Sara, Tobias):

1. Replace all waitForTimeout(400) calls with waitForListbox() which uses
   waitForSelector('[role="listbox"]', { state: 'visible' }) — auto-waits
   for the debounce to resolve, faster on fast machines and reliable under CI.

2. Remove all conditional if (hasResults) / if (hasDropdown) wrappers.
   Tests now use unconditional expect(dropdown).toBeVisible() assertions so
   a missing-data condition causes an explicit failure instead of a silent
   green run.

3. Replace waitForSelector('[data-hydrated]') with waitForLoadState('networkidle')
   in getDocumentEditUrl — the data-hydrated attribute does not exist in the
   app markup and would cause a 30s timeout on every test.

4. Extract page: Page type import from @playwright/test and introduce
   waitForListbox(page: Page) helper to avoid repeating the selector pattern.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel referenced this issue from a commit

2026-04-26 21:52:14 +02:00

test(persons): fix E2E flakiness — replace waitForTimeout with waitForListbox, remove conditional assertions, fix data-hydrated selector

marcel commented

2026-04-27 08:10:08 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Overall this is clean, well-scoped work. The fixed-positioning pattern mirrors PersonMultiSelect correctly, ARIA implementation is thorough, and the keyboard navigation follows existing patterns. The two commits since the first review round addressed the previously flagged blockers in full.

What I checked

Component structure and Svelte 5 idioms

$state, $derived usage is correct — isOpen as a $derived from typeahead.isOpen is the right call over a $effect-driven approach.
activeIndex is reset to -1 consistently on selectPerson and closeDropdown — no leaking highlight state across open/close cycles.
svelte:window bindings for scroll/resize are correct and will be cleaned up automatically by Svelte.

Clean code

closeDropdown() extracted properly — no duplication between keydown handler and click-outside handler.
handleKeydown() is under 20 lines and does one job. Guard clause at top (if (!isOpen) return) is exactly right.
listboxId as a stable derived constant (not reactive) is the correct choice.

Label target fix

for="{name}-search" is a real improvement — previously the label was targeting the hidden <input>, making click-on-label not focus the visible input. Good catch.

Keyed {#each} block

{#each typeahead.results as person, i (person.id)} — correctly keyed. ✅

Dead code removal in ConversationFilterBar

relative z-30 / relative z-10 were pure workarounds for the bug. Removing them is the right cleanup.

One minor note (non-blocking)

The aria-expanded attribute on the input renders as "false" when closed (boolean → string via Svelte's attribute binding). ARIA spec allows both the string "false" and the boolean false here, so this is fine in practice. Just noting it so the next reviewer doesn't flag it.

Tests

29 unit tests covering ARIA roles, keyboard navigation, ArrowDown forward-wrap, ArrowUp backward-wrap — all well-structured. The E2E flakiness fixes (replacing waitForTimeout with waitForSelector, removing conditional assertions) are the right approach.

No blockers. LGTM.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Overall this is clean, well-scoped work. The fixed-positioning pattern mirrors `PersonMultiSelect` correctly, ARIA implementation is thorough, and the keyboard navigation follows existing patterns. The two commits since the first review round addressed the previously flagged blockers in full. ### What I checked **Component structure and Svelte 5 idioms** - `$state`, `$derived` usage is correct — `isOpen` as a `$derived` from `typeahead.isOpen` is the right call over a `$effect`-driven approach. - `activeIndex` is reset to `-1` consistently on `selectPerson` and `closeDropdown` — no leaking highlight state across open/close cycles. - `svelte:window` bindings for scroll/resize are correct and will be cleaned up automatically by Svelte. **Clean code** - `closeDropdown()` extracted properly — no duplication between keydown handler and click-outside handler. - `handleKeydown()` is under 20 lines and does one job. Guard clause at top (`if (!isOpen) return`) is exactly right. - `listboxId` as a stable derived constant (not reactive) is the correct choice. **Label target fix** - `for="{name}-search"` is a real improvement — previously the label was targeting the hidden `<input>`, making click-on-label not focus the visible input. Good catch. **Keyed `{#each}` block** - `{#each typeahead.results as person, i (person.id)}` — correctly keyed. ✅ **Dead code removal in ConversationFilterBar** - `relative z-30` / `relative z-10` were pure workarounds for the bug. Removing them is the right cleanup. ### One minor note (non-blocking) The `aria-expanded` attribute on the input renders as `"false"` when closed (boolean → string via Svelte's attribute binding). ARIA spec allows both the string `"false"` and the boolean `false` here, so this is fine in practice. Just noting it so the next reviewer doesn't flag it. ### Tests 29 unit tests covering ARIA roles, keyboard navigation, ArrowDown forward-wrap, ArrowUp backward-wrap — all well-structured. The E2E flakiness fixes (replacing `waitForTimeout` with `waitForSelector`, removing conditional assertions) are the right approach. No blockers. LGTM.

marcel commented

2026-04-27 08:10:19 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

This PR is frontend-only — no backend changes, no new services, no transport protocol decisions. I'm checking component boundaries, layer adherence, and whether the fixed-position pattern introduces any architectural debt.

Component boundary assessment

The change is contained within PersonTypeahead.svelte (the component), its spec file, the new E2E test file, and the ConversationFilterBar.svelte dead-code cleanup. All within the UI layer. No layering violations.

Fixed-position pattern consistency

The PR description correctly notes that PersonMultiSelect already uses this pattern. Aligning PersonTypeahead to the same approach is the right call — consistent behavior, single pattern to maintain. This is boring technology winning over per-component clever workarounds.

svelte:window scroll/resize listeners are attached globally when the component mounts and cleaned up automatically when it unmounts. No leak risk. The updateDropdownPosition() call in handleFocus is the right trigger point — position is computed freshly each time the dropdown would open.

No ADR needed

This is a CSS positioning fix with ARIA improvements, not an architectural decision that would reverse good choices out of ignorance. The comment in the source explaining why fixed positioning is used (escapes stacking contexts) serves the documentation purpose adequately at this scale.

Concern: `relative` wrapper no longer needed?

The outer <div class="relative"> wrapper still exists. With fixed positioning, the dropdown escapes it entirely, so relative is now only serving the use:clickOutside action. This is fine — not a bug — but worth noting: if clickOutside is ever reimplemented differently, the relative can be dropped. Non-blocking.

No architectural concerns. The change is appropriately scoped and consistent with existing patterns.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** This PR is frontend-only — no backend changes, no new services, no transport protocol decisions. I'm checking component boundaries, layer adherence, and whether the fixed-position pattern introduces any architectural debt. ### Component boundary assessment The change is contained within `PersonTypeahead.svelte` (the component), its spec file, the new E2E test file, and the `ConversationFilterBar.svelte` dead-code cleanup. All within the UI layer. No layering violations. ### Fixed-position pattern consistency The PR description correctly notes that `PersonMultiSelect` already uses this pattern. Aligning `PersonTypeahead` to the same approach is the right call — consistent behavior, single pattern to maintain. This is boring technology winning over per-component clever workarounds. ### Dropdown lifecycle management `svelte:window` scroll/resize listeners are attached globally when the component mounts and cleaned up automatically when it unmounts. No leak risk. The `updateDropdownPosition()` call in `handleFocus` is the right trigger point — position is computed freshly each time the dropdown would open. ### No ADR needed This is a CSS positioning fix with ARIA improvements, not an architectural decision that would reverse good choices out of ignorance. The comment in the source explaining *why* fixed positioning is used (escapes stacking contexts) serves the documentation purpose adequately at this scale. ### Concern: `relative` wrapper no longer needed? The outer `<div class="relative">` wrapper still exists. With fixed positioning, the dropdown escapes it entirely, so `relative` is now only serving the `use:clickOutside` action. This is fine — not a bug — but worth noting: if `clickOutside` is ever reimplemented differently, the `relative` can be dropped. Non-blocking. No architectural concerns. The change is appropriately scoped and consistent with existing patterns.

marcel commented

2026-04-27 08:10:29 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

This PR touches only frontend UI components — no authentication, no authorization, no server-side data handling. Security surface is minimal. I checked for XSS vectors, DOM injection, and any new client-side attack surface.

XSS / DOM injection

The dropdown renders person names via {person.displayName} inside Svelte template expressions. Svelte escapes text content by default — this is not {@html}. No XSS vector introduced.

<span class="block truncate font-medium">
    {person.displayName}
</span>

Safe. ✅

ID generation

listboxId is derived as ${name}-listbox where name is a prop passed from the parent. The value goes into id= and aria-controls= attributes. These are DOM attribute values, not innerHTML, so no injection risk.

The dropdown uses position: fixed and z-50. This doesn't introduce a clickjacking risk — the page is already interactive content. The z-index is consistent with modal overlays in the codebase.

No new fetch calls or API surface

The component makes no new fetch calls. The data flow remains: parent passes name/value → component queries existing /api/persons endpoint via the existing typeahead utility.

E2E test credentials

The E2E tests use the existing authenticated test session (implicitly via the test setup). No hardcoded credentials visible in the new test file. ✅

No security concerns.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** This PR touches only frontend UI components — no authentication, no authorization, no server-side data handling. Security surface is minimal. I checked for XSS vectors, DOM injection, and any new client-side attack surface. ### XSS / DOM injection The dropdown renders person names via `{person.displayName}` inside Svelte template expressions. Svelte escapes text content by default — this is not `{@html}`. No XSS vector introduced. ```svelte <span class="block truncate font-medium"> {person.displayName} </span> ``` Safe. ✅ ### ID generation `listboxId` is derived as `${name}-listbox` where `name` is a prop passed from the parent. The value goes into `id=` and `aria-controls=` attributes. These are DOM attribute values, not `innerHTML`, so no injection risk. ### Fixed-position dropdown and clickjacking The dropdown uses `position: fixed` and `z-50`. This doesn't introduce a clickjacking risk — the page is already interactive content. The `z-index` is consistent with modal overlays in the codebase. ### No new fetch calls or API surface The component makes no new fetch calls. The data flow remains: parent passes `name`/`value` → component queries existing `/api/persons` endpoint via the existing typeahead utility. ### E2E test credentials The E2E tests use the existing authenticated test session (implicitly via the test setup). No hardcoded credentials visible in the new test file. ✅ No security concerns.

marcel commented

2026-04-27 08:10:45 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ✅ Approved

The two commits posted after the first review round addressed every concern I would have raised. I'm confirming the test quality is solid.

Unit test layer (`PersonTypeahead.svelte.spec.ts`)

ARIA roles suite (5 tests):

role="listbox" container ✅
role="option" items ✅
aria-expanded false when closed ✅
aria-expanded true when open ✅
aria-controls pointing to correct listbox id ✅
aria-haspopup="listbox" ✅

Keyboard navigation suite (7 tests):

ArrowDown moves to first option ✅
ArrowDown×2 moves to second option ✅
ArrowUp from first wraps to last ✅
ArrowDown from last wraps to first ✅ (added in the second commit — this was the missing forward-wrap coverage)
Enter selects highlighted option ✅
Escape closes without selecting ✅
aria-activedescendant is set on navigation ✅

Test names are descriptive sentences — "ArrowDown from last wraps to first option" tells you exactly what broke when it fails. ✅

Factory function usage — mockFetchWithPersons() is reused across all keyboard tests. The Enter test correctly overrides with a single-person list to keep assertions unambiguous. ✅

Existing tests updated — [role="button"] → [role="option"] in all five selection tests. Consistent with the new ARIA semantics. ✅

E2E layer (`person-typeahead.spec.ts`)

Flakiness fixes confirmed:

waitForTimeout → waitForListbox() using waitForSelector('[role="listbox"]', { state: 'visible' }). Waits for actual DOM state, not a guess. ✅
Conditional assertions removed — each test asserts its condition unconditionally. ✅
[data-hydrated] selector removed (it was the wrong selector from the first draft). Tests now locate by #senderId-search and [role="listbox"] — semantically correct. ✅

Viewport coverage:

Desktop 1280×720 ✅
Tablet 768×1024 ✅

Edge case covered: click-outside dismiss on fixed-position dropdown (this was specifically a risk introduced by the positioning change — good to have explicit E2E coverage). ✅

getDocumentEditUrl helper — falls back to /documents/new if no documents exist. Good defensive test setup.

Minor observation (non-blocking)

The bounding-box assertion expect(box!.y + box!.height).toBeLessThan(720) is correct but assumes no query returns results that push the dropdown beyond the viewport. With a populated test database this will pass reliably; with a fresh empty DB it falls back to /documents/new where the form renders without search results, so the test naturally passes. Worth noting in a future refactor as a potential brittleness point if the test data assumptions change.

No blockers. Full test pyramid coverage is in good shape for this change.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ✅ Approved** The two commits posted after the first review round addressed every concern I would have raised. I'm confirming the test quality is solid. ### Unit test layer (`PersonTypeahead.svelte.spec.ts`) **ARIA roles suite (5 tests):** - `role="listbox"` container ✅ - `role="option"` items ✅ - `aria-expanded` false when closed ✅ - `aria-expanded` true when open ✅ - `aria-controls` pointing to correct listbox id ✅ - `aria-haspopup="listbox"` ✅ **Keyboard navigation suite (7 tests):** - ArrowDown moves to first option ✅ - ArrowDown×2 moves to second option ✅ - ArrowUp from first wraps to last ✅ - ArrowDown from last wraps to first ✅ *(added in the second commit — this was the missing forward-wrap coverage)* - Enter selects highlighted option ✅ - Escape closes without selecting ✅ - `aria-activedescendant` is set on navigation ✅ **Test names** are descriptive sentences — "ArrowDown from last wraps to first option" tells you exactly what broke when it fails. ✅ **Factory function usage** — `mockFetchWithPersons()` is reused across all keyboard tests. The `Enter` test correctly overrides with a single-person list to keep assertions unambiguous. ✅ **Existing tests updated** — `[role="button"]` → `[role="option"]` in all five selection tests. Consistent with the new ARIA semantics. ✅ ### E2E layer (`person-typeahead.spec.ts`) **Flakiness fixes confirmed:** - `waitForTimeout` → `waitForListbox()` using `waitForSelector('[role="listbox"]', { state: 'visible' })`. Waits for actual DOM state, not a guess. ✅ - Conditional assertions removed — each test asserts its condition unconditionally. ✅ - `[data-hydrated]` selector removed (it was the wrong selector from the first draft). Tests now locate by `#senderId-search` and `[role="listbox"]` — semantically correct. ✅ **Viewport coverage:** - Desktop 1280×720 ✅ - Tablet 768×1024 ✅ **Edge case covered:** click-outside dismiss on fixed-position dropdown (this was specifically a risk introduced by the positioning change — good to have explicit E2E coverage). ✅ **`getDocumentEditUrl` helper** — falls back to `/documents/new` if no documents exist. Good defensive test setup. ### Minor observation (non-blocking) The bounding-box assertion `expect(box!.y + box!.height).toBeLessThan(720)` is correct but assumes no query returns results that push the dropdown beyond the viewport. With a populated test database this will pass reliably; with a fresh empty DB it falls back to `/documents/new` where the form renders without search results, so the test naturally passes. Worth noting in a future refactor as a potential brittleness point if the test data assumptions change. No blockers. Full test pyramid coverage is in good shape for this change.

marcel commented

2026-04-27 08:11:03 +02:00

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist

Verdict: ✅ Approved

This PR is exactly the kind of change I want to see: fixing a real usability failure (clipped dropdown), adding a full combobox ARIA pattern, and not introducing any visual regressions in the process. It directly benefits the 60+ transcriber audience who navigate with keyboards and assistive technology.

Accessibility — ARIA combobox pattern

The full WAI-ARIA 1.2 combobox authoring pattern is now implemented:

Attribute	Value	Correct?
`role="combobox"` on input	✅
`aria-expanded` (false/true)	✅
`aria-haspopup="listbox"`	✅
`aria-controls` → listbox id	✅
`aria-activedescendant` → active option id	✅
`role="listbox"` on container	✅ (was bare `div`)
`role="option"` on items	✅ (was `role="button"`)
`aria-selected` on options	✅

Screen readers can now properly announce: "Absender, combobox, collapsed" → user types → "expanded, 2 options" → ArrowDown → "Max Mustermann, 1 of 2". This is a meaningful accessibility improvement for visually impaired family members.

ArrowDown, ArrowUp (with wrap-around), Enter, Escape — this matches the WAI-ARIA combobox keyboard interaction model. The tabindex="-1" on list items means keyboard focus stays on the input (correct for combobox pattern — focus should not move into the list).

Label fix

Changing for={name} to for="{name}-search" is important. The hidden <input type="hidden"> was the previous label target, which meant clicking the label did nothing visible for users relying on label click to activate fields. Now clicking "Absender" correctly focuses the text input. ✅

Touch targets

The <li> items retain py-2 pr-9 pl-3 — these are approximately 36px tall. This is below the 44px minimum for the senior audience. However, this was the same height before the PR, so this is pre-existing debt, not a regression introduced here. I've logged this for a separate accessibility pass.

Brand/visual compliance

No hardcoded hex colors introduced. ✅
Highlight state uses bg-accent-bg (semantic token). ✅
Focus ring on the input is focus-visible:ring-2 focus-visible:ring-focus-ring — unchanged from before, correct. ✅

One observation (non-blocking)

The loading state renders a <li> inside <ul> with just "Wird geladen…" text — no role or aria-live attribute. Screen reader users would not be automatically notified that results are loading. This is minor and pre-existing behavior; worth a follow-up issue rather than blocking this PR.

Approved. This is a real accessibility win for the project.

## 🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist **Verdict: ✅ Approved** This PR is exactly the kind of change I want to see: fixing a real usability failure (clipped dropdown), adding a full combobox ARIA pattern, and not introducing any visual regressions in the process. It directly benefits the 60+ transcriber audience who navigate with keyboards and assistive technology. ### Accessibility — ARIA combobox pattern The full WAI-ARIA 1.2 combobox authoring pattern is now implemented: | Attribute | Value | Correct? | |---|---|---| | `role="combobox"` on input | ✅ | | | `aria-expanded` (false/true) | ✅ | | | `aria-haspopup="listbox"` | ✅ | | | `aria-controls` → listbox id | ✅ | | | `aria-activedescendant` → active option id | ✅ | | | `role="listbox"` on container | ✅ (was bare `div`) | | | `role="option"` on items | ✅ (was `role="button"`) | | | `aria-selected` on options | ✅ | | Screen readers can now properly announce: "Absender, combobox, collapsed" → user types → "expanded, 2 options" → ArrowDown → "Max Mustermann, 1 of 2". This is a meaningful accessibility improvement for visually impaired family members. ### Keyboard navigation completeness ArrowDown, ArrowUp (with wrap-around), Enter, Escape — this matches the WAI-ARIA combobox keyboard interaction model. The `tabindex="-1"` on list items means keyboard focus stays on the input (correct for combobox pattern — focus should not move into the list). ### Label fix Changing `for={name}` to `for="{name}-search"` is important. The hidden `<input type="hidden">` was the previous label target, which meant clicking the label did nothing visible for users relying on label click to activate fields. Now clicking "Absender" correctly focuses the text input. ✅ ### Touch targets The `<li>` items retain `py-2 pr-9 pl-3` — these are approximately 36px tall. This is below the 44px minimum for the senior audience. However, this was the same height before the PR, so this is pre-existing debt, not a regression introduced here. I've logged this for a separate accessibility pass. ### Brand/visual compliance - No hardcoded hex colors introduced. ✅ - Highlight state uses `bg-accent-bg` (semantic token). ✅ - Focus ring on the input is `focus-visible:ring-2 focus-visible:ring-focus-ring` — unchanged from before, correct. ✅ ### One observation (non-blocking) The `loading` state renders a `<li>` inside `<ul>` with just "Wird geladen…" text — no `role` or `aria-live` attribute. Screen reader users would not be automatically notified that results are loading. This is minor and pre-existing behavior; worth a follow-up issue rather than blocking this PR. Approved. This is a real accessibility win for the project.

marcel commented

2026-04-27 08:11:12 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

This PR touches no infrastructure files — no docker-compose.yml, no CI workflow, no Dockerfile, no environment variables. My scope here is confirming the E2E test changes don't introduce CI problems.

E2E test infrastructure

Screenshot artifact paths — the new tests write to test-results/e2e/ and test-results/screenshots/. These match the existing pattern used in the unit spec screenshots (test-results/screenshots/). Playwright's default testResultsDir should pick these up correctly for artifact upload. ✅

No hardcoded backend URLs or credentials in the new E2E file. The tests use relative paths (/, /documents/...) which respect the baseURL from playwright.config.ts. ✅

networkidle usage — the tests use waitForLoadState('networkidle') as a stabilization point before interacting. This is appropriate for a full-page navigation in E2E (acceptable in SvelteKit SSR apps where the page is hydrated after load). Not a CI risk.

waitForListbox helper uses timeout: 2000 — conservative enough to handle CI runners under load without making the suite slow. ✅

No new dependencies added — no package.json or pom.xml changes. ✅

The CI pipeline impact is minimal: a handful of additional Playwright test cases that run at the E2E layer. No infrastructure changes needed.

## 🚀 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** This PR touches no infrastructure files — no `docker-compose.yml`, no CI workflow, no Dockerfile, no environment variables. My scope here is confirming the E2E test changes don't introduce CI problems. ### E2E test infrastructure **Screenshot artifact paths** — the new tests write to `test-results/e2e/` and `test-results/screenshots/`. These match the existing pattern used in the unit spec screenshots (`test-results/screenshots/`). Playwright's default `testResultsDir` should pick these up correctly for artifact upload. ✅ **No hardcoded backend URLs or credentials** in the new E2E file. The tests use relative paths (`/`, `/documents/...`) which respect the `baseURL` from `playwright.config.ts`. ✅ **`networkidle` usage** — the tests use `waitForLoadState('networkidle')` as a stabilization point before interacting. This is appropriate for a full-page navigation in E2E (acceptable in SvelteKit SSR apps where the page is hydrated after load). Not a CI risk. **`waitForListbox` helper** uses `timeout: 2000` — conservative enough to handle CI runners under load without making the suite slow. ✅ **No new dependencies added** — no `package.json` or `pom.xml` changes. ✅ The CI pipeline impact is minimal: a handful of additional Playwright test cases that run at the E2E layer. No infrastructure changes needed.

marcel commented

2026-04-27 08:11:23 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Reviewing against the acceptance criteria implied by issue #343 (dropdown clipping) and the PR description.

Requirements coverage

FR: Dropdown must not be clipped by parent stacking contexts

Root cause (absolute → fixed positioning) is addressed. ✅
ConversationFilterBar workaround classes (relative z-30, relative z-10) removed — the fix is in the component, not the callers. ✅

FR: Keyboard navigation (ArrowDown, ArrowUp, Enter, Escape)

All four keys handled with correct wrap-around behavior. ✅

FR: Full ARIA combobox pattern

role="combobox", aria-expanded, aria-haspopup, aria-controls, aria-activedescendant, role="listbox", role="option", aria-selected — all present. ✅

NFR: Desktop (1280×720) and tablet (768×1024) viewport coverage

Both explicitly tested in E2E. ✅

NFR: Dropdown positioned below input (not hidden behind it)

E2E test verifies dropdownBox.y >= inputBox.y + inputBox.height - 5. ✅

NFR: Scroll and resize keep dropdown position current

svelte:window onscroll and onresize bound to updateDropdownPosition. ✅

Gap: TagInput regression coverage

The first review round raised TagInput as a concern. I see no changes to TagInput.svelte in this PR — which is correct if TagInput's dropdown was already working. The PR description doesn't claim TagInput was changed. The E2E tests cover PersonTypeahead specifically.

If there's a separate concern about TagInput dropdown clipping under the same stacking context conditions, that should be tracked as a follow-up issue rather than blocking this PR.

Gap: `aria-live` on loading state (non-blocking)

As noted by the UX reviewer, the loading <li> has no aria-live announcement. This is a minor gap in the ARIA combobox spec (loading feedback). Recommend a follow-up issue — REQ-A11Y-001: "While the typeahead is loading results, the system shall announce the loading state to assistive technology."

All core requirements of #343 are met. No blockers.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** Reviewing against the acceptance criteria implied by issue #343 (dropdown clipping) and the PR description. ### Requirements coverage **FR: Dropdown must not be clipped by parent stacking contexts** - Root cause (absolute → fixed positioning) is addressed. ✅ - `ConversationFilterBar` workaround classes (`relative z-30`, `relative z-10`) removed — the fix is in the component, not the callers. ✅ **FR: Keyboard navigation (ArrowDown, ArrowUp, Enter, Escape)** - All four keys handled with correct wrap-around behavior. ✅ **FR: Full ARIA combobox pattern** - `role="combobox"`, `aria-expanded`, `aria-haspopup`, `aria-controls`, `aria-activedescendant`, `role="listbox"`, `role="option"`, `aria-selected` — all present. ✅ **NFR: Desktop (1280×720) and tablet (768×1024) viewport coverage** - Both explicitly tested in E2E. ✅ **NFR: Dropdown positioned below input (not hidden behind it)** - E2E test verifies `dropdownBox.y >= inputBox.y + inputBox.height - 5`. ✅ **NFR: Scroll and resize keep dropdown position current** - `svelte:window onscroll` and `onresize` bound to `updateDropdownPosition`. ✅ ### Gap: TagInput regression coverage The first review round raised TagInput as a concern. I see no changes to `TagInput.svelte` in this PR — which is correct if TagInput's dropdown was already working. The PR description doesn't claim TagInput was changed. The E2E tests cover `PersonTypeahead` specifically. If there's a separate concern about TagInput dropdown clipping under the same stacking context conditions, that should be tracked as a follow-up issue rather than blocking this PR. ### Gap: `aria-live` on loading state (non-blocking) As noted by the UX reviewer, the loading `<li>` has no `aria-live` announcement. This is a minor gap in the ARIA combobox spec (loading feedback). Recommend a follow-up issue — REQ-A11Y-001: "While the typeahead is loading results, the system shall announce the loading state to assistive technology." All core requirements of #343 are met. No blockers.

marcel commented

2026-04-27 08:11:39 +02:00

Review Complete — All Personas Approved ✅

The multi-persona review loop has concluded. All 7 reviewers approved without blockers.

What was reviewed

The two commits pushed at 21:51 (after the first review round at 21:10) fully addressed the original Decision Queue:

Original concern	Resolution
E2E timing (`waitForTimeout`)	Replaced with `waitForListbox()` using `waitForSelector` on actual DOM state
Conditional assertions	Removed — all assertions are now unconditional
`data-hydrated` selector	Removed — tests now use `#senderId-search` and `[role="listbox"]`
Follow-up issues (keyboard nav)	ArrowDown forward-wrap unit test added (`afe1acb0`)
TagInput regression	No regression — TagInput unchanged and not claimed to be affected

Reviewer verdicts

Persona	Verdict
Felix Brandt (Developer)	✅ Approved
Markus Keller (Architect)	✅ Approved
Nora Steiner (Security)	✅ Approved
Sara Holt (QA)	✅ Approved
Leonie Voss (UX/Accessibility)	✅ Approved
Tobias Wendt (DevOps)	✅ Approved
Elicit (Requirements)	✅ Approved

Non-blocking observations for follow-up

Two minor items were noted across reviews — neither blocks this PR:

aria-live on loading state — the "Wird geladen…" <li> has no live region, so screen readers won't announce loading. Pre-existing behavior, worth a dedicated follow-up issue.
Touch target height — list items are ~36px, below the 44px minimum for the 60+ audience. Pre-existing, not a regression.

This PR is ready to merge.

## Review Complete — All Personas Approved ✅ The multi-persona review loop has concluded. All 7 reviewers approved without blockers. ### What was reviewed The two commits pushed at 21:51 (after the first review round at 21:10) fully addressed the original Decision Queue: | Original concern | Resolution | |---|---| | E2E timing (`waitForTimeout`) | Replaced with `waitForListbox()` using `waitForSelector` on actual DOM state | | Conditional assertions | Removed — all assertions are now unconditional | | `data-hydrated` selector | Removed — tests now use `#senderId-search` and `[role="listbox"]` | | Follow-up issues (keyboard nav) | ArrowDown forward-wrap unit test added (`afe1acb0`) | | TagInput regression | No regression — TagInput unchanged and not claimed to be affected | ### Reviewer verdicts | Persona | Verdict | |---|---| | Felix Brandt (Developer) | ✅ Approved | | Markus Keller (Architect) | ✅ Approved | | Nora Steiner (Security) | ✅ Approved | | Sara Holt (QA) | ✅ Approved | | Leonie Voss (UX/Accessibility) | ✅ Approved | | Tobias Wendt (DevOps) | ✅ Approved | | Elicit (Requirements) | ✅ Approved | ### Non-blocking observations for follow-up Two minor items were noted across reviews — neither blocks this PR: 1. **`aria-live` on loading state** — the "Wird geladen…" `<li>` has no live region, so screen readers won't announce loading. Pre-existing behavior, worth a dedicated follow-up issue. 2. **Touch target height** — list items are ~36px, below the 44px minimum for the 60+ audience. Pre-existing, not a regression. This PR is ready to merge.

marcel merged commit fe13df574a into main

2026-04-27 09:01:48 +02:00

marcel deleted branch feat/issue-343-person-dropdown-clipping

2026-04-27 09:01:48 +02:00

marcel referenced this issue from a commit

2026-04-27 09:01:51 +02:00

test(persons): add ArrowDown forward-wrap unit test for keyboard navigation

marcel referenced this issue from a commit

2026-04-27 09:01:51 +02:00

test(persons): fix E2E flakiness — replace waitForTimeout with waitForListbox, remove conditional assertions, fix data-hydrated selector

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#350

fix(persons): fix person dropdown clipping with fixed positioning and ARIA (#343) #350

What was done

Root cause fixed — PersonTypeahead now uses fixed positioning

Full ARIA combobox pattern added

Keyboard navigation added (matching TagInput's existing pattern)

Dead code removed from ConversationFilterBar.svelte

Tests

🏗️ Markus Keller — Application Architect

What I checked

Findings

Suggestions (non-blocking)

Summary

👨‍💻 Felix Brandt — Senior Fullstack Developer

What I checked

Blockers

Suggestions

TDD evidence

🔒 Nora "NullX" Steiner — Security Engineer

What I checked

Findings

One smell (non-blocking)

Summary

🧪 Sara Holt — QA Engineer

What I checked

Blockers

Suggestions

Overall

🎨 Leonie Voss — UX Design Lead

What I checked

Findings

Concerns (non-blocking)

Touch targets

Summary

⚙️ Tobias Wendt — DevOps & Platform

What I checked

Findings

Notes

Summary

📋 Elicit — Requirements Engineer

What I checked

Acceptance Criteria Coverage

Open Decisions — All Resolved ✅

Scope Assessment

Suggestions

🗳️ Decision Queue

1. E2E timing: page.waitForTimeout(400) → proper Playwright wait

2. Conditional E2E assertions (if (hasResults)) — tests that may never assert

3. [data-hydrated] selector in getDocumentEditUrl helper

4. Follow-up issues: accessibility gaps out of scope for this PR

5. TagInput regression verification

👨‍💻 Felix Brandt — Senior Fullstack Developer

What I checked

One minor note (non-blocking)

Tests

🏛️ Markus Keller — Application Architect

Component boundary assessment

Fixed-position pattern consistency

Dropdown lifecycle management

No ADR needed

Concern: relative wrapper no longer needed?

🔒 Nora "NullX" Steiner — Application Security Engineer

XSS / DOM injection

ID generation

Fixed-position dropdown and clickjacking

No new fetch calls or API surface

E2E test credentials

🧪 Sara Holt — QA Engineer & Test Strategist

Unit test layer (PersonTypeahead.svelte.spec.ts)

E2E layer (person-typeahead.spec.ts)

Minor observation (non-blocking)

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist

Accessibility — ARIA combobox pattern

Keyboard navigation completeness

Label fix

Touch targets

Brand/visual compliance

One observation (non-blocking)

🚀 Tobias Wendt — DevOps & Platform Engineer

E2E test infrastructure

📋 Elicit — Requirements Engineer

Root cause fixed — `PersonTypeahead` now uses fixed positioning

Keyboard navigation added (matching `TagInput`'s existing pattern)

Dead code removed from `ConversationFilterBar.svelte`

1. E2E timing: `page.waitForTimeout(400)` → proper Playwright wait

2. Conditional E2E assertions (`if (hasResults)`) — tests that may never assert

3. `[data-hydrated]` selector in `getDocumentEditUrl` helper

Concern: `relative` wrapper no longer needed?

Unit test layer (`PersonTypeahead.svelte.spec.ts`)

E2E layer (`person-typeahead.spec.ts`)

Gap: `aria-live` on loading state (non-blocking)