refactor: migrate Ant Design notifications to use App.useApp() cont…#20549
refactor: migrate Ant Design notifications to use App.useApp() cont…#20549yuneng-jiang merged 3 commits intoBerriAI:mainfrom
App.useApp() cont…#20549Conversation
…ext via a new global provider. 1. notifications_manager.tsx - Hybrid notification approach: Added notificationInstance variable to store the context-based instance Added setNotificationInstance() function to inject the instance from context Created getNotification() helper that prefers context instance, falls back to static Added COMMON_NOTIFICATION_PROPS (exported) with showProgress: true and pauseOnHover: true All notification methods ( error , warning , info , success , fromBackend ) now spread COMMON_NOTIFICATION_PROPS 2. AntdGlobalProvider.tsx - New context provider: Wraps app with Antd's <App> component Uses App.useApp() hook to get the context-based notification instance Injects it into NotificationManager via setNotificationInstance()
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Greptile OverviewGreptile SummaryThis PR migrates the dashboard’s Ant Design notification usage toward the The main behavioral change is that notifications can now be driven by the Antd context, but the implementation uses a module-level mutable singleton to store the instance, which makes notification routing depend on render/mount order and can be problematic in SSR/multi-user contexts. Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/app/layout.tsx | Wraps app children in new AntdGlobalProvider; introduces a root client boundary which can affect SSR/perf across the whole dashboard. |
| ui/litellm-dashboard/src/components/molecules/notifications_manager.tsx | Refactors notifications to prefer a context-provided instance, but stores it in a module-level mutable singleton which can cause SSR/user bleed and order-dependent behavior. |
| ui/litellm-dashboard/src/contexts/AntdGlobalProvider.tsx | Adds a new client-side Antd provider and injects App.useApp().notification into NotificationManager; effect may repeatedly set a global instance depending on hook stability. |
Sequence Diagram
sequenceDiagram
participant RootLayout as RootLayout (layout.tsx)
participant Provider as AntdGlobalProvider
participant App as Antd <App>
participant Init as AntdAppInit
participant NM as NotificationManager
participant Cmp as Any UI component
RootLayout->>Provider: render(children)
Provider->>App: wrap subtree
App-->>Init: App.useApp() provides notification API
Init->>NM: setNotificationInstance(notification)
Cmp->>NM: NotificationManager.error/warning/info/success/fromBackend()
NM->>NM: getNotification()
alt instance injected
NM-->>App: use context notification instance
else not injected yet
NM-->>NM: fallback to staticNotification
end
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let notificationInstance: NotificationInstance | null = null; | ||
|
|
||
| export const setNotificationInstance = (instance: NotificationInstance) => { | ||
| notificationInstance = instance; | ||
| }; | ||
|
|
||
| // Helper to get the best available notification instance | ||
| const getNotification = () => notificationInstance || staticNotification; |
There was a problem hiding this comment.
Global mutable instance
notificationInstance is a module-level mutable singleton that gets set by setNotificationInstance() and then used by getNotification(). In Next.js/React this can lead to cross-request/user bleed in SSR/hydration scenarios (the module is shared), and also makes notification behavior depend on mount order (calls before AntdAppInit runs will silently use the static API). A safer pattern is to keep notifications entirely context-driven (or pass the instance explicitly) rather than storing it in a global variable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/litellm-dashboard/src/components/molecules/notifications_manager.tsx
Line: 7:14
Comment:
**Global mutable instance**
`notificationInstance` is a module-level mutable singleton that gets set by `setNotificationInstance()` and then used by `getNotification()`. In Next.js/React this can lead to cross-request/user bleed in SSR/hydration scenarios (the module is shared), and also makes notification behavior depend on mount order (calls before `AntdAppInit` runs will silently use the static API). A safer pattern is to keep notifications entirely context-driven (or pass the instance explicitly) rather than storing it in a global variable.
How can I resolve this? If you propose a fix, please make it concise.| const AntdAppInit = () => { | ||
| const { notification } = App.useApp(); | ||
|
|
||
| useEffect(() => { | ||
| setNotificationInstance(notification); | ||
| }, [notification]); |
There was a problem hiding this comment.
Effect runs every render
useEffect(() => setNotificationInstance(notification), [notification]) will re-run whenever App.useApp() returns a new notification object reference. If the hook returns a new object each render (implementation-dependent), this can cause repeated global writes and potential notification instance churn. Consider ensuring the instance is stable (e.g., only set once on mount) or guard against redundant sets.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/litellm-dashboard/src/contexts/AntdGlobalProvider.tsx
Line: 8:13
Comment:
**Effect runs every render**
`useEffect(() => setNotificationInstance(notification), [notification])` will re-run whenever `App.useApp()` returns a new `notification` object reference. If the hook returns a new object each render (implementation-dependent), this can cause repeated global writes and potential notification instance churn. Consider ensuring the instance is stable (e.g., only set once on mount) or guard against redundant sets.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: ui/litellm-dashboard/src/app/layout.tsx
Line: 15:25
Comment:
**Client provider in layout**
`RootLayout` is a Server Component by default, but it now renders `AntdGlobalProvider` (a Client Component). This forces a client boundary at the root and may impact SSR behavior/perf for the entire app. If the intent is only to provide Antd notifications, consider scoping the client provider to the smallest subtree that needs it, rather than wrapping all `children` at the root layout.
How can I resolve this? If you propose a fix, please make it concise. |
…pp.useApp` for notification management.
3 participants
…ext via a new global provider.
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unitCI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🐛 Bug Fix
🧹 Refactoring
Changes
1. [notifications_manager.tsx] - Hybrid notification approach:
notificationInstancevariable to store the context-based instanceCOMMON_NOTIFICATION_PROPSwithshowProgress: trueandpauseOnHover: trueCOMMON_NOTIFICATION_PROPS2. [AntdGlobalProvider.tsx] - New context provider:
<App>componentApp.useApp()hook to get the context-based notification instanceNotificationManagervia [setNotificationInstance()]