Skip to content

PT-4193: Add secondary notification action, position, and dismissible#2561

Merged
lyonsil merged 16 commits into
mainfrom
pt-4193-notification-secondary-action
Jul 17, 2026
Merged

PT-4193: Add secondary notification action, position, and dismissible#2561
lyonsil merged 16 commits into
mainfrom
pt-4193-notification-secondary-action

Conversation

@rolfheij-sil

@rolfheij-sil rolfheij-sil commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

What

Adds four additive, backward-compatible optional fields to PlatformNotification so an extension can raise a two-button, specifically-placed, must-answer toast:

  • secondaryClickCommandLabel / secondaryClickCommand — a second action, mapped to Sonner's cancel slot alongside the existing action. Both send the notification id as the command's single argument (no new args plumbing — the model still carries no command args).
  • position — a per-toast placement (NotificationPosition, e.g. 'top-center'), passed straight through to Sonner; undefined keeps the Toaster's default.
  • dismissible — passed through to Sonner; set false (with duration: 0) for a toast the user must answer via an action button rather than swipe/drag away.

Wired through notification.service-host.ts (the existing action block gains a parallel cancel block; position and dismissible go straight into toastOptions) and the OpenRPC schema; papi.d.ts regenerated via npm run build:types.

Why

This is the reusable platform capability the scheduled Send/Receive consent toast (PT-4193, extension side) needs: a persistent top-center prompt with "Send/Receive now" / "Postpone" buttons the user must answer. dismissible: false matters — Sonner 1.7.4 defaults to swipe-to-dismiss, which for a toast an extension awaits a decision on would otherwise let an accidental drag strand the waiting code.

Decision record: PT-4158 UX meeting 2026-07-15 (Q1 = option B); PT-4193 ticket comment 27495.

Back-compat

All four fields are optional. When omitted, cancel, position, and dismissible are undefined, so existing single-action / default-placement toasts are unchanged (Sonner treats explicit undefined as absent). No private code — this is public scaffolding; the private consent gate lives in the extension.

Tests

notification.service-host.test.ts +4 cases: cancel built from the secondary fields; position forwarded; dismissible forwarded; all three undefined by default. npx vitest run src/renderer/services/notification.service-host.test.ts → 9/9. Targeted eslint clean; typecheck:core adds nothing over the pre-existing baseline (one unrelated missing-buildInfo.json error).

Review fix (PT-4193): the dismissible: false cancel-slot trap

Review flagged a BLOCKER, verified against the Sonner 1.7.4 source (node_modules/sonner/dist/index.mjs): Sonner computes V = toast.dismissible !== false and gates the cancel-slot (our secondaryClickCommand) button's onClick on V, but does not gate the action button the same way:

// cancel button: onClick: r => { tt(t.cancel) && V && ((t.cancel.onClick)?.(r), $()) }        // gated on V
// action button:  onClick: r => { tt(t.action) && ((t.action.onClick)?.(r), !r.defaultPrevented && $()) } // NOT gated

