From 1ff72bc6fffb7a79fffe1c147a0d5aba0eb51f4b Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Wed, 15 Jul 2026 17:29:46 +0200 Subject: [PATCH 01/16] PT-4193: Add secondary notification action, position, and dismissible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- lib/papi-dts/papi.d.ts | 38 +++++++++++ .../notification.service-host.test.ts | 64 +++++++++++++++++++ .../services/notification.service-host.ts | 31 ++++++++- .../models/notification.service-model.ts | 39 +++++++++++ 4 files changed, 171 insertions(+), 1 deletion(-) diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index 92e9a81bc08..1707f295ef1 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -5212,6 +5212,17 @@ declare module 'shared/models/notification.service-model' { import { CommandHandlers } from 'papi-shared-types'; import { LocalizeKey } from 'platform-bible-utils'; export type Severity = 'info' | 'warning' | 'error'; + /** + * Where a notification is shown on screen. Mirrors the placements the host toast library supports. + * Omit to use the app's default placement. + */ + export type NotificationPosition = + | 'top-left' + | 'top-center' + | 'top-right' + | 'bottom-left' + | 'bottom-center' + | 'bottom-right'; /** Data needed to display a notification to the user */ export interface PlatformNotification { /** @@ -5237,6 +5248,33 @@ declare module 'shared/models/notification.service-model' { * The command handler should have the type signature {@link NotificationClickCommandHandler}. */ clickCommand?: keyof CommandHandlers; + /** + * Optional label for a second action button, shown alongside {@link clickCommandLabel}. Provide + * this together with {@link secondaryClickCommand} to give the notification two actions. + * + * Automatically localized if this is a {@link LocalizeKey}. + */ + secondaryClickCommandLabel?: string | LocalizeKey; + /** + * Optional command to run if users click on the secondary label in the notification. Like + * {@link clickCommand}, the command is sent one argument: + * + * - NotificationId: The ID of the notification that was clicked + * + * The command handler should have the type signature {@link NotificationClickCommandHandler}. + */ + secondaryClickCommand?: keyof CommandHandlers; + /** + * Optional placement of the notification on screen. When omitted, the app's default placement is + * used. + */ + position?: NotificationPosition; + /** + * Whether the user can dismiss the notification directly (e.g. by swiping/dragging it away). + * Defaults to `true`. Set to `false` for a notification that must be acknowledged through one of + * its action buttons — combine with a `duration` of `0` or less to keep it up until then. + */ + dismissible?: boolean; /** Optional ID of a previous notification to update instead of showing a new notification */ notificationId?: string | number; /** diff --git a/src/renderer/services/notification.service-host.test.ts b/src/renderer/services/notification.service-host.test.ts index cd7e30042f3..62acaf4572d 100644 --- a/src/renderer/services/notification.service-host.test.ts +++ b/src/renderer/services/notification.service-host.test.ts @@ -100,6 +100,70 @@ describe('notification service host', () => { }); }); + describe('secondary action and position', () => { + it('builds a Sonner cancel action from the secondary click fields', async () => { + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + secondaryClickCommandLabel: 'Postpone', + // The host passes this straight to Sonner without validating it against real command names, + // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. + // eslint-disable-next-line no-type-assertion/no-type-assertion + secondaryClickCommand: 'test.secondary' as never, + }; + + await capturedService.send(notification); + + expect(mockToastInfo).toHaveBeenCalledWith( + 'test', + expect.objectContaining({ + cancel: expect.objectContaining({ label: 'Postpone' }), + }), + ); + }); + + it('forwards a per-toast position to Sonner', async () => { + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + position: 'top-center', + }; + + await capturedService.send(notification); + + expect(mockToastInfo).toHaveBeenCalledWith( + 'test', + expect.objectContaining({ position: 'top-center' }), + ); + }); + + it('forwards dismissible so a toast can be made non-swipe-dismissible', async () => { + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + dismissible: false, + }; + + await capturedService.send(notification); + + expect(mockToastInfo).toHaveBeenCalledWith( + 'test', + expect.objectContaining({ dismissible: false }), + ); + }); + + it('leaves cancel, position, and dismissible undefined when the new fields are omitted (back-compat)', async () => { + const notification: PlatformNotification = { message: 'test', severity: 'info' }; + + await capturedService.send(notification); + + expect(mockToastInfo).toHaveBeenCalledWith( + 'test', + expect.objectContaining({ cancel: undefined, position: undefined, dismissible: undefined }), + ); + }); + }); + describe('send with a reused notificationId', () => { it('passes the existing toast id as the top-level id so Sonner updates instead of duplicating', async () => { mockToastInfo.mockReturnValueOnce('toast-1'); diff --git a/src/renderer/services/notification.service-host.ts b/src/renderer/services/notification.service-host.ts index 777bd447820..dd2af5cfd69 100644 --- a/src/renderer/services/notification.service-host.ts +++ b/src/renderer/services/notification.service-host.ts @@ -25,7 +25,17 @@ async function send(notification: PlatformNotification): Promise commandService.sendCommand(clickCommand, effectiveNotificationId), } : undefined, + // Second action button, rendered by Sonner as its `cancel` slot alongside `action`. Built the + // same way as `action`, and like it, sends the notification id as the command's single argument. + cancel: + secondaryClickCommandLabel && secondaryClickCommand + ? { + label: await localize(secondaryClickCommandLabel), + onClick: () => + commandService.sendCommand(secondaryClickCommand, effectiveNotificationId), + } + : undefined, + // Per-toast placement override; undefined leaves the Toaster's default placement in effect. + position, + // Whether the user can swipe/drag the toast away; undefined leaves Sonner's default (true). Set + // false (with duration 0) for a toast that must be answered via an action button. + dismissible, // Duration calc from https://paratextstudio.atlassian.net/browse/PT-2196?focusedCommentId=13075 duration, }; @@ -107,6 +132,10 @@ export async function startNotificationService(): Promise { severity: { type: 'string' }, clickCommand: { type: 'string' }, clickCommandLabel: { type: 'string' }, + secondaryClickCommand: { type: 'string' }, + secondaryClickCommandLabel: { type: 'string' }, + position: { type: 'string' }, + dismissible: { type: 'boolean' }, notificationId: { type: ['string', 'number'] }, duration: { type: 'number' }, }, diff --git a/src/shared/models/notification.service-model.ts b/src/shared/models/notification.service-model.ts index 0875b4e0275..9ec67e999ff 100644 --- a/src/shared/models/notification.service-model.ts +++ b/src/shared/models/notification.service-model.ts @@ -3,6 +3,18 @@ import { LocalizeKey } from 'platform-bible-utils'; export type Severity = 'info' | 'warning' | 'error'; +/** + * Where a notification is shown on screen. Mirrors the placements the host toast library supports. + * Omit to use the app's default placement. + */ +export type NotificationPosition = + | 'top-left' + | 'top-center' + | 'top-right' + | 'bottom-left' + | 'bottom-center' + | 'bottom-right'; + /** Data needed to display a notification to the user */ export interface PlatformNotification { /** @@ -28,6 +40,33 @@ export interface PlatformNotification { * The command handler should have the type signature {@link NotificationClickCommandHandler}. */ clickCommand?: keyof CommandHandlers; + /** + * Optional label for a second action button, shown alongside {@link clickCommandLabel}. Provide + * this together with {@link secondaryClickCommand} to give the notification two actions. + * + * Automatically localized if this is a {@link LocalizeKey}. + */ + secondaryClickCommandLabel?: string | LocalizeKey; + /** + * Optional command to run if users click on the secondary label in the notification. Like + * {@link clickCommand}, the command is sent one argument: + * + * - NotificationId: The ID of the notification that was clicked + * + * The command handler should have the type signature {@link NotificationClickCommandHandler}. + */ + secondaryClickCommand?: keyof CommandHandlers; + /** + * Optional placement of the notification on screen. When omitted, the app's default placement is + * used. + */ + position?: NotificationPosition; + /** + * Whether the user can dismiss the notification directly (e.g. by swiping/dragging it away). + * Defaults to `true`. Set to `false` for a notification that must be acknowledged through one of + * its action buttons — combine with a `duration` of `0` or less to keep it up until then. + */ + dismissible?: boolean; /** Optional ID of a previous notification to update instead of showing a new notification */ notificationId?: string | number; /** From 74330d83054d541319103893d2f9de48d500145b Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Wed, 15 Jul 2026 18:51:13 +0200 Subject: [PATCH 02/16] PT-4193: Fix dismissible: false silently disabling the secondary button 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 --- lib/papi-dts/papi.d.ts | 32 +++++- .../components/notification-display.test.tsx | 89 +++++++++++++++ .../notification.service-host.test.ts | 106 +++++++++++++++++- .../services/notification.service-host.ts | 45 +++++++- .../models/notification.service-model.ts | 32 +++++- 5 files changed, 291 insertions(+), 13 deletions(-) create mode 100644 src/renderer/components/notification-display.test.tsx diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index 1707f295ef1..f3a7b4d7720 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -5264,15 +5264,41 @@ declare module 'shared/models/notification.service-model' { * The command handler should have the type signature {@link NotificationClickCommandHandler}. */ secondaryClickCommand?: keyof CommandHandlers; + /** + * Optional command to run if the user dismisses the notification themselves — by swiping/dragging + * it away, or by clicking the close button (if the host ever enables one). Sent no arguments other + * than the notification id, like {@link clickCommand}: + * + * - NotificationId: The ID of the notification that was dismissed + * + * The command handler should have the type signature {@link NotificationClickCommandHandler}. + * + * IMPORTANT: this fires ONLY for that user gesture. It does NOT fire when the notification is + * dismissed programmatically via {@link INotificationService.dismiss}, when it auto-closes because + * `duration` elapsed, or when the user clicks {@link clickCommand} / {@link secondaryClickCommand} + * (verified against the Sonner 1.7.4 source: `onDismiss` is invoked only from the swipe-release and + * close-button handlers, never from the programmatic-dismiss or auto-close paths). Use this to treat + * a user's swipe-away as an explicit decision — e.g. pairing it with a "postpone" command lets a + * two-button, must-answer-style toast still keep {@link dismissible} `true` (see the warning on + * {@link dismissible} for why `false` is usually the wrong tool for that). + */ + dismissClickCommand?: keyof CommandHandlers; /** * Optional placement of the notification on screen. When omitted, the app's default placement is * used. */ position?: NotificationPosition; /** - * Whether the user can dismiss the notification directly (e.g. by swiping/dragging it away). - * Defaults to `true`. Set to `false` for a notification that must be acknowledged through one of - * its action buttons — combine with a `duration` of `0` or less to keep it up until then. + * Whether the user can dismiss the notification directly (e.g. by swiping/dragging it away, or via + * a close button). Defaults to `true`. + * + * WARNING: the host toast library (Sonner) renders {@link secondaryClickCommandLabel} in the same + * slot it gates on this flag, so setting `dismissible: false` also disables the secondary action + * button (and the close button) — the secondary button will still render but silently do nothing + * when clicked. Only set this to `false` on a notification with no secondary action. For a + * notification the user must explicitly answer, prefer leaving `dismissible: true` and using + * {@link dismissClickCommand} so a swipe-away still counts as a real (e.g. "postpone") decision + * instead of a dead button. */ dismissible?: boolean; /** Optional ID of a previous notification to update instead of showing a new notification */ diff --git a/src/renderer/components/notification-display.test.tsx b/src/renderer/components/notification-display.test.tsx new file mode 100644 index 00000000000..1ea9b4bf550 --- /dev/null +++ b/src/renderer/components/notification-display.test.tsx @@ -0,0 +1,89 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import type { + INotificationService, + PlatformNotification, +} from '@shared/models/notification.service-model'; +import * as commandService from '@shared/services/command.service'; +import { NotificationDisplay } from './notification-display'; + +// This is the ONE render-level test in the notification suite that uses REAL Sonner instead of +// mocking it (every other notification.service-host.test.ts case mocks 'sonner' wholesale, which is +// exactly why the cancel-slot-button-is-dead-when-dismissible-is-false blocker slipped through +// review undetected - see PT-4193's "Review fix" PR notes). Rendering the real Toaster and clicking +// the real DOM button pins the actual Sonner contract instead of just the shape we hand it. +vi.mock('@shared/services/command.service', () => ({ sendCommand: vi.fn() })); +vi.mock('@shared/services/localization.service', () => ({ + localizationService: { + getLocalizedString: vi.fn(({ localizeKey }: { localizeKey: string }) => + Promise.resolve(localizeKey), + ), + }, +})); +vi.mock('@shared/services/logger.service', () => ({ + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})); + +let capturedService: INotificationService; +vi.mock('@shared/services/network-object.service', () => ({ + networkObjectService: { + set: vi.fn((_name: string, service: INotificationService) => { + capturedService = service; + return Promise.resolve(); + }), + }, +})); + +const mockSendCommand = vi.mocked(commandService.sendCommand); + +// jsdom does not implement `window.matchMedia`; Sonner's Toaster calls it directly (unrelated to +// this app's own theme service) to pick its light/dark default. Precedent: +// share-layout.dialog.test.tsx hits the same gap for a different matchMedia caller. +function stubMatchMedia() { + window.matchMedia = vi.fn().mockImplementation((query: string) => ({ + matches: false, + media: query, + onchange: undefined, + addListener: vi.fn(), + removeListener: vi.fn(), + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + dispatchEvent: vi.fn(), + })); +} + +describe('NotificationDisplay with real Sonner', () => { + beforeEach(async () => { + vi.clearAllMocks(); + vi.resetModules(); + mockSendCommand.mockResolvedValue(undefined); + stubMatchMedia(); + const { startNotificationService } = await import( + '@renderer/services/notification.service-host' + ); + await startNotificationService(); + }); + + it('sends the secondary command when the user clicks the rendered cancel-slot button', async () => { + render(); + + const notification: PlatformNotification = { + message: 'A decision is needed', + severity: 'info', + secondaryClickCommandLabel: 'Postpone', + // The test only needs a command NAME to send/assert on; it never resolves to a real handler. + // eslint-disable-next-line no-type-assertion/no-type-assertion + secondaryClickCommand: 'test.secondary' as never, + // Left true (the default) deliberately: this is the exact "two working buttons" shape the + // blocker was about - dismissible: false would render this same button inert. + }; + + const notificationId = await capturedService.send(notification); + + const cancelButton = await screen.findByRole('button', { name: 'Postpone' }); + fireEvent.click(cancelButton); + + expect(mockSendCommand).toHaveBeenCalledWith('test.secondary', notificationId); + }); +}); diff --git a/src/renderer/services/notification.service-host.test.ts b/src/renderer/services/notification.service-host.test.ts index 62acaf4572d..5b07c912a9b 100644 --- a/src/renderer/services/notification.service-host.test.ts +++ b/src/renderer/services/notification.service-host.test.ts @@ -3,8 +3,24 @@ import type { INotificationService, PlatformNotification, } from '@shared/models/notification.service-model'; +import * as commandService from '@shared/services/command.service'; +import { logger } from '@shared/services/logger.service'; -const mockToastInfo = vi.fn(() => 'mock-toast-id'); +/** + * Minimal shape of the toastOptions object the host passes to Sonner's `toast.*` functions - typed + * just precisely enough to invoke `cancel.onClick` / `onDismiss` from tests without a type + * assertion (see `.eslintrc.cjs` `no-type-assertion` rule). + */ +interface CapturedToastOptions { + cancel?: { label: string; onClick: () => Promise }; + onDismiss?: () => Promise; +} + +// Typed via the explicit generic (rather than named-but-unused parameters on the implementation) +// so `mock.calls` is typed as `[string, CapturedToastOptions?]` without unused-arg lint errors. +const mockToastInfo = vi.fn<(message: string, options?: CapturedToastOptions) => string>( + () => 'mock-toast-id', +); const mockToastWarning = vi.fn(() => 'mock-toast-id'); const mockToastError = vi.fn(() => 'mock-toast-id'); const mockToast = Object.assign( @@ -30,6 +46,8 @@ vi.mock('@shared/services/logger.service', () => ({ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, })); +const mockSendCommand = vi.mocked(commandService.sendCommand); + let capturedService: INotificationService; vi.mock('@shared/services/network-object.service', () => ({ networkObjectService: { @@ -44,6 +62,7 @@ describe('notification service host', () => { beforeEach(async () => { vi.clearAllMocks(); vi.resetModules(); + mockSendCommand.mockResolvedValue(undefined); const { startNotificationService } = await import( '@renderer/services/notification.service-host' ); @@ -152,16 +171,97 @@ describe('notification service host', () => { ); }); - it('leaves cancel, position, and dismissible undefined when the new fields are omitted (back-compat)', async () => { + it('leaves cancel, position, dismissible, and onDismiss undefined when the new fields are omitted (back-compat)', async () => { const notification: PlatformNotification = { message: 'test', severity: 'info' }; await capturedService.send(notification); expect(mockToastInfo).toHaveBeenCalledWith( 'test', - expect.objectContaining({ cancel: undefined, position: undefined, dismissible: undefined }), + expect.objectContaining({ + cancel: undefined, + position: undefined, + dismissible: undefined, + onDismiss: undefined, + }), ); }); + + it('invokes the secondary command with the notification id when the cancel button is clicked', async () => { + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + secondaryClickCommandLabel: 'Postpone', + // The host passes this straight to Sonner without validating it against real command names, + // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. + // eslint-disable-next-line no-type-assertion/no-type-assertion + secondaryClickCommand: 'test.secondary' as never, + }; + + const notificationId = await capturedService.send(notification); + const options = mockToastInfo.mock.calls[0][1]; + await options?.cancel?.onClick(); + + expect(mockSendCommand).toHaveBeenCalledWith('test.secondary', notificationId); + }); + + it('logs a warning instead of throwing when the secondary command rejects', async () => { + mockSendCommand.mockRejectedValueOnce(new Error('boom')); + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + secondaryClickCommandLabel: 'Postpone', + // The host passes this straight to Sonner without validating it against real command names, + // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. + // eslint-disable-next-line no-type-assertion/no-type-assertion + secondaryClickCommand: 'test.secondary' as never, + }; + + await capturedService.send(notification); + const options = mockToastInfo.mock.calls[0][1]; + + // The catch handler must swallow the rejection - awaiting the returned promise must not throw. + await expect(options?.cancel?.onClick()).resolves.toBeUndefined(); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('boom')); + }); + }); + + describe('dismissClickCommand (Sonner onDismiss)', () => { + it("maps dismissClickCommand to Sonner's onDismiss option and sends it the notification id", async () => { + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + // The host passes this straight to Sonner without validating it against real command names, + // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. + // eslint-disable-next-line no-type-assertion/no-type-assertion + dismissClickCommand: 'test.dismiss' as never, + }; + + const notificationId = await capturedService.send(notification); + const options = mockToastInfo.mock.calls[0][1]; + await options?.onDismiss?.(); + + expect(mockSendCommand).toHaveBeenCalledWith('test.dismiss', notificationId); + }); + + it('logs a warning instead of throwing when the dismiss command rejects', async () => { + mockSendCommand.mockRejectedValueOnce(new Error('boom')); + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + // The host passes this straight to Sonner without validating it against real command names, + // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. + // eslint-disable-next-line no-type-assertion/no-type-assertion + dismissClickCommand: 'test.dismiss' as never, + }; + + await capturedService.send(notification); + const options = mockToastInfo.mock.calls[0][1]; + + // The catch handler must swallow the rejection - awaiting the returned promise must not throw. + await expect(options?.onDismiss?.()).resolves.toBeUndefined(); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('boom')); + }); }); describe('send with a reused notificationId', () => { diff --git a/src/renderer/services/notification.service-host.ts b/src/renderer/services/notification.service-host.ts index dd2af5cfd69..534c0debc7f 100644 --- a/src/renderer/services/notification.service-host.ts +++ b/src/renderer/services/notification.service-host.ts @@ -2,14 +2,27 @@ import { toast } from 'sonner'; import { NotificationServiceNetworkObjectName, type INotificationService, + type NotificationPosition, type PlatformNotification, } from '@shared/models/notification.service-model'; import * as commandService from '@shared/services/command.service'; import { networkObjectService } from '@shared/services/network-object.service'; -import { isLocalizeKey } from 'platform-bible-utils'; +import { getErrorMessage, isLocalizeKey } from 'platform-bible-utils'; import { localizationService } from '@shared/services/localization.service'; import { logger } from '@shared/services/logger.service'; +// The six placements accepted by `NotificationPosition`. OpenRPC schemas are plain data with no +// link back to the TS type, so this can't be derived automatically; the `NotificationPosition[]` +// annotation at least guards against typos. Keep in sync with that type by hand. +const notificationPositionValues: NotificationPosition[] = [ + 'top-left', + 'top-center', + 'top-right', + 'bottom-left', + 'bottom-center', + 'bottom-right', +]; + const mapOfNotificationIdsToToastIds = new Map(); async function localize(text: string): Promise { @@ -34,6 +47,7 @@ async function send(notification: PlatformNotification): Promise - commandService.sendCommand(secondaryClickCommand, effectiveNotificationId), + commandService + .sendCommand(secondaryClickCommand, effectiveNotificationId) + .catch((e) => + logger.warn( + `Notification service host secondary click command '${secondaryClickCommand}' failed: ${getErrorMessage(e)}`, + ), + ), } : undefined, // Per-toast placement override; undefined leaves the Toaster's default placement in effect. position, // Whether the user can swipe/drag the toast away; undefined leaves Sonner's default (true). Set - // false (with duration 0) for a toast that must be answered via an action button. + // false only for a toast with no secondary action - see the warning on + // PlatformNotification.dismissible for why: Sonner gates the cancel/secondary button's onClick on + // this same flag, so `false` silently disables a second action button too. dismissible, + // Fires only when the USER dismisses the toast (swipe/drag past Sonner's threshold, or a close + // button click if one is ever enabled) - never for our own programmatic `dismiss()`, nor for + // auto-close when `duration` elapses. See PlatformNotification.dismissClickCommand for the full + // contract (verified against the Sonner 1.7.4 source). + onDismiss: dismissClickCommand + ? () => + commandService + .sendCommand(dismissClickCommand, effectiveNotificationId) + .catch((e) => + logger.warn( + `Notification service host dismiss command '${dismissClickCommand}' failed: ${getErrorMessage(e)}`, + ), + ) + : undefined, // Duration calc from https://paratextstudio.atlassian.net/browse/PT-2196?focusedCommentId=13075 duration, }; @@ -134,7 +170,8 @@ export async function startNotificationService(): Promise { clickCommandLabel: { type: 'string' }, secondaryClickCommand: { type: 'string' }, secondaryClickCommandLabel: { type: 'string' }, - position: { type: 'string' }, + dismissClickCommand: { type: 'string' }, + position: { type: 'string', enum: notificationPositionValues }, dismissible: { type: 'boolean' }, notificationId: { type: ['string', 'number'] }, duration: { type: 'number' }, diff --git a/src/shared/models/notification.service-model.ts b/src/shared/models/notification.service-model.ts index 9ec67e999ff..8aebb9d5ea4 100644 --- a/src/shared/models/notification.service-model.ts +++ b/src/shared/models/notification.service-model.ts @@ -56,15 +56,41 @@ export interface PlatformNotification { * The command handler should have the type signature {@link NotificationClickCommandHandler}. */ secondaryClickCommand?: keyof CommandHandlers; + /** + * Optional command to run if the user dismisses the notification themselves — by swiping/dragging + * it away, or by clicking the close button (if the host ever enables one). Sent no arguments + * other than the notification id, like {@link clickCommand}: + * + * - NotificationId: The ID of the notification that was dismissed + * + * The command handler should have the type signature {@link NotificationClickCommandHandler}. + * + * IMPORTANT: this fires ONLY for that user gesture. It does NOT fire when the notification is + * dismissed programmatically via {@link INotificationService.dismiss}, when it auto-closes because + * `duration` elapsed, or when the user clicks {@link clickCommand} / {@link secondaryClickCommand} + * (verified against the Sonner 1.7.4 source: `onDismiss` is invoked only from the swipe-release + * and close-button handlers, never from the programmatic-dismiss or auto-close paths). Use this + * to treat a user's swipe-away as an explicit decision — e.g. pairing it with a "postpone" + * command lets a two-button, must-answer-style toast still keep {@link dismissible} `true` (see + * the warning on {@link dismissible} for why `false` is usually the wrong tool for that). + */ + dismissClickCommand?: keyof CommandHandlers; /** * Optional placement of the notification on screen. When omitted, the app's default placement is * used. */ position?: NotificationPosition; /** - * Whether the user can dismiss the notification directly (e.g. by swiping/dragging it away). - * Defaults to `true`. Set to `false` for a notification that must be acknowledged through one of - * its action buttons — combine with a `duration` of `0` or less to keep it up until then. + * Whether the user can dismiss the notification directly (e.g. by swiping/dragging it away, or + * via a close button). Defaults to `true`. + * + * WARNING: the host toast library (Sonner) renders {@link secondaryClickCommandLabel} in the same + * slot it gates on this flag, so setting `dismissible: false` also disables the secondary action + * button (and the close button) — the secondary button will still render but silently do nothing + * when clicked. Only set this to `false` on a notification with no secondary action. For a + * notification the user must explicitly answer, prefer leaving `dismissible: true` and using + * {@link dismissClickCommand} so a swipe-away still counts as a real (e.g. "postpone") decision + * instead of a dead button. */ dismissible?: boolean; /** Optional ID of a previous notification to update instead of showing a new notification */ From c90014f9d34c6c50219212b2b14b078385e5afff Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Wed, 15 Jul 2026 19:08:47 +0200 Subject: [PATCH 03/16] PT-4193: Regenerate papi.d.ts to match reflowed source JSDoc 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 --- lib/papi-dts/papi.d.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index f3a7b4d7720..0df3665cfda 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -5266,8 +5266,8 @@ declare module 'shared/models/notification.service-model' { secondaryClickCommand?: keyof CommandHandlers; /** * Optional command to run if the user dismisses the notification themselves — by swiping/dragging - * it away, or by clicking the close button (if the host ever enables one). Sent no arguments other - * than the notification id, like {@link clickCommand}: + * it away, or by clicking the close button (if the host ever enables one). Sent no arguments + * other than the notification id, like {@link clickCommand}: * * - NotificationId: The ID of the notification that was dismissed * @@ -5276,11 +5276,11 @@ declare module 'shared/models/notification.service-model' { * IMPORTANT: this fires ONLY for that user gesture. It does NOT fire when the notification is * dismissed programmatically via {@link INotificationService.dismiss}, when it auto-closes because * `duration` elapsed, or when the user clicks {@link clickCommand} / {@link secondaryClickCommand} - * (verified against the Sonner 1.7.4 source: `onDismiss` is invoked only from the swipe-release and - * close-button handlers, never from the programmatic-dismiss or auto-close paths). Use this to treat - * a user's swipe-away as an explicit decision — e.g. pairing it with a "postpone" command lets a - * two-button, must-answer-style toast still keep {@link dismissible} `true` (see the warning on - * {@link dismissible} for why `false` is usually the wrong tool for that). + * (verified against the Sonner 1.7.4 source: `onDismiss` is invoked only from the swipe-release + * and close-button handlers, never from the programmatic-dismiss or auto-close paths). Use this + * to treat a user's swipe-away as an explicit decision — e.g. pairing it with a "postpone" + * command lets a two-button, must-answer-style toast still keep {@link dismissible} `true` (see + * the warning on {@link dismissible} for why `false` is usually the wrong tool for that). */ dismissClickCommand?: keyof CommandHandlers; /** @@ -5289,8 +5289,8 @@ declare module 'shared/models/notification.service-model' { */ position?: NotificationPosition; /** - * Whether the user can dismiss the notification directly (e.g. by swiping/dragging it away, or via - * a close button). Defaults to `true`. + * Whether the user can dismiss the notification directly (e.g. by swiping/dragging it away, or + * via a close button). Defaults to `true`. * * WARNING: the host toast library (Sonner) renders {@link secondaryClickCommandLabel} in the same * slot it gates on this flag, so setting `dismissible: false` also disables the secondary action From 181ea8217dd724f2af67ff2ad374ff4c750f054f Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 14:42:58 +0200 Subject: [PATCH 04/16] PT-4193: Fix two-button toast crushing message content to a sliver 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 --- .../components/notification-display.scss | 42 +++++++++ .../components/notification-display.test.tsx | 90 +++++++++++++++++++ .../components/notification-display.tsx | 18 +++- 3 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 src/renderer/components/notification-display.scss diff --git a/src/renderer/components/notification-display.scss b/src/renderer/components/notification-display.scss new file mode 100644 index 00000000000..0740850a4e9 --- /dev/null +++ b/src/renderer/components/notification-display.scss @@ -0,0 +1,42 @@ +// PT-4193: a toast with BOTH a primary action ("action") and a secondary/cancel action ("cancel") +// renders, by Sonner's own stylesheet (node_modules/sonner/dist/styles.css), as a single un-wrapped +// flex row: icon, content, cancel button, action button. Sonner's buttons are `flex-shrink: 0`, so +// two wide buttons crush the message content down to a sliver - confirmed live via a screenshot of a +// real Send/Receive consent toast. Scope the fix to exactly that two-button shape via `:has()` so +// plain-message and single-button toasts stay pixel-identical to before. +.notification-toast:has(.notification-toast-cancel-button):has(.notification-toast-action-button) { + flex-wrap: wrap; + + .notification-toast-content { + // Let the message take the rest of the row next to the icon, instead of shrink-wrapping to the + // text width - matching the "full toast width" content row of a plain or single-button toast. + flex-grow: 1; + min-inline-size: 0; + } + + // Force a line break between the content row and the button row without an extra wrapper element: + // an empty, zero-size flex item whose `flex-basis: 100%` can't fit on the content's line, so the + // wrap algorithm starts a new line with it - and the buttons (next in flex order below) start + // fresh on the line after that. Sonner's own `[data-button]` auto-margin then right-aligns + // whichever buttons end up sharing that new line, and lets a second button wrap to a third, + // stacked row if the two together still don't fit - both behaviors come for free, no extra rules + // needed here. + &::after { + content: ''; + order: 1; + flex-basis: 100%; + block-size: 0; + } +} + +// Ordered above the default (0) so both buttons sort after the content/break-point above, in +// whichever single- or multi-line layout is in effect. Harmless when only one of the two exists (or +// neither does): it keeps the same after-content position a plain-message or single-button toast +// already had. +.notification-toast-cancel-button { + order: 2; +} + +.notification-toast-action-button { + order: 3; +} diff --git a/src/renderer/components/notification-display.test.tsx b/src/renderer/components/notification-display.test.tsx index 1ea9b4bf550..afb75f8da9a 100644 --- a/src/renderer/components/notification-display.test.tsx +++ b/src/renderer/components/notification-display.test.tsx @@ -86,4 +86,94 @@ describe('NotificationDisplay with real Sonner', () => { expect(mockSendCommand).toHaveBeenCalledWith('test.secondary', notificationId); }); + + // PT-4193 layout fix: a live E2E screenshot showed a toast with BOTH an action and a cancel + // button collapsing its message down to a ~1-character-wide sliver - Sonner's default layout + // puts icon/content/cancel/action in a single un-wrapped flex row, and two non-shrinking buttons + // crush the content column. notification-display.scss fixes this with a `:has()`-scoped stylesheet + // rule keyed off the classNames wired up in notification-display.tsx. jsdom doesn't compute actual + // flex-wrap layout, so these tests pin the DOM shape (classes + shared flex-container ancestry) + // that stylesheet rule depends on, rather than pixel positions. + it('gives a two-button toast the content/action/cancel class hooks the layout fix keys off, and keeps both buttons working', async () => { + render(); + + const notification: PlatformNotification = { + message: 'Time to sync', + severity: 'info', + clickCommandLabel: 'Send/Receive now', + // The test only needs a command NAME to send/assert on; it never resolves to a real handler. + // eslint-disable-next-line no-type-assertion/no-type-assertion + clickCommand: 'test.primary' as never, + secondaryClickCommandLabel: 'Postpone until 3:24 PM', + // The test only needs a command NAME to send/assert on; it never resolves to a real handler. + // eslint-disable-next-line no-type-assertion/no-type-assertion + secondaryClickCommand: 'test.secondary' as never, + }; + + const notificationId = await capturedService.send(notification); + + const actionButton = await screen.findByRole('button', { name: 'Send/Receive now' }); + const cancelButton = await screen.findByRole('button', { name: 'Postpone until 3:24 PM' }); + const title = await screen.findByText('Time to sync'); + const content = title.closest('.notification-toast-content'); + const toastRoot = document.querySelector('.notification-toast'); + + // The fix's CSS rule is + // `.notification-toast:has(.notification-toast-cancel-button):has(.notification-toast-action-button)` + // - assert that shape exists: one shared flex-container ancestor directly containing the + // content, the action button, and the cancel button as siblings (not nested inside each other), + // which is what lets `flex-grow` on the content and `order` on the buttons rearrange them into + // separate rows. + expect(toastRoot).not.toBeNull(); + expect(content).not.toBeNull(); + expect(actionButton).toHaveClass('notification-toast-action-button'); + expect(cancelButton).toHaveClass('notification-toast-cancel-button'); + expect(content?.parentElement).toBe(toastRoot); + expect(actionButton.parentElement).toBe(toastRoot); + expect(cancelButton.parentElement).toBe(toastRoot); + + fireEvent.click(actionButton); + fireEvent.click(cancelButton); + + expect(mockSendCommand).toHaveBeenCalledWith('test.primary', notificationId); + expect(mockSendCommand).toHaveBeenCalledWith('test.secondary', notificationId); + }); + + it('does not add the action-button hook to a single-button (cancel-only) toast', async () => { + render(); + + const notification: PlatformNotification = { + message: 'A decision is needed', + severity: 'info', + secondaryClickCommandLabel: 'Postpone', + // The test only needs a command NAME to send/assert on; it never resolves to a real handler. + // eslint-disable-next-line no-type-assertion/no-type-assertion + secondaryClickCommand: 'test.secondary' as never, + }; + + await capturedService.send(notification); + + const cancelButton = await screen.findByRole('button', { name: 'Postpone' }); + // The layout fix only engages when BOTH button classes are present under the same toast (see + // the `:has()...:has()` rule in notification-display.scss). A single-button toast must not + // accidentally satisfy that condition, so the content keeps its plain, un-grown structure. + expect(cancelButton).toHaveClass('notification-toast-cancel-button'); + expect(document.querySelector('.notification-toast-action-button')).not.toBeInTheDocument(); + }); + + it('renders a plain string toast with the content hook and no button hooks', async () => { + render(); + + const notification: PlatformNotification = { + message: 'Just an update', + severity: 'info', + }; + + await capturedService.send(notification); + + const title = await screen.findByText('Just an update'); + expect(title.closest('.notification-toast-content')).not.toBeNull(); + expect(document.querySelector('.notification-toast-cancel-button')).not.toBeInTheDocument(); + expect(document.querySelector('.notification-toast-action-button')).not.toBeInTheDocument(); + }); }); diff --git a/src/renderer/components/notification-display.tsx b/src/renderer/components/notification-display.tsx index 8962b1d868b..7c93b0e858c 100644 --- a/src/renderer/components/notification-display.tsx +++ b/src/renderer/components/notification-display.tsx @@ -1,7 +1,23 @@ import { Toaster } from 'sonner'; +import './notification-display.scss'; +// PT-4193: class hooks for notification-display.scss's fix for the two-button (action + cancel) +// toast layout collapse - see that file for the full explanation. Applied here (the `Toaster`'s +// shared `toastOptions`) rather than per-notification in notification.service-host.ts so every +// toast gets the hooks uniformly; the CSS itself only changes layout when both buttons are present. export function NotificationDisplay() { - return ; + return ( + + ); } export default NotificationDisplay; From b07cde0083a6fd2e12ac7ad71b5e0cdaa54f0584 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 15:12:30 +0200 Subject: [PATCH 05/16] PT-4193: Drop stale send() updates racing their own dismiss() 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). --- .../notification.service-host.test.ts | 79 ++++++++++++++++++- .../services/notification.service-host.ts | 53 +++++++++++++ 2 files changed, 130 insertions(+), 2 deletions(-) diff --git a/src/renderer/services/notification.service-host.test.ts b/src/renderer/services/notification.service-host.test.ts index 5b07c912a9b..9ba658fcb9c 100644 --- a/src/renderer/services/notification.service-host.test.ts +++ b/src/renderer/services/notification.service-host.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import type { INotificationService, PlatformNotification, @@ -43,7 +43,7 @@ vi.mock('@shared/services/localization.service', () => ({ }, })); vi.mock('@shared/services/logger.service', () => ({ - logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, })); const mockSendCommand = vi.mocked(commandService.sendCommand); @@ -69,6 +69,13 @@ describe('notification service host', () => { await startNotificationService(); }); + afterEach(() => { + // Guarantee real timers are restored even if a fake-timer test throws before its own + // `vi.useRealTimers()` - otherwise a single assertion failure can leave fake timers active + // for every subsequent test. + vi.useRealTimers(); + }); + describe('send with duration', () => { it('uses the provided duration when specified', async () => { const notification: PlatformNotification = { @@ -287,4 +294,72 @@ describe('notification service host', () => { ); }); }); + + describe('dismiss grace window (guards against a resurrected toast)', () => { + it('drops a send() for a notificationId that was just dismissed instead of creating a new toast', async () => { + mockToastInfo.mockReturnValueOnce('toast-1'); + const notification: PlatformNotification = { + message: 'Syncing (0%)', + severity: 'info', + notificationId: 'sync-status', + }; + + const notificationId = await capturedService.send(notification); + await capturedService.dismiss(notificationId); + // A stale in-flight update for the same id, racing its own producer's dismiss(), arrives + // right after the dismiss - it must be dropped rather than resurrecting a new toast. + const result = await capturedService.send(notification); + + expect(mockToastInfo).toHaveBeenCalledTimes(1); + expect(mockToast.dismiss).toHaveBeenCalledTimes(1); + expect(result).toBe('sync-status'); + expect(logger.debug).toHaveBeenCalledWith(expect.stringContaining('sync-status')); + }); + + it('allows sending with the same notificationId again once the grace window has elapsed', async () => { + vi.useFakeTimers(); + mockToastInfo.mockReturnValueOnce('toast-1').mockReturnValueOnce('toast-2'); + const notification: PlatformNotification = { + message: 'Syncing (0%)', + severity: 'info', + notificationId: 'sync-status', + }; + + const notificationId = await capturedService.send(notification); + await capturedService.dismiss(notificationId); + // Past the grace window, so this is treated as a legitimate new send, not a stale update. + vi.advanceTimersByTime(5001); + await capturedService.send(notification); + + expect(mockToastInfo).toHaveBeenCalledTimes(2); + // toastId mapping was cleared by dismiss(), so this creates a fresh toast (id: undefined) + // rather than updating the dismissed one. + expect(mockToastInfo).toHaveBeenNthCalledWith( + 2, + 'Syncing (0%)', + expect.objectContaining({ id: undefined }), + ); + }); + + it('does not affect a brand-new send with no notificationId right after an unrelated dismiss', async () => { + mockToastInfo.mockReturnValueOnce('toast-1').mockReturnValueOnce('toast-2'); + const dismissedNotification: PlatformNotification = { + message: 'Syncing (0%)', + severity: 'info', + notificationId: 'sync-status', + }; + const notificationId = await capturedService.send(dismissedNotification); + await capturedService.dismiss(notificationId); + + const freshNotification: PlatformNotification = { message: 'Unrelated', severity: 'info' }; + await capturedService.send(freshNotification); + + expect(mockToastInfo).toHaveBeenCalledTimes(2); + expect(mockToastInfo).toHaveBeenNthCalledWith( + 2, + 'Unrelated', + expect.objectContaining({ id: undefined }), + ); + }); + }); }); diff --git a/src/renderer/services/notification.service-host.ts b/src/renderer/services/notification.service-host.ts index 534c0debc7f..3c2ea13967b 100644 --- a/src/renderer/services/notification.service-host.ts +++ b/src/renderer/services/notification.service-host.ts @@ -25,6 +25,36 @@ const notificationPositionValues: NotificationPosition[] = [ const mapOfNotificationIdsToToastIds = new Map(); +/** + * How long, in milliseconds, `dismiss()` remembers a notification id after dismissing it. Guards + * against a stale in-flight `send()` update for that same id arriving shortly after its own + * dismissal (e.g. a fire-and-forget progress update racing the producer's own `dismiss()` call, + * observed live with the Send/Receive progress toast when a sync aborted quickly): without this, + * `send()` would find no toast mapping and resurrect a brand-new toast that nothing then dismisses. + * Deliberately short - long enough to catch a race, short enough that a caller legitimately reusing + * the id later still works normally. + */ +const dismissedNotificationIdGracePeriodMs = 5000; + +/** + * NotificationId -> the `Date.now()` timestamp `dismiss()` was called for it. Read and pruned by + * {@link pruneRecentlyDismissedNotificationIds}; see {@link dismissedNotificationIdGracePeriodMs}. + */ +const recentlyDismissedNotificationIds = new Map(); + +/** + * Drop entries from {@link recentlyDismissedNotificationIds} older than + * {@link dismissedNotificationIdGracePeriodMs}. Called opportunistically from `send()` and + * `dismiss()` so the map doesn't grow unboundedly; no timers involved. + */ +function pruneRecentlyDismissedNotificationIds(): void { + const now = Date.now(); + recentlyDismissedNotificationIds.forEach((dismissedAt, notificationId) => { + if (now - dismissedAt > dismissedNotificationIdGracePeriodMs) + recentlyDismissedNotificationIds.delete(notificationId); + }); +} + async function localize(text: string): Promise { return isLocalizeKey(text) ? localizationService.getLocalizedString({ localizeKey: text }) : text; } @@ -50,6 +80,18 @@ async function send(notification: PlatformNotification): Promise { + pruneRecentlyDismissedNotificationIds(); const toastId = mapOfNotificationIdsToToastIds.get(notificationId); if (toastId !== undefined) { toast.dismiss(toastId); mapOfNotificationIdsToToastIds.delete(notificationId); } + recentlyDismissedNotificationIds.set(notificationId, Date.now()); } const notificationService: INotificationService = { From 44a8809690f1dba7a23ac6ef5f96761421a66aae Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 23:17:28 +0200 Subject: [PATCH 06/16] fix(notifications): remove the 5s dismiss grace window (PT-4193 review 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 Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe --- .../notification.service-host.test.ts | 77 +------------------ .../services/notification.service-host.ts | 53 +------------ 2 files changed, 2 insertions(+), 128 deletions(-) diff --git a/src/renderer/services/notification.service-host.test.ts b/src/renderer/services/notification.service-host.test.ts index 9ba658fcb9c..e22a61812ec 100644 --- a/src/renderer/services/notification.service-host.test.ts +++ b/src/renderer/services/notification.service-host.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import type { INotificationService, PlatformNotification, @@ -69,13 +69,6 @@ describe('notification service host', () => { await startNotificationService(); }); - afterEach(() => { - // Guarantee real timers are restored even if a fake-timer test throws before its own - // `vi.useRealTimers()` - otherwise a single assertion failure can leave fake timers active - // for every subsequent test. - vi.useRealTimers(); - }); - describe('send with duration', () => { it('uses the provided duration when specified', async () => { const notification: PlatformNotification = { @@ -294,72 +287,4 @@ describe('notification service host', () => { ); }); }); - - describe('dismiss grace window (guards against a resurrected toast)', () => { - it('drops a send() for a notificationId that was just dismissed instead of creating a new toast', async () => { - mockToastInfo.mockReturnValueOnce('toast-1'); - const notification: PlatformNotification = { - message: 'Syncing (0%)', - severity: 'info', - notificationId: 'sync-status', - }; - - const notificationId = await capturedService.send(notification); - await capturedService.dismiss(notificationId); - // A stale in-flight update for the same id, racing its own producer's dismiss(), arrives - // right after the dismiss - it must be dropped rather than resurrecting a new toast. - const result = await capturedService.send(notification); - - expect(mockToastInfo).toHaveBeenCalledTimes(1); - expect(mockToast.dismiss).toHaveBeenCalledTimes(1); - expect(result).toBe('sync-status'); - expect(logger.debug).toHaveBeenCalledWith(expect.stringContaining('sync-status')); - }); - - it('allows sending with the same notificationId again once the grace window has elapsed', async () => { - vi.useFakeTimers(); - mockToastInfo.mockReturnValueOnce('toast-1').mockReturnValueOnce('toast-2'); - const notification: PlatformNotification = { - message: 'Syncing (0%)', - severity: 'info', - notificationId: 'sync-status', - }; - - const notificationId = await capturedService.send(notification); - await capturedService.dismiss(notificationId); - // Past the grace window, so this is treated as a legitimate new send, not a stale update. - vi.advanceTimersByTime(5001); - await capturedService.send(notification); - - expect(mockToastInfo).toHaveBeenCalledTimes(2); - // toastId mapping was cleared by dismiss(), so this creates a fresh toast (id: undefined) - // rather than updating the dismissed one. - expect(mockToastInfo).toHaveBeenNthCalledWith( - 2, - 'Syncing (0%)', - expect.objectContaining({ id: undefined }), - ); - }); - - it('does not affect a brand-new send with no notificationId right after an unrelated dismiss', async () => { - mockToastInfo.mockReturnValueOnce('toast-1').mockReturnValueOnce('toast-2'); - const dismissedNotification: PlatformNotification = { - message: 'Syncing (0%)', - severity: 'info', - notificationId: 'sync-status', - }; - const notificationId = await capturedService.send(dismissedNotification); - await capturedService.dismiss(notificationId); - - const freshNotification: PlatformNotification = { message: 'Unrelated', severity: 'info' }; - await capturedService.send(freshNotification); - - expect(mockToastInfo).toHaveBeenCalledTimes(2); - expect(mockToastInfo).toHaveBeenNthCalledWith( - 2, - 'Unrelated', - expect.objectContaining({ id: undefined }), - ); - }); - }); }); diff --git a/src/renderer/services/notification.service-host.ts b/src/renderer/services/notification.service-host.ts index 3c2ea13967b..3cb34863375 100644 --- a/src/renderer/services/notification.service-host.ts +++ b/src/renderer/services/notification.service-host.ts @@ -25,36 +25,6 @@ const notificationPositionValues: NotificationPosition[] = [ const mapOfNotificationIdsToToastIds = new Map(); -/** - * How long, in milliseconds, `dismiss()` remembers a notification id after dismissing it. Guards - * against a stale in-flight `send()` update for that same id arriving shortly after its own - * dismissal (e.g. a fire-and-forget progress update racing the producer's own `dismiss()` call, - * observed live with the Send/Receive progress toast when a sync aborted quickly): without this, - * `send()` would find no toast mapping and resurrect a brand-new toast that nothing then dismisses. - * Deliberately short - long enough to catch a race, short enough that a caller legitimately reusing - * the id later still works normally. - */ -const dismissedNotificationIdGracePeriodMs = 5000; - -/** - * NotificationId -> the `Date.now()` timestamp `dismiss()` was called for it. Read and pruned by - * {@link pruneRecentlyDismissedNotificationIds}; see {@link dismissedNotificationIdGracePeriodMs}. - */ -const recentlyDismissedNotificationIds = new Map(); - -/** - * Drop entries from {@link recentlyDismissedNotificationIds} older than - * {@link dismissedNotificationIdGracePeriodMs}. Called opportunistically from `send()` and - * `dismiss()` so the map doesn't grow unboundedly; no timers involved. - */ -function pruneRecentlyDismissedNotificationIds(): void { - const now = Date.now(); - recentlyDismissedNotificationIds.forEach((dismissedAt, notificationId) => { - if (now - dismissedAt > dismissedNotificationIdGracePeriodMs) - recentlyDismissedNotificationIds.delete(notificationId); - }); -} - async function localize(text: string): Promise { return isLocalizeKey(text) ? localizationService.getLocalizedString({ localizeKey: text }) : text; } @@ -81,17 +51,6 @@ async function send(notification: PlatformNotification): Promise { - pruneRecentlyDismissedNotificationIds(); const toastId = mapOfNotificationIdsToToastIds.get(notificationId); if (toastId !== undefined) { toast.dismiss(toastId); mapOfNotificationIdsToToastIds.delete(notificationId); } - recentlyDismissedNotificationIds.set(notificationId, Date.now()); } const notificationService: INotificationService = { From e5acbc7cf760b2bc8e9acdda83da3ee698f04a74 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Thu, 16 Jul 2026 23:27:25 +0200 Subject: [PATCH 07/16] fix(notifications): rework two-button toast CSS + reach every position 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 `
    ` per position sharing a single ref, so Alt+T (and Escape) only reach the last list, and each `
      ` 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-`
        ` 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 Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe --- .../components/notification-display.scss | 63 ++++++++++-------- .../components/notification-display.test.tsx | 41 +++++++++++- .../components/notification-display.tsx | 65 +++++++++++++++++++ 3 files changed, 142 insertions(+), 27 deletions(-) diff --git a/src/renderer/components/notification-display.scss b/src/renderer/components/notification-display.scss index 0740850a4e9..966bdd0701c 100644 --- a/src/renderer/components/notification-display.scss +++ b/src/renderer/components/notification-display.scss @@ -1,42 +1,53 @@ // PT-4193: a toast with BOTH a primary action ("action") and a secondary/cancel action ("cancel") -// renders, by Sonner's own stylesheet (node_modules/sonner/dist/styles.css), as a single un-wrapped -// flex row: icon, content, cancel button, action button. Sonner's buttons are `flex-shrink: 0`, so -// two wide buttons crush the message content down to a sliver - confirmed live via a screenshot of a -// real Send/Receive consent toast. Scope the fix to exactly that two-button shape via `:has()` so -// plain-message and single-button toasts stay pixel-identical to before. +// renders, by Sonner 1.7.4's own stylesheet (node_modules/sonner/dist/styles.css), as a single +// un-wrapped flex row: icon, content, cancel button, action button. Sonner's buttons are +// `flex-shrink: 0` and its toast width is a fixed `--width` (356px), so two wide buttons crush the +// message content down to a sliver - confirmed live via a screenshot of a real Send/Receive consent +// toast. We give exactly that two-button shape a two-row layout instead: [icon][message] on the +// first row, and the [cancel][action] pair right-aligned on a row of their own. Scope the fix to +// that shape via `:has()` so plain-message and single-button toasts stay pixel-identical to before. +// +// Sonner exposes no supported API for a two-row toast body (the only real escape hatch, +// `toast.custom`, means re-implementing Sonner's severity icon/colour rendering), so this reaches +// into its flex layout. It therefore DEPENDS on Sonner 1.7.4 rendering the icon, content, cancel, +// and action as direct flex children of the toast `
      1. `; revisit if Sonner changes its toast DOM. +// Deliberately avoids Sonner's own `::after` (its hover-bridge between stacked toasts) and only +// touches `::before` while the toast is at rest (Sonner styles `::before` transiently mid-swipe/ +// removal). See PT-4193 review C61-3/4/15/23/26 for the failure modes this replaces. .notification-toast:has(.notification-toast-cancel-button):has(.notification-toast-action-button) { flex-wrap: wrap; .notification-toast-content { - // Let the message take the rest of the row next to the icon, instead of shrink-wrapping to the - // text width - matching the "full toast width" content row of a plain or single-button toast. - flex-grow: 1; + // Grow to fill the first row beside the icon. `flex-basis: 0` keeps the message's hypothetical + // width at 0 so the flex-wrap algorithm never bumps the message onto a line below the icon + // (which would leave the icon orphaned on a row of its own); the message wraps *within* this + // grown box instead. + flex: 1 1 0; min-inline-size: 0; } - // Force a line break between the content row and the button row without an extra wrapper element: - // an empty, zero-size flex item whose `flex-basis: 100%` can't fit on the content's line, so the - // wrap algorithm starts a new line with it - and the buttons (next in flex order below) start - // fresh on the line after that. Sonner's own `[data-button]` auto-margin then right-aligns - // whichever buttons end up sharing that new line, and lets a second button wrap to a third, - // stacked row if the two together still don't fit - both behaviors come for free, no extra rules - // needed here. - &::after { + // Full-width, zero-height flex break that forces the buttons onto their own row. `::before` is + // untouched by Sonner except transiently while swiping/removing a toast, so - unlike the previous + // `::after` hack - this does not clobber Sonner's `::after` hover-bridge. Restrict it to the + // resting toast so it can't fight Sonner's own swipe/removal `::before`. + &:not([data-swiping='true']):not([data-removed='true'])::before { content: ''; order: 1; flex-basis: 100%; block-size: 0; } -} -// Ordered above the default (0) so both buttons sort after the content/break-point above, in -// whichever single- or multi-line layout is in effect. Harmless when only one of the two exists (or -// neither does): it keeps the same after-content position a plain-message or single-button toast -// already had. -.notification-toast-cancel-button { - order: 2; -} + // Collect the two buttons as a right-aligned pair on the button row. Sonner puts + // `margin-inline-start: auto` on *every* button, which on a shared row splits the free space + // *between* them instead of pushing them together; keep the auto margin only on the leading + // (cancel) button so the pair sits flush at the row's end, separated by the toast's own gap. + .notification-toast-cancel-button { + order: 2; + margin-inline-start: auto; + } -.notification-toast-action-button { - order: 3; + .notification-toast-action-button { + order: 3; + margin-inline-start: 0; + } } diff --git a/src/renderer/components/notification-display.test.tsx b/src/renderer/components/notification-display.test.tsx index afb75f8da9a..8b796c3ab63 100644 --- a/src/renderer/components/notification-display.test.tsx +++ b/src/renderer/components/notification-display.test.tsx @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { render, screen, fireEvent } from '@testing-library/react'; +import { render, screen, fireEvent, act } from '@testing-library/react'; import '@testing-library/jest-dom'; import type { INotificationService, @@ -176,4 +176,43 @@ describe('NotificationDisplay with real Sonner', () => { expect(document.querySelector('.notification-toast-cancel-button')).not.toBeInTheDocument(); expect(document.querySelector('.notification-toast-action-button')).not.toBeInTheDocument(); }); + + // PT-4193 (review C61-2): a per-toast `position` makes Sonner render one
          + // per distinct position, all sharing a single ref, so Sonner's own Alt+T hotkey only ever focuses + // the last list. NotificationDisplay layers a focus-cycling handler on top so repeated Alt+T + // reaches every list. (This pins that both lists become the active element across presses; jsdom + // does not model Sonner's real-browser per-
            focus-trap, so the blur-reset guarding that is + // manually verified - see the PR notes.) + it('cycles keyboard focus across every toast position list on the Alt+T hotkey', async () => { + render(); + + // One default-position (bottom-right) toast and one top-center toast => two separate
              lists. + await capturedService.send({ message: 'Bottom toast', severity: 'info' }); + await capturedService.send({ message: 'Top toast', severity: 'info', position: 'top-center' }); + await screen.findByText('Bottom toast'); + await screen.findByText('Top toast'); + + const lists = Array.from(document.querySelectorAll('[data-sonner-toaster]')); + expect(lists).toHaveLength(2); + + async function pressHotkey() { + await act(async () => { + document.dispatchEvent( + new KeyboardEvent('keydown', { altKey: true, code: 'KeyT', bubbles: true }), + ); + // Let the handler's queued microtask (which does the actual focus move) run. + await Promise.resolve(); + }); + return document.activeElement; + } + + const firstFocused = await pressHotkey(); + const secondFocused = await pressHotkey(); + + // Each press lands on one of the two lists, and the two presses reach different lists - so no + // position group is left keyboard-unreachable. + expect(lists).toContain(firstFocused); + expect(lists).toContain(secondFocused); + expect(firstFocused).not.toBe(secondFocused); + }); }); diff --git a/src/renderer/components/notification-display.tsx b/src/renderer/components/notification-display.tsx index 7c93b0e858c..a89dc32a087 100644 --- a/src/renderer/components/notification-display.tsx +++ b/src/renderer/components/notification-display.tsx @@ -1,13 +1,78 @@ +import { useEffect, useRef } from 'react'; import { Toaster } from 'sonner'; import './notification-display.scss'; +// Sonner's default focus hotkey (Alt+T). Declared here so the exact same combo drives both Sonner's +// own and our focus-cycling handler below - the two must not drift apart. +const NOTIFICATION_TOASTER_HOTKEY = ['altKey', 'KeyT']; + +/** + * Whether a keydown matches {@link NOTIFICATION_TOASTER_HOTKEY}, using the same semantics as Sonner + * 1.7.4 (a modifier name like `altKey` is read as a boolean flag; anything else is matched against + * `event.code`). Spelled out rather than indexing the event by a dynamic key so it stays type-safe + * without a type assertion. + */ +function eventMatchesHotkey(event: KeyboardEvent, hotkey: readonly string[]): boolean { + return hotkey.every((key) => { + switch (key) { + case 'altKey': + return event.altKey; + case 'ctrlKey': + return event.ctrlKey; + case 'metaKey': + return event.metaKey; + case 'shiftKey': + return event.shiftKey; + default: + return event.code === key; + } + }); +} + // PT-4193: class hooks for notification-display.scss's fix for the two-button (action + cancel) // toast layout collapse - see that file for the full explanation. Applied here (the `Toaster`'s // shared `toastOptions`) rather than per-notification in notification.service-host.ts so every // toast gets the hooks uniformly; the CSS itself only changes layout when both buttons are present. export function NotificationDisplay() { + // Index of the toast list Alt+T last focused, so repeated presses cycle across every list. + const focusedListIndexRef = useRef(-1); + + // PT-4193 (review C61-2): with a per-toast `position`, Sonner 1.7.4 renders one `
                ` per + // distinct position but assigns them all a single shared `ref`, so its own Alt+T handler only + // ever focuses the LAST list - leaving toasts in every other position group unreachable by + // keyboard (each `
                  ` is also its own focus trap that ejects focus when you try to Tab out to a + // sibling list). Cycle focus across every non-empty list so repeated Alt+T reaches them all. + useEffect(() => { + function cycleToastListFocus() { + const lists = Array.from( + document.querySelectorAll('[data-sonner-toaster]'), + ).filter((list) => list.querySelector('[data-sonner-toast]')); + // One (or zero) lists: Sonner's built-in hotkey already reaches it correctly - don't interfere. + if (lists.length <= 1) return; + const nextIndex = (focusedListIndexRef.current + 1) % lists.length; + focusedListIndexRef.current = nextIndex; + // Neutralize Sonner's per-`
                    ` focus trap: each list restores focus to the pre-region + // element when focus leaves it, which would otherwise eject us the moment we move to a sibling + // list. Blurring first lets that restore run harmlessly; then we focus the target list. + if (document.activeElement instanceof HTMLElement) document.activeElement.blur(); + lists[nextIndex].focus(); + } + + function handleKeyDown(event: KeyboardEvent) { + if (!eventMatchesHotkey(event, NOTIFICATION_TOASTER_HOTKEY)) return; + // Run after Sonner's own synchronous hotkey handler (which expands the stack and focuses its + // single shared ref) has finished for this event, so our focus target wins regardless of + // listener order. + queueMicrotask(cycleToastListFocus); + } + + document.addEventListener('keydown', handleKeyDown); + return () => document.removeEventListener('keydown', handleKeyDown); + }, []); + return ( Date: Thu, 16 Jul 2026 23:38:20 +0200 Subject: [PATCH 08/16] fix(notifications): harden the notification service host (PT-4193 review 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 Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe --- lib/papi-dts/papi.d.ts | 63 +++--- .../components/notification-display.test.tsx | 2 +- .../notification.service-host.test.ts | 156 ++++++++++++++- .../services/notification.service-host.ts | 183 +++++++++++------- .../models/notification.service-model.ts | 64 +++--- .../services/__mocks__/logger.service.ts | 1 + 6 files changed, 349 insertions(+), 120 deletions(-) diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index 0df3665cfda..b3ba102dbd8 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -5212,17 +5212,24 @@ declare module 'shared/models/notification.service-model' { import { CommandHandlers } from 'papi-shared-types'; import { LocalizeKey } from 'platform-bible-utils'; export type Severity = 'info' | 'warning' | 'error'; + /** + * The placements a notification can appear in, as a frozen array so it can be the single source of + * truth for both the {@link NotificationPosition} type and the notification service's OpenRPC + * `position` enum (which the service host spreads from this). + */ + export const NOTIFICATION_POSITIONS: readonly [ + 'top-left', + 'top-center', + 'top-right', + 'bottom-left', + 'bottom-center', + 'bottom-right', + ]; /** * Where a notification is shown on screen. Mirrors the placements the host toast library supports. * Omit to use the app's default placement. */ - export type NotificationPosition = - | 'top-left' - | 'top-center' - | 'top-right' - | 'bottom-left' - | 'bottom-center' - | 'bottom-right'; + export type NotificationPosition = (typeof NOTIFICATION_POSITIONS)[number]; /** Data needed to display a notification to the user */ export interface PlatformNotification { /** @@ -5273,14 +5280,16 @@ declare module 'shared/models/notification.service-model' { * * The command handler should have the type signature {@link NotificationClickCommandHandler}. * - * IMPORTANT: this fires ONLY for that user gesture. It does NOT fire when the notification is - * dismissed programmatically via {@link INotificationService.dismiss}, when it auto-closes because - * `duration` elapsed, or when the user clicks {@link clickCommand} / {@link secondaryClickCommand} - * (verified against the Sonner 1.7.4 source: `onDismiss` is invoked only from the swipe-release - * and close-button handlers, never from the programmatic-dismiss or auto-close paths). Use this - * to treat a user's swipe-away as an explicit decision — e.g. pairing it with a "postpone" - * command lets a two-button, must-answer-style toast still keep {@link dismissible} `true` (see - * the warning on {@link dismissible} for why `false` is usually the wrong tool for that). + * IMPORTANT: this fires when the user dismisses the notification themselves (swiping/dragging it + * away, or clicking a close button if the host ever enables one) AND when the notification + * auto-closes because its `duration` elapsed — a timeout is treated as an implicit dismissal, so + * a must-answer toast that times out still runs this command instead of vanishing silently. It + * does NOT fire when the notification is dismissed programmatically via + * {@link INotificationService.dismiss}, nor when the user clicks {@link clickCommand} / + * {@link secondaryClickCommand}. Use this to treat a swipe-away (or timeout) as an explicit + * decision — e.g. pairing it with a "postpone" command lets a two-button, must-answer-style toast + * keep {@link dismissible} `true` (see the warning on {@link dismissible}). If you need the toast + * to persist until the user actually answers, also set `duration` to `0`. */ dismissClickCommand?: keyof CommandHandlers; /** @@ -5292,16 +5301,24 @@ declare module 'shared/models/notification.service-model' { * Whether the user can dismiss the notification directly (e.g. by swiping/dragging it away, or * via a close button). Defaults to `true`. * - * WARNING: the host toast library (Sonner) renders {@link secondaryClickCommandLabel} in the same - * slot it gates on this flag, so setting `dismissible: false` also disables the secondary action - * button (and the close button) — the secondary button will still render but silently do nothing - * when clicked. Only set this to `false` on a notification with no secondary action. For a - * notification the user must explicitly answer, prefer leaving `dismissible: true` and using - * {@link dismissClickCommand} so a swipe-away still counts as a real (e.g. "postpone") decision - * instead of a dead button. + * The host toast library (Sonner) gates both the {@link secondaryClickCommand} button and the + * user-dismiss gesture that fires {@link dismissClickCommand} on this same flag, so a naive + * `dismissible: false` would silently turn those controls into dead buttons. To prevent that, the + * platform IGNORES `dismissible: false` when a {@link secondaryClickCommand} or + * {@link dismissClickCommand} is also set — the notification stays user-dismissible so those + * controls keep working. `dismissible: false` therefore only takes effect on a notification with + * no secondary/dismiss command. For a notification the user must explicitly answer, prefer + * leaving `dismissible: true` and using {@link dismissClickCommand} so a swipe-away still counts + * as a real (e.g. "postpone") decision. */ dismissible?: boolean; - /** Optional ID of a previous notification to update instead of showing a new notification */ + /** + * Optional ID of a previous notification to update instead of showing a new notification. + * + * On an update (a `send` reusing an id that is still showing), any optional field you omit keeps + * the value it had on the previous `send` for that id — omitting a field never clears it. Pass + * the field explicitly to change it. + */ notificationId?: string | number; /** * Optional duration in milliseconds for how long the notification is displayed. To make the diff --git a/src/renderer/components/notification-display.test.tsx b/src/renderer/components/notification-display.test.tsx index 8b796c3ab63..4400d71ada7 100644 --- a/src/renderer/components/notification-display.test.tsx +++ b/src/renderer/components/notification-display.test.tsx @@ -22,7 +22,7 @@ vi.mock('@shared/services/localization.service', () => ({ }, })); vi.mock('@shared/services/logger.service', () => ({ - logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, })); let capturedService: INotificationService; diff --git a/src/renderer/services/notification.service-host.test.ts b/src/renderer/services/notification.service-host.test.ts index e22a61812ec..5416e96eae6 100644 --- a/src/renderer/services/notification.service-host.test.ts +++ b/src/renderer/services/notification.service-host.test.ts @@ -8,12 +8,14 @@ import { logger } from '@shared/services/logger.service'; /** * Minimal shape of the toastOptions object the host passes to Sonner's `toast.*` functions - typed - * just precisely enough to invoke `cancel.onClick` / `onDismiss` from tests without a type - * assertion (see `.eslintrc.cjs` `no-type-assertion` rule). + * just precisely enough to invoke `action.onClick` / `cancel.onClick` / `onDismiss` / `onAutoClose` + * from tests without a type assertion (see `.eslintrc.cjs` `no-type-assertion` rule). */ interface CapturedToastOptions { - cancel?: { label: string; onClick: () => Promise }; - onDismiss?: () => Promise; + action?: { label: string; onClick: () => Promise | void }; + cancel?: { label: string; onClick: () => Promise | void }; + onDismiss?: () => Promise | void; + onAutoClose?: () => Promise | void; } // Typed via the explicit generic (rather than named-but-unused parameters on the implementation) @@ -171,22 +173,80 @@ describe('notification service host', () => { ); }); - it('leaves cancel, position, dismissible, and onDismiss undefined when the new fields are omitted (back-compat)', async () => { + it('ignores dismissible:false when a secondary command is present so that button stays live', async () => { + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + dismissible: false, + secondaryClickCommandLabel: 'Postpone', + // The host passes this straight to Sonner without validating it against real command names, + // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. + // eslint-disable-next-line no-type-assertion/no-type-assertion + secondaryClickCommand: 'test.secondary' as never, + }; + + await capturedService.send(notification); + + // Sonner gates the cancel/secondary button on `dismissible`, so forwarding false would make it + // a dead button; the host must drop the false and keep the toast dismissible. + expect(mockToastInfo).toHaveBeenCalledWith( + 'test', + expect.objectContaining({ dismissible: undefined }), + ); + }); + + it('renders no action/cancel button and no position/dismissible override when those fields are omitted (back-compat)', async () => { const notification: PlatformNotification = { message: 'test', severity: 'info' }; await capturedService.send(notification); + // Assert the rendered outcome (no buttons, default placement, default dismissibility) rather + // than the exact option-object shape, so this doesn't cement any particular way of passing the + // omitted fields through to Sonner. expect(mockToastInfo).toHaveBeenCalledWith( 'test', expect.objectContaining({ + action: undefined, cancel: undefined, position: undefined, dismissible: undefined, - onDismiss: undefined, }), ); }); + it('keeps an omitted optional field at its previous value on an update-send instead of clobbering it', async () => { + const first: PlatformNotification = { + message: 'Consent?', + severity: 'info', + notificationId: 'consent', + position: 'top-center', + secondaryClickCommandLabel: 'Postpone', + // The host passes this straight to Sonner without validating it against real command names, + // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. + // eslint-disable-next-line no-type-assertion/no-type-assertion + secondaryClickCommand: 'test.postpone' as never, + dismissible: false, + }; + await capturedService.send(first); + + // Update that omits position, the secondary action, and dismissible entirely. + await capturedService.send({ + message: 'Consent? (retrying)', + severity: 'info', + notificationId: 'consent', + }); + + const secondOptions = mockToastInfo.mock.calls[1][1]; + // The position the first send set is preserved, not reverted to the Toaster default... + expect(mockToastInfo).toHaveBeenNthCalledWith( + 2, + 'Consent? (retrying)', + expect.objectContaining({ position: 'top-center' }), + ); + // ...and the secondary (cancel) button survives the update. + expect(secondOptions?.cancel?.label).toBe('Postpone'); + }); + it('invokes the secondary command with the notification id when the cancel button is clicked', async () => { const notification: PlatformNotification = { message: 'test', @@ -286,5 +346,89 @@ describe('notification service host', () => { expect.objectContaining({ id: 'toast-1' }), ); }); + + it('updates instead of duplicating for the legal id 0 (does not treat it as absent)', async () => { + mockToastInfo.mockReturnValueOnce('toast-0'); + const notification: PlatformNotification = { + message: 'first', + severity: 'info', + notificationId: 0, + }; + + await capturedService.send(notification); + await capturedService.send({ message: 'second', severity: 'info', notificationId: 0 }); + + expect(mockToastInfo).toHaveBeenCalledTimes(2); + // The second send must UPDATE (pass the mapped toast id), not create a fresh toast with id + // undefined - which a truthiness test of `notificationId` would wrongly do for 0. + expect(mockToastInfo).toHaveBeenNthCalledWith( + 2, + 'second', + expect.objectContaining({ id: 'toast-0' }), + ); + }); + + it('gives an id-less send an id in its own namespace so a numeric caller id cannot collide', async () => { + const autoId = await capturedService.send({ message: 'A', severity: 'info' }); + // The returned id is our own string handle, not Sonner's internal numeric auto-id. + expect(typeof autoId).toBe('string'); + expect(autoId).not.toBe(1); + + // An unrelated caller using the numeric id 1 must get a brand-new toast, not an update of A. + await capturedService.send({ message: 'B', severity: 'info', notificationId: 1 }); + + expect(mockToastInfo).toHaveBeenCalledTimes(2); + expect(mockToastInfo).toHaveBeenNthCalledWith( + 2, + 'B', + expect.objectContaining({ id: undefined }), + ); + }); + }); + + describe('primary action command', () => { + it('logs a warning instead of leaving an unhandled rejection when the primary command rejects', async () => { + mockSendCommand.mockRejectedValueOnce(new Error('boom')); + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + clickCommandLabel: 'Send/Receive now', + // The host passes this straight to Sonner without validating it against real command names, + // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. + // eslint-disable-next-line no-type-assertion/no-type-assertion + clickCommand: 'test.primary' as never, + }; + + await capturedService.send(notification); + const options = mockToastInfo.mock.calls[0][1]; + + // The primary action must have a .catch too (it historically did not) - awaiting must not throw. + await expect(options?.action?.onClick()).resolves.toBeUndefined(); + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('boom')); + }); + }); + + describe('onAutoClose', () => { + it('runs the dismiss command and cleans up bookkeeping when the toast auto-closes', async () => { + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + // The host passes this straight to Sonner without validating it against real command names, + // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. + // eslint-disable-next-line no-type-assertion/no-type-assertion + dismissClickCommand: 'test.dismiss' as never, + }; + + const notificationId = await capturedService.send(notification); + const options = mockToastInfo.mock.calls[0][1]; + // Timer expiry: Sonner fires onAutoClose (not onDismiss). It must run the same dismiss command. + await options?.onAutoClose?.(); + + expect(mockSendCommand).toHaveBeenCalledWith('test.dismiss', notificationId); + + // And the mapping is cleaned up, so a later dismiss() for that id is a no-op (no leak). + await capturedService.dismiss(notificationId); + expect(mockToast.dismiss).not.toHaveBeenCalled(); + }); }); }); diff --git a/src/renderer/services/notification.service-host.ts b/src/renderer/services/notification.service-host.ts index 3cb34863375..01e85ef57d2 100644 --- a/src/renderer/services/notification.service-host.ts +++ b/src/renderer/services/notification.service-host.ts @@ -1,8 +1,9 @@ import { toast } from 'sonner'; +import { CommandHandlers } from 'papi-shared-types'; import { + NOTIFICATION_POSITIONS, NotificationServiceNetworkObjectName, type INotificationService, - type NotificationPosition, type PlatformNotification, } from '@shared/models/notification.service-model'; import * as commandService from '@shared/services/command.service'; @@ -11,20 +12,34 @@ import { getErrorMessage, isLocalizeKey } from 'platform-bible-utils'; import { localizationService } from '@shared/services/localization.service'; import { logger } from '@shared/services/logger.service'; -// The six placements accepted by `NotificationPosition`. OpenRPC schemas are plain data with no -// link back to the TS type, so this can't be derived automatically; the `NotificationPosition[]` -// annotation at least guards against typos. Keep in sync with that type by hand. -const notificationPositionValues: NotificationPosition[] = [ - 'top-left', - 'top-center', - 'top-right', - 'bottom-left', - 'bottom-center', - 'bottom-right', -]; - +/** Caller-facing notification id -> the toast id Sonner actually rendered it under. */ const mapOfNotificationIdsToToastIds = new Map(); +/** + * Caller-facing notification id -> the last notification we sent for it. An update-send merges over + * this so omitting an optional field keeps its previously-set value instead of clobbering it. + */ +const lastNotificationById = new Map(); + +/** + * Counter backing {@link generateAutoNotificationId}. A send without a `notificationId` gets an id + * from our own namespace rather than Sonner's internal numeric auto-ids, which would otherwise + * share a namespace with caller-supplied numeric ids and collide. + */ +let autoAssignedNotificationIdCount = 0; + +/** Mint a unique caller-facing id, in our own namespace, for a notification sent without one. */ +function generateAutoNotificationId(): string { + autoAssignedNotificationIdCount += 1; + return `platform-notification-auto-${autoAssignedNotificationIdCount}`; +} + +/** Drop all bookkeeping for a notification once its toast is removed (via any removal path). */ +function forgetNotification(notificationId: string | number): void { + mapOfNotificationIdsToToastIds.delete(notificationId); + lastNotificationById.delete(notificationId); +} + async function localize(text: string): Promise { return isLocalizeKey(text) ? localizationService.getLocalizedString({ localizeKey: text }) : text; } @@ -38,6 +53,17 @@ async function send(notification: PlatformNotification): Promise + (): Promise | void => { + forgetNotification(effectiveNotificationId); + if (command === undefined) return undefined; + return commandService + .sendCommand(command, effectiveNotificationId) + .catch((e) => + logger.warn( + `Notification service host ${description} command '${command}' failed: ${getErrorMessage(e)}`, + ), + ); + }; + const toastOptions = { - // When re-sending with the same notificationId, reuse the existing toast id so Sonner - // updates the existing toast instead of creating a duplicate. Sonner reads this from the - // top-level `id` (not from `action.id`, which it ignores). - id: toastId, + // Reuse the existing toast id so Sonner updates in place. Sonner reads this from the top-level + // `id` (not from `action.id`, which it ignores). + id: existingToastId, action: - clickCommandLabel && clickCommand - ? { - label: await localize(clickCommandLabel), - onClick: () => commandService.sendCommand(clickCommand, effectiveNotificationId), - } + localizedActionLabel !== undefined + ? { label: localizedActionLabel, onClick: runRemovalCommand(clickCommand, 'click') } : undefined, - // Second action button, rendered by Sonner as its `cancel` slot alongside `action`. Built the - // same way as `action`, and like it, sends the notification id as the command's single argument. + // Second action button, rendered by Sonner as its `cancel` slot alongside `action`. Sends the + // notification id as the command's single argument, like `action`. cancel: - secondaryClickCommandLabel && secondaryClickCommand + localizedSecondaryLabel !== undefined ? { - label: await localize(secondaryClickCommandLabel), - onClick: () => - commandService - .sendCommand(secondaryClickCommand, effectiveNotificationId) - .catch((e) => - logger.warn( - `Notification service host secondary click command '${secondaryClickCommand}' failed: ${getErrorMessage(e)}`, - ), - ), + label: localizedSecondaryLabel, + onClick: runRemovalCommand(secondaryClickCommand, 'secondary click'), } : undefined, // Per-toast placement override; undefined leaves the Toaster's default placement in effect. position, - // Whether the user can swipe/drag the toast away; undefined leaves Sonner's default (true). Set - // false only for a toast with no secondary action - see the warning on - // PlatformNotification.dismissible for why: Sonner gates the cancel/secondary button's onClick on - // this same flag, so `false` silently disables a second action button too. - dismissible, - // Fires only when the USER dismisses the toast (swipe/drag past Sonner's threshold, or a close - // button click if one is ever enabled) - never for our own programmatic `dismiss()`, nor for - // auto-close when `duration` elapses. See PlatformNotification.dismissClickCommand for the full - // contract (verified against the Sonner 1.7.4 source). - onDismiss: dismissClickCommand - ? () => - commandService - .sendCommand(dismissClickCommand, effectiveNotificationId) - .catch((e) => - logger.warn( - `Notification service host dismiss command '${dismissClickCommand}' failed: ${getErrorMessage(e)}`, - ), - ) - : undefined, + dismissible: effectiveDismissible, + // Fires when the USER dismisses the toast (swipe/drag past Sonner's threshold, or a close button + // if one is ever enabled). Also forgets the notification so its map entries don't leak. + onDismiss: runRemovalCommand(dismissClickCommand, 'dismiss'), + // Fires when the toast auto-closes because `duration` elapsed. Runs the same dismiss command as a + // user dismissal (a timeout counts as an implicit dismissal, so a must-answer toast can't vanish + // having fired nothing) and likewise forgets the notification - covering the auto-close path that + // dismiss() alone never cleaned up. + onAutoClose: runRemovalCommand(dismissClickCommand, 'auto-close'), // Duration calc from https://paratextstudio.atlassian.net/browse/PT-2196?focusedCommentId=13075 duration, }; + let toastId: string | number; switch (severity) { case 'info': toastId = toast.info(localizedMessage, toastOptions); @@ -126,8 +175,8 @@ async function send(notification: PlatformNotification): Promise { const toastId = mapOfNotificationIdsToToastIds.get(notificationId); if (toastId !== undefined) { toast.dismiss(toastId); - mapOfNotificationIdsToToastIds.delete(notificationId); + forgetNotification(notificationId); } } @@ -173,7 +222,7 @@ export async function startNotificationService(): Promise { secondaryClickCommand: { type: 'string' }, secondaryClickCommandLabel: { type: 'string' }, dismissClickCommand: { type: 'string' }, - position: { type: 'string', enum: notificationPositionValues }, + position: { type: 'string', enum: [...NOTIFICATION_POSITIONS] }, dismissible: { type: 'boolean' }, notificationId: { type: ['string', 'number'] }, duration: { type: 'number' }, diff --git a/src/shared/models/notification.service-model.ts b/src/shared/models/notification.service-model.ts index 8aebb9d5ea4..a7da07028ac 100644 --- a/src/shared/models/notification.service-model.ts +++ b/src/shared/models/notification.service-model.ts @@ -3,17 +3,25 @@ import { LocalizeKey } from 'platform-bible-utils'; export type Severity = 'info' | 'warning' | 'error'; +/** + * The placements a notification can appear in, as a frozen array so it can be the single source of + * truth for both the {@link NotificationPosition} type and the notification service's OpenRPC + * `position` enum (which the service host spreads from this). + */ +export const NOTIFICATION_POSITIONS = Object.freeze([ + 'top-left', + 'top-center', + 'top-right', + 'bottom-left', + 'bottom-center', + 'bottom-right', +] as const); + /** * Where a notification is shown on screen. Mirrors the placements the host toast library supports. * Omit to use the app's default placement. */ -export type NotificationPosition = - | 'top-left' - | 'top-center' - | 'top-right' - | 'bottom-left' - | 'bottom-center' - | 'bottom-right'; +export type NotificationPosition = (typeof NOTIFICATION_POSITIONS)[number]; /** Data needed to display a notification to the user */ export interface PlatformNotification { @@ -65,14 +73,16 @@ export interface PlatformNotification { * * The command handler should have the type signature {@link NotificationClickCommandHandler}. * - * IMPORTANT: this fires ONLY for that user gesture. It does NOT fire when the notification is - * dismissed programmatically via {@link INotificationService.dismiss}, when it auto-closes because - * `duration` elapsed, or when the user clicks {@link clickCommand} / {@link secondaryClickCommand} - * (verified against the Sonner 1.7.4 source: `onDismiss` is invoked only from the swipe-release - * and close-button handlers, never from the programmatic-dismiss or auto-close paths). Use this - * to treat a user's swipe-away as an explicit decision — e.g. pairing it with a "postpone" - * command lets a two-button, must-answer-style toast still keep {@link dismissible} `true` (see - * the warning on {@link dismissible} for why `false` is usually the wrong tool for that). + * IMPORTANT: this fires when the user dismisses the notification themselves (swiping/dragging it + * away, or clicking a close button if the host ever enables one) AND when the notification + * auto-closes because its `duration` elapsed — a timeout is treated as an implicit dismissal, so + * a must-answer toast that times out still runs this command instead of vanishing silently. It + * does NOT fire when the notification is dismissed programmatically via + * {@link INotificationService.dismiss}, nor when the user clicks {@link clickCommand} / + * {@link secondaryClickCommand}. Use this to treat a swipe-away (or timeout) as an explicit + * decision — e.g. pairing it with a "postpone" command lets a two-button, must-answer-style toast + * keep {@link dismissible} `true` (see the warning on {@link dismissible}). If you need the toast + * to persist until the user actually answers, also set `duration` to `0`. */ dismissClickCommand?: keyof CommandHandlers; /** @@ -84,16 +94,24 @@ export interface PlatformNotification { * Whether the user can dismiss the notification directly (e.g. by swiping/dragging it away, or * via a close button). Defaults to `true`. * - * WARNING: the host toast library (Sonner) renders {@link secondaryClickCommandLabel} in the same - * slot it gates on this flag, so setting `dismissible: false` also disables the secondary action - * button (and the close button) — the secondary button will still render but silently do nothing - * when clicked. Only set this to `false` on a notification with no secondary action. For a - * notification the user must explicitly answer, prefer leaving `dismissible: true` and using - * {@link dismissClickCommand} so a swipe-away still counts as a real (e.g. "postpone") decision - * instead of a dead button. + * The host toast library (Sonner) gates both the {@link secondaryClickCommand} button and the + * user-dismiss gesture that fires {@link dismissClickCommand} on this same flag, so a naive + * `dismissible: false` would silently turn those controls into dead buttons. To prevent that, the + * platform IGNORES `dismissible: false` when a {@link secondaryClickCommand} or + * {@link dismissClickCommand} is also set — the notification stays user-dismissible so those + * controls keep working. `dismissible: false` therefore only takes effect on a notification with + * no secondary/dismiss command. For a notification the user must explicitly answer, prefer + * leaving `dismissible: true` and using {@link dismissClickCommand} so a swipe-away still counts + * as a real (e.g. "postpone") decision. */ dismissible?: boolean; - /** Optional ID of a previous notification to update instead of showing a new notification */ + /** + * Optional ID of a previous notification to update instead of showing a new notification. + * + * On an update (a `send` reusing an id that is still showing), any optional field you omit keeps + * the value it had on the previous `send` for that id — omitting a field never clears it. Pass + * the field explicitly to change it. + */ notificationId?: string | number; /** * Optional duration in milliseconds for how long the notification is displayed. To make the diff --git a/src/shared/services/__mocks__/logger.service.ts b/src/shared/services/__mocks__/logger.service.ts index 8a0cbc05c93..35328aec73e 100644 --- a/src/shared/services/__mocks__/logger.service.ts +++ b/src/shared/services/__mocks__/logger.service.ts @@ -6,4 +6,5 @@ export const logger = { info: () => {}, warn: () => {}, error: () => {}, + debug: () => {}, }; From 7daea47f93ae37e24e5e9c6c4f2f117aa8839126 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Fri, 17 Jul 2026 11:43:24 +0200 Subject: [PATCH 09/16] fix(notifications): extend the two-row toast reflow to single-button 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 --- .../components/notification-display.scss | 31 ++++++---- .../components/notification-display.test.tsx | 62 +++++++++++++++---- .../components/notification-display.tsx | 8 +-- 3 files changed, 73 insertions(+), 28 deletions(-) diff --git a/src/renderer/components/notification-display.scss b/src/renderer/components/notification-display.scss index 966bdd0701c..aeab180ccaf 100644 --- a/src/renderer/components/notification-display.scss +++ b/src/renderer/components/notification-display.scss @@ -1,11 +1,12 @@ -// PT-4193: a toast with BOTH a primary action ("action") and a secondary/cancel action ("cancel") -// renders, by Sonner 1.7.4's own stylesheet (node_modules/sonner/dist/styles.css), as a single -// un-wrapped flex row: icon, content, cancel button, action button. Sonner's buttons are -// `flex-shrink: 0` and its toast width is a fixed `--width` (356px), so two wide buttons crush the -// message content down to a sliver - confirmed live via a screenshot of a real Send/Receive consent -// toast. We give exactly that two-button shape a two-row layout instead: [icon][message] on the -// first row, and the [cancel][action] pair right-aligned on a row of their own. Scope the fix to -// that shape via `:has()` so plain-message and single-button toasts stay pixel-identical to before. +// PT-4193: a toast with buttons renders, by Sonner 1.7.4's own stylesheet +// (node_modules/sonner/dist/styles.css), as a single un-wrapped flex row: icon, content, cancel +// button, action button. Sonner's buttons are `flex-shrink: 0` and its toast width is a fixed +// `--width` (356px), so wide buttons crush the message content down to a sliver - confirmed live +// twice: a two-button Send/Receive consent toast squeezed its message to ~1 character, and (round-4 +// E2E) a single-action break-lock toast ("Break lock and retry" beside a long warning) was just as +// cramped. So give EVERY toast with at least one button a two-row layout instead: [icon][message] +// on the first row, and the button(s) right-aligned on a row of their own. Scope the fix to +// buttoned toasts via `:has()` so plain-message toasts stay pixel-identical to before. // // Sonner exposes no supported API for a two-row toast body (the only real escape hatch, // `toast.custom`, means re-implementing Sonner's severity icon/colour rendering), so this reaches @@ -14,7 +15,7 @@ // Deliberately avoids Sonner's own `::after` (its hover-bridge between stacked toasts) and only // touches `::before` while the toast is at rest (Sonner styles `::before` transiently mid-swipe/ // removal). See PT-4193 review C61-3/4/15/23/26 for the failure modes this replaces. -.notification-toast:has(.notification-toast-cancel-button):has(.notification-toast-action-button) { +.notification-toast:has(.notification-toast-cancel-button, .notification-toast-action-button) { flex-wrap: wrap; .notification-toast-content { @@ -37,10 +38,11 @@ block-size: 0; } - // Collect the two buttons as a right-aligned pair on the button row. Sonner puts - // `margin-inline-start: auto` on *every* button, which on a shared row splits the free space - // *between* them instead of pushing them together; keep the auto margin only on the leading - // (cancel) button so the pair sits flush at the row's end, separated by the toast's own gap. + // Collect the button(s) right-aligned on the button row. Sonner puts `margin-inline-start: auto` + // on *every* button, which right-aligns a lone button all by itself but, when both buttons share + // a row, splits the free space *between* them instead of pushing them together; so leave the auto + // margin alone except on an action button whose toast also has a cancel button - zeroing only + // that one leaves the pair flush at the row's end, separated by the toast's own gap. .notification-toast-cancel-button { order: 2; margin-inline-start: auto; @@ -48,6 +50,9 @@ .notification-toast-action-button { order: 3; + } + + &:has(.notification-toast-cancel-button) .notification-toast-action-button { margin-inline-start: 0; } } diff --git a/src/renderer/components/notification-display.test.tsx b/src/renderer/components/notification-display.test.tsx index 4400d71ada7..7f816a42d69 100644 --- a/src/renderer/components/notification-display.test.tsx +++ b/src/renderer/components/notification-display.test.tsx @@ -87,13 +87,14 @@ describe('NotificationDisplay with real Sonner', () => { expect(mockSendCommand).toHaveBeenCalledWith('test.secondary', notificationId); }); - // PT-4193 layout fix: a live E2E screenshot showed a toast with BOTH an action and a cancel - // button collapsing its message down to a ~1-character-wide sliver - Sonner's default layout - // puts icon/content/cancel/action in a single un-wrapped flex row, and two non-shrinking buttons - // crush the content column. notification-display.scss fixes this with a `:has()`-scoped stylesheet - // rule keyed off the classNames wired up in notification-display.tsx. jsdom doesn't compute actual - // flex-wrap layout, so these tests pin the DOM shape (classes + shared flex-container ancestry) - // that stylesheet rule depends on, rather than pixel positions. + // PT-4193 layout fix: live E2E screenshots showed toasts with buttons collapsing their message + // down to a sliver - first a toast with BOTH an action and a cancel button, then (round-4 E2E) + // a single-action break-lock toast. Sonner's default layout puts icon/content/cancel/action in a + // single un-wrapped flex row, and non-shrinking buttons crush the content column. + // notification-display.scss fixes this with a `:has()`-scoped stylesheet rule - engaging for ANY + // toast with at least one button - keyed off the classNames wired up in notification-display.tsx. + // jsdom doesn't compute actual flex-wrap layout, so these tests pin the DOM shape (classes + + // shared flex-container ancestry) that stylesheet rule depends on, rather than pixel positions. it('gives a two-button toast the content/action/cancel class hooks the layout fix keys off, and keeps both buttons working', async () => { render(); @@ -119,7 +120,7 @@ describe('NotificationDisplay with real Sonner', () => { const toastRoot = document.querySelector('.notification-toast'); // The fix's CSS rule is - // `.notification-toast:has(.notification-toast-cancel-button):has(.notification-toast-action-button)` + // `.notification-toast:has(.notification-toast-cancel-button, .notification-toast-action-button)` // - assert that shape exists: one shared flex-container ancestor directly containing the // content, the action button, and the cancel button as siblings (not nested inside each other), // which is what lets `flex-grow` on the content and `order` on the buttons rearrange them into @@ -139,6 +140,45 @@ describe('NotificationDisplay with real Sonner', () => { expect(mockSendCommand).toHaveBeenCalledWith('test.secondary', notificationId); }); + // Round-4 E2E finding: the break-lock toast (one long warning + one wide "Break lock and retry" + // action button, no cancel button) rendered text and button crammed side-by-side because the + // original rule only engaged when BOTH buttons were present. The rule now engages for any + // buttoned toast, so a single-action toast must present the same shape it keys off. + it('gives a single-button (action-only) toast the content/action class hooks the layout fix keys off, and keeps the button working', async () => { + render(); + + const notification: PlatformNotification = { + message: + 'The project is locked on the server by another user. Breaking the lock may discard their unfinished send.', + severity: 'warning', + clickCommandLabel: 'Break lock and retry', + // The test only needs a command NAME to send/assert on; it never resolves to a real handler. + // eslint-disable-next-line no-type-assertion/no-type-assertion + clickCommand: 'test.primary' as never, + }; + + const notificationId = await capturedService.send(notification); + + const actionButton = await screen.findByRole('button', { name: 'Break lock and retry' }); + const title = await screen.findByText(/locked on the server/); + const content = title.closest('.notification-toast-content'); + const toastRoot = document.querySelector('.notification-toast'); + + // Same structural pin as the two-button case: the `:has()` rule matches on either button class, + // and content + action button must be direct siblings under the toast flex container for the + // `flex-grow`/`order` reflow to put them on separate rows. + expect(toastRoot).not.toBeNull(); + expect(content).not.toBeNull(); + expect(actionButton).toHaveClass('notification-toast-action-button'); + expect(content?.parentElement).toBe(toastRoot); + expect(actionButton.parentElement).toBe(toastRoot); + expect(document.querySelector('.notification-toast-cancel-button')).not.toBeInTheDocument(); + + fireEvent.click(actionButton); + + expect(mockSendCommand).toHaveBeenCalledWith('test.primary', notificationId); + }); + it('does not add the action-button hook to a single-button (cancel-only) toast', async () => { render(); @@ -154,9 +194,9 @@ describe('NotificationDisplay with real Sonner', () => { await capturedService.send(notification); const cancelButton = await screen.findByRole('button', { name: 'Postpone' }); - // The layout fix only engages when BOTH button classes are present under the same toast (see - // the `:has()...:has()` rule in notification-display.scss). A single-button toast must not - // accidentally satisfy that condition, so the content keeps its plain, un-grown structure. + // A cancel-only toast now gets the two-row layout too (the `:has()` rule matches either button + // class), but it must do so with ONLY the cancel hook present - a spurious action button here + // would both render a button nobody asked for and trip the pair-only margin override. expect(cancelButton).toHaveClass('notification-toast-cancel-button'); expect(document.querySelector('.notification-toast-action-button')).not.toBeInTheDocument(); }); diff --git a/src/renderer/components/notification-display.tsx b/src/renderer/components/notification-display.tsx index a89dc32a087..30184534e42 100644 --- a/src/renderer/components/notification-display.tsx +++ b/src/renderer/components/notification-display.tsx @@ -29,10 +29,10 @@ function eventMatchesHotkey(event: KeyboardEvent, hotkey: readonly string[]): bo }); } -// PT-4193: class hooks for notification-display.scss's fix for the two-button (action + cancel) -// toast layout collapse - see that file for the full explanation. Applied here (the `Toaster`'s -// shared `toastOptions`) rather than per-notification in notification.service-host.ts so every -// toast gets the hooks uniformly; the CSS itself only changes layout when both buttons are present. +// PT-4193: class hooks for notification-display.scss's fix for the buttoned-toast layout collapse - +// see that file for the full explanation. Applied here (the `Toaster`'s shared `toastOptions`) +// rather than per-notification in notification.service-host.ts so every toast gets the hooks +// uniformly; the CSS itself only changes layout when at least one button is present. export function NotificationDisplay() { // Index of the toast list Alt+T last focused, so repeated presses cycle across every list. const focusedListIndexRef = useRef(-1); From 7ca04dc13a0a207f2745382379596fc278a3b0a5 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Fri, 17 Jul 2026 17:03:04 +0200 Subject: [PATCH 10/16] fix(notifications): keep the buttoned-toast row break through Sonner'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 --- .../components/notification-display.scss | 31 +++++++--- .../components/notification-display.test.tsx | 62 +++++++++++++++++++ 2 files changed, 84 insertions(+), 9 deletions(-) diff --git a/src/renderer/components/notification-display.scss b/src/renderer/components/notification-display.scss index aeab180ccaf..60bf2c997b0 100644 --- a/src/renderer/components/notification-display.scss +++ b/src/renderer/components/notification-display.scss @@ -9,12 +9,13 @@ // buttoned toasts via `:has()` so plain-message toasts stay pixel-identical to before. // // Sonner exposes no supported API for a two-row toast body (the only real escape hatch, -// `toast.custom`, means re-implementing Sonner's severity icon/colour rendering), so this reaches +// `toast.custom`, means re-implementing Sonner's severity icon/color rendering), so this reaches // into its flex layout. It therefore DEPENDS on Sonner 1.7.4 rendering the icon, content, cancel, // and action as direct flex children of the toast `
                  1. `; revisit if Sonner changes its toast DOM. -// Deliberately avoids Sonner's own `::after` (its hover-bridge between stacked toasts) and only -// touches `::before` while the toast is at rest (Sonner styles `::before` transiently mid-swipe/ -// removal). See PT-4193 review C61-3/4/15/23/26 for the failure modes this replaces. +// Deliberately avoids Sonner's own `::after` (its ALWAYS-ON hover-bridge between stacked toasts) +// and repurposes `::before`, which Sonner only styles in the swiping/removed states - see the +// comment on the row break below for how that collision is resolved. See PT-4193 review +// C61-3/4/15/23/26 for the failure modes this replaces. .notification-toast:has(.notification-toast-cancel-button, .notification-toast-action-button) { flex-wrap: wrap; @@ -27,12 +28,24 @@ min-inline-size: 0; } - // Full-width, zero-height flex break that forces the buttons onto their own row. `::before` is - // untouched by Sonner except transiently while swiping/removing a toast, so - unlike the previous - // `::after` hack - this does not clobber Sonner's `::after` hover-bridge. Restrict it to the - // resting toast so it can't fight Sonner's own swipe/removal `::before`. - &:not([data-swiping='true']):not([data-removed='true'])::before { + // Full-width, zero-height flex break that forces the buttons onto their own row. Unlike the + // previous `::after` hack this leaves Sonner's `::after` hover-bridge alone, and it must apply in + // EVERY toast state: Sonner flips the toast to `data-swiping="true"` the instant any mouse button + // is pressed on the toast body (before any movement - a plain press-and-hold counts), and an + // earlier revision that gated this break out of the swiping/removed states collapsed the toast to + // a one-character-wide sliver for the duration of every such press, because the `flex-wrap` and + // `flex-basis: 0` rules above stayed active with no row break. Sonner's own use of `::before` in + // those states is an invisible, ABSOLUTELY-positioned hover/hit-area extender declared at zero + // specificity (`:where(...)`), so this higher-specificity rule wins on every property it declares; + // `position: static` is declared explicitly to keep the pseudo-element in flex flow when Sonner's + // swipe/removal styles try to absolutely position it (its leftover declarations - inset, height, + // transform, z-index - are inert on a zero-height in-flow item, `height` losing to `block-size` + // by specificity). Trade-off: buttoned toasts lose Sonner's enlarged swipe/removal hover hit + // area; swipe-to-dismiss tracking is unaffected because Sonner captures the pointer at press + // time. + &::before { content: ''; + position: static; order: 1; flex-basis: 100%; block-size: 0; diff --git a/src/renderer/components/notification-display.test.tsx b/src/renderer/components/notification-display.test.tsx index 7f816a42d69..a9255c6332b 100644 --- a/src/renderer/components/notification-display.test.tsx +++ b/src/renderer/components/notification-display.test.tsx @@ -179,6 +179,68 @@ describe('NotificationDisplay with real Sonner', () => { expect(mockSendCommand).toHaveBeenCalledWith('test.primary', notificationId); }); + // PT-4193 press-collapse regression (found in external triage): Sonner flips the toast to + // `data-swiping="true"` on pointerdown - ANY mouse button, on the toast body, BEFORE any movement + // (its handler checks neither `event.button` nor movement, only that the target is not a BUTTON) + // - and clears it again on pointerup. An earlier notification-display.scss revision gated the + // button-row flex break out of that state, so merely holding the mouse down on a buttoned toast + // collapsed it to a one-character-wide sliver until release. The break is now unconditional + // (see the `::before` rule's comment); jsdom computes no CSS layout and exposes no + // pseudo-elements, so the stylesheet side cannot be asserted here. What this test pins is the DOM + // contract that fix rests on: a plain press really does enter Sonner's swiping state (if a Sonner + // upgrade stops doing that, the scss comment needs revisiting), and the class/sibling hooks the + // layout rules key off remain in place throughout the press. + it('enters Sonner swiping state on a plain press and keeps the layout-fix hooks throughout', async () => { + render(); + + const notification: PlatformNotification = { + message: 'Time to sync', + severity: 'info', + clickCommandLabel: 'Send/Receive now', + // The test only needs a command NAME to exist; it never resolves to a real handler. + // eslint-disable-next-line no-type-assertion/no-type-assertion + clickCommand: 'test.primary' as never, + secondaryClickCommandLabel: 'Postpone until 3:24 PM', + // The test only needs a command NAME to exist; it never resolves to a real handler. + // eslint-disable-next-line no-type-assertion/no-type-assertion + secondaryClickCommand: 'test.secondary' as never, + }; + + await capturedService.send(notification); + + const title = await screen.findByText('Time to sync'); + const toastRoot = document.querySelector('.notification-toast'); + expect(toastRoot).toHaveAttribute('data-swiping', 'false'); + + // Sonner's pointerdown handler pointer-captures the target; jsdom does not implement pointer + // capture, so stub it (same category of jsdom gap as the matchMedia stub above). + // eslint-disable-next-line no-type-assertion/no-type-assertion + const titleElement = title as HTMLElement & { + setPointerCapture: (pointerId: number) => void; + }; + titleElement.setPointerCapture = vi.fn(); + + // Left-button press on the toast body (the title div, not a button), no movement. + fireEvent.pointerDown(title, { pointerId: 1, button: 0 }); + expect(toastRoot).toHaveAttribute('data-swiping', 'true'); + // Mid-press, the hooks the two-row layout keys off must all still be present and siblings. + const content = title.closest('.notification-toast-content'); + const actionButton = screen.getByRole('button', { name: 'Send/Receive now' }); + const cancelButton = screen.getByRole('button', { name: 'Postpone until 3:24 PM' }); + expect(content?.parentElement).toBe(toastRoot); + expect(actionButton.parentElement).toBe(toastRoot); + expect(cancelButton.parentElement).toBe(toastRoot); + + fireEvent.pointerUp(title, { pointerId: 1, button: 0 }); + expect(toastRoot).toHaveAttribute('data-swiping', 'false'); + + // Right-button press behaves identically (the live bug reproduced with either button). + fireEvent.pointerDown(title, { pointerId: 1, button: 2 }); + expect(toastRoot).toHaveAttribute('data-swiping', 'true'); + fireEvent.pointerUp(title, { pointerId: 1, button: 2 }); + expect(toastRoot).toHaveAttribute('data-swiping', 'false'); + }); + it('does not add the action-button hook to a single-button (cancel-only) toast', async () => { render(); From f780c8d8c4d43395f9d7b7d3bf555872c661e723 Mon Sep 17 00:00:00 2001 From: Rolf Heij Date: Fri, 17 Jul 2026 17:33:15 +0200 Subject: [PATCH 11/16] docs(notifications): tag the secondary-action/position/dismissible surface @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 Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe --- lib/papi-dts/papi.d.ts | 14 ++++++++++++++ src/shared/models/notification.service-model.ts | 14 ++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index b3ba102dbd8..dbf79a9c9b7 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -5216,6 +5216,8 @@ declare module 'shared/models/notification.service-model' { * The placements a notification can appear in, as a frozen array so it can be the single source of * truth for both the {@link NotificationPosition} type and the notification service's OpenRPC * `position` enum (which the service host spreads from this). + * + * @experimental */ export const NOTIFICATION_POSITIONS: readonly [ 'top-left', @@ -5228,6 +5230,8 @@ declare module 'shared/models/notification.service-model' { /** * Where a notification is shown on screen. Mirrors the placements the host toast library supports. * Omit to use the app's default placement. + * + * @experimental */ export type NotificationPosition = (typeof NOTIFICATION_POSITIONS)[number]; /** Data needed to display a notification to the user */ @@ -5260,6 +5264,8 @@ declare module 'shared/models/notification.service-model' { * this together with {@link secondaryClickCommand} to give the notification two actions. * * Automatically localized if this is a {@link LocalizeKey}. + * + * @experimental */ secondaryClickCommandLabel?: string | LocalizeKey; /** @@ -5269,6 +5275,8 @@ declare module 'shared/models/notification.service-model' { * - NotificationId: The ID of the notification that was clicked * * The command handler should have the type signature {@link NotificationClickCommandHandler}. + * + * @experimental */ secondaryClickCommand?: keyof CommandHandlers; /** @@ -5290,11 +5298,15 @@ declare module 'shared/models/notification.service-model' { * decision — e.g. pairing it with a "postpone" command lets a two-button, must-answer-style toast * keep {@link dismissible} `true` (see the warning on {@link dismissible}). If you need the toast * to persist until the user actually answers, also set `duration` to `0`. + * + * @experimental */ dismissClickCommand?: keyof CommandHandlers; /** * Optional placement of the notification on screen. When omitted, the app's default placement is * used. + * + * @experimental */ position?: NotificationPosition; /** @@ -5310,6 +5322,8 @@ declare module 'shared/models/notification.service-model' { * no secondary/dismiss command. For a notification the user must explicitly answer, prefer * leaving `dismissible: true` and using {@link dismissClickCommand} so a swipe-away still counts * as a real (e.g. "postpone") decision. + * + * @experimental */ dismissible?: boolean; /** diff --git a/src/shared/models/notification.service-model.ts b/src/shared/models/notification.service-model.ts index a7da07028ac..b1ec1e84207 100644 --- a/src/shared/models/notification.service-model.ts +++ b/src/shared/models/notification.service-model.ts @@ -7,6 +7,8 @@ export type Severity = 'info' | 'warning' | 'error'; * The placements a notification can appear in, as a frozen array so it can be the single source of * truth for both the {@link NotificationPosition} type and the notification service's OpenRPC * `position` enum (which the service host spreads from this). + * + * @experimental */ export const NOTIFICATION_POSITIONS = Object.freeze([ 'top-left', @@ -20,6 +22,8 @@ export const NOTIFICATION_POSITIONS = Object.freeze([ /** * Where a notification is shown on screen. Mirrors the placements the host toast library supports. * Omit to use the app's default placement. + * + * @experimental */ export type NotificationPosition = (typeof NOTIFICATION_POSITIONS)[number]; @@ -53,6 +57,8 @@ export interface PlatformNotification { * this together with {@link secondaryClickCommand} to give the notification two actions. * * Automatically localized if this is a {@link LocalizeKey}. + * + * @experimental */ secondaryClickCommandLabel?: string | LocalizeKey; /** @@ -62,6 +68,8 @@ export interface PlatformNotification { * - NotificationId: The ID of the notification that was clicked * * The command handler should have the type signature {@link NotificationClickCommandHandler}. + * + * @experimental */ secondaryClickCommand?: keyof CommandHandlers; /** @@ -83,11 +91,15 @@ export interface PlatformNotification { * decision — e.g. pairing it with a "postpone" command lets a two-button, must-answer-style toast * keep {@link dismissible} `true` (see the warning on {@link dismissible}). If you need the toast * to persist until the user actually answers, also set `duration` to `0`. + * + * @experimental */ dismissClickCommand?: keyof CommandHandlers; /** * Optional placement of the notification on screen. When omitted, the app's default placement is * used. + * + * @experimental */ position?: NotificationPosition; /** @@ -103,6 +115,8 @@ export interface PlatformNotification { * no secondary/dismiss command. For a notification the user must explicitly answer, prefer * leaving `dismissible: true` and using {@link dismissClickCommand} so a swipe-away still counts * as a real (e.g. "postpone") decision. + * + * @experimental */ dismissible?: boolean; /** From 90c2bc5d4a13d94a7df106700d049e8cf8f3a398 Mon Sep 17 00:00:00 2001 From: Matt Lyons Date: Fri, 17 Jul 2026 14:26:12 -0500 Subject: [PATCH 12/16] fix(notifications): address PT-4193 round-5 review findings - 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 --- lib/papi-dts/papi.d.ts | 23 ++++-- .../components/notification-display.test.tsx | 49 ++++++------ .../components/notification-display.tsx | 9 ++- .../notification.service-host.test.ts | 79 ++++++++++--------- .../services/notification.service-host.ts | 64 ++++++++------- .../models/notification.service-model.ts | 23 ++++-- src/stories/keyboard-shortcuts.data.ts | 10 +++ 7 files changed, 148 insertions(+), 109 deletions(-) diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index dbf79a9c9b7..35635dd86ce 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -5280,7 +5280,7 @@ declare module 'shared/models/notification.service-model' { */ secondaryClickCommand?: keyof CommandHandlers; /** - * Optional command to run if the user dismisses the notification themselves — by swiping/dragging + * Optional command to run if the user dismisses the notification themselves - by swiping/dragging * it away, or by clicking the close button (if the host ever enables one). Sent no arguments * other than the notification id, like {@link clickCommand}: * @@ -5290,12 +5290,12 @@ declare module 'shared/models/notification.service-model' { * * IMPORTANT: this fires when the user dismisses the notification themselves (swiping/dragging it * away, or clicking a close button if the host ever enables one) AND when the notification - * auto-closes because its `duration` elapsed — a timeout is treated as an implicit dismissal, so + * auto-closes because its `duration` elapsed - a timeout is treated as an implicit dismissal, so * a must-answer toast that times out still runs this command instead of vanishing silently. It * does NOT fire when the notification is dismissed programmatically via * {@link INotificationService.dismiss}, nor when the user clicks {@link clickCommand} / * {@link secondaryClickCommand}. Use this to treat a swipe-away (or timeout) as an explicit - * decision — e.g. pairing it with a "postpone" command lets a two-button, must-answer-style toast + * decision - e.g. pairing it with a "postpone" command lets a two-button, must-answer-style toast * keep {@link dismissible} `true` (see the warning on {@link dismissible}). If you need the toast * to persist until the user actually answers, also set `duration` to `0`. * @@ -5316,13 +5316,20 @@ declare module 'shared/models/notification.service-model' { * The host toast library (Sonner) gates both the {@link secondaryClickCommand} button and the * user-dismiss gesture that fires {@link dismissClickCommand} on this same flag, so a naive * `dismissible: false` would silently turn those controls into dead buttons. To prevent that, the - * platform IGNORES `dismissible: false` when a {@link secondaryClickCommand} or - * {@link dismissClickCommand} is also set — the notification stays user-dismissible so those - * controls keep working. `dismissible: false` therefore only takes effect on a notification with - * no secondary/dismiss command. For a notification the user must explicitly answer, prefer + * platform IGNORES `dismissible: false` when the notification renders a secondary action button + * (a {@link secondaryClickCommand} paired with its {@link secondaryClickCommandLabel}) or has a + * {@link dismissClickCommand} - the notification stays user-dismissible so those controls keep + * working. `dismissible: false` therefore only takes effect on a notification with no secondary + * button and no dismiss command. For a notification the user must explicitly answer, prefer * leaving `dismissible: true` and using {@link dismissClickCommand} so a swipe-away still counts * as a real (e.g. "postpone") decision. * + * NOTE: `dismissible: false` does not keep the notification on screen. Auto-close is governed + * solely by `duration` (when omitted, 10-35 seconds computed from message length), so a + * non-dismissible notification still auto-closes on that timer. Also set `duration` to `0` (or + * less) if the notification must stay up until it is answered or programmatically dismissed via + * {@link INotificationService.dismiss}. + * * @experimental */ dismissible?: boolean; @@ -5330,7 +5337,7 @@ declare module 'shared/models/notification.service-model' { * Optional ID of a previous notification to update instead of showing a new notification. * * On an update (a `send` reusing an id that is still showing), any optional field you omit keeps - * the value it had on the previous `send` for that id — omitting a field never clears it. Pass + * the value it had on the previous `send` for that id - omitting a field never clears it. Pass * the field explicitly to change it. */ notificationId?: string | number; diff --git a/src/renderer/components/notification-display.test.tsx b/src/renderer/components/notification-display.test.tsx index a9255c6332b..dc75e503098 100644 --- a/src/renderer/components/notification-display.test.tsx +++ b/src/renderer/components/notification-display.test.tsx @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { render, screen, fireEvent, act } from '@testing-library/react'; import '@testing-library/jest-dom'; +import type { CommandHandlers } from 'papi-shared-types'; import type { INotificationService, PlatformNotification, @@ -8,6 +9,18 @@ import type { import * as commandService from '@shared/services/command.service'; import { NotificationDisplay } from './notification-display'; +/** + * Make a `keyof CommandHandlers` value from a test-only command name. These tests only need a + * command NAME to send/assert on - it never resolves to a real handler - so this single cast + * satisfies the field type without repeating the assertion (and its eslint-disable) at every use + * site. + */ +function commandStub(name: string): keyof CommandHandlers { + // No real handler is ever registered for these names, so a cast is the only way to type them + // eslint-disable-next-line no-type-assertion/no-type-assertion + return name as keyof CommandHandlers; +} + // This is the ONE render-level test in the notification suite that uses REAL Sonner instead of // mocking it (every other notification.service-host.test.ts case mocks 'sonner' wholesale, which is // exactly why the cancel-slot-button-is-dead-when-dismissible-is-false blocker slipped through @@ -22,7 +35,7 @@ vi.mock('@shared/services/localization.service', () => ({ }, })); vi.mock('@shared/services/logger.service', () => ({ - logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, })); let capturedService: INotificationService; @@ -72,9 +85,7 @@ describe('NotificationDisplay with real Sonner', () => { message: 'A decision is needed', severity: 'info', secondaryClickCommandLabel: 'Postpone', - // The test only needs a command NAME to send/assert on; it never resolves to a real handler. - // eslint-disable-next-line no-type-assertion/no-type-assertion - secondaryClickCommand: 'test.secondary' as never, + secondaryClickCommand: commandStub('test.secondary'), // Left true (the default) deliberately: this is the exact "two working buttons" shape the // blocker was about - dismissible: false would render this same button inert. }; @@ -102,13 +113,9 @@ describe('NotificationDisplay with real Sonner', () => { message: 'Time to sync', severity: 'info', clickCommandLabel: 'Send/Receive now', - // The test only needs a command NAME to send/assert on; it never resolves to a real handler. - // eslint-disable-next-line no-type-assertion/no-type-assertion - clickCommand: 'test.primary' as never, + clickCommand: commandStub('test.primary'), secondaryClickCommandLabel: 'Postpone until 3:24 PM', - // The test only needs a command NAME to send/assert on; it never resolves to a real handler. - // eslint-disable-next-line no-type-assertion/no-type-assertion - secondaryClickCommand: 'test.secondary' as never, + secondaryClickCommand: commandStub('test.secondary'), }; const notificationId = await capturedService.send(notification); @@ -152,9 +159,7 @@ describe('NotificationDisplay with real Sonner', () => { 'The project is locked on the server by another user. Breaking the lock may discard their unfinished send.', severity: 'warning', clickCommandLabel: 'Break lock and retry', - // The test only needs a command NAME to send/assert on; it never resolves to a real handler. - // eslint-disable-next-line no-type-assertion/no-type-assertion - clickCommand: 'test.primary' as never, + clickCommand: commandStub('test.primary'), }; const notificationId = await capturedService.send(notification); @@ -197,13 +202,9 @@ describe('NotificationDisplay with real Sonner', () => { message: 'Time to sync', severity: 'info', clickCommandLabel: 'Send/Receive now', - // The test only needs a command NAME to exist; it never resolves to a real handler. - // eslint-disable-next-line no-type-assertion/no-type-assertion - clickCommand: 'test.primary' as never, + clickCommand: commandStub('test.primary'), secondaryClickCommandLabel: 'Postpone until 3:24 PM', - // The test only needs a command NAME to exist; it never resolves to a real handler. - // eslint-disable-next-line no-type-assertion/no-type-assertion - secondaryClickCommand: 'test.secondary' as never, + secondaryClickCommand: commandStub('test.secondary'), }; await capturedService.send(notification); @@ -248,9 +249,7 @@ describe('NotificationDisplay with real Sonner', () => { message: 'A decision is needed', severity: 'info', secondaryClickCommandLabel: 'Postpone', - // The test only needs a command NAME to send/assert on; it never resolves to a real handler. - // eslint-disable-next-line no-type-assertion/no-type-assertion - secondaryClickCommand: 'test.secondary' as never, + secondaryClickCommand: commandStub('test.secondary'), }; await capturedService.send(notification); @@ -302,8 +301,10 @@ describe('NotificationDisplay with real Sonner', () => { document.dispatchEvent( new KeyboardEvent('keydown', { altKey: true, code: 'KeyT', bubbles: true }), ); - // Let the handler's queued microtask (which does the actual focus move) run. - await Promise.resolve(); + // Let the handler's queued macrotask (which does the actual focus move) run. + await new Promise((resolve) => { + setTimeout(resolve, 0); + }); }); return document.activeElement; } diff --git a/src/renderer/components/notification-display.tsx b/src/renderer/components/notification-display.tsx index 30184534e42..f13c0e7f466 100644 --- a/src/renderer/components/notification-display.tsx +++ b/src/renderer/components/notification-display.tsx @@ -61,9 +61,12 @@ export function NotificationDisplay() { function handleKeyDown(event: KeyboardEvent) { if (!eventMatchesHotkey(event, NOTIFICATION_TOASTER_HOTKEY)) return; // Run after Sonner's own synchronous hotkey handler (which expands the stack and focuses its - // single shared ref) has finished for this event, so our focus target wins regardless of - // listener order. - queueMicrotask(cycleToastListFocus); + // single shared ref) has finished for this event, so our focus target wins. A macrotask (not + // a microtask) makes that ordering independent of listener registration order: a timer + // callback never runs until the whole keydown dispatch task - every listener, in any order - + // has completed, whereas a microtask queued by a listener that happened to run BEFORE + // Sonner's would flush between listeners and let Sonner steal the focus back. + setTimeout(cycleToastListFocus, 0); } document.addEventListener('keydown', handleKeyDown); diff --git a/src/renderer/services/notification.service-host.test.ts b/src/renderer/services/notification.service-host.test.ts index 5416e96eae6..eb5cf8ac634 100644 --- a/src/renderer/services/notification.service-host.test.ts +++ b/src/renderer/services/notification.service-host.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { CommandHandlers } from 'papi-shared-types'; import type { INotificationService, PlatformNotification, @@ -6,6 +7,18 @@ import type { import * as commandService from '@shared/services/command.service'; import { logger } from '@shared/services/logger.service'; +/** + * Make a `keyof CommandHandlers` value from a test-only command name. The host passes command names + * straight through to Sonner/commandService without validating them against real registered + * commands, so a stub name suffices; this single cast satisfies the field type without repeating + * the assertion (and its eslint-disable) at every use site. + */ +function commandStub(name: string): keyof CommandHandlers { + // No real handler is ever registered for these names, so a cast is the only way to type them + // eslint-disable-next-line no-type-assertion/no-type-assertion + return name as keyof CommandHandlers; +} + /** * Minimal shape of the toastOptions object the host passes to Sonner's `toast.*` functions - typed * just precisely enough to invoke `action.onClick` / `cancel.onClick` / `onDismiss` / `onAutoClose` @@ -45,7 +58,7 @@ vi.mock('@shared/services/localization.service', () => ({ }, })); vi.mock('@shared/services/logger.service', () => ({ - logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, })); const mockSendCommand = vi.mocked(commandService.sendCommand); @@ -127,10 +140,7 @@ describe('notification service host', () => { message: 'test', severity: 'info', secondaryClickCommandLabel: 'Postpone', - // The host passes this straight to Sonner without validating it against real command names, - // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. - // eslint-disable-next-line no-type-assertion/no-type-assertion - secondaryClickCommand: 'test.secondary' as never, + secondaryClickCommand: commandStub('test.secondary'), }; await capturedService.send(notification); @@ -179,10 +189,7 @@ describe('notification service host', () => { severity: 'info', dismissible: false, secondaryClickCommandLabel: 'Postpone', - // The host passes this straight to Sonner without validating it against real command names, - // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. - // eslint-disable-next-line no-type-assertion/no-type-assertion - secondaryClickCommand: 'test.secondary' as never, + secondaryClickCommand: commandStub('test.secondary'), }; await capturedService.send(notification); @@ -195,6 +202,25 @@ describe('notification service host', () => { ); }); + it('keeps dismissible:false when the secondary command has no label (no button renders)', async () => { + const notification: PlatformNotification = { + message: 'test', + severity: 'info', + dismissible: false, + secondaryClickCommand: commandStub('test.secondary'), + }; + + await capturedService.send(notification); + + // Without a label the cancel button never renders, so there is no control for + // `dismissible: false` to kill - the caller's explicit request must be honored, not silently + // dropped based on the raw command field alone. + expect(mockToastInfo).toHaveBeenCalledWith( + 'test', + expect.objectContaining({ dismissible: false, cancel: undefined }), + ); + }); + it('renders no action/cancel button and no position/dismissible override when those fields are omitted (back-compat)', async () => { const notification: PlatformNotification = { message: 'test', severity: 'info' }; @@ -221,10 +247,7 @@ describe('notification service host', () => { notificationId: 'consent', position: 'top-center', secondaryClickCommandLabel: 'Postpone', - // The host passes this straight to Sonner without validating it against real command names, - // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. - // eslint-disable-next-line no-type-assertion/no-type-assertion - secondaryClickCommand: 'test.postpone' as never, + secondaryClickCommand: commandStub('test.postpone'), dismissible: false, }; await capturedService.send(first); @@ -252,10 +275,7 @@ describe('notification service host', () => { message: 'test', severity: 'info', secondaryClickCommandLabel: 'Postpone', - // The host passes this straight to Sonner without validating it against real command names, - // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. - // eslint-disable-next-line no-type-assertion/no-type-assertion - secondaryClickCommand: 'test.secondary' as never, + secondaryClickCommand: commandStub('test.secondary'), }; const notificationId = await capturedService.send(notification); @@ -271,10 +291,7 @@ describe('notification service host', () => { message: 'test', severity: 'info', secondaryClickCommandLabel: 'Postpone', - // The host passes this straight to Sonner without validating it against real command names, - // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. - // eslint-disable-next-line no-type-assertion/no-type-assertion - secondaryClickCommand: 'test.secondary' as never, + secondaryClickCommand: commandStub('test.secondary'), }; await capturedService.send(notification); @@ -291,10 +308,7 @@ describe('notification service host', () => { const notification: PlatformNotification = { message: 'test', severity: 'info', - // The host passes this straight to Sonner without validating it against real command names, - // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. - // eslint-disable-next-line no-type-assertion/no-type-assertion - dismissClickCommand: 'test.dismiss' as never, + dismissClickCommand: commandStub('test.dismiss'), }; const notificationId = await capturedService.send(notification); @@ -309,10 +323,7 @@ describe('notification service host', () => { const notification: PlatformNotification = { message: 'test', severity: 'info', - // The host passes this straight to Sonner without validating it against real command names, - // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. - // eslint-disable-next-line no-type-assertion/no-type-assertion - dismissClickCommand: 'test.dismiss' as never, + dismissClickCommand: commandStub('test.dismiss'), }; await capturedService.send(notification); @@ -393,10 +404,7 @@ describe('notification service host', () => { message: 'test', severity: 'info', clickCommandLabel: 'Send/Receive now', - // The host passes this straight to Sonner without validating it against real command names, - // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. - // eslint-disable-next-line no-type-assertion/no-type-assertion - clickCommand: 'test.primary' as never, + clickCommand: commandStub('test.primary'), }; await capturedService.send(notification); @@ -413,10 +421,7 @@ describe('notification service host', () => { const notification: PlatformNotification = { message: 'test', severity: 'info', - // The host passes this straight to Sonner without validating it against real command names, - // so a stub literal suffices; the cast only satisfies the keyof CommandHandlers field type. - // eslint-disable-next-line no-type-assertion/no-type-assertion - dismissClickCommand: 'test.dismiss' as never, + dismissClickCommand: commandStub('test.dismiss'), }; const notificationId = await capturedService.send(notification); diff --git a/src/renderer/services/notification.service-host.ts b/src/renderer/services/notification.service-host.ts index 01e85ef57d2..3d42cc22a57 100644 --- a/src/renderer/services/notification.service-host.ts +++ b/src/renderer/services/notification.service-host.ts @@ -8,36 +8,33 @@ import { } from '@shared/models/notification.service-model'; import * as commandService from '@shared/services/command.service'; import { networkObjectService } from '@shared/services/network-object.service'; -import { getErrorMessage, isLocalizeKey } from 'platform-bible-utils'; +import { getErrorMessage, isLocalizeKey, newGuid } from 'platform-bible-utils'; import { localizationService } from '@shared/services/localization.service'; import { logger } from '@shared/services/logger.service'; -/** Caller-facing notification id -> the toast id Sonner actually rendered it under. */ -const mapOfNotificationIdsToToastIds = new Map(); - /** - * Caller-facing notification id -> the last notification we sent for it. An update-send merges over - * this so omitting an optional field keeps its previously-set value instead of clobbering it. + * Caller-facing notification id -> everything we track for its live toast: the toast id Sonner + * actually rendered it under, and the last notification we sent for it (an update-send merges over + * this so omitting an optional field keeps its previously-set value instead of clobbering it). One + * map rather than parallel maps so no removal path can clean up one half and leak the other. */ -const lastNotificationById = new Map(); +const trackedNotificationsById = new Map< + string | number, + { toastId: string | number; lastNotification: PlatformNotification } +>(); /** - * Counter backing {@link generateAutoNotificationId}. A send without a `notificationId` gets an id - * from our own namespace rather than Sonner's internal numeric auto-ids, which would otherwise + * Mint a unique caller-facing id, in our own namespace, for a notification sent without one. Uses + * `newGuid` (non-numeric) rather than Sonner's internal numeric auto-ids, which would otherwise * share a namespace with caller-supplied numeric ids and collide. */ -let autoAssignedNotificationIdCount = 0; - -/** Mint a unique caller-facing id, in our own namespace, for a notification sent without one. */ function generateAutoNotificationId(): string { - autoAssignedNotificationIdCount += 1; - return `platform-notification-auto-${autoAssignedNotificationIdCount}`; + return `platform-notification-auto-${newGuid()}`; } /** Drop all bookkeeping for a notification once its toast is removed (via any removal path). */ function forgetNotification(notificationId: string | number): void { - mapOfNotificationIdsToToastIds.delete(notificationId); - lastNotificationById.delete(notificationId); + trackedNotificationsById.delete(notificationId); } async function localize(text: string): Promise { @@ -58,10 +55,10 @@ async function send(notification: PlatformNotification): Promise { - const toastId = mapOfNotificationIdsToToastIds.get(notificationId); - if (toastId !== undefined) { - toast.dismiss(toastId); + const trackedNotification = trackedNotificationsById.get(notificationId); + if (trackedNotification !== undefined) { + toast.dismiss(trackedNotification.toastId); forgetNotification(notificationId); } } diff --git a/src/shared/models/notification.service-model.ts b/src/shared/models/notification.service-model.ts index b1ec1e84207..12758b0933b 100644 --- a/src/shared/models/notification.service-model.ts +++ b/src/shared/models/notification.service-model.ts @@ -73,7 +73,7 @@ export interface PlatformNotification { */ secondaryClickCommand?: keyof CommandHandlers; /** - * Optional command to run if the user dismisses the notification themselves — by swiping/dragging + * Optional command to run if the user dismisses the notification themselves - by swiping/dragging * it away, or by clicking the close button (if the host ever enables one). Sent no arguments * other than the notification id, like {@link clickCommand}: * @@ -83,12 +83,12 @@ export interface PlatformNotification { * * IMPORTANT: this fires when the user dismisses the notification themselves (swiping/dragging it * away, or clicking a close button if the host ever enables one) AND when the notification - * auto-closes because its `duration` elapsed — a timeout is treated as an implicit dismissal, so + * auto-closes because its `duration` elapsed - a timeout is treated as an implicit dismissal, so * a must-answer toast that times out still runs this command instead of vanishing silently. It * does NOT fire when the notification is dismissed programmatically via * {@link INotificationService.dismiss}, nor when the user clicks {@link clickCommand} / * {@link secondaryClickCommand}. Use this to treat a swipe-away (or timeout) as an explicit - * decision — e.g. pairing it with a "postpone" command lets a two-button, must-answer-style toast + * decision - e.g. pairing it with a "postpone" command lets a two-button, must-answer-style toast * keep {@link dismissible} `true` (see the warning on {@link dismissible}). If you need the toast * to persist until the user actually answers, also set `duration` to `0`. * @@ -109,13 +109,20 @@ export interface PlatformNotification { * The host toast library (Sonner) gates both the {@link secondaryClickCommand} button and the * user-dismiss gesture that fires {@link dismissClickCommand} on this same flag, so a naive * `dismissible: false` would silently turn those controls into dead buttons. To prevent that, the - * platform IGNORES `dismissible: false` when a {@link secondaryClickCommand} or - * {@link dismissClickCommand} is also set — the notification stays user-dismissible so those - * controls keep working. `dismissible: false` therefore only takes effect on a notification with - * no secondary/dismiss command. For a notification the user must explicitly answer, prefer + * platform IGNORES `dismissible: false` when the notification renders a secondary action button + * (a {@link secondaryClickCommand} paired with its {@link secondaryClickCommandLabel}) or has a + * {@link dismissClickCommand} - the notification stays user-dismissible so those controls keep + * working. `dismissible: false` therefore only takes effect on a notification with no secondary + * button and no dismiss command. For a notification the user must explicitly answer, prefer * leaving `dismissible: true` and using {@link dismissClickCommand} so a swipe-away still counts * as a real (e.g. "postpone") decision. * + * NOTE: `dismissible: false` does not keep the notification on screen. Auto-close is governed + * solely by `duration` (when omitted, 10-35 seconds computed from message length), so a + * non-dismissible notification still auto-closes on that timer. Also set `duration` to `0` (or + * less) if the notification must stay up until it is answered or programmatically dismissed via + * {@link INotificationService.dismiss}. + * * @experimental */ dismissible?: boolean; @@ -123,7 +130,7 @@ export interface PlatformNotification { * Optional ID of a previous notification to update instead of showing a new notification. * * On an update (a `send` reusing an id that is still showing), any optional field you omit keeps - * the value it had on the previous `send` for that id — omitting a field never clears it. Pass + * the value it had on the previous `send` for that id - omitting a field never clears it. Pass * the field explicitly to change it. */ notificationId?: string | number; diff --git a/src/stories/keyboard-shortcuts.data.ts b/src/stories/keyboard-shortcuts.data.ts index 6042161940b..e8da05aeb87 100644 --- a/src/stories/keyboard-shortcuts.data.ts +++ b/src/stories/keyboard-shortcuts.data.ts @@ -124,6 +124,16 @@ export const rootKeyboardShortcuts: KeyboardShortcutEntry[] = [ keys: { macOS: '⌘]', windows: 'Alt+→', linux: 'Alt+→' }, locations: ['src/main/main.ts', 'src/main/reference-history-keyboard.util.ts'], }, + { + id: 'focus-notification-toasts', + purpose: 'Focus the notification toasts, cycling across position groups on repeated presses', + category: 'Navigation', + context: 'Renderer (global)', + // Sonner's built-in Toaster hotkey and NotificationDisplay's focus-cycling handler share this + // combo (NOTIFICATION_TOASTER_HOTKEY) - they must not drift apart. + keys: { macOS: '⌥T', windows: 'Alt+T', linux: 'Alt+T' }, + locations: ['src/renderer/components/notification-display.tsx'], + }, { id: 'zoom-in', purpose: 'Zoom in', From 9c3be3334ee9434182d1c335c9c067d500a0a2ce Mon Sep 17 00:00:00 2001 From: Matt Lyons Date: Fri, 17 Jul 2026 14:59:51 -0500 Subject: [PATCH 13/16] feat(notifications): style the secondary toast button as shadcn secondary (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 --- lib/papi-dts/papi.d.ts | 8 +++++++- .../components/notification-display.scss | 20 +++++++++++++++++++ .../models/notification.service-model.ts | 8 +++++++- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/lib/papi-dts/papi.d.ts b/lib/papi-dts/papi.d.ts index 35635dd86ce..cc4711b39c7 100644 --- a/lib/papi-dts/papi.d.ts +++ b/lib/papi-dts/papi.d.ts @@ -5245,7 +5245,9 @@ declare module 'shared/models/notification.service-model' { /** Severity of the notification */ severity: Severity; /** - * Optional label for users to click when the notification shows. + * Optional label for users to click when the notification shows. Always rendered as the + * notification's PRIMARY action button - the visually emphasized one - while + * {@link secondaryClickCommandLabel} always gets the muted secondary styling. * * Automatically localized if this is a {@link LocalizeKey}. */ @@ -5263,6 +5265,10 @@ declare module 'shared/models/notification.service-model' { * Optional label for a second action button, shown alongside {@link clickCommandLabel}. Provide * this together with {@link secondaryClickCommand} to give the notification two actions. * + * Always rendered as the visually SECONDARY button (muted styling, like the shadcn `secondary` + * button variant) so the {@link clickCommandLabel} button keeps the emphasis - the platform + * decides each button's styling from which field it came from, never from ordering. + * * Automatically localized if this is a {@link LocalizeKey}. * * @experimental diff --git a/src/renderer/components/notification-display.scss b/src/renderer/components/notification-display.scss index 60bf2c997b0..75fe585373a 100644 --- a/src/renderer/components/notification-display.scss +++ b/src/renderer/components/notification-display.scss @@ -68,4 +68,24 @@ &:has(.notification-toast-cancel-button) .notification-toast-action-button { margin-inline-start: 0; } + + // Settle the DEFAULT look of the secondary (cancel-slot) button as the shadcn `secondary` button + // variant (button.tsx: bg-secondary / text-secondary-foreground / hover:bg-secondary/80), per UX + // direction on PR #2561. Without this, Sonner 1.7.4's styled mode renders BOTH buttons + // identically dark: its base `[data-sonner-toast][data-styled='true'] [data-button]` rule + // (specificity (0,3,0)) sets the action colors on every `[data-button]` - including the cancel + // button, which also carries `data-button` - and its softer `:where([data-cancel])` defaults are + // zero-specificity so they never win. Which button gets this look is deterministic: the service + // host maps `clickCommand` to Sonner's `action` slot (keeps the emphasized primary look) and + // `secondaryClickCommand` to the `cancel` slot, whose class this targets. The extra `&:has()` + // keeps specificity at (0,4,0) so this beats Sonner's (0,3,0) base rule regardless of which + // stylesheet loads last (same pattern as the margin rule above). + &:has(.notification-toast-cancel-button) .notification-toast-cancel-button { + background: var(--secondary); + color: var(--secondary-foreground); + + &:hover { + background: color-mix(in oklab, var(--secondary) 80%, transparent); + } + } } diff --git a/src/shared/models/notification.service-model.ts b/src/shared/models/notification.service-model.ts index 12758b0933b..cbb882f6a16 100644 --- a/src/shared/models/notification.service-model.ts +++ b/src/shared/models/notification.service-model.ts @@ -38,7 +38,9 @@ export interface PlatformNotification { /** Severity of the notification */ severity: Severity; /** - * Optional label for users to click when the notification shows. + * Optional label for users to click when the notification shows. Always rendered as the + * notification's PRIMARY action button - the visually emphasized one - while + * {@link secondaryClickCommandLabel} always gets the muted secondary styling. * * Automatically localized if this is a {@link LocalizeKey}. */ @@ -56,6 +58,10 @@ export interface PlatformNotification { * Optional label for a second action button, shown alongside {@link clickCommandLabel}. Provide * this together with {@link secondaryClickCommand} to give the notification two actions. * + * Always rendered as the visually SECONDARY button (muted styling, like the shadcn `secondary` + * button variant) so the {@link clickCommandLabel} button keeps the emphasis - the platform + * decides each button's styling from which field it came from, never from ordering. + * * Automatically localized if this is a {@link LocalizeKey}. * * @experimental From 3918b134c7ab4fd4502750677f8df82b45602c9a Mon Sep 17 00:00:00 2001 From: Matt Lyons Date: Fri, 17 Jul 2026 15:14:28 -0500 Subject: [PATCH 14/16] chore(deps): pin sonner to exactly 1.7.4 (PT-4193) 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 --- lib/platform-bible-react/package.json | 2 +- package-lock.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/platform-bible-react/package.json b/lib/platform-bible-react/package.json index e2800835588..faff7725e06 100644 --- a/lib/platform-bible-react/package.json +++ b/lib/platform-bible-react/package.json @@ -100,7 +100,7 @@ "radix-ui": "^1.4.3", "react-hotkeys-hook": "^4.6.1", "react-resizable-panels": "^4.10.0", - "sonner": "^1.7.4", + "sonner": "1.7.4", "tailwind-merge": "^3.5.0", "tw-animate-css": "^1.4.0", "vaul": "^1.1.2" diff --git a/package-lock.json b/package-lock.json index 36d57ef0318..1b901e65f59 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1204,7 +1204,7 @@ "radix-ui": "^1.4.3", "react-hotkeys-hook": "^4.6.1", "react-resizable-panels": "^4.10.0", - "sonner": "^1.7.4", + "sonner": "1.7.4", "tailwind-merge": "^3.5.0", "tw-animate-css": "^1.4.0", "vaul": "^1.1.2" From 4db351921fec190940b23584bc3ea828362c490e Mon Sep 17 00:00:00 2001 From: Matt Lyons Date: Fri, 17 Jul 2026 15:14:29 -0500 Subject: [PATCH 15/16] fix(notifications): follow the app theme in the notification Toaster (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 --- .../components/notification-display.test.tsx | 41 +++++++++++++++++++ .../components/notification-display.tsx | 33 +++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/src/renderer/components/notification-display.test.tsx b/src/renderer/components/notification-display.test.tsx index dc75e503098..ff1572b55fa 100644 --- a/src/renderer/components/notification-display.test.tsx +++ b/src/renderer/components/notification-display.test.tsx @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { render, screen, fireEvent, act } from '@testing-library/react'; import '@testing-library/jest-dom'; import type { CommandHandlers } from 'papi-shared-types'; +import type { ThemeDefinitionExpanded } from 'platform-bible-utils'; import type { INotificationService, PlatformNotification, @@ -38,6 +39,21 @@ vi.mock('@shared/services/logger.service', () => ({ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, })); +// Theme type the CurrentTheme data hook reports to NotificationDisplay; tests flip this to 'dark' +// to pin the Toaster's theme wiring. The papi data hooks are stubbed wholesale because pulling the +// real ones into jsdom drags the whole data-provider network stack along with them. +let mockCurrentThemeType = 'light'; +vi.mock('@renderer/hooks/papi-hooks', () => ({ + useDataProvider: vi.fn(), + useData: vi.fn(() => ({ + CurrentTheme: (_selector: undefined, defaultValue: ThemeDefinitionExpanded) => [ + { ...defaultValue, type: mockCurrentThemeType, id: mockCurrentThemeType }, + vi.fn(), + false, + ], + })), +})); + let capturedService: INotificationService; vi.mock('@shared/services/network-object.service', () => ({ networkObjectService: { @@ -70,6 +86,7 @@ describe('NotificationDisplay with real Sonner', () => { beforeEach(async () => { vi.clearAllMocks(); vi.resetModules(); + mockCurrentThemeType = 'light'; mockSendCommand.mockResolvedValue(undefined); stubMatchMedia(); const { startNotificationService } = await import( @@ -278,6 +295,30 @@ describe('NotificationDisplay with real Sonner', () => { expect(document.querySelector('.notification-toast-action-button')).not.toBeInTheDocument(); }); + // PT-4193 (PR #2561): the Toaster follows the app theme from the theme service. Sonner's own + // default is a fixed light theme, which left dark-themed apps with white toasts - and made the + // shadcn-token-styled secondary button (which reads the app-level `--secondary` variables) flip + // to dark-theme colors on a still-white toast, collapsing the primary/secondary hierarchy. + it('themes the toaster to match the current app theme', async () => { + mockCurrentThemeType = 'dark'; + render(); + + await capturedService.send({ message: 'Dark toast', severity: 'info' }); + await screen.findByText('Dark toast'); + + expect(document.querySelector('[data-sonner-toaster]')).toHaveAttribute('data-theme', 'dark'); + }); + + it('falls back to the light look for a theme type Sonner does not understand', async () => { + mockCurrentThemeType = 'paratext-classic'; + render(); + + await capturedService.send({ message: 'Custom-theme toast', severity: 'info' }); + await screen.findByText('Custom-theme toast'); + + expect(document.querySelector('[data-sonner-toaster]')).toHaveAttribute('data-theme', 'light'); + }); + // PT-4193 (review C61-2): a per-toast `position` makes Sonner render one
                      // per distinct position, all sharing a single ref, so Sonner's own Alt+T hotkey only ever focuses // the last list. NotificationDisplay layers a focus-cycling handler on top so repeated Alt+T diff --git a/src/renderer/components/notification-display.tsx b/src/renderer/components/notification-display.tsx index f13c0e7f466..2035ab0157d 100644 --- a/src/renderer/components/notification-display.tsx +++ b/src/renderer/components/notification-display.tsx @@ -1,7 +1,24 @@ import { useEffect, useRef } from 'react'; import { Toaster } from 'sonner'; +import { isPlatformError, type ThemeDefinitionExpanded } from 'platform-bible-utils'; +import { themeServiceDataProviderName } from '@shared/services/theme.service-model'; +import { useData, useDataProvider } from '@renderer/hooks/papi-hooks'; import './notification-display.scss'; +/** + * Placeholder passed as the default value for the `CurrentTheme` data hook so it has something + * structurally valid to return while the real theme loads (same pattern as + * `user-profile-popover.component.tsx`). Only `type` is ever read; the light default matches what + * Sonner would render on its own before the theme arrives. + */ +const DEFAULT_THEME_VALUE: ThemeDefinitionExpanded = { + themeFamilyId: '', + type: 'light', + id: 'light', + label: '%toolbar_theme_loading%', + cssVariables: {}, +}; + // Sonner's default focus hotkey (Alt+T). Declared here so the exact same combo drives both Sonner's // own and our focus-cycling handler below - the two must not drift apart. const NOTIFICATION_TOASTER_HOTKEY = ['altKey', 'KeyT']; @@ -37,6 +54,21 @@ export function NotificationDisplay() { // Index of the toast list Alt+T last focused, so repeated presses cycle across every list. const focusedListIndexRef = useRef(-1); + // PT-4193 (PR #2561): follow the app theme so toasts render coherently in dark mode. Sonner's + // own default is a fixed light theme, which left dark-themed apps with white toasts - and made + // the shadcn-token-styled secondary button (notification-display.scss reads the app-level + // `--secondary` variables, which flip with the app theme) render dark-theme colors on a + // still-white toast, collapsing the primary/secondary button hierarchy. + const themeDataProvider = useDataProvider(themeServiceDataProviderName); + const [currentTheme] = useData( + themeDataProvider, + ).CurrentTheme(undefined, DEFAULT_THEME_VALUE); + // Theme `type` is an open string (theme families can define their own types), but Sonner only + // understands light/dark - so anything that isn't exactly 'dark' gets the light look, matching + // the platform's own fallback behavior for unknown theme types. + const themeType = + !isPlatformError(currentTheme) && currentTheme.type === 'dark' ? 'dark' : 'light'; + // PT-4193 (review C61-2): with a per-toast `position`, Sonner 1.7.4 renders one `
                        ` per // distinct position but assigns them all a single shared `ref`, so its own Alt+T handler only // ever focuses the LAST list - leaving toasts in every other position group unreachable by @@ -75,6 +107,7 @@ export function NotificationDisplay() { return ( Date: Fri, 17 Jul 2026 15:30:18 -0500 Subject: [PATCH 16/16] docs(notifications): drop PR/review-round references from code comments 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 --- src/renderer/components/notification-display.scss | 7 +++---- src/renderer/components/notification-display.test.tsx | 10 +++++----- src/renderer/components/notification-display.tsx | 10 +++++----- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/renderer/components/notification-display.scss b/src/renderer/components/notification-display.scss index 75fe585373a..1beed98fd5f 100644 --- a/src/renderer/components/notification-display.scss +++ b/src/renderer/components/notification-display.scss @@ -1,4 +1,4 @@ -// PT-4193: a toast with buttons renders, by Sonner 1.7.4's own stylesheet +// A toast with buttons renders, by Sonner 1.7.4's own stylesheet // (node_modules/sonner/dist/styles.css), as a single un-wrapped flex row: icon, content, cancel // button, action button. Sonner's buttons are `flex-shrink: 0` and its toast width is a fixed // `--width` (356px), so wide buttons crush the message content down to a sliver - confirmed live @@ -14,8 +14,7 @@ // and action as direct flex children of the toast `
                      1. `; revisit if Sonner changes its toast DOM. // Deliberately avoids Sonner's own `::after` (its ALWAYS-ON hover-bridge between stacked toasts) // and repurposes `::before`, which Sonner only styles in the swiping/removed states - see the -// comment on the row break below for how that collision is resolved. See PT-4193 review -// C61-3/4/15/23/26 for the failure modes this replaces. +// comment on the row break below for how that collision is resolved. .notification-toast:has(.notification-toast-cancel-button, .notification-toast-action-button) { flex-wrap: wrap; @@ -71,7 +70,7 @@ // Settle the DEFAULT look of the secondary (cancel-slot) button as the shadcn `secondary` button // variant (button.tsx: bg-secondary / text-secondary-foreground / hover:bg-secondary/80), per UX - // direction on PR #2561. Without this, Sonner 1.7.4's styled mode renders BOTH buttons + // direction. Without this, Sonner 1.7.4's styled mode renders BOTH buttons // identically dark: its base `[data-sonner-toast][data-styled='true'] [data-button]` rule // (specificity (0,3,0)) sets the action colors on every `[data-button]` - including the cancel // button, which also carries `data-button` - and its softer `:where([data-cancel])` defaults are diff --git a/src/renderer/components/notification-display.test.tsx b/src/renderer/components/notification-display.test.tsx index ff1572b55fa..8bcf852a832 100644 --- a/src/renderer/components/notification-display.test.tsx +++ b/src/renderer/components/notification-display.test.tsx @@ -25,7 +25,7 @@ function commandStub(name: string): keyof CommandHandlers { // This is the ONE render-level test in the notification suite that uses REAL Sonner instead of // mocking it (every other notification.service-host.test.ts case mocks 'sonner' wholesale, which is // exactly why the cancel-slot-button-is-dead-when-dismissible-is-false blocker slipped through -// review undetected - see PT-4193's "Review fix" PR notes). Rendering the real Toaster and clicking +// review undetected). Rendering the real Toaster and clicking // the real DOM button pins the actual Sonner contract instead of just the shape we hand it. vi.mock('@shared/services/command.service', () => ({ sendCommand: vi.fn() })); vi.mock('@shared/services/localization.service', () => ({ @@ -115,7 +115,7 @@ describe('NotificationDisplay with real Sonner', () => { expect(mockSendCommand).toHaveBeenCalledWith('test.secondary', notificationId); }); - // PT-4193 layout fix: live E2E screenshots showed toasts with buttons collapsing their message + // Layout fix: live E2E screenshots showed toasts with buttons collapsing their message // down to a sliver - first a toast with BOTH an action and a cancel button, then (round-4 E2E) // a single-action break-lock toast. Sonner's default layout puts icon/content/cancel/action in a // single un-wrapped flex row, and non-shrinking buttons crush the content column. @@ -201,7 +201,7 @@ describe('NotificationDisplay with real Sonner', () => { expect(mockSendCommand).toHaveBeenCalledWith('test.primary', notificationId); }); - // PT-4193 press-collapse regression (found in external triage): Sonner flips the toast to + // Press-collapse regression (found in external triage): Sonner flips the toast to // `data-swiping="true"` on pointerdown - ANY mouse button, on the toast body, BEFORE any movement // (its handler checks neither `event.button` nor movement, only that the target is not a BUTTON) // - and clears it again on pointerup. An earlier notification-display.scss revision gated the @@ -295,7 +295,7 @@ describe('NotificationDisplay with real Sonner', () => { expect(document.querySelector('.notification-toast-action-button')).not.toBeInTheDocument(); }); - // PT-4193 (PR #2561): the Toaster follows the app theme from the theme service. Sonner's own + // The Toaster follows the app theme from the theme service. Sonner's own // default is a fixed light theme, which left dark-themed apps with white toasts - and made the // shadcn-token-styled secondary button (which reads the app-level `--secondary` variables) flip // to dark-theme colors on a still-white toast, collapsing the primary/secondary hierarchy. @@ -319,7 +319,7 @@ describe('NotificationDisplay with real Sonner', () => { expect(document.querySelector('[data-sonner-toaster]')).toHaveAttribute('data-theme', 'light'); }); - // PT-4193 (review C61-2): a per-toast `position` makes Sonner render one
                          + // A per-toast `position` makes Sonner render one
                            // per distinct position, all sharing a single ref, so Sonner's own Alt+T hotkey only ever focuses // the last list. NotificationDisplay layers a focus-cycling handler on top so repeated Alt+T // reaches every list. (This pins that both lists become the active element across presses; jsdom diff --git a/src/renderer/components/notification-display.tsx b/src/renderer/components/notification-display.tsx index 2035ab0157d..f7afee11fc0 100644 --- a/src/renderer/components/notification-display.tsx +++ b/src/renderer/components/notification-display.tsx @@ -46,7 +46,7 @@ function eventMatchesHotkey(event: KeyboardEvent, hotkey: readonly string[]): bo }); } -// PT-4193: class hooks for notification-display.scss's fix for the buttoned-toast layout collapse - +// Class hooks for notification-display.scss's fix for the buttoned-toast layout collapse - // see that file for the full explanation. Applied here (the `Toaster`'s shared `toastOptions`) // rather than per-notification in notification.service-host.ts so every toast gets the hooks // uniformly; the CSS itself only changes layout when at least one button is present. @@ -54,9 +54,9 @@ export function NotificationDisplay() { // Index of the toast list Alt+T last focused, so repeated presses cycle across every list. const focusedListIndexRef = useRef(-1); - // PT-4193 (PR #2561): follow the app theme so toasts render coherently in dark mode. Sonner's - // own default is a fixed light theme, which left dark-themed apps with white toasts - and made - // the shadcn-token-styled secondary button (notification-display.scss reads the app-level + // Follow the app theme so toasts render coherently in dark mode. Sonner's own default is a + // fixed light theme, which left dark-themed apps with white toasts - and made the + // shadcn-token-styled secondary button (notification-display.scss reads the app-level // `--secondary` variables, which flip with the app theme) render dark-theme colors on a // still-white toast, collapsing the primary/secondary button hierarchy. const themeDataProvider = useDataProvider(themeServiceDataProviderName); @@ -69,7 +69,7 @@ export function NotificationDisplay() { const themeType = !isPlatformError(currentTheme) && currentTheme.type === 'dark' ? 'dark' : 'light'; - // PT-4193 (review C61-2): with a per-toast `position`, Sonner 1.7.4 renders one `
                              ` per + // With a per-toast `position`, Sonner 1.7.4 renders one `
                                ` per // distinct position but assigns them all a single shared `ref`, so its own Alt+T handler only // ever focuses the LAST list - leaving toasts in every other position group unreachable by // keyboard (each `
                                  ` is also its own focus trap that ejects focus when you try to Tab out to a