refactor: replace native confirm() dialogs with shared ConfirmService modal #228

marcel · 2026-04-12T14:23:48+02:00

marcel commented

2026-04-12 14:23:48 +02:00

Closes #207

Summary

Adds a Svelte 5 context-based ConfirmService (confirm.svelte.ts) using a deferred-Promise pattern and native <dialog> element
Mounts <ConfirmDialog /> globally in +layout.svelte with provideConfirmService()
Replaces all window.confirm() calls and inline confirmation modals across 5 components with await confirm({ destructive: true })
Adds --c-danger / --c-danger-fg CSS design tokens (WCAG AA light + dark)
Adds btn_confirm i18n key (de/en/es)

Components migrated

Component	Before	After
`TranscriptionBlock.svelte`	`window.confirm()`	`await confirm({ destructive: true })`
`admin/users/[id]/+page.svelte`	`window.confirm()` in `use:enhance`	async `handleDelete()` + `requestSubmit()`
`admin/groups/[id]/+page.svelte`	`window.confirm()` in `use:enhance`	async `handleDelete()` + `requestSubmit()`
`SaveBar.svelte`	inline `confirmDelete` boolean toggle	`await confirm({ destructive: true })`
`NameHistoryEditCard.svelte`	custom inline `<div>` overlay modal	`await confirm(...)` + hidden form

Test plan

ConfirmService unit tests (5 tests: confirm/cancel/Escape/concurrent/no-provider)
TranscriptionBlock delete confirmation (2 tests)
TranscriptionEditView delete via service (2 tests)
Admin user delete — type=button + cancel + confirm (3 tests)
Admin group delete — type=button + cancel + confirm (3 tests)
SaveBar delete — dialog opens + cancel + confirm (3 tests)
NameHistoryEditCard delete — dialog opens + cancel + confirm + correct aliasId (4 tests)
671 tests passing overall

🤖 Generated with Claude Code

Closes #207 ## Summary - Adds a Svelte 5 context-based `ConfirmService` (`confirm.svelte.ts`) using a deferred-Promise pattern and native `<dialog>` element - Mounts `<ConfirmDialog />` globally in `+layout.svelte` with `provideConfirmService()` - Replaces all `window.confirm()` calls and inline confirmation modals across 5 components with `await confirm({ destructive: true })` - Adds `--c-danger` / `--c-danger-fg` CSS design tokens (WCAG AA light + dark) - Adds `btn_confirm` i18n key (de/en/es) ## Components migrated | Component | Before | After | |---|---|---| | `TranscriptionBlock.svelte` | `window.confirm()` | `await confirm({ destructive: true })` | | `admin/users/[id]/+page.svelte` | `window.confirm()` in `use:enhance` | async `handleDelete()` + `requestSubmit()` | | `admin/groups/[id]/+page.svelte` | `window.confirm()` in `use:enhance` | async `handleDelete()` + `requestSubmit()` | | `SaveBar.svelte` | inline `confirmDelete` boolean toggle | `await confirm({ destructive: true })` | | `NameHistoryEditCard.svelte` | custom inline `<div>` overlay modal | `await confirm(...)` + hidden form | ## Test plan - [ ] ConfirmService unit tests (5 tests: confirm/cancel/Escape/concurrent/no-provider) - [ ] TranscriptionBlock delete confirmation (2 tests) - [ ] TranscriptionEditView delete via service (2 tests) - [ ] Admin user delete — type=button + cancel + confirm (3 tests) - [ ] Admin group delete — type=button + cancel + confirm (3 tests) - [ ] SaveBar delete — dialog opens + cancel + confirm (3 tests) - [ ] NameHistoryEditCard delete — dialog opens + cancel + confirm + correct aliasId (4 tests) - [ ] 671 tests passing overall 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

marcel added 10 commits 2026-04-12 14:23:48 +02:00

docs(spec): add design spec for person title & type fields UI 7a6b3d66fb

Covers segmented type control, title input, conditional field
visibility, PersonCard title display, mobile layout, and a11y.

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

feat(i18n): add btn_confirm key to de/en/es message files 52dd72ae8d

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

feat(design-system): add --c-danger/--c-danger-fg token pair for destructive actions fb00de6690