So dismissible: false combined with a secondaryClickCommand produced a dead secondary button — no command fires, no dismiss happens, silently. The consuming consent toast (extension PR #183) uses exactly that combination, and this PR's own original JSDoc on dismissible recommended it ("set to false … combine with duration: 0" for an awaited decision) — i.e. the doc was steering callers straight at the trap. Sonner has only one ungated button slot (action), so a toast with two working buttons must keep dismissible: true.

Fix — swipe-dismiss becomes a first-class exit:

  • Added dismissClickCommand?: keyof CommandHandlers to PlatformNotification: a parameterless-besides-the-notification-id command invoked when the user dismisses the toast (swipe/drag past Sonner's threshold, or a close button if one is ever enabled). Mapped in notification.service-host.ts to Sonner's onDismiss option, with a .catchlogger.warn so a rejected command handler can't become an unhandled rejection.
  • Verified against the Sonner source which paths call onDismiss: only the swipe-release handler (onPointerUp, gated on the same dismissible flag) and the close-button onClick (also gated, and disabled by default since this app doesn't pass closeButton to <Toaster />) call it. Programmatic dismissal (INotificationService.dismiss()toast.dismiss(id)) marks the toast delete: true, which only triggers the internal removal animation — it does not call onDismiss. Auto-close when duration elapses calls a different callback (onAutoClose), also not onDismiss. This is documented on the new field so consumers awaiting a decision know a programmatic dismiss() call will not trigger it.
  • Rewrote the dismissible JSDoc: removed the "combine with duration: 0" recommendation that steered callers into the trap; added a warning that dismissible: false also disables the secondary/cancel button (and the close button), so it's only safe on a notification with no secondary action. Points callers at dismissClickCommand + dismissible: true for the "must answer" case instead.
  • Same-review hardening: added a matching .catchlogger.warn on the secondary button's sendCommand call (it previously had none, unlike the primary path's now-consistent handling); constrained the OpenRPC schema's position property with an enum of the six NotificationPosition values (was a bare type: 'string').
  • Regenerated papi.d.ts; the diff touches only the notification.service-model module.

Tests: extended notification.service-host.test.ts from 9 to 13 cases — added the onDismiss mapping, both new .catch-swallows-rejection-and-logs paths (secondary click, dismiss click), and broadened the back-compat case to also assert onDismiss: undefined. Also added notification-display.test.tsx (1 test) — the one test in this suite that renders the real <Toaster /> with real Sonner (every other case here mocks sonner wholesale, which is exactly why this blocker shipped undetected): it sends a toast with a secondary command and dismissible left at its default true, clicks the actual rendered cancel-slot DOM button, and asserts the secondary command was sent — pinning the real Sonner button-gating contract instead of just the shape we hand it. 14/14 green across both files; targeted eslint clean; typecheck:core still adds nothing over the pre-existing baseline.

Contract note for the consumer: extension PR #183 will drop dismissible: false and set dismissClickCommand to a "postpone" command, keeping dismissible: true.

Follow-up fix (PT-4193, 938749040d7): a live E2E screenshot showed a two-button toast (action + cancel, both long labels) with the message text crushed to a ~1-character-wide sliver — Sonner's default layout puts icon/content/cancel/action in one un-wrapped flex row, and two non-shrinking buttons squeeze the content column to nothing. Fixed with CSS only, scoped via :has() so single-button/plain toasts are unaffected: notification-display.tsx now wires the Toaster's toastOptions.classNames (verified against Sonner 1.7.4's index.d.ts) onto toast/content/actionButton/cancelButton, and the new notification-display.scss grows the content row to fill the toast and forces the two buttons onto their own row beneath it (via the standard flex-wrap "empty flex-basis: 100% item" line-break technique), right-aligned and RTL-safe for free via Sonner's existing auto-margin CSS. Extended notification-display.test.tsx from 1 to 4 cases pinning the DOM shape the stylesheet rule depends on (two-button toasts get all three class hooks under one shared flex-container ancestor with both buttons still working; single-button/plain toasts don't). 17/17 across the notification suite, 979/979 full core suite; targeted eslint + stylelint clean; typecheck:core and papi.d.ts unchanged.

Follow-up fix (PT-4193, 5a4b763af4e): live E2E testing of Send/Receive surfaced an orphaned "Syncing (0%)" toast that never went away — a sync abort's dismiss(notificationId) clears the toast-id mapping, but an in-flight, fire-and-forget progress send() for that same id can land just after, find no mapping, and resurrect a brand-new toast that nothing then dismisses. Fixed with a short (5s) grace window: dismiss() now remembers each notificationId briefly after dismissing it, and send() drops an update-style send (an explicit, existing notificationId) that falls within that window instead of creating a new toast, logging a debug line. Brand-new notifications (no id passed) are never affected, and legitimately reusing an id after the window elapses still works normally; entries are pruned lazily on send()/dismiss() — no timers. Extended notification.service-host.test.ts with 3 cases (drop-on-race + debug log, post-grace-window reuse via fake timers, id-less send unaffected) — 20/20 green across the notification suite; targeted eslint clean; typecheck:core and papi.d.ts unchanged.

Rebase note (2026-07-16): rebased onto main after #2555 merged. No conflicts — the papi.d.ts auto-merge (both sides had regenerated it) was verified by re-running npm run build:types post-rebase: zero drift, so the committed file is exactly the machine-generated output. Notification suites 20/20, targeted eslint + stylelint clean. New tip: 43d761546d7.

🤖 Generated with Claude Code


Fix round — 2026-07-17 (response to lyonsil's review)

All 30 findings addressed at tip 1e2a99c (three commits):

  • a0cd67cgrace window deleted (findings 1, 5, 6, 7, 8, 16, 18, 22, 29; moots 24, 30). dismiss() is a true no-op when absent again; send() has no silent-drop path.
  • 14dc10btoast CSS reworked + keyboard reach (findings 2, 3, 4, 15, 23, 26): inert ::after/flex-wrap/block-size:0 overrides removed; deterministic two-row layout scoped to two-button toasts; hover bridge untouched; Alt+T now focus-cycles across every position list. Known residual: Escape still targets only Sonner's last list (cosmetic — Escape doesn't dismiss; the top-center group holds a single toast). Say the word if Escape parity is wanted as a follow-up.
  • 1e2a99cservice hardening (findings 9, 10, 11, 12, 13, 14, 17, 19, 20, 21, 25, 27, 28): update-merge keeps omitted fields (documented: no unset-on-update), onAutoClose wired, dismissible:false no longer silently kills the dismiss command, shared removal-command helper with .catch by construction, parallel localization, namespaced auto-ids, !== undefined id checks, derived position values.
  • 579bbfdround-4 E2E finding F3: the break-lock toast (long warning + single wide "Break lock and retry" action button, no cancel) still rendered text and button crammed side-by-side — the two-row reflow only engaged when BOTH buttons were present; the :has() rule now matches either button class so any buttoned toast reflows (lone buttons stay right-aligned via a pair-only margin override), +1 real-Sonner render test pinning the single-action DOM shape.
  • 0a566bfpress-collapse fix (credit: Sebastian's triage): holding any mouse button down on a buttoned toast collapsed it to a one-character sliver until release — Sonner sets data-swiping="true" on pointerdown before any movement, and the button-row ::before break was gated out of that state while flex-wrap/flex-basis: 0 stayed active. The break is now unconditional with an explicit position: static, out-winning Sonner's zero-specificity :where() swipe/removal ::before styles, so the two-row layout is stable through press, swipe, and removal (the ::after hover-bridge stays untouched); +1 real-Sonner render test pinning that a plain press (either button) enters the swiping state with the layout hooks intact.

Merge pairing: this PR and paratext-10-studio#163 should land in the same release window — the producer-side _syncGeneration guard (reworked on #163's branch) is now the protection for the S/R dismiss race. paratext-bible-internal-extensions#186 must not merge before this PR (its consent/lock toasts depend on position/dismissible/secondary-action fields).

Pending manual verification (needs a real browser; jsdom can't model Sonner's focus-trap): Alt+T cycling with a top-center consent toast + bottom-right toast present; the break-lock fast re-send flow (now un-broken by the window deletion).


This change is Reviewable

@rolfheij-sil
rolfheij-sil force-pushed the pt-4193-notification-secondary-action branch from 5a4b763 to 43d7615 Compare July 16, 2026 17:06

@lyonsil lyonsil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep review of the notification changes. 30 findings, filed inline and numbered by severity. Claims about Sonner are verified against the real 1.7.4 source; CSS claims are measured in a real browser against Sonner's actual self-injected stylesheet and DOM; behavioral claims are backed by probe tests. Where I checked a suspicion and it turned out wrong, I've said so in the comment rather than leaving it implied.

Headline: paratext-10-studio#163 already fixes the race this PR's grace window was written for

It fixes it at the producer, with the right primitive - an Interlocked _syncGeneration bumped before dismiss, re-checked before the queued send, with a dedicated NUnit test, labeled PT-4211 (the same bug).

And PT10's progress toast passes no NotificationId, while the drop path is gated on notificationId !== undefined - so the window can never fire for the producer it exists to protect. It fires for nobody it was built for, while silently breaking the "Break lock and retry" flow in paranext/paratext-bible-internal-extensions#186. Keeping both would also let this window mask a future regression in _syncGeneration, whose test only pins the increment, not the skip.

Recommendation: delete the grace window. Findings 5, 6, 7, 8, 16, 18, 22 and 29 all evaporate with it.

Also live

  • The ::after line-break in the new stylesheet is inert (3). Sonner already owns that pseudo-element with position: absolute. Deleting the entire block produces byte-identical geometry in Chromium; position: static fixes it.
  • flex-wrap: wrap introduces an orphaned icon row (4) for #186's real message strings - so the layout fix doesn't fix, and regresses.
  • Per-toast position breaks Alt+T and Escape (2). Sonner renders one <ol> per position sharing a single ref; the consent toast alone creates two, for its whole Infinity lifetime.
  • An update-send that omits an optional field clobbers it (9) - dismissible: false -> true, top-center -> bottom-right, cancel button gone.

Cross-PR coordination

#186's ConsentNotification intersection type makes its dependency on this PR look soft when it is hard. If #186 ships first, these fields are silently ignored: Postpone never renders, swipe fires nothing, requestConsent's promise never settles, and isConsentPending stays true - auto-sync dies silently for the session. #186 should not merge ahead of this PR, and a runtime capability check would be safer than the type-level intersection.

Credit where due: #186's comments show the team already found and worked around the dismissible: false trap in development, which is exactly why finding 11 matters for the next caller.

Notes

  • Five comments are anchored to the nearest line inside a diff hunk because their real target is an unchanged context line; each names its true line.
  • Findings tagged [pre-existing] aren't regressions from this PR - raised because the PR touches or re-exposes them, and several become trivial to fix alongside these changes.

This review and all of its inline comments were AI-generated with my guidance.

Comment thread src/renderer/services/notification.service-host.ts Outdated
Comment thread src/renderer/services/notification.service-host.ts
Comment thread src/renderer/components/notification-display.scss Outdated
Comment thread src/renderer/components/notification-display.scss
Comment thread src/renderer/services/notification.service-host.ts Outdated
Comment thread src/renderer/components/notification-display.scss Outdated
Comment thread src/renderer/services/notification.service-host.ts Outdated
Comment thread src/renderer/services/notification.service-host.ts Outdated
Comment thread src/renderer/services/notification.service-host.ts Outdated
Comment thread src/renderer/services/notification.service-host.test.ts Outdated
@rolfheij-sil

Copy link
Copy Markdown
Contributor Author

🤖 Claude: All 30 findings addressed; tip 1e2a99c. Summary:

  • Grace window deleted (a0cd67c): findings 5, 6, 7, 8, 16, 18, 22, 29 evaporate with it; 24 and 30 are moot once it's gone. On finding 1 specifically — we agree with deletion and did it — but the "can never fire for its producer" premise is off: the fire-and-forget UPDATE sends carry the minted NotificationId (verified in the current studio main patch ~L2031 and Adjusted Network Object and Data Provider Types #163 ~L3280), and dropping those id-bearing updates was the window's actual mechanism. Deletion stands on the other legs (ran before the awaits, wrong primitive, broke #186 flows), with Adjusted Network Object and Data Provider Types #163's atomic producer-side _syncGeneration guard as the right fix. Details inline on finding 1.
  • CSS override replaced, not patched (14dc10b): a deterministic two-row :has(.cancel):has(.action) layout, the line break moved to ::before so Sonner's ::after hover bridge is untouched, explicit right-alignment, and the Sonner-1.7.4 DOM dependency named in a comment (findings 3, 4, 15, 23, 26).
  • Position kept (UX-specified top-center) with keyboard reachability fixed at the source — Alt+T focus-cycling across lists; a separate Toaster per position was empirically disproven since Sonner renders every toast in every Toaster (finding 2).
  • Service host hardened (1e2a99c): merge-over-stored-state (9/14), onAutoClose wired (10), the dismissible: false footgun closed (11), a shared command helper with .catch by construction (12/25), map cleanup on all removal paths (13), Promise.all localization (17/27), namespaced string auto-ids (19), the falsy-0/'' fix (20), logger mocks (21), and a derived position enum (28).
  • Two honest residuals: Escape still targets only Sonner's single shared list, so it collapses an expanded stack on the last-mounted list only (cosmetic — it collapses rather than dismisses; follow-up on request); and the merge semantics have no unset-on-update (documented in TSDoc — omitting a field keeps it, and JSON-RPC drops undefined, so a field can't be cleared on update).
  • Merge coordination: this PR pairs with studio Adjusted Network Object and Data Provider Types #163 in the same release window, and #186 should not merge ahead of this PR — both recorded in the PR bodies.
  • Verification: the passing vitest suites cover the service-host and keyboard-registration logic, and a live build this morning confirmed the three things jsdom can't exercise — Alt+T cycles focus between the top-center consent toast and the bottom-right toasts; long messages wrap into the two-row layout with the icon staying beside the message (not orphaned); and #186's "Break lock and retry" re-send now renders a toast normally with the grace window gone. The one residual left unverified is Escape scope (it collapses only the last-mounted list). Requesting re-review.

@rolfheij-sil
rolfheij-sil requested a review from lyonsil July 17, 2026 09:36
rolfheij-sil and others added 11 commits July 17, 2026 17:33
Extend PlatformNotification with four optional, backward-compatible fields
so an extension can raise a two-button, specifically-placed, must-answer
toast:

- secondaryClickCommandLabel / secondaryClickCommand — a second action,
  mapped to Sonner's `cancel` slot alongside the existing `action` (both
  send the notification id as the command's single argument; no new args
  plumbing).
- position — a per-toast placement (NotificationPosition), passed straight
  through to Sonner; undefined keeps the Toaster's default.
- dismissible — passed through to Sonner; set false (with duration 0) for a
  toast the user must answer via an action button rather than swipe away.

Wire the new fields through the notification service host and the OpenRPC
schema, and regenerate papi.d.ts. This is the reusable platform capability
the scheduled Send/Receive consent toast (PT-4193, extension side) needs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sonner 1.7.4 gates the cancel-slot (secondaryClickCommand) button's onClick
on `dismissible !== false`, but not the action button. Combined with
dismissible: false - which this PR's own JSDoc recommended for a
must-answer toast - a two-button notification's secondary button quietly
did nothing: no command, no dismiss.

Add dismissClickCommand, a command invoked only when the user dismisses a
toast themselves (swipe past Sonner's threshold, or a close button, if one
is ever enabled) - verified against the Sonner source that this does NOT
fire for programmatic dismiss() or auto-close on duration. Wire it to
Sonner's onDismiss with a .catch to logger.warn, matching a similar catch
now added to the secondary button's onClick. Rewrite the dismissible JSDoc
to warn about the secondary-button trap instead of recommending it, and
point callers at dismissClickCommand + dismissible: true instead.

Also constrain the OpenRPC position property with an enum of the six
NotificationPosition values (was a bare string), and regenerate papi.d.ts.

Extend notification.service-host.test.ts for the onDismiss mapping and the
new catch paths, and add notification-display.test.tsx: the one test in
this suite that renders the real Toaster with real Sonner (every other
case mocks sonner wholesale, which is how this slipped through review) to
pin the actual cancel-slot button contract.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The committed papi.d.ts still carried the pre-reflow JSDoc line wrapping for
the notification model's dismissClickCommand/dismissible fields, while the
source notification.service-model.ts had been reflowed to prettier's 100-col
width. `npm run build` regenerates papi.d.ts from the source, reflowing those
comments, so CI's "Verify no files changed after build" step failed with
CHANGED_FILES: lib/papi-dts/papi.d.ts. Commit the regenerated (idempotent,
prettier-clean) output so the tree matches the build.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A live E2E screenshot of a Send/Receive consent toast (action "Send/Receive
now" + cancel "Postpone until 3:24 PM") showed the message text collapsed to
a ~1-character-wide vertical strip. Sonner 1.7.4 (node_modules/sonner/dist/
styles.css) lays out icon/content/cancel/action as one un-wrapped flex row;
its buttons are `flex-shrink: 0`, so two non-shrinking wide buttons crush the
content column. Any two-button toast with long labels hits this.

Fix is CSS-only, scoped via `:has()` so single-button and plain-message
toasts are unaffected: notification-display.tsx wires the Toaster's shared
toastOptions.classNames (documented Sonner customization hooks - verified
against index.d.ts) onto toast/content/actionButton/cancelButton, and the new
notification-display.scss keys a `.notification-toast:has(cancel):has(action)`
rule off them - grows the content row to fill the toast, and forces a line
break (an empty `flex-basis: 100%` pseudo-item, the standard flex-wrap "force
a new row" technique) so the two buttons wrap onto their own row beneath it.
Sonner's existing auto-margin CSS then right-aligns that row for free, and
lets the buttons stack further if they still don't both fit - all direction-
aware already (RTL-safe), no extra rules needed.

Extends notification-display.test.tsx (the one real-Sonner render test in
this suite) with 3 cases pinning the DOM shape the stylesheet rule assumes:
two-button toasts get all three class hooks under one shared flex-container
ancestor and both buttons still fire their commands; single-button and plain
toasts don't pick up the other button's hook. 4/4 in that file, 17/17 across
the notification suite, 979/979 full core suite. Targeted eslint + stylelint
clean; typecheck:core unchanged (pre-existing missing-buildInfo.json error
only); build:types confirms papi.d.ts has no drift (this is rendering-only).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Live E2E testing of Send/Receive surfaced an orphaned "Syncing (0%)" toast
that never went away: a sync abort calls dismiss(notificationId), but an
in-flight fire-and-forget progress send() for the same id can arrive just
after, find no toast mapping, and resurrect a brand-new toast that nothing
then dismisses.

dismiss() now remembers each notificationId for a short (5s) grace window
after dismissing it; send() drops an update-style send (an explicit,
existing notificationId) that falls in that window instead of creating a
new toast, logging a debug line. Brand-new notifications (no id passed) are
never affected, and reusing an id after the window elapses works normally.
Entries are pruned lazily on send()/dismiss() - no timers.

Adds 3 tests to notification.service-host.test.ts covering the drop, the
post-grace-window reuse (fake timers), and that id-less sends are
unaffected - 20 tests green across the two notification test files
(17 baseline + 3 new).
…w C61-1)

The 5-second dismissed-id grace window was meant to stop a fire-and-forget
progress update from resurrecting a toast after its own dismiss(), but it guards
the race with the wrong primitive - a time+id blacklist that can't tell a stale
straggler from a deliberate reuse. As coded it runs before send()'s awaits, so it
never closes the interleaved race it names; it silently drops legitimate fast
dismiss-then-resend flows; and it makes dismiss() stamp an id even when no toast
existed, breaking its documented no-op contract.

The underlying producer race is fixed at the source in paratext-10-studio#163
(PT-4211) with an Interlocked _syncGeneration re-checked before each queued send,
so this consumer-side window is redundant - and keeping both would let the window
mask a regression in that generation guard. Delete it: the grace map, the prune
helper, the send() drop guard, dismiss()'s unconditional re-stamp, the related
TSDoc, and the tests that pinned the window's behavior. dismiss() returns to its
documented "no-op if not found" contract and send() no longer has a silent-drop
path.

Resolves review findings C61-1/5/6/7/8/16/18/22/29 (and moots C61-24).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe
…n list by keyboard (PT-4193 review C61-2/3/4/15/23/26)

CSS (C61-3/4/15/23/26): the previous notification-display.scss override was built
on an inert `::after` (Sonner already owns that pseudo-element with
`position: absolute`, so the `order`/`flex-basis` never applied), regressed the
layout with `flex-wrap: wrap` (orphaning the icon onto its own row for realistic
long messages), zeroed Sonner's `::after` hover-bridge with `block-size: 0`
(causing a collapse/re-expand flicker between stacked toasts), and relied on a
per-button `margin-left: auto` that splits space *between* the two buttons rather
than pairing them. Replace it with a deliberate two-row layout for the two-button
shape: the message keeps the icon's row (content `flex: 1 1 0` so it never wraps
below the icon), a full-width zero-height `::before` break (untouched by Sonner at
rest, so the hover-bridge `::after` is left alone) pushes the buttons onto their
own row, and the auto-margin is kept only on the leading button so cancel+action
sit as a right-aligned pair. A comment names the Sonner 1.7.4 DOM dependency this
necessarily relies on.

Hotkey (C61-2): a per-toast `position` makes Sonner render one `<ol>` per position
sharing a single ref, so Alt+T (and Escape) only reach the last list, and each
`<ol>` is a focus trap that ejects focus when tabbing toward a sibling list. Keep
per-toast position and layer a focus-cycling handler on NotificationDisplay so
repeated Alt+T reaches every non-empty list (blur-reset first to neutralize
Sonner's per-`<ol>` focus-trap restore; run on a microtask so it wins regardless
of listener order). The hotkey is declared once and shared with the Toaster.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe
…iew C61-9..30)

Fixes a cluster of latent/pre-existing defects the review surfaced:

- C61-9/14: an update-send that omits an optional field no longer clobbers the
  previously-set value. The host now merges each send over the last notification
  stored for that id before handing it to Sonner (whose own update re-derives
  every field, and even forces dismissible back to true when omitted). The
  "back-compat" test that cemented the clobbering now pins the merge behaviour.
- C61-10: onAutoClose is wired to the same dismiss-command path as onDismiss, so a
  timer-expired must-answer toast fires its dismissClickCommand (an implicit
  dismissal) instead of vanishing having fired nothing.
- C61-11: the host no longer forwards `dismissible: false` when a secondary or
  dismiss command is present (Sonner gates both those controls on that flag, so
  false would silently kill them). Documented on the public dismissible TSDoc.
- C61-12/25: the three near-identical localize -> sendCommand -> catch/log blocks
  are factored into one runRemovalCommand helper, which makes a .catch-less
  handler unconstructible - closing the primary action button's missing .catch.
- C61-13: toast-id bookkeeping is cleaned on every removal path (auto-close,
  swipe, action/cancel click, dismiss), not just dismiss(), via that helper.
- C61-17/27: the (up to three) localize round-trips run in one Promise.all,
  cutting display latency and closing the concurrent-send re-entrancy window.
- C61-19: id-less sends get an id in our own namespace instead of exposing
  Sonner's numeric auto-ids, so a caller using a numeric id can't collide.
- C61-20: notificationId presence is tested with `!== undefined` (and `??`), so
  the legal ids `0` and `''` update in place instead of duplicating.
- C61-21: `debug` added to the shared logger mock and the display test's mock.
- C61-28: NOTIFICATION_POSITIONS is a frozen array in the model that the type and
  the OpenRPC enum both derive from, replacing the hand-kept duplicate + its wrong
  "can't be derived" comment.

(C61-30's fake-timers gap vanished with the grace-window deletion in the first
commit.) Regenerates papi.d.ts for the model changes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe
…toasts (PT-4193 round-4 E2E F3)

Live round-4 E2E found the break-lock toast - one long warning message plus
one wide "Break lock and retry" action button, no cancel button - rendering
text and button crammed side-by-side: the `:has()` reflow rule only engaged
when BOTH an action and a cancel button were present. Match either button
class instead so any buttoned toast gets the [icon][message] / [buttons]
two-row layout, and keep the action button's auto-margin zeroing scoped to
the paired case so a lone action button stays right-aligned via Sonner's
own `margin-inline-start: auto`. Adds a real-Sonner render test pinning the
single-action-button DOM shape the rule keys off.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s swiping state (PT-4193)

Holding any mouse button down on a buttoned toast collapsed it to a
one-character-wide sliver until release: Sonner sets data-swiping="true"
on pointerdown, before any movement, and the button-row ::before flex
break was gated out of that state while flex-wrap and the content's
flex-basis: 0 stayed active. Ungate the break and declare position:
static so it beats Sonner's zero-specificity :where() swipe/removal
::before styles and stays in flex flow in every toast state; Sonner's
::after hover-bridge stays untouched (review C61). Found by Sebastian's
external triage.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rface @experimental

Mark the new notification secondary-action API as @experimental:
NOTIFICATION_POSITIONS, NotificationPosition, and PlatformNotification's
secondaryClickCommandLabel, secondaryClickCommand, dismissClickCommand,
position, and dismissible fields. Regenerate papi.d.ts.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe
@rolfheij-sil
rolfheij-sil force-pushed the pt-4193-notification-secondary-action branch from 0a566bf to f780c8d Compare July 17, 2026 15:33
@Sebastian-ubs

Copy link
Copy Markdown
Contributor

Since this PR is what introduces the notification secondary/cancel button as a platform capability, it should also settle its default look rather than leaving it as Sonner's built-in styling.

Proposal: .notification-toast-cancel-button in notification-display.scss should render as the shadcn secondary variant (from lib/platform-bible-react/src/components/shadcn-ui/button.tsxtw:bg-secondary tw:text-secondary-foreground tw:hover:bg-secondary/80 …). Semantically this matches shadcn's action = default / cancel = secondary convention, and it means every current and future two-button platform notification (S/R consent toast in #186, and anything downstream) gets a consistent look without each caller re-deciding.

One thing to watch when you add the rules: Sonner 1.7.4 emits data-cancel defaults at zero specificity via :where() — the new rules will likely need at least .notification-toast .notification-toast-cancel-button to win reliably (same pattern you already use on the ::before overrides).

@lyonsil lyonsil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified review of the notification changes (correctness plus a few cleanup/convention notes) - findings are inline. The only one I'd treat as a blocker is the first (the prod-only CSS specificity tie): it breaks the two-button toast alignment in packaged builds while looking fine in dev. The rest are lower severity.

(AI-assisted, with my guidance)

Comment thread src/renderer/components/notification-display.scss
Comment thread src/renderer/services/notification.service-host.ts
Comment thread src/shared/models/notification.service-model.ts
Comment thread src/renderer/components/notification-display.tsx Outdated
Comment thread src/renderer/components/notification-display.scss
Comment thread src/renderer/services/notification.service-host.ts Outdated
Comment thread src/renderer/services/notification.service-host.ts
Comment thread src/renderer/components/notification-display.tsx
Comment thread src/shared/models/notification.service-model.ts Outdated
Comment thread src/renderer/services/notification.service-host.test.ts Outdated

@lyonsil lyonsil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyonsil reviewed 8 files and all commit messages, and resolved 30 discussions.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on rolfheij-sil).

lyonsil and others added 4 commits July 17, 2026 14:26
- Honor dismissible:false when a secondary command has no label: the
  cancel button only renders when label+command are both set, so the
  drop-the-false gate now keys on the same localized-label condition the
  cancel block uses instead of the raw command field. New test pins it;
  model TSDoc updated to match.
- Document on `dismissible` that false does not keep the toast on
  screen - auto-close is governed solely by `duration`, so also set
  duration 0 for a sticky toast. Regenerate papi.d.ts.
- Merge the two lockstep bookkeeping maps (notification id -> toast id,
  notification id -> last notification) into one trackedNotificationsById
  so no removal path can clean up one half and leak the other. The
  existing-toast-id read stays post-await to keep concurrent same-id
  sends updating in place.
- Mint auto notification ids with newGuid() (like dialog.service-host)
  instead of a bespoke module-global counter.
- Make the Alt+T focus-cycling run after Sonner's handler via a
  macrotask instead of a microtask, so the ordering no longer depends on
  listener registration order.
- Add the Alt+T notification-toast entry to the keyboard shortcuts
  catalog per .claude/rules/keyboard-shortcuts-catalog.md.
- Test cleanup: commandStub() helper collapses the repeated
  as-never/eslint-disable casts (16 sites); drop the dead debug logger
  mocks left from the removed grace window.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dary (PT-4193)

Per UX direction on PR #2561: settle the platform default look of the
secondary (cancel-slot) notification button as the shadcn `secondary`
button variant (bg-secondary / text-secondary-foreground /
hover:bg-secondary/80). Without this, Sonner 1.7.4's styled mode renders
both buttons identically dark because its (0,3,0) base [data-button]
rule also hits the cancel button (which carries data-button) and its
softer :where([data-cancel]) defaults are zero-specificity.

Which button gets which look is deterministic and documented in the
model TSDoc: clickCommand always renders as the emphasized primary
button (Sonner's action slot), secondaryClickCommand always as the
muted secondary one (Sonner's cancel slot) - styling follows the field,
never ordering. The rule uses the same &:has() pattern as the layout
rules to sit at (0,4,0), beating Sonner's base rule regardless of
stylesheet injection order (verified in-browser in both orders, and
live: light theme shows muted-vs-dark hierarchy; papi.d.ts regenerated).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
notification-display.scss deliberately reaches into Sonner 1.7.4's
private toast DOM and stylesheet behavior (documented in that file), and
the jsdom tests can pin the DOM contract but not Sonner's CSS. Pin the
exact version so an upgrade is always a deliberate decision made while
looking at those comments, never a lockfile-regeneration side effect.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(PT-4193)

Sonner's Toaster defaulted to a fixed light theme, so a dark-themed app
showed white toasts - and once the secondary button took its colors from
the app-level --secondary variables (which flip with the app theme), it
rendered dark-theme colors on a still-white toast, collapsing the
primary/secondary hierarchy in dark mode.

Subscribe to the theme service's CurrentTheme (same pattern as
user-profile-popover) and pass the theme type to the Toaster. Theme type
is an open string, so anything that isn't exactly 'dark' gets the light
look. Verified live: toasts and both buttons flip correctly on theme
change, including on an already-open toast.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lyonsil
lyonsil requested a review from jolierabideau as a code owner July 17, 2026 20:18
@lyonsil

lyonsil commented Jul 17, 2026

Copy link
Copy Markdown
Member

@Sebastian-ubs Implemented in 9c3be33 - .notification-toast-cancel-button now renders as the shadcn secondary variant (var(--secondary) / var(--secondary-foreground), color-mix 80% hover), with three notes from implementation:

  • Specificity had to go higher than suggested. The cancel button also carries data-button, so Sonner 1.7.4's base [data-sonner-toast][data-styled='true'] [data-button] rule (0,3,0) - the reason both buttons currently render identically dark - would beat .notification-toast .notification-toast-cancel-button (0,2,0). The rule uses the same &:has() pattern as the layout overrides to sit at (0,4,0), order-independent of stylesheet injection.
  • Which button gets which look is deterministic and now documented in the model TSDoc: clickCommand always renders as the emphasized primary button (Sonner's action slot), secondaryClickCommand always as the muted secondary one (Sonner's cancel slot) - styling follows the field, never ordering.
  • Dark mode surfaced a gap while verifying this: the Toaster was hardcoded to Sonner's light theme, so in a dark-themed app the token-driven secondary button flipped to dark colors on a still-white toast and the hierarchy collapsed. Fixed in 4db3519 by subscribing the Toaster to the theme service's CurrentTheme (same pattern as user-profile-popover) - toasts and both buttons now follow the app theme, verified live in both themes including flipping the theme with a toast already open.

Light/dark screenshots of the new rendering to follow.

(AI-assisted, with my guidance)

Code comments should stand on their own: remove "PR #2561", "PT-4193",
and "review C61-N" prefixes/pointers from the notification display
comments, keeping the explanations themselves. The pre-existing deep
link to the duration formula's rationale (PT-2196 focused comment) stays
- it points at context that can't be inlined.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@lyonsil lyonsil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyonsil reviewed 10 files and all commit messages, and resolved 10 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved.

@lyonsil

lyonsil commented Jul 17, 2026

Copy link
Copy Markdown
Member
pr-2561-toast-light pr-2561-toast-dark

@lyonsil
lyonsil enabled auto-merge (squash) July 17, 2026 20:41
@lyonsil
lyonsil merged commit 86e4346 into main Jul 17, 2026
7 checks passed
@lyonsil
lyonsil deleted the pt-4193-notification-secondary-action branch July 17, 2026 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants