feat(persons): show merge action inline with danger hint, remove Gefahrenzone collapsible (#342) #347
Reference in New Issue
Block a user
Delete Branch "feat/issue-342-person-merge-inline"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #342
What was implemented
Per the Decision Queue resolution on #342: the merge panel stays on the edit page (
/persons/[id]/edit) and is made inline — the panel was not moved to the detail page.Changes
PersonMergePanel.svelte— container class changed fromborder-linetoborder-red-200. The danger indicator is now intrinsic to the panel itself, not dependent on the accordion wrapper.edit/+page.svelte— replaced<PersonDangerZone person={person} form={form} />with a direct inline render:{#key person.id}<PersonMergePanel person={person} form={form} />{/key}. The{#key}wrapper is preserved to reset internal merge state when navigating between persons.edit/PersonDangerZone.svelte— deleted. No dead code left behind.PersonMergePanel.svelte.spec.ts— new component test file (TDD, written before the fix) covering: red border class, merge heading renders, button disabled when no target selected, mergeError displays.Acceptance criteria
/persons/[id]/edit, the "Personen zusammenführen" section is visible without expanding anythingborder-red-200) is visible on the section at all times?/mergeaction, redirect on success) is unchangedPersonDangerZonecollapsible is removed and deleted👨💻 Markus Keller — Application Architect
Verdict: ✅ Approved
What I checked
Module boundaries, component ownership, dead code, layer separation, and the
{#key}state-reset pattern.Findings
Clean deletion of
PersonDangerZone.svelte— The accordion wrapper is gone entirely. No dead code left behind. This is correct.{#key person.id}wrapper preserved correctly — The state-reset wrapper moved from insidePersonDangerZonetoedit/+page.svelte. This is the right place for it: the orchestrating page controls when child state is reset, not the child component itself. Good.PersonMergePanelowns its danger signal intrinsically now — Theborder-red-200class moved from the wrapper (PersonDangerZone) into the panel itself. This is architecturally correct: the panel is a destructive-action component and its visual contract should not depend on a decorator wrapper. The danger signal is now part of the component's identity.Import path is correct —
edit/+page.svelteimports from'../PersonMergePanel.svelte'(up one level topersons/[id]/). The merge panel has always lived at the[id]level, not theedit/level. The accordion was the edit-page decoration, and that decoration is now gone. The module boundary was already right; this change simply removes the indirection.Scope confirmed as edit-page only — Per the Decision Queue resolution, the merge panel stays on
/persons/[id]/edit. The detail page is untouched. This keeps action routing simple:?/mergecontinues to be served byedit/+page.server.tswith no duplication.No blockers. The structural outcome is simpler and cleaner than before.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
What I checked
TDD evidence, Svelte 5 patterns, component size, naming, dead code, test quality, and the diff holistically.
The core change (issue #342)
The
PersonMergePanel.svelteandedit/+page.sveltechanges are clean and correct:border-line→border-red-200on the panel container: one-line, correct, intrinsic to the component.<PersonDangerZone>→ direct{#key person.id}<PersonMergePanel .../>{/key}: exactly right. The{#key}wrapper is preserved, which is important — it resetsmergeTargetIdandshowMergeConfirmwhen navigating between persons.PersonDangerZone.sveltedeleted: no dead code left.PersonMergePanel.svelte.spec.tswas written TDD-first (per the completion comment). The 4 tests cover: red border class, heading renders, button disabled when no target, mergeError displays. These are the right behaviors to cover. Test names are sentence-style and readable.Concerns (suggestions, not blockers)
1. The
PersonMergePanel.svelte.spec.tsmock forPersonTypeaheadis incorrect Svelte 5 syntaxThis is Svelte 3/4 component mock syntax (
$$internals). In Svelte 5 withvitest-browser-svelte, the recommended approach is either to mock at the module boundary returning a simple stub component, or to let the real component render (sincevitest-browser-svelteuses a real browser). If thePersonTypeaheadrequires a network call, stub at the API level instead. The current mock may work incidentally but will break if Svelte 5 internals change.Suggested fix:
2. The diff contains unrelated files
This PR diff includes:
frontend/e2e/person-typeahead.spec.ts(E2E tests forPersonTypeaheaddropdown visibility — issue #343)frontend/src/lib/components/AnnotationShape.svelte(delete button feature — unrelated)frontend/src/lib/components/PdfControls.svelte+.spec.ts(annotation toggle contrast fix — unrelated)frontend/src/lib/components/TranscriptionEditView.svelte.spec.ts(bulk-mark-reviewed tests — issue #345)These are not part of issue #342. They appear to be uncommitted work from other branches that landed on this branch. They should be reviewed separately. I'm reviewing only the #342 changes above.
3.
PdfControls.sveltecolor token change:text-accent→text-primaryThis is in the diff (line
'bg-surface/10 text-primary').text-primaryis not a defined token inlayout.cssbased on the brand palette in CLAUDE.md (which definesbrand-navy,brand-mint,brand-sand). Iftext-primaryresolves to something meaningful via Tailwind, fine — but this should be verified. The originaltext-accentwas presumablybrand-mint. This change is unrelated to #342 but is in the diff.Summary
The #342 core change (merge panel inline, accordion deleted, red border intrinsic,
{#key}preserved) is clean and correct. The two items to resolve: fix the Svelte 5 mock syntax in the spec, and confirm whether the unrelated files in the diff are intentional inclusions or accidental.👨💻 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
Infrastructure impact, CI pipeline, SvelteKit routing side effects, Docker Compose, E2E test path changes.
Findings
No infrastructure changes required. This is a pure frontend refactor. No new environment variables, no Docker Compose changes, no Caddy config updates, no new ports or services.
SvelteKit action routing is unaffected. The
?/mergeform action continues to be served byedit/+page.server.tsunder/persons/[id]/edit. No routing config changes needed. SvelteKit's file-based routing handles this automatically.Backend endpoint unchanged.
POST /api/persons/{id}/mergealready exists and is tested. This PR adds no new API surface.E2E test paths. The PR adds
frontend/e2e/person-typeahead.spec.tswhich navigates to document edit pages, not person edit pages. No existing E2E test for the merge flow appears to be broken by this change. Worth confirming in CI that the Playwright suite passes, but there's no structural reason it wouldn't.One observation on the new E2E file:
person-typeahead.spec.tsusespage.waitForTimeout(400)for the 300ms debounce.waitForTimeoutis a fixed sleep — it's brittle and flagged as an antipattern in Playwright best practices. PreferwaitForSelector('[role="listbox"]')orwaitForResponseafter the debounce fires. This is unrelated to #342 but is in the diff. File it under the issue that introduced it (#343).No CI pipeline changes needed. Existing Vitest and Playwright runs cover the new test files without configuration changes.
Nothing to block here from an infrastructure perspective.
👨💻 Elicit — Requirements Engineer
Verdict: ✅ Approved
What I checked
Acceptance criteria traceability, scope alignment with the Decision Queue resolution, and requirement completeness.
Traceability against the resolved acceptance criteria
The Decision Queue resolution clarified that the merge panel stays on the edit page and is made inline — not moved to the detail page. Mapping the diff against the resolved AC:
/persons/[id]/edit, "Personen zusammenführen" is visible without expanding anythingPersonDangerZoneaccordion deleted;PersonMergePanelrendered directlyborder-red-200now intrinsic toPersonMergePanel?/mergeaction, two-step confirm,{#key}state reset all preservedPersonDangerZonecollapsible removed and deletedObservations
Scope is tightly contained. The change does exactly what the resolved Decision Queue specified — no more, no less. The detail page is untouched. The edit page loses the accordion and gains a directly rendered panel.
The irreversibility text question (Decision Queue Theme 3) was resolved as "keep the old text." The existing
person_merge_warningi18n key is unchanged. The panel still shows the irreversibility warning only in the confirmation step, not at the section level. This is a deliberate product decision and is consistent with the resolution.Note on scope of this PR's diff: The diff contains changes unrelated to #342 (E2E tests,
AnnotationShape,PdfControls,TranscriptionEditViewtests). These should be attributed to their respective issues. From a requirements perspective, #342 is fully satisfied by the subset of changes inPersonMergePanel.svelte,edit/+page.svelte, the deletion ofPersonDangerZone.svelte, andPersonMergePanel.svelte.spec.ts.No open requirements gaps for #342. The delivery matches the resolution.
👨💻 Nora "NullX" Steiner — Security Engineer
Verdict: ✅ Approved
What I checked
Authorization surface changes, action routing security, CSRF exposure, and any new attack surface introduced by the unrelated diff files.
Core change (#342): No new security concerns
The merge panel stays on the edit page. The
?/mergeform action remains inedit/+page.server.ts, which already has thecanWriteguard in itsloadfunction:The Decision Queue concern raised in issue #342 (defense-in-depth gap if the action moved to the detail page without a route-level guard) does not apply here because the action did not move. The existing guard is in place and unchanged.
PersonMergePanelitself has no authorization logic — correctly so. Authorization lives at the server action layer, not in the UI component.No new API endpoints, no new form actions, no new parameters. The
targetPersonIdcontinues to be passed to the existing backend endpoint that enforces@RequirePermission(Permission.WRITE_ALL)independently.Unrelated files in the diff — security notes
AnnotationShape.svelte— The new delete button callsonDeleteRequest?.()withe.stopPropagation(). No XSS risk: no user-controlled content is rendered via innerHTML. Thearia-label="Löschen"is hardcoded, not user-supplied. Clean.PdfControls.svelte— Thetext-accent→text-primarytoken change is a CSS concern only. No security impact.person-typeahead.spec.ts— E2E test file, no production security surface.No security issues found. The authorization boundary is intact.
👨💻 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
What I checked
Test coverage completeness, TDD evidence, test quality, spec file correctness, and the broader test pyramid impact of this PR.
PersonMergePanel.svelte.spec.ts— new spec fileTDD evidence confirmed: Per the completion comment, the spec was written before the fix. 4 tests cover the key behaviors:
mergeErrordisplayed whenform.mergeErroris set ✅This is the minimum viable test set for this component. Coverage is appropriate for the scope.
One quality concern: the
PersonTypeaheadmock syntaxThis uses Svelte 3/4 internal conventions (
$$). Withvitest-browser-svelteagainst a real browser DOM in Svelte 5, this may work by accident but is not correct. IfPersonTypeaheadrequires the$lib/api.servermodule (which is server-only), the mock prevents a module-resolution error — but the mock shape is wrong. A minimal correct Svelte 5 stub would be:Suggestion (not a blocker): Add a test for the two-step confirmation flow — "after selecting a target and clicking Zusammenführen, a confirmation message appears." This is the UX behavior most likely to regress if the internal
showMergeConfirmstate logic changes, and it is not currently covered.Unrelated test files in the diff
TranscriptionEditView.svelte.spec.ts— 6 new tests for the bulk-mark-reviewed feature (issue #345). These look correct in structure. Test names are descriptive. The in-flight disablement test is good. These belong to #345, not #342.PdfControls.svelte.spec.ts— 5 new tests including a WCAG contrast class assertion. The contrast test (expect(annotationBtn!.className).toContain('text-primary')) is a class-name assertion, not a true contrast ratio check — it verifies the fix was applied but not thattext-primaryactually passes WCAG AA. Acceptable as a regression guard, but note the limitation.person-typeahead.spec.ts— E2E tests for issue #343. Thepage.waitForTimeout(400)calls are flaky-by-design: fixed sleeps for debounce timing. Should usewaitForSelector('[role="listbox"]')instead. Not a blocker for this PR since these belong to #343.Summary
The #342 test coverage is adequate. The mock syntax should be corrected. The confirmation-flow test is a gap worth addressing in a follow-up. The unrelated test files look structurally sound but belong to their respective issues.
👨💻 Leonie Voss — UI/UX Design Lead
Verdict: ⚠️ Approved with concerns
What I checked
Brand token usage, accessibility, touch targets, danger signal visibility, mobile layout, and the senior-audience constraints (60+, 44px minimum touch targets, sufficient contrast).
The core change: danger indicator
border-red-200is now intrinsic to the panel — correct direction. The danger signal no longer depends on the accordion wrapper. This satisfies the acceptance criterion: the red border is visible at all times on the section.However,
border-red-200is a very light red (#fecacain Tailwind). Againstbg-surface(#E4E2D7), this is subtle. For a destructive-action section, the visual contrast of the danger signal matters. Considerborder-red-300orborder-red-400for better visibility, especially in bright ambient light on the senior audience's screens. This is a suggestion, not a blocker.Missing persistent irreversibility hint (Medium concern)
The acceptance criterion says: "The section has a clear danger indicator (red border or warning icon + text e.g. 'Diese Aktion kann nicht rückgängig gemacht werden')."
The red border satisfies the OR condition. But the warning text only appears after the user selects a target and clicks "Zusammenführen" — it is part of the confirmation step, not visible on section load. A senior user who skips the confirmation step mentally (or dismisses it quickly) never sees a persistent warning.
Recommendation (suggestion, not blocker): Add a small persistent text hint above the form:
New i18n key
person_merge_danger_hint:"Diese Aktion kann nicht rückgängig gemacht werden."(distinct from the existingperson_merge_warningused in the confirmation step).AnnotationShape.sveltedelete button — in-diff concern (Medium)The new delete button uses inline
stylefor all positioning:Two issues:
background-color: #fffhardcoded — should usevar(--color-surface)orbg-whitetoken so it works in dark mode.var(--color-error, #e53e3e)fallback —#e53e3eis not in the project's brand palette. The project does not define a--color-errortoken inlayout.css. Either define the token properly inlayout.css, or use Tailwind'stext-red-600/border-red-600utilities to stay consistent with the rest of the codebase (which usesborder-red-200,text-red-600inPersonDangerZoneand nowPersonMergePanel).The touch target is
min-width: 44px; min-height: 44px— correct for the senior audience. Good.PdfControls.sveltetoken change — in-diff concern (Low)text-accent→text-primary.text-accentmaps tobrand-mint(#A6DAD8).text-primaryis not defined in the project'slayout.csstoken set per CLAUDE.md. Iftext-primaryis undefined, it falls through to nothing — the button would render with inherited text color rather than the intended highlight. Verify this token exists before merging.Summary
The core #342 change meets the acceptance criteria (red border intrinsic, accordion gone). Two suggestions for the panel: slightly stronger red border for senior visibility, and a persistent irreversibility hint. One concern in the unrelated
AnnotationShapechange: replace hardcoded colors with project tokens.286a31af65tof162c9b55b👨💻 Markus Keller — Application Architect
Verdict: ✅ Approved
What I checked
Module boundaries, component ownership, layer separation, dead code elimination, and the
{#key}state-reset placement.Findings
Scope is now clean. The previous iteration had unrelated files (issues #343, #345) bundled into the single commit. The diff is now exactly 4 files:
PersonMergePanel.svelte,PersonMergePanel.svelte.spec.ts,edit/+page.svelte, and the deletedPersonDangerZone.svelte. This is the correct scope for #342.Module boundary is correct.
PersonMergePanelhas always lived atpersons/[id]/— shared across the detail and edit sub-routes. The accordion wrapper (PersonDangerZone) was edit-page decoration, not a component in its own right. Deleting it and importing directly from'../PersonMergePanel.svelte'is the right call. No indirection remains.{#key}placement is architecturally correct. The orchestrating page (edit/+page.svelte) controls state reset, not the child component. This is the right pattern: children declare their state; parents decide when to reset it. ThePersonMergePanelcorrectly holdsmergeTargetIdandshowMergeConfirmas local$state— it doesn't need to know it gets reset; the key block handles it declaratively from the outside.Dead code is fully removed.
PersonDangerZone.sveltedeleted. No stubs, no commented-out imports, no orphaned i18n keys referenced only by the deleted component. Theperson_danger_zone_headingtranslation key that was used exclusively inPersonDangerZoneis now unused — worth a cleanup issue, but not a blocker.border-red-200intrinsic to the panel. The danger signal is now part of the component's identity, not a decorator. This is the correct layering: a component that owns a destructive action should carry its own visual warning, not inherit it from a wrapper.No blockers. The diff is clean and structurally sound.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
What I checked
TDD evidence, Svelte 5 patterns, mock correctness, component size, naming, dead code, and diff cleanliness.
The core change
All four files are correct:
border-line→border-red-200: one line, intrinsic, correct.<PersonDangerZone>→{#key person.id}<PersonMergePanel .../>{/key}: the state-reset wrapper is preserved and moved to the right level (the orchestrating page).PersonDangerZone.sveltedeleted: no dead code.PersonMergePanel.svelte.spec.ts: 4 tests covering red border class, heading renders, button disabled when no target, and mergeError display.Mock syntax — fixed correctly
The previous review flagged the Svelte 3/4
$$internals mock:This is now:
This is the right fix. A Svelte 5 component is a function; returning
nullproduces an empty mount. The mock prevents module-resolution errors from$lib/api.serverimports insidePersonTypeaheadwithout introducing incorrect internal coupling.Diff is clean
The unrelated files from issues #343 and #345 are no longer present. The single commit now contains only the 4 files belonging to #342. This is correct.
No open concerns. Tests pass (4/4 confirmed locally). The change is ready.
👨💻 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
Infrastructure impact, CI pipeline, SvelteKit routing, E2E test files, Docker Compose, and diff scope.
Findings
No infrastructure changes required. This is a pure frontend refactor. No new environment variables, no Docker Compose changes, no Caddy config updates, no new ports or services. Zero operational impact.
SvelteKit action routing is unaffected.
?/mergecontinues to be served byedit/+page.server.tsunder/persons/[id]/edit. SvelteKit's file-based routing derives this automatically — no routing config exists to break.Backend endpoint unchanged.
POST /api/persons/{id}/mergeis pre-existing. This PR adds no new API surface.The unrelated E2E file is gone. The previous iteration included
frontend/e2e/person-typeahead.spec.tsin this diff (issue #343 content). That file is no longer present. The E2E suite is unaffected by this PR.Vitest runs are unaffected. The new
PersonMergePanel.svelte.spec.tsintegrates into the existing Vitest browser setup with no configuration changes. The corrected mock (default: () => null) won't require anyvi.config.tsor alias changes.CI pipeline: no changes needed. Existing Vitest and linting runs cover the new test file automatically. No new test runner config, no new artifact paths.
Nothing to block here from a platform perspective.
👨💻 Elicit — Requirements Engineer
Verdict: ✅ Approved
What I checked
Acceptance criteria traceability against the Decision Queue resolution, scope alignment, i18n completeness, and requirement completeness.
Traceability against the resolved acceptance criteria
The Decision Queue resolution on #342 specified: merge panel stays on the edit page, made inline (not moved to detail page). Mapping the current diff:
/persons/[id]/edit, "Personen zusammenführen" is visible without expanding anythingPersonDangerZoneaccordion deleted;PersonMergePanelrendered directlyborder-red-200now intrinsic toPersonMergePanel?/mergeaction, redirect on success)PersonDangerZonecollapsible removed and deletedScope is now correct
The previous iteration bundled unrelated changes from issues #343, #345. The diff now contains exactly the 4 files required by #342. The scope matches the issue specification precisely.
One minor observation: orphaned i18n key
person_danger_zone_headingwas used exclusively in the now-deletedPersonDangerZone.svelte. That translation key is now unreferenced. This is not a functional defect — unused i18n keys do not cause errors — but it is requirements debt. Recommend filing a small cleanup issue to remove it fromde.json,en.json, andes.jsonto keep the translation files accurate.No open requirements gaps for #342. The delivery matches the resolution.
👨💻 Nora "NullX" Steiner — Security Engineer
Verdict: ✅ Approved
What I checked
Authorization surface changes, action routing security, CSRF exposure, server action guard presence, and new attack surface in the changed files.
Core change (#342): No new security concerns
The merge panel stays on the edit page. The
?/mergeform action remains inedit/+page.server.ts, which carries the existingcanWriteguard. The Decision Queue concern from the previous review cycle (defense-in-depth gap if the action moved to the detail page without a route-level guard) does not apply — the action did not move.Authorization layer is intact:
canWritecheck inedit/+page.server.tsload function returns 403 before the page renders.?/mergeaction in the same file inherits the same session context.POST /api/persons/{id}/mergeenforces@RequirePermission(Permission.WRITE_ALL)independently.Defense in depth is preserved across all three layers.
No new API endpoints, no new form action parameters.
targetPersonIdis unchanged; it continues to be validated server-side. ThePersonTypeaheadmock in the spec file (default: () => null) returnsnull— no user-controlled data renders through it in tests, and in production the real component is type-safe.PersonMergePanelcorrectly has no authorization logic. Authorization belongs at the server action layer. The component renders the form UI only — correct separation.Diff scope is clean
The previous iteration included
AnnotationShape.sveltechanges in this diff. Those changes are no longer present. The security surface of this PR is limited to the merge panel UI refactor and its spec file.No security issues found.
👨💻 Sara Holt — QA Engineer
Verdict: ✅ Approved
What I checked
TDD evidence, spec file quality, mock correctness, test coverage completeness, and test pyramid placement.
PersonMergePanel.svelte.spec.ts— assessment4 tests, all behavioral, all well-named:
renders outer container with red border class— verifies the danger signal is intrinsic to the component. DOM assertion viaclassList.containsis appropriate here because the test's explicit purpose is to assert a CSS class contract.renders merge heading—getByRole('heading', { level: 2 })is correct: tests what the user sees, not internals.merge button is disabled when no target selected—getByRole('button', { name: /zusammenführen/i })+toBeDisabled()is the right assertion for this behavior.renders mergeError when form contains error—getByText('...')is correct for displayed error text.Mock syntax is now correct. The previous
$$-based Svelte 3/4 mock has been replaced withdefault: () => null, which is the correct Svelte 5 functional stub pattern. This prevents module-resolution errors fromPersonTypeahead's server-only imports without introducing fragile internal coupling.Factory function is idiomatic.
makePerson()with spreaded overrides follows the project pattern.afterEach(cleanup)is present. No test contamination between cases.Coverage gaps — suggestions (not blockers)
The two-step confirmation flow is not covered. The sequence: (1) select a target → (2) click "Zusammenführen" → (3) confirmation message appears with
showMergeConfirm = trueis the primary UX regression risk in this component. It is not currently tested. This is the most important missing test; recommend a follow-up.showMergeConfirmresets when target changes — not covered. When the user selects a new target after the confirmation is showing,showMergeConfirmshould reset tofalse. Worth a test to guard against regression.Both are suggestions for a follow-up issue, not blockers on this PR. The minimum viable test set for the acceptance criteria is present and correct.
Diff scope
The unrelated test files (
TranscriptionEditView.svelte.spec.ts,PdfControls.svelte.spec.ts,person-typeahead.spec.ts) are no longer in this diff. The test changes are now scoped to #342 only.No blockers. The spec file is clean and the mock fix is correct.
👨💻 Leonie Voss — UI/UX Design Lead
Verdict: ✅ Approved
What I checked
Brand token usage, danger signal visibility, touch targets, accessibility, mobile layout, and the senior-audience constraints (60+, 44px minimum, sufficient contrast).
Core change: danger indicator
border-red-200intrinsic to the panel — correct direction. The danger signal is no longer conditional on an accordion being open. The red border is visible at all times on the section, which satisfies the acceptance criterion.border-red-200contrast note (suggestion, not blocker). Tailwind'sred-200is#fecaca— a very light red. Againstbg-surface(#E4E2D7), the border contrast is subtle. For a destructive-action section on a senior audience's screen (bright ambient light, 60+ eyes),border-red-300orborder-red-400would provide meaningfully better visual differentiation. This is a design refinement, not a blocker.Accessibility assessment
Touch targets on buttons: The "Zusammenführen" button uses
px-4 py-2— actual rendered height is approximately 36–38px, below the 44px minimum for the senior audience (WCAG 2.2 SC 2.5.8). Considerpy-3(48px height) for both the initial button and the confirm/cancel buttons in the two-step flow.The confirmation warning text is semantic.
<p class="... text-red-700">wraps the irreversibility warning in the confirmation step.text-red-700onbg-red-50passes WCAG AA contrast (approximately 5.9:1). Good.No color-only danger signal. The red border serves as a visual cue alongside the heading text "Personen zusammenführen" — the function is communicated by text, not color alone. This is correct.
Focus rings: The buttons use Tailwind's default focus handling. No explicit
focus-visible:ring-*is set on the merge buttons — worth adding for keyboard navigability, especially for the senior audience who may rely on keyboard. Suggestion only.Persistent irreversibility hint (suggestion, not blocker)
The irreversibility warning (
person_merge_warning) only appears after the user has already selected a target and clicked the first button. A senior user who skims the section may not see it. The acceptance criterion is met by the red border alone (the OR condition), but a small persistent note — e.g.text-sm text-red-500under the section heading — would strengthen the danger signal for the target audience. This is a product enhancement for a follow-up, not a blocker on this PR.Diff scope
The unrelated
AnnotationShape.sveltechanges (with inline-style color concerns raised in the previous review) are no longer in this diff. The in-diff concerns from the previous cycle are resolved by scope cleanup.The acceptance criteria are met. Suggestions are follow-up polish items.