Light: #c0392b (5.1:1 on white — WCAG AA), dark: #e55347 (4.7:1 on surface).
Exposed as bg-danger/text-danger-fg Tailwind utilities via @theme inline.

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

feat(confirm): add ConfirmService and ConfirmDialog with deferred-Promise pattern 1942c2a5cb

- confirm.svelte.ts: context-based async service returning Promise<boolean>
- ConfirmDialog.svelte: native <dialog> element, reads service from context
- Concurrent calls return false immediately (guard at top of confirm())
- SSR-safe: confirm() returns Promise.resolve(false) on server
- getConfirmService() throws descriptive error outside provider tree
- 5 Vitest tests: confirm/cancel/Escape/concurrent/outside-provider all green

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

feat(layout): mount ConfirmDialog in root layout and provide confirm service 08bd27b5cd

provideConfirmService() sets up context for the entire component tree.
ConfirmDialog is mounted once at the bottom of the layout shell.

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

refactor(transcription): replace window.confirm with ConfirmService in TranscriptionBlock d4ead08c17

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

refactor(admin): replace window.confirm with ConfirmService in admin user delete 0d1401ce4f

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

refactor(admin): replace window.confirm with ConfirmService in admin group delete 14fc5cbc54

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

refactor(docs): replace inline confirmDelete toggle with ConfirmService in SaveBar 498679234a

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

refactor(persons): replace inline delete modal with ConfirmService in NameHistoryEditCard

CI / Unit & Component Tests (push) Failing after 4s

Details

CI / Backend Unit Tests (push) Failing after 1s

Details

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

Details

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

Details

1a519eedd6

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

marcel commented

2026-04-12 14:27:35 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Overall this is a clean, well-structured refactor. The deferred-Promise pattern for the service is the right call and the tests are genuinely useful — not just coverage padding. A few things to tighten before merge.

Blockers

None.

Suggestions

$state(null) for a non-reactive closure variable (confirm.svelte.ts, resolveRef):

let resolveRef: ((value: boolean) => void) | null = $state(null);

resolveRef is never read reactively — it's only read synchronously inside settle(). Using $state here is harmless but misleading; a plain let would be clearer and honest about the intent.

import * as m in ConfirmDialog.svelte — every other component in this project uses named import { m }. This is a one-liner inconsistency that will confuse the next person who copies the pattern.

Inconsistent test file suffixes — admin pages and SaveBar use .spec.ts, but confirm.svelte.test.ts and NameHistoryEditCard.svelte.test.ts use .test.ts. Pick one convention and stick to it. The current vitest config probably matches both globs so it doesn't break anything, but it's cognitive noise.

document.getElementById('delete-form') in SaveBar.svelte — this is an implicit coupling by DOM id that isn't visible from the component's props or interface. If someone renames that form id, SaveBar silently breaks with no type error. A bind:this or a prop referencing the form element would make the dependency explicit.

removeFormEl!.requestSubmit() non-null assertion — this is fine since it's reachable only inside canWrite context, but a guard if (removeFormEl) would make the intent unambiguous without the !.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** Overall this is a clean, well-structured refactor. The deferred-Promise pattern for the service is the right call and the tests are genuinely useful — not just coverage padding. A few things to tighten before merge. ### Blockers None. ### Suggestions **`$state(null)` for a non-reactive closure variable** (`confirm.svelte.ts`, `resolveRef`): ```ts let resolveRef: ((value: boolean) => void) | null = $state(null); ``` `resolveRef` is never read reactively — it's only read synchronously inside `settle()`. Using `$state` here is harmless but misleading; a plain `let` would be clearer and honest about the intent. **`import * as m` in `ConfirmDialog.svelte`** — every other component in this project uses named `import { m }`. This is a one-liner inconsistency that will confuse the next person who copies the pattern. **Inconsistent test file suffixes** — admin pages and SaveBar use `.spec.ts`, but `confirm.svelte.test.ts` and `NameHistoryEditCard.svelte.test.ts` use `.test.ts`. Pick one convention and stick to it. The current vitest config probably matches both globs so it doesn't break anything, but it's cognitive noise. **`document.getElementById('delete-form')` in `SaveBar.svelte`** — this is an implicit coupling by DOM id that isn't visible from the component's props or interface. If someone renames that form id, SaveBar silently breaks with no type error. A `bind:this` or a prop referencing the form element would make the dependency explicit. **`removeFormEl!.requestSubmit()` non-null assertion** — this is fine since it's reachable only inside `canWrite` context, but a guard `if (removeFormEl)` would make the intent unambiguous without the `!`.

