Notification toast #581
Notification toast #581
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces a structured notification system across multiple components in the application. It adds a Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/router/src/gateway-ui/components/federations/ConnectFederation.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /apps/router/.eslintrc.js
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
apps/router/src/home/NotificationProvider.tsx (1)
15-69: Consider making notification defaults configurableThe component has hardcoded values for duration (5000ms) and position ('top-right'). Consider accepting these as props with defaults to make the provider more flexible for different use cases.
-export const NotificationProvider: React.FC<React.PropsWithChildren> = ({ +interface NotificationProviderProps extends React.PropsWithChildren { + defaultDuration?: number; + defaultPosition?: UseToastOptions['position']; +} + +export const NotificationProvider: React.FC<NotificationProviderProps> = ({ children, + defaultDuration = 5000, + defaultPosition = 'top-right', }) => { // ... const showNotification = useCallback( (message: string, status: UseToastOptions['status'], options?: UseToastOptions) => { toast({ description: message, status, - duration: 5000, + duration: defaultDuration, isClosable: true, - position: 'top-right', + position: defaultPosition, ...options, }); }, - [toast] + [toast, defaultDuration, defaultPosition] );apps/router/src/guardian-ui/components/setup/RunDKG.tsx (1)
Line range hint
1-18: Add missing notification importsThe notification functions (
showInfo,showSuccess) are used but not imported. Add the required imports from the notification provider.Add this import:
import { useTranslation } from '@fedimint/utils'; +import { useNotification } from '../../../providers/NotificationProvider'; import { useEllipsis } from '../../hooks';Then initialize the notification hooks at the start of the component:
const { showInfo, showSuccess } = useNotification();apps/router/src/gateway-ui/components/federations/ConnectFederation.tsx (1)
Line range hint
171-171: Fix modal onClose handler implementationThe onClose prop should be a function that checks the loading state, not a boolean expression.
- <ConnectFedModal isOpen={loading} onClose={() => !loading} /> + <ConnectFedModal isOpen={loading} onClose={() => loading ? undefined : onClose()} />apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx (1)
154-155: Consider combining success notificationsTwo consecutive notifications (success and info) might overwhelm the user. Consider combining these into a single success message.
- showSuccess(t('verify-guardians.verified')); - showInfo(t('verify-guardians.all-guardians-verified')); + showSuccess(t('verify-guardians.verified') + '. ' + t('verify-guardians.all-guardians-verified'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
apps/router/src/gateway-ui/components/federations/ConnectFederation.tsx(3 hunks)apps/router/src/guardian-ui/components/setup/RunDKG.tsx(1 hunks)apps/router/src/guardian-ui/components/setup/screens/connectGuardians/ConnectGuardians.tsx(2 hunks)apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx(1 hunks)apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx(4 hunks)apps/router/src/guardian-ui/setup/FederationSetup.tsx(4 hunks)apps/router/src/home/NotificationProvider.tsx(1 hunks)apps/router/src/index.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx
🧰 Additional context used
📓 Learnings (1)
apps/router/src/guardian-ui/setup/FederationSetup.tsx (1)
Learnt from: Kodylow
PR: fedimint/ui#570
File: apps/router/src/guardian-ui/setup/FederationSetup.tsx:23-23
Timestamp: 2024-11-12T07:32:55.886Z
Learning: In `apps/router/src/guardian-ui/setup/FederationSetup.tsx`, the hooks `useGuardianSetupApi` and `useGuardianSetupContext` are correctly imported from `'../../hooks'`.
🔇 Additional comments (7)
apps/router/src/home/NotificationProvider.tsx (3)
1-9: LGTM: Well-structured interface definition
The interface properly defines the notification methods with correct typing for messages and options.
11-13: LGTM: Proper context initialization
The undefined initial value ensures proper provider usage.
71-79: LGTM: Well-implemented custom hook
The hook follows React best practices and provides clear error messages.
apps/router/src/guardian-ui/components/setup/screens/connectGuardians/ConnectGuardians.tsx (1)
27-27: LGTM! Clean notification integration.
The notification system integration is well-implemented:
- Proper hook usage and dependency management
- Notification is shown before navigation
- Translations are correctly utilized
Also applies to: 34-34, 60-62
apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx (3)
44-44: LGTM: Notification hooks properly integrated
The NotificationProvider import and hook usage are correctly implemented.
Also applies to: 73-73
170-173: LGTM: Dependencies properly declared
The dependency arrays for both useEffect and handleNext are correctly updated to include the notification functions.
Also applies to: 187-187
180-180: Ensure error handling is comprehensive
The error notification is shown after setting the error state. Consider adding more specific error messages for different failure scenarios.
Also applies to: 184-184
| //showInfo(t('run-dkg.waiting-header')); | ||
| break; | ||
| case GuardianServerStatus.ReadyForConfigGen: | ||
| setIsWaitingForOthers(true); | ||
| //showInfo(t('run-dkg.waiting-for-others')); | ||
| break; | ||
| case GuardianServerStatus.VerifyingConfigs: | ||
| //showSuccess(t('run-dkg.completed')); |
There was a problem hiding this comment.
Critical: Uncomment notification calls for user feedback
The commented out notification calls at critical stages of the DKG process should be enabled to provide essential user feedback, as specified in the PR objectives. These notifications are crucial for:
- Informing users when DKG is running
- Indicating when the system is waiting for other peers
- Confirming successful verification
Apply this diff to restore the notifications:
- //showInfo(t('run-dkg.waiting-header'));
+ showInfo(t('run-dkg.waiting-header'));
break;
case GuardianServerStatus.ReadyForConfigGen:
setIsWaitingForOthers(true);
- //showInfo(t('run-dkg.waiting-for-others'));
+ showInfo(t('run-dkg.waiting-for-others'));
break;
case GuardianServerStatus.VerifyingConfigs:
- //showSuccess(t('run-dkg.completed'));
+ showSuccess(t('run-dkg.completed'));
next();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //showInfo(t('run-dkg.waiting-header')); | |
| break; | |
| case GuardianServerStatus.ReadyForConfigGen: | |
| setIsWaitingForOthers(true); | |
| //showInfo(t('run-dkg.waiting-for-others')); | |
| break; | |
| case GuardianServerStatus.VerifyingConfigs: | |
| //showSuccess(t('run-dkg.completed')); | |
| showInfo(t('run-dkg.waiting-header')); | |
| break; | |
| case GuardianServerStatus.ReadyForConfigGen: | |
| setIsWaitingForOthers(true); | |
| showInfo(t('run-dkg.waiting-for-others')); | |
| break; | |
| case GuardianServerStatus.VerifyingConfigs: | |
| showSuccess(t('run-dkg.completed')); |
| <SharedChakraProvider theme={theme}> | ||
| <ColorModeScript /> | ||
| <Fonts spaceGroteskTtf={spaceGroteskTtf} interTtf={interTtf} /> | ||
| <AppContextProvider> | ||
| <NotificationProvider> | ||
| <App /> | ||
| </NotificationProvider> | ||
| </AppContextProvider> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving ColorModeScript and Fonts inside NotificationProvider
The current provider order might prevent notifications from respecting theme/font settings since ColorModeScript and Fonts are outside the NotificationProvider.
Apply this diff to fix the provider order:
<SharedChakraProvider theme={theme}>
- <ColorModeScript />
- <Fonts spaceGroteskTtf={spaceGroteskTtf} interTtf={interTtf} />
<AppContextProvider>
<NotificationProvider>
+ <ColorModeScript />
+ <Fonts spaceGroteskTtf={spaceGroteskTtf} interTtf={interTtf} />
<App />
</NotificationProvider>
</AppContextProvider>
</SharedChakraProvider>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <SharedChakraProvider theme={theme}> | |
| <ColorModeScript /> | |
| <Fonts spaceGroteskTtf={spaceGroteskTtf} interTtf={interTtf} /> | |
| <AppContextProvider> | |
| <NotificationProvider> | |
| <App /> | |
| </NotificationProvider> | |
| </AppContextProvider> | |
| <SharedChakraProvider theme={theme}> | |
| <AppContextProvider> | |
| <NotificationProvider> | |
| <ColorModeScript /> | |
| <Fonts spaceGroteskTtf={spaceGroteskTtf} interTtf={interTtf} /> | |
| <App /> | |
| </NotificationProvider> | |
| </AppContextProvider> |
| <ChakraProvider> | ||
| <SharedChakraProvider theme={theme}> | ||
| <ColorModeScript /> | ||
| <Fonts spaceGroteskTtf={spaceGroteskTtf} interTtf={interTtf} /> | ||
| <AppContextProvider> | ||
| <NotificationProvider> | ||
| <App /> | ||
| </NotificationProvider> | ||
| </AppContextProvider> | ||
| </SharedChakraProvider> | ||
| </ChakraProvider> |
There was a problem hiding this comment.
Remove redundant ChakraProvider wrapper
The ChakraProvider wrapping SharedChakraProvider is redundant and could cause theme conflicts or performance issues, as SharedChakraProvider likely already includes ChakraProvider internally.
Apply this diff to fix the provider hierarchy:
- <ChakraProvider>
<SharedChakraProvider theme={theme}>
<ColorModeScript />
<Fonts spaceGroteskTtf={spaceGroteskTtf} interTtf={interTtf} />
<AppContextProvider>
<NotificationProvider>
<App />
</NotificationProvider>
</AppContextProvider>
</SharedChakraProvider>
- </ChakraProvider>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ChakraProvider> | |
| <SharedChakraProvider theme={theme}> | |
| <ColorModeScript /> | |
| <Fonts spaceGroteskTtf={spaceGroteskTtf} interTtf={interTtf} /> | |
| <AppContextProvider> | |
| <NotificationProvider> | |
| <App /> | |
| </NotificationProvider> | |
| </AppContextProvider> | |
| </SharedChakraProvider> | |
| </ChakraProvider> | |
| <SharedChakraProvider theme={theme}> | |
| <ColorModeScript /> | |
| <Fonts spaceGroteskTtf={spaceGroteskTtf} interTtf={interTtf} /> | |
| <AppContextProvider> | |
| <NotificationProvider> | |
| <App /> | |
| </NotificationProvider> | |
| </AppContextProvider> | |
| </SharedChakraProvider> |
| // showSuccess(t('connect-federation.success-message')); | ||
| renderConnectedFedCallback(federation); | ||
| setConnectInfo(''); | ||
| onClose(); |
There was a problem hiding this comment.
Add success notification for federation connection
The success case should display a notification to maintain consistent user feedback across the application.
- // showSuccess(t('connect-federation.success-message'));
+ showSuccess(t('connect-federation.success-message'));
renderConnectedFedCallback(federation);
setConnectInfo('');
onClose();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // showSuccess(t('connect-federation.success-message')); | |
| renderConnectedFedCallback(federation); | |
| setConnectInfo(''); | |
| onClose(); | |
| showSuccess(t('connect-federation.success-message')); | |
| renderConnectedFedCallback(federation); | |
| setConnectInfo(''); | |
| onClose(); |
| showInfo(t('connect-federation.progress-modal-text')); | ||
|
|
||
| api | ||
| .connectFederation(connectInfo.trim()) | ||
| .then((federation) => { | ||
| // showSuccess(t('connect-federation.success-message')); | ||
| renderConnectedFedCallback(federation); | ||
| setConnectInfo(''); | ||
| onClose(); | ||
| }) | ||
| .catch(({ message, error }) => { | ||
| console.error(error); | ||
| setErrorMsg(t('connect-federation.error-message', { error: message })); | ||
| const errorMessage = t('connect-federation.error-message', { | ||
| error: message, | ||
| }); | ||
| showError(errorMessage); | ||
| setErrorMsg(errorMessage); |
There was a problem hiding this comment.
Clear error state before new connection attempt
The error message state should be cleared when starting a new connection to prevent stale errors from being displayed.
const handleConnectFederation = () => {
setLoading(true);
+ setErrorMsg('');
showInfo(t('connect-federation.progress-modal-text'));Committable suggestion skipped: line range outside the PR's diff.
| showSuccess(t('common.back')); | ||
| }, [dispatch, prevProgress, showSuccess, t]); |
There was a problem hiding this comment.
Reconsider showing success notification for back navigation
Showing a success notification for back navigation doesn't align with typical UX patterns, as going back is generally considered a neutral action that doesn't warrant success feedback. Consider removing this notification to avoid unnecessary visual noise.
const handleBack = useCallback(() => {
if (!prevProgress) return;
dispatch({ type: SETUP_ACTION_TYPE.SET_PROGRESS, payload: prevProgress });
window.scrollTo(0, 0);
- showSuccess(t('common.back'));
}, [dispatch, prevProgress, showSuccess, t]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| showSuccess(t('common.back')); | |
| }, [dispatch, prevProgress, showSuccess, t]); | |
| if (!prevProgress) return; | |
| dispatch({ type: SETUP_ACTION_TYPE.SET_PROGRESS, payload: prevProgress }); | |
| window.scrollTo(0, 0); | |
| }, [dispatch, prevProgress, showSuccess, t]); |
Issue #496
Implementation Summary
Objective: Implement a unified notification system using Chakra UI's toast functionality to ensure a consistent user experience and improve code maintainability.
Key Changes:
NotificationProvider: Created to centralize and manage notifications (success, error, warning, info) using Chakra UI's
useToast.Updated Components:
Features:
Provider Implementation:
SharedChakraProvider.ChakraProviderwithNotificationProviderfor full toast functionality.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These updates aim to enhance user interaction and provide clearer feedback during key processes.