Skip to content

Commit df613fa

Browse files
os-zhuangclaude
andauthored
fix(notifications): the spec icon is read instead of stored and ignored (#3014 follow-up) (#3076)
`NotificationSchema.icon` — "Icon name override" — reached `NotificationItem` and stopped there. Every surface drew the severity icon, so an author writing `icon: 'rocket'` got the success checkmark. Same shape as the `displayType` collapse #3071 fixed: a value that validates, is carried, and renders nothing. All five presentations now resolve it through one rule (`notificationIcon`): a declared Lucide name — kebab-case or PascalCase — replaces the severity icon; anything else falls back to it. That includes the console's sonner toast (`presentNotificationToast`, now .tsx so it can build the icon element), so the override behaves identically on all five. The fallback is the interesting part. `getLazyIcon` degrades an unknown name to a `Database` glyph — right for a data-shaped schema slot, wrong here, where it would swap a meaningful icon for a meaningless one on an error notification. So the name is checked first via a new `isLucideIconName` export, and a typo costs the author their override and nothing more. The two `react-hooks/static-components` disables follow the existing repo convention for this rule (MetricCard / MetricWidget / NavigationRenderer): the factory is module-cached per name, so the component identity is stable and the rule is a false positive here. Verified in the running console: a toast declaring `icon: 'rocket'` renders the rocket instead of the success checkmark, while a snackbar declaring `icon: 'not-a-real-icon'` renders the info icon — not a Database glyph. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 5340879 commit df613fa

14 files changed

Lines changed: 172 additions & 11 deletions
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
---
2+
"@object-ui/components": minor
3+
"@object-ui/app-shell": minor
4+
---
5+
6+
fix(notifications): the spec `icon` is read instead of stored and ignored (#3014 follow-up)
7+
8+
`NotificationSchema.icon` — "Icon name override" — reached `NotificationItem` and
9+
stopped there. Every surface drew the severity icon, so an author writing
10+
`icon: 'rocket'` got the success checkmark. Same shape as the `displayType`
11+
collapse #3071 fixed: a value that validates, is carried, and renders nothing.
12+
13+
All five presentations now resolve it through one rule (`notificationIcon`): a
14+
declared Lucide name — kebab-case or PascalCase — replaces the severity icon;
15+
anything else falls back to it. That includes the console's sonner toast, so the
16+
override behaves identically on a toast, a banner, a snackbar, an alert and an
17+
inline message.
18+
19+
**The fallback is the interesting part.** `getLazyIcon` degrades an unknown name
20+
to a `Database` glyph, which is right for a data-shaped schema slot and wrong
21+
here — on an error notification it swaps a meaningful icon for a meaningless one.
22+
So the name is checked first, via a new `isLucideIconName` export, and a typo
23+
costs the author their override and nothing more.

content/docs/guide/notifications.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,21 @@ Omit `scope` on both ends for a page-level inline outlet. `scope` is renderer-lo
115115
routing metadata, not a spec field — the spec describes what a notification *is*,
116116
not which React subtree hosts it.
117117

118+
### `icon`
119+
120+
Every surface — including the console's sonner toast — resolves `icon` through
121+
the same rule: a declared Lucide name (kebab-case or PascalCase) replaces the
122+
severity icon; anything else falls back to it.
123+
124+
```tsx
125+
notify({ title: 'Deploy finished', severity: 'success', displayType: 'banner', icon: 'rocket' });
126+
```
127+
128+
A name Lucide doesn't have costs the author their override and nothing more —
129+
deliberately *not* the generic `Database` glyph `getLazyIcon` returns for
130+
data-shaped schema slots, which on an error notification would replace a
131+
meaningful icon with a meaningless one.
132+
118133
### `dismissible`
119134

120135
Defaults to `true`. On the persistent presentations, `dismissible: false` removes

packages/app-shell/src/chrome/notificationToast.test.ts renamed to packages/app-shell/src/chrome/notificationToast.test.tsx

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
*/
77

88
import { describe, it, expect, vi, beforeEach } from 'vitest';
9+
import { isValidElement } from 'react';
910
import type { NotificationItem } from '@object-ui/react';
1011

1112
vi.mock('sonner', () => ({
@@ -16,6 +17,10 @@ vi.mock('sonner', () => ({
1617

1718
import { toast } from 'sonner';
1819
import { presentNotificationToast } from './notificationToast';
20+
// Imported at module scope, not inside a test: `@object-ui/components` is a
21+
// heavy barrel and resolving it mid-assertion would race the RTL timeouts
22+
// (AGENTS.md § 测试纪律).
23+
import '@object-ui/components';
1924

2025
function item(overrides: Partial<NotificationItem> = {}): NotificationItem {
2126
return {
@@ -101,4 +106,31 @@ describe('presentNotificationToast', () => {
101106
dismissible: false,
102107
}));
103108
});
109+
110+
it('passes the spec icon override to sonner', () => {
111+
presentNotificationToast(item({ icon: 'rocket' }));
112+
const { icon } = vi.mocked(toast.success).mock.calls[0][1] as { icon?: unknown };
113+
expect(isValidElement(icon)).toBe(true);
114+
expect((icon as { props: { name?: string } }).props.name).toBe('rocket');
115+
});
116+
117+
it('accepts a PascalCase icon name', () => {
118+
presentNotificationToast(item({ icon: 'CircleCheck' }));
119+
const { icon } = vi.mocked(toast.success).mock.calls[0][1] as { icon?: unknown };
120+
expect(isValidElement(icon)).toBe(true);
121+
});
122+
123+
it('omits the icon key for a name Lucide does not have', () => {
124+
// Passing it through would render LazyIcon's `Database` fallback — a
125+
// meaningless glyph where ConsoleToaster's severity icon belongs.
126+
presentNotificationToast(item({ icon: 'not-a-real-icon' }));
127+
const options = vi.mocked(toast.success).mock.calls[0][1] as Record<string, unknown>;
128+
expect(options).not.toHaveProperty('icon');
129+
});
130+
131+
it('omits the icon key when none is declared', () => {
132+
presentNotificationToast(item());
133+
const options = vi.mocked(toast.success).mock.calls[0][1] as Record<string, unknown>;
134+
expect(options).not.toHaveProperty('icon');
135+
});
104136
});

packages/app-shell/src/chrome/notificationToast.ts renamed to packages/app-shell/src/chrome/notificationToast.tsx

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import { toast } from 'sonner';
1717
import type { NotificationItem, NotificationSeverityLevel } from '@object-ui/react';
18+
import { isLucideIconName, LazyIcon } from '@object-ui/components';
1819

1920
/**
2021
* Typed as `Record<NotificationSeverityLevel, …>` so a new severity in the spec
@@ -40,11 +41,19 @@ export function presentNotificationToast(notification: NotificationItem): void {
4041
// A toast carries at most one action button — sonner has one `action` slot,
4142
// and a notification needing more than one belongs on a banner or an alert.
4243
const action = notification.actions?.[0];
44+
// The spec `icon` override, matching what the four surface components do.
45+
// Only a REAL Lucide name is passed: `LazyIcon` degrades an unknown one to a
46+
// `Database` glyph, whereas omitting the key leaves ConsoleToaster's severity
47+
// icon in place — the better fallback for a typo.
48+
const icon = isLucideIconName(notification.icon)
49+
? <LazyIcon name={notification.icon} className="h-4 w-4" />
50+
: undefined;
4351

4452
show(notification.title, {
4553
description: notification.message,
4654
duration: notification.duration === 0 ? Infinity : notification.duration,
4755
dismissible: notification.dismissible,
56+
...(icon ? { icon } : {}),
4857
...(action ? { action: { label: action.label, onClick: action.onClick } } : {}),
4958
});
5059
}

packages/components/README.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,9 @@ raised through `NotificationProvider` from `@object-ui/react`. One per spec
163163
| `<NotificationAlerts />` | `alert` | anywhere inside the provider — blocking dialog, FIFO queue |
164164
| `<NotificationInline scope="…" />` | `inline` | in the surface that raises them |
165165

166-
`toast` stays with the host's `onToast` delegate (sonner in the console). See the
166+
`toast` stays with the host's `onToast` delegate (sonner in the console). All of
167+
them draw the notification's `severity` icon unless it declares an `icon`
168+
override naming a real Lucide icon. See the
167169
[notifications guide](https://objectui.org/docs/guide/notifications).
168170

169171
```tsx

packages/components/src/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ import './renderers';
3939
export { cn } from './lib/utils';
4040
export { renderChildren } from './lib/utils';
4141
export { cva } from 'class-variance-authority';
42-
export { getLazyIcon, LazyIcon, toKebabIconName } from './lib/lazy-icon';
42+
export { getLazyIcon, isLucideIconName, LazyIcon, toKebabIconName } from './lib/lazy-icon';
4343

4444
// Export placeholder registration
4545
export { registerPlaceholders } from './renderers/placeholders';

packages/components/src/lib/lazy-icon.tsx

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,19 @@ function isLucideIcon(kebab: string): boolean {
4040
return VALID_ICON_NAMES.has(kebab);
4141
}
4242

43+
/**
44+
* Whether `name` (kebab-case or PascalCase) resolves to a real Lucide icon.
45+
*
46+
* Exported because `getLazyIcon` degrades an unknown name to the `Database`
47+
* icon, which is the right default for a data-shaped schema slot but wrong
48+
* where a caller has a BETTER fallback of its own — a notification, for
49+
* instance, would rather show its severity icon than a stray database glyph.
50+
* Ask first, then choose.
51+
*/
52+
export function isLucideIconName(name?: string): boolean {
53+
return !!name && isLucideIcon(toKebabIconName(name));
54+
}
55+
4356
const cache = new Map<string, React.ElementType>();
4457

4558
/**

packages/components/src/notifications/NotificationAlerts.tsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ import {
3232
AlertDialogHeader,
3333
AlertDialogTitle,
3434
} from '../ui/alert-dialog';
35-
import { notificationSeverityStyle } from './severity';
35+
import { notificationIcon, notificationSeverityStyle } from './severity';
3636

3737
export interface NotificationAlertsProps {
3838
/** Extra classes for the dialog content. */
@@ -61,7 +61,8 @@ export function NotificationAlerts({ className, acknowledgeLabel = 'OK' }: Notif
6161
const notification = alerts[alerts.length - 1];
6262
if (!notification) return null;
6363

64-
const { Icon, iconTone } = notificationSeverityStyle(notification.severity);
64+
const { iconTone } = notificationSeverityStyle(notification.severity);
65+
const Icon = notificationIcon(notification);
6566
const acknowledge = () => dismiss(notification.id);
6667
// `dismissible: false` removes the wave-it-away paths (Escape); it never
6768
// removes the acknowledge button — an alert nobody can acknowledge is a trap.
@@ -77,6 +78,7 @@ export function NotificationAlerts({ className, acknowledgeLabel = 'OK' }: Notif
7778
>
7879
<AlertDialogHeader>
7980
<AlertDialogTitle className="flex items-center gap-2">
81+
{/* eslint-disable-next-line react-hooks/static-components -- notificationIcon returns a module-cached stable component per name, not one created during render */}
8082
<Icon className={cn('h-5 w-5 shrink-0', iconTone)} aria-hidden="true" />
8183
{notification.title}
8284
</AlertDialogTitle>

packages/components/src/notifications/NotificationBanners.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import { X } from 'lucide-react';
2121
import { useNotifications, useNotificationsByPresentation } from '@object-ui/react';
2222
import { cn } from '../lib/utils';
2323
import { Button } from '../ui/button';
24-
import { notificationSeverityStyle } from './severity';
24+
import { notificationIcon, notificationSeverityStyle } from './severity';
2525

2626
export interface NotificationBannersProps {
2727
/** Extra classes for the banner stack container. */
@@ -53,7 +53,8 @@ export function NotificationBanners({ className, max = 3 }: NotificationBannersP
5353
return (
5454
<div className={cn('flex w-full flex-col', className)} data-notification-surface="banner">
5555
{banners.slice(0, max).map((notification) => {
56-
const { Icon, tone } = notificationSeverityStyle(notification.severity);
56+
const { tone } = notificationSeverityStyle(notification.severity);
57+
const Icon = notificationIcon(notification);
5758
return (
5859
<div
5960
key={notification.id}

packages/components/src/notifications/NotificationInline.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import { X } from 'lucide-react';
2121
import { useNotifications, useNotificationsByPresentation } from '@object-ui/react';
2222
import { cn } from '../lib/utils';
2323
import { Button } from '../ui/button';
24-
import { notificationSeverityStyle } from './severity';
24+
import { notificationIcon, notificationSeverityStyle } from './severity';
2525

2626
export interface NotificationInlineProps {
2727
/**
@@ -57,7 +57,8 @@ export function NotificationInline({ scope, className }: NotificationInlineProps
5757
data-scope={scope}
5858
>
5959
{items.map((notification) => {
60-
const { Icon, tone } = notificationSeverityStyle(notification.severity);
60+
const { tone } = notificationSeverityStyle(notification.severity);
61+
const Icon = notificationIcon(notification);
6162
return (
6263
<div
6364
key={notification.id}

0 commit comments

Comments
 (0)