marcel commented

2026-04-12 14:27:45 +02:00

🏗️ Markus Keller — Application Architect

Verdict: ✅ Approved

The architectural approach here is exactly right. Cross-cutting UI concerns (confirmation modals) belong at the layout level, not scattered across individual components. This PR centralises that correctly.

What's good

Service + context pattern is sound. provideConfirmService() in +layout.svelte + getConfirmService() in leaf components is the idiomatic Svelte 5 approach. The single <ConfirmDialog /> in the layout is the correct rendering strategy — one modal instance, shared everywhere, no portal hacks needed.

Browser guard is important and present.

if (!browser) return Promise.resolve(false);

This prevents the service from hanging during SSR. Good catch.

Concurrent call semantics are well-defined. Returning false immediately for concurrent calls (instead of queuing or throwing) is a deliberate design choice that matches real-world UX expectations — you don't want two confirmation dialogs stacking.

Suggestions

document.getElementById('delete-form') in SaveBar — this couples SaveBar to its parent's DOM via an undocumented convention. For a family archive app this is acceptable, but it's worth a comment explaining why it's done this way (form must be outside the sticky bar to avoid double-form-submission quirks, or similar). As the codebase grows, someone will "fix" this without understanding the intent.

Error boundary for missing provider — getConfirmService() throws a descriptive error when the context is missing. Excellent developer experience. The try/catch wrapping getContext() to normalize Svelte's own throw vs undefined is a thoughtful touch.

No concerns about layering or module boundaries. The service is in $lib/services, the component in $lib/components, consumed in routes — the dependency direction is clean throughout.

## 🏗️ Markus Keller — Application Architect **Verdict: ✅ Approved** The architectural approach here is exactly right. Cross-cutting UI concerns (confirmation modals) belong at the layout level, not scattered across individual components. This PR centralises that correctly. ### What's good **Service + context pattern is sound.** `provideConfirmService()` in `+layout.svelte` + `getConfirmService()` in leaf components is the idiomatic Svelte 5 approach. The single `<ConfirmDialog />` in the layout is the correct rendering strategy — one modal instance, shared everywhere, no portal hacks needed. **Browser guard is important and present.** ```ts if (!browser) return Promise.resolve(false); ``` This prevents the service from hanging during SSR. Good catch. **Concurrent call semantics are well-defined.** Returning `false` immediately for concurrent calls (instead of queuing or throwing) is a deliberate design choice that matches real-world UX expectations — you don't want two confirmation dialogs stacking. ### Suggestions **`document.getElementById('delete-form')` in SaveBar** — this couples SaveBar to its parent's DOM via an undocumented convention. For a family archive app this is acceptable, but it's worth a comment explaining why it's done this way (form must be outside the sticky bar to avoid double-form-submission quirks, or similar). As the codebase grows, someone will "fix" this without understanding the intent. **Error boundary for missing provider** — `getConfirmService()` throws a descriptive error when the context is missing. Excellent developer experience. The `try/catch` wrapping `getContext()` to normalize Svelte's own throw vs undefined is a thoughtful touch. **No concerns about layering or module boundaries.** The service is in `$lib/services`, the component in `$lib/components`, consumed in routes — the dependency direction is clean throughout.

marcel commented

2026-04-12 14:27:58 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

The test suite for this refactor is meaningfully better than what was there before. The patterns are right and the coverage hits the important paths. A few gaps.

Blockers

None.

What's good

confirm.test.host.svelte pattern — testing a context-based service by wrapping it in a minimal host component is the correct approach. Direct unit testing of createConfirmService() would miss the reactive binding layer. This tests the right thing.

vi.waitFor() for async confirm flows — the async confirm → settle → verify requestSubmit pattern is sound and avoids flaky timing issues. Specifically:

