-
Notifications
You must be signed in to change notification settings - Fork 13
Refactor/nuxtui notifications #1817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/docker-overview-table
Are you sure you want to change the base?
Refactor/nuxtui notifications #1817
Conversation
- update VSCode settings for Tailwind CSS support
…rror messages commit addresses the following two bugs/issues: 1. infinite network requests 2. make error messages more accurate bug details: - when scrolled all the way down in the notification pane (when api is down), unraid infinitely sends network requests. - must be scrolled all the way to the bottom and stay at the bottom of the pane while the api is down technical details: - for infinite loop, added try/catch that sets a canLoadMore flag to false when it encounters an error, preventing infinite loop - errors now look at non-standard locations as well impact: - performance benefits - more graceful ux on failure
… improved consistency - replaced Heroicons components with UIcon for better integration - refactored Sidebar.vue to utilize USlideover and UButton for a cleaner UI - removed unused imports and styles in main.css for better maintainability NOTES: - had to change main.css variables for it to work properly. Need to make sure this doesn't ruin other people's code. - still needs to be further refactored to align with existing ui variables
…mproved UI consistency - modified vite.config.ts to integrate app configuration into UI setup - updated app.config.ts to include new button, tabs, and slideover variants for better theming - cleaned up main.css by removing unused styles and ensuring proper imports - refactored notification components to streamline structure and improve readability
> [!Note] This stubs the unraid-ui/src/components/common/toast. Initially created a shim to convert vue-sonnner toasts to nuxtui. However, since there weren't that many, I just did a clean replacement. # Other Changes - replace router link with window.location.assign The `UButton` component attempts to inject the Vue Router instance when the `:to` prop is used. In the standalone component environment (where the router is not installed), this caused a "TypeError: inject(...) is undefined" crash when rendering notifications with links. This change replaces the `:to` prop with a standard `@click` handler that uses `window.location.assign`, ensuring navigation works correctly without requiring the router context.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
…ng logic Problem this solution addresses: Basically, when users filtered by alert, warning, or info, results were being paginated first, then filtered by the requested importance, so filtered notifications were not working properly in some (a lot) of cases. - added a new async generator method to load notifications in batches, enhancing performance and error handling. - refactored the notification loading logic to utilize the generator, improving readability and maintainability. - updated filtering logic to streamline the process of matching notifications based on importance and type.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- updated file watching logic to ignore initial files, improving performance. - added duplicate check for archive notifications to prevent double counting. - implemented retry mechanism for loading notifications, enhancing reliability. - introduced handling for risky notifications to ensure proper file creation and avoid legacy script failures.
This change ensures that Nuxt UI notifications respect the display position configured in the legacy webGUI settings. Backend: - Added `NotificationSettings` to the GraphQL model. - Exposed `settings` field on the `Notifications` resolver. - Implemented `getSettings` in `NotificationsService` to read `notify.position` from the Dynamix store. Frontend: - Added `getNotificationSettings` GraphQL query. - Updated `mount-engine.ts` to fetch settings before mounting. - Mapped legacy position values (e.g., 'center') to Nuxt UI compatible values (e.g., 'top-center').
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| export function useClipboardWithToast() { | ||
| const { copy, copied, isSupported } = useClipboard(); | ||
| const toast = useToast(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import useToast in clipboard helper
The clipboard composable now calls useToast() to show success/error messages, but the module only imports useClipboard. With no useToast import in this file, the build will fail at compile time with an undefined identifier when copyWithNotification executes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good callout.
…with Nuxt UI 4.0.0-alpha.0
zackspear
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly focused on front-end related changes in this review. It would be helpful to have @pujitm review the API related changes.
@Ajit-Mehrotra and I also discussed how to manage unrelated bugs that arise during scoped work. And to do his best to not let unrelated bugs introduce scope creep.
| // Also, for toasts, BUT this is imported in the Root UApp in mount-engine.ts | ||
| // https://ui.nuxt.com/docs/components/toast#examples | ||
| toaster: { | ||
| position: 'top-center' as const, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure this is set to the matching default Unraid webgui value
| <template> | ||
| <div class="relative flex items-center justify-center"> | ||
| <BellIcon class="text-header-text-primary h-6 w-6" /> | ||
| <UIcon name="i-heroicons-bell-20-solid" class="h-6 w-6" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use size-6 to replace h-6 w-6
| }, ''); | ||
| const icon = computed<{ component: Component; color: string } | null>(() => { | ||
| const icon = computed<{ name: string; color: string } | null>(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create an interface for this type so it can be shared across files. Since this is also used in Notifications/Indicator.vue.
| function dbgApolloError(prefix: string, err: ApolloError | null | undefined) { | ||
| if (!err) return; | ||
| console.group(`[Notifications] ${prefix}`); | ||
| console.log('top message:', err.message); | ||
| console.log('graphQLErrors:', err.graphQLErrors); | ||
| console.log('networkError:', err.networkError); | ||
| try { | ||
| console.log('json:', JSON.parse(JSON.stringify(err))); | ||
| } catch { | ||
| console.log('json:', 'failed to parse'); | ||
| console.log('json:', err); | ||
| } | ||
| console.groupEnd(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug code to potentially clean up.
if you're wanting to keep this for later, abstract this into a helper file
| const displayErrorMessage = computed(() => { | ||
| if (offlineError.value) return offlineError.value.message; | ||
| const apolloErr = error.value as ApolloError | null | undefined; | ||
| const firstGqlErr = apolloErr?.graphQLErrors?.[0] as | ||
| | (GraphQLError & { | ||
| extensions?: { error?: { message?: string } }; | ||
| error?: { message?: string }; | ||
| }) | ||
| | undefined; | ||
| const gqlEmbedded = firstGqlErr?.extensions?.error?.message; | ||
| const gqlTop = firstGqlErr?.error?.message; | ||
| const gqlMessage = firstGqlErr?.message; | ||
| const netMessage = (apolloErr?.networkError as { message?: string } | undefined)?.message; | ||
| const topMessage = apolloErr?.message; | ||
| return gqlEmbedded || gqlTop || gqlMessage || netMessage || topMessage || 'An unknown error occurred.'; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also potentially be used elsewhere in other components (especially in the future). Might be worth it to abstract into a display apollo error component.
| <div v-if="loading" class="w-full max-w-md space-y-4"> | ||
| <div v-for="n in 3" :key="n" class="py-1.5"> | ||
| <div class="flex items-center gap-2"> | ||
| <USkeleton class="h-5 w-5 rounded-full" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size-5 instead of h-5 w-5
|
|
||
| <!-- Default (empty state) --> | ||
| <div v-else class="contents"> | ||
| <UIcon name="i-heroicons-check-20-solid" class="text-unraid-green h-10 w-10 translate-y-3" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size-10 instead of h-10 w-10
| }); | ||
| const openSettings = () => { | ||
| window.location.assign('/Settings/Notifications'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was called out in the description of the PR due to lack of vue-router not being used.
The more instances I see of this the more I'm wondering if we need to create a helper / composable to ensure that the usage of this is consistent across components.
| 'top-right': 'top-right', | ||
| 'bottom-left': 'bottom-left', | ||
| 'bottom-right': 'bottom-right', | ||
| center: 'top-center', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we discussed creating an issue for updating Nuxt UI to the latest version so we can support top-center & bottom-center post update of the dep in a follow up PR.
| export function useClipboardWithToast() { | ||
| const { copy, copied, isSupported } = useClipboard(); | ||
| const toast = useToast(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good callout.
…istency with legacy webGUI
Purpose
Migrate Notification-related Components from Shadcn to Nuxtui
Summary of changes and notable additions:
webgui/emhttp/webGui/scripts/notify(and no this script did not check for the title size...see bug fixes below :/)NotificationSettingsto the graphql api model so we can sync toast position with legacy settingsBug Fixes
Some Notes
window.location.assignnot the:toprop as there is no vue router in a standalone component enviornment.Unfixed Bugs (issues need to be created)