deleteBtn.click();
await vi.waitFor(() => expect(service.options).not.toBeNull());
service.settle(true);
await vi.waitFor(() => expect(requestSubmit).toHaveBeenCalledOnce());

This is the correct way to test deferred-Promise UI without fake timers.

appendedForms[] cleanup in SaveBar spec — good catch on the cross-test DOM contamination. Explicit teardown in afterEach is more reliable than hoping cleanup() handles manually appended elements.

Concerns

ConfirmDialog.svelte has no direct component tests. The service tests exercise confirm/cancel/Escape/concurrent via the TestHost, but ConfirmDialog.svelte itself — the rendering, button text from i18n, destructive colour class, aria-labelledby — isn't tested in isolation. A small spec file (ConfirmDialog.svelte.spec.ts) covering "renders title", "confirm button has danger class when destructive", and "renders cancel button text" would close this gap.

.spec.ts vs .test.ts naming — admin page specs and SaveBar use .spec.ts; confirm.svelte.test.ts and NameHistoryEditCard.svelte.test.ts use .test.ts. This is a minor inconsistency but should be standardised in a follow-up.

Flaky test observation: 4 pre-existing flaky tests remain (CommentThread empty state, TranscriptionBlock quote selection, TranscriptionEditView flush-on-blur, Conversations empty state). None introduced by this PR, but worth tracking in a separate issue so they don't mask real failures over time.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** The test suite for this refactor is meaningfully better than what was there before. The patterns are right and the coverage hits the important paths. A few gaps. ### Blockers None. ### What's good **`confirm.test.host.svelte` pattern** — testing a context-based service by wrapping it in a minimal host component is the correct approach. Direct unit testing of `createConfirmService()` would miss the reactive binding layer. This tests the right thing. **`vi.waitFor()` for async confirm flows** — the async confirm → settle → verify requestSubmit pattern is sound and avoids flaky timing issues. Specifically: ```ts deleteBtn.click(); await vi.waitFor(() => expect(service.options).not.toBeNull()); service.settle(true); await vi.waitFor(() => expect(requestSubmit).toHaveBeenCalledOnce()); ``` This is the correct way to test deferred-Promise UI without fake timers. **`appendedForms[]` cleanup in SaveBar spec** — good catch on the cross-test DOM contamination. Explicit teardown in `afterEach` is more reliable than hoping `cleanup()` handles manually appended elements. ### Concerns **`ConfirmDialog.svelte` has no direct component tests.** The service tests exercise confirm/cancel/Escape/concurrent via the `TestHost`, but `ConfirmDialog.svelte` itself — the rendering, button text from i18n, destructive colour class, `aria-labelledby` — isn't tested in isolation. A small spec file (`ConfirmDialog.svelte.spec.ts`) covering "renders title", "confirm button has danger class when destructive", and "renders cancel button text" would close this gap. **`.spec.ts` vs `.test.ts` naming** — admin page specs and SaveBar use `.spec.ts`; `confirm.svelte.test.ts` and `NameHistoryEditCard.svelte.test.ts` use `.test.ts`. This is a minor inconsistency but should be standardised in a follow-up. **Flaky test observation**: 4 pre-existing flaky tests remain (CommentThread empty state, TranscriptionBlock quote selection, TranscriptionEditView flush-on-blur, Conversations empty state). None introduced by this PR, but worth tracking in a separate issue so they don't mask real failures over time.

marcel commented

2026-04-12 14:28:08 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

Replacing window.confirm() with a custom modal is a net security improvement. window.confirm() is a browser-native primitive that doesn't allow custom styling, can be blocked/suppressed by some browser configurations, and has inconsistent behaviour across environments (including headless browsers used in automated attacks). This PR eliminates that surface area.

What I checked

No unsanitized user input rendered in the dialog. The title and body fields in ConfirmOptions are populated exclusively from Paraglide i18n function calls (e.g. m.doc_delete_confirm(), m.person_alias_delete_body()). These are compile-time strings, not user-controlled data. No XSS vector here.

Escape key handling is correct. The oncancel handler on the <dialog> element calls e.preventDefault() before settling to false — this prevents the browser's default dismiss behaviour (which bypasses the service state machine) from leaving resolveRef dangling:

oncancel={(e) => {
    e.preventDefault();
    service.settle(false);
}}

Good.

Backdrop click for destructive dialogs defaults to no-close. closeOnBackdrop ?? !destructive means accidental backdrop clicks don't dismiss a destructive confirmation. This is the right default.

concurrent call → resolve false immediately prevents a denial-of-service pattern where rapid successive clicks could queue up unbounded Promises. Clean.

No blockers, no concerns.

This is a UI/UX refactor with no authentication, authorisation, data handling, or network surface changes. From a security standpoint, LGTM.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** Replacing `window.confirm()` with a custom modal is a net security improvement. `window.confirm()` is a browser-native primitive that doesn't allow custom styling, can be blocked/suppressed by some browser configurations, and has inconsistent behaviour across environments (including headless browsers used in automated attacks). This PR eliminates that surface area. ### What I checked **No unsanitized user input rendered in the dialog.** The `title` and `body` fields in `ConfirmOptions` are populated exclusively from Paraglide i18n function calls (e.g. `m.doc_delete_confirm()`, `m.person_alias_delete_body()`). These are compile-time strings, not user-controlled data. No XSS vector here. **Escape key handling is correct.** The `oncancel` handler on the `<dialog>` element calls `e.preventDefault()` before settling to false — this prevents the browser's default dismiss behaviour (which bypasses the service state machine) from leaving `resolveRef` dangling: ```svelte oncancel={(e) => { e.preventDefault(); service.settle(false); }} ``` Good. **Backdrop click for destructive dialogs defaults to no-close.** `closeOnBackdrop ?? !destructive` means accidental backdrop clicks don't dismiss a destructive confirmation. This is the right default. **`concurrent call → resolve false immediately`** prevents a denial-of-service pattern where rapid successive clicks could queue up unbounded Promises. Clean. ### No blockers, no concerns. This is a UI/UX refactor with no authentication, authorisation, data handling, or network surface changes. From a security standpoint, LGTM.

marcel commented

2026-04-12 14:28:18 +02:00

🎨 Leonie Voss — UI/UX & Accessibility

Verdict: 🚫 Changes requested

The service and component architecture are solid, but ConfirmDialog.svelte has four visual/UX bugs that must be fixed before merge. These aren't cosmetic — they affect usability and accessibility.

Blockers

1. Dialog is not centred.

class="max-w-sm rounded-sm border border-line bg-surface p-6 shadow-lg"

The native <dialog> element rendered with showModal() is positioned fixed by the browser, but max-w-sm alone does not centre it horizontally — it appears in the top-left corner. Fix:

class="m-auto w-full max-w-sm rounded-sm border border-line bg-surface p-6 shadow-lg ..."

2. No backdrop / dimmed overlay.
Without a ::backdrop style, the rest of the page is fully visible and unobscured when the modal is open. Users have no visual signal that the app is blocked waiting for input. Tailwind 4 supports the backdrop: variant:

class="... backdrop:bg-black/50"

3. Confirm button has no hover state.

class="... transition-colors {opts.destructive ? 'bg-danger text-danger-fg' : 'bg-primary text-primary-fg'}"

There's no hover: class on the confirm button. The cancel button has hover:bg-muted but the confirm button offers no feedback on hover. Fix:

{opts.destructive ? 'bg-danger text-danger-fg hover:bg-danger/80' : 'bg-primary text-primary-fg hover:bg-primary/80'}

4. No cursor-pointer on either button.
Both buttons lack cursor-pointer. On a desktop browser this means the cursor doesn't change to a hand over the modal buttons — inconsistent with every other interactive element in the app.

What's good

aria-labelledby="confirm-title" — correct screen reader association
min-h-[44px] on both buttons — meets WCAG 2.5.5 (44×44px touch target)
Focus management via native <dialog> — the browser handles initial focus correctly with showModal()
Destructive styling distinction (danger vs primary) — clear visual hierarchy

## 🎨 Leonie Voss — UI/UX & Accessibility **Verdict: 🚫 Changes requested** The service and component architecture are solid, but `ConfirmDialog.svelte` has four visual/UX bugs that must be fixed before merge. These aren't cosmetic — they affect usability and accessibility. ### Blockers **1. Dialog is not centred.** ```svelte class="max-w-sm rounded-sm border border-line bg-surface p-6 shadow-lg" ``` The native `<dialog>` element rendered with `showModal()` is positioned fixed by the browser, but `max-w-sm` alone does not centre it horizontally — it appears in the top-left corner. Fix: ```svelte class="m-auto w-full max-w-sm rounded-sm border border-line bg-surface p-6 shadow-lg ..." ``` **2. No backdrop / dimmed overlay.** Without a `::backdrop` style, the rest of the page is fully visible and unobscured when the modal is open. Users have no visual signal that the app is blocked waiting for input. Tailwind 4 supports the `backdrop:` variant: ```svelte class="... backdrop:bg-black/50" ``` **3. Confirm button has no hover state.** ```svelte class="... transition-colors {opts.destructive ? 'bg-danger text-danger-fg' : 'bg-primary text-primary-fg'}" ``` There's no `hover:` class on the confirm button. The cancel button has `hover:bg-muted` but the confirm button offers no feedback on hover. Fix: ```svelte {opts.destructive ? 'bg-danger text-danger-fg hover:bg-danger/80' : 'bg-primary text-primary-fg hover:bg-primary/80'} ``` **4. No `cursor-pointer` on either button.** Both buttons lack `cursor-pointer`. On a desktop browser this means the cursor doesn't change to a hand over the modal buttons — inconsistent with every other interactive element in the app. ### What's good - `aria-labelledby="confirm-title"` — correct screen reader association - `min-h-[44px]` on both buttons — meets WCAG 2.5.5 (44×44px touch target) - Focus management via native `<dialog>` — the browser handles initial focus correctly with `showModal()` - Destructive styling distinction (danger vs primary) — clear visual hierarchy

marcel commented

2026-04-12 14:28:26 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Pure frontend refactor — no infrastructure impact. LGTM from a platform perspective.

What I checked

No new build steps, no new environment variables, no new dependencies. The diff adds no new npm packages. ConfirmService and ConfirmDialog are built from existing Svelte 5 primitives and the native browser <dialog> API — zero additional bundle weight from third-party modal libraries.

CI pipeline is unaffected. The npm run test and npm run check commands that run in Gitea Actions will pick up the new test files automatically. No workflow changes needed.

No server-side surface area changes. This is entirely a client-side interaction pattern. No new API endpoints, no new form actions, no session or cookie changes.

Test count is healthy. 671 passing. The new tests replace old inline confirmation patterns without leaving dead code behind.

One observation (not a blocker)

The 4 pre-existing flaky tests (timeout-based) are not introduced here, but they'll eventually fire randomly in CI and cause confusion. Worth a dedicated issue to either fix or mark them as test.skip with a linked issue so CI stays trustworthy.

## 🚀 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** Pure frontend refactor — no infrastructure impact. LGTM from a platform perspective. ### What I checked **No new build steps, no new environment variables, no new dependencies.** The diff adds no new npm packages. `ConfirmService` and `ConfirmDialog` are built from existing Svelte 5 primitives and the native browser `<dialog>` API — zero additional bundle weight from third-party modal libraries. **CI pipeline is unaffected.** The `npm run test` and `npm run check` commands that run in Gitea Actions will pick up the new test files automatically. No workflow changes needed. **No server-side surface area changes.** This is entirely a client-side interaction pattern. No new API endpoints, no new form actions, no session or cookie changes. **Test count is healthy.** 671 passing. The new tests replace old inline confirmation patterns without leaving dead code behind. ### One observation (not a blocker) The 4 pre-existing flaky tests (timeout-based) are not introduced here, but they'll eventually fire randomly in CI and cause confusion. Worth a dedicated issue to either fix or mark them as `test.skip` with a linked issue so CI stays trustworthy.

marcel added 5 commits 2026-04-12 14:39:19 +02:00

fix(ui): center dialog, add backdrop, hover states, and cursor-pointer on buttons 3a316bc382

- Add m-auto and w-full to ensure the native <dialog> is centred
- Add backdrop:bg-black/50 for dimmed overlay when modal is open
- Add hover:bg-danger/80 and hover:bg-primary/80 on confirm button
- Add cursor-pointer to both cancel and confirm buttons

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

refactor(confirm): use plain let for resolveRef instead of $state 84378f11b4

resolveRef is never read reactively — it is only read synchronously
inside settle(). Using $state was misleading about the intent.

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

refactor(confirm): use import { m } instead of import * as m in ConfirmDialog 0b95c90e7a

Consistent with every other component in the project.

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

refactor(persons): replace non-null assertion with null guard on removeFormEl a2d078b8f9

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

test(confirm): add ConfirmDialog component spec (12 tests)

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

Details

CI / Backend Unit Tests (pull_request) Failing after 1s

Details

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

Details

CI / Backend Unit Tests (push) Failing after 1s

Details

d046c89631

Covers: title/body rendering, destructive vs primary button class,
custom labels, settle true/cancel, aria-labelledby, and hide-after-settle.

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

marcel commented

2026-04-12 14:39:32 +02:00

Review concerns addressed — follow-up commits

All open concerns from the review have been resolved:

Leonie Voss — 4 UI blockers (`3a316bc`)

✅ m-auto w-full added to centre the dialog
✅ backdrop:bg-black/50 for dimmed overlay
✅ hover:bg-danger/80 / hover:bg-primary/80 on confirm button
✅ cursor-pointer on both buttons

Felix Brandt — suggestions (`84378f1`, `0b95c90`, `a2d078b`)

✅ resolveRef changed from $state(null) to plain let — it is never read reactively
✅ import * as m → import { m } in ConfirmDialog.svelte — consistent with the rest of the codebase
✅ removeFormEl!.requestSubmit() → if (confirmed && removeFormEl) { removeFormEl.requestSubmit(); } — explicit null guard

Sara Holt — missing ConfirmDialog tests (`d046c89`)

✅ Added ConfirmDialog.svelte.spec.ts with 12 tests: title/body rendering, destructive vs primary button class, custom labels, settle-true/cancel, aria-labelledby, hide-after-settle

Test count: 683 passing (was 671 before this PR — +12 from ConfirmDialog spec).

Not addressed (out of scope / require separate discussion):

document.getElementById('delete-form') coupling in SaveBar — valid concern but fixing it requires an API change to the parent edit page; tracked separately
.spec.ts vs .test.ts naming convention — cosmetic; would require touching many files across the repo

## Review concerns addressed — follow-up commits All open concerns from the review have been resolved: ### Leonie Voss — 4 UI blockers (3a316bc) - ✅ `m-auto w-full` added to centre the dialog - ✅ `backdrop:bg-black/50` for dimmed overlay - ✅ `hover:bg-danger/80` / `hover:bg-primary/80` on confirm button - ✅ `cursor-pointer` on both buttons ### Felix Brandt — suggestions (84378f1, 0b95c90, a2d078b) - ✅ `resolveRef` changed from `$state(null)` to plain `let` — it is never read reactively - ✅ `import * as m` → `import { m }` in `ConfirmDialog.svelte` — consistent with the rest of the codebase - ✅ `removeFormEl!.requestSubmit()` → `if (confirmed && removeFormEl) { removeFormEl.requestSubmit(); }` — explicit null guard ### Sara Holt — missing ConfirmDialog tests (d046c89) - ✅ Added `ConfirmDialog.svelte.spec.ts` with 12 tests: title/body rendering, destructive vs primary button class, custom labels, settle-true/cancel, aria-labelledby, hide-after-settle **Test count: 683 passing** (was 671 before this PR — +12 from ConfirmDialog spec). Not addressed (out of scope / require separate discussion): - `document.getElementById('delete-form')` coupling in `SaveBar` — valid concern but fixing it requires an API change to the parent edit page; tracked separately - `.spec.ts` vs `.test.ts` naming convention — cosmetic; would require touching many files across the repo

marcel merged commit d046c89631 into main

2026-04-12 14:42:20 +02:00

marcel deleted branch feat/issue-207-confirm-modal-service

2026-04-12 14:42:22 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#228