fix(web): notifications consistency on unraid servers #961
Conversation
WalkthroughThe pull request introduces modifications to the notification management system, primarily in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
web/components/Notifications/List.vue (3)
Line range hint
24-35: Consider enhancing error handling.The current error handling only logs to console, which isn't ideal for production. Consider:
- Using a proper logging service
- Showing user-friendly error messages
- Implementing error recovery mechanisms
watch(error, (newVal) => { - console.log('[getNotifications] error:', newVal); + if (newVal) { + // Use your app's error handling service + errorHandler.log('[getNotifications]', newVal); + // Show user-friendly message + notify.error('Failed to load notifications. Please try again later.'); + } });
47-47: Remove debug logging before production deployment.Debug logging should be removed or replaced with proper production logging.
- console.log('[getNotifications] onLoadMore');
Line range hint
48-61: Consider enhancing the load more logic.While the current implementation works, it could be more robust:
- Handle edge cases where incomingCount is undefined
- Consider adding loading state management
- Add error handling for the fetchMore operation
async function onLoadMore() { + try { + const loading = ref(true); const a = await fetchMore({ variables: { filter: { offset: notifications.value.length, limit: props.pageSize, type: props.type, importance: props.importance, }, }, }); const incomingCount = a?.data.notifications.list.length ?? 0; if (incomingCount === 0) { canLoadMore.value = false; } + } catch (error) { + errorHandler.log('[getNotifications] Failed to load more:', error); + notify.error('Failed to load more notifications'); + } finally { + loading.value = false; + } }web/helpers/create-apollo-client.ts (2)
48-49: TODO comment needs implementationThe TODO comment about restarting the API should be addressed.
Would you like me to help implement the API restart functionality or create a GitHub issue to track this task?
83-83: Enhance warning message clarityThe warning message could be more descriptive to help with debugging.
-console.warn('connectPluginInstalled is false, aborting request'); +console.warn('Aborting GraphQL request: Unraid Connect Plugin is not installed');web/components/Notifications/Sidebar.vue (2)
21-21: Consider using CSS variables for dimension valuesWhile the current implementation works, using CSS variables for the width value would improve maintainability and allow for easier theme customization.
- <SheetContent :to="teleportTarget" class="w-full sm:max-w-[540px] h-screen px-0"> + <SheetContent :to="teleportTarget" class="w-full sm:max-w-[--sidebar-width] h-screen px-0">
48-52: Consider using proper TypeScript typing for the select valueThe explicit type casting to
Importancecould be avoided by properly typing the event parameter.- @update:model-value=" - (val) => { - importance = val as Importance; - } - " + @update:model-value=" + (val: Importance | undefined) => { + importance = val; + } + "api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (2)
532-550: LGTM! Well-implemented duplicate notification filtering.The changes effectively handle the edge case where notifications appear in both unread & archive folders due to legacy script behavior. Using Set for efficient lookup is a good choice.
Consider creating a follow-up task to refactor the legacy script to prevent duplicate notifications in the first place, as indicated by your comment that this should be a temporary measure.
563-568: Consider optimizing stats reading for filtered files.The current implementation reads stats for all files before applying the filter. For better performance, consider applying the filter before reading stats, especially when the filter might exclude many files.
Here's a suggested optimization:
private async listFilesInFolder( folderPath: string, narrowContent: (contents: string[]) => string[] = (contents) => contents, sortFn: SortFn<Stats> = (fileA, fileB) => fileB.birthtimeMs - fileA.birthtimeMs ): Promise<string[]> { - const contents = narrowContent(await readdir(folderPath)); - return contents + const filteredContents = narrowContent(await readdir(folderPath)); + return filteredContents .map((content) => { const path = join(folderPath, content); return { path, stats: statSync(path) }; }) .sort((fileA, fileB) => sortFn(fileA.stats, fileB.stats)) .map(({ path }) => path); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts(2 hunks)web/.env.example(1 hunks)web/components/Notifications/List.vue(4 hunks)web/components/Notifications/Sidebar.vue(5 hunks)web/helpers/create-apollo-client.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/.env.example
🧰 Additional context used
🪛 Biome
web/helpers/create-apollo-client.ts
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 47-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
web/components/Notifications/List.vue (3)
2-7: LGTM! Well-organized imports and properly typed props.
The imports are logically grouped and the props are well-defined with TypeScript types.
37-44: LGTM! Well-implemented computed property with clear documentation.
The filtering logic is correct and the comment clearly explains the rationale for client-side filtering.
66-73: LGTM! Well-structured template with proper empty state handling.
The infinite scroll configuration and empty state handling are implemented correctly.
web/helpers/create-apollo-client.ts (3)
91-94: LGTM! Clean and standard implementation
The split logic correctly routes subscription operations to WebSocket and other operations to HTTP, following Apollo best practices.
3-14: Verify the necessity of explicit index.js imports
The explicit .../index.js imports are unusual. While they work, they might cause issues with some bundlers that prefer to handle index resolution automatically.
#!/bin/bash
# Check if other files in the project use similar import patterns
rg -l "from '@apollo.*index\.js'" --type tsLine range hint 64-73: Review retry strategy configuration
The current retry configuration is quite aggressive:
- 20 maximum attempts might be excessive
- The
retryIfcondition retries on any error - Initial delay of 300ms with max 10s might lead to long wait times
Consider adjusting these values based on your specific needs and error scenarios.
web/components/Notifications/Sidebar.vue (3)
2-6: LGTM! Import organization improves readability
The consolidation of imports and their logical ordering enhances code maintainability.
69-69: LGTM! Consistent implementation of notification lists
The NotificationsList components are properly implemented with consistent prop passing for both unread and archived states.
Also applies to: 73-73
30-31: Verify the manual activation mode change
The addition of activation-mode="manual" changes how tabs are activated. This means tabs won't automatically activate on focus, which might affect accessibility and user experience.
✅ Verification successful
Manual activation mode appears to be an isolated implementation
The activation-mode="manual" attribute is only used in the Notifications Sidebar component and appears to be intentional based on the context. The component uses shadcn's Tabs implementation, and the manual mode is likely needed for specific interaction requirements in the notifications interface. The comment above the Tabs component suggests careful consideration of layout behavior, indicating this implementation has been thoughtfully designed.
- This is the only instance of manual activation mode in the codebase
- The implementation is consistent with the component's layout requirements
- No conflicting patterns were found in other tab implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tab implementations to ensure consistency
rg -l "activation-mode.*manual" --type vue
Length of output: 72
Script:
#!/bin/bash
# Search for Tabs component usage and activation-mode patterns
# Using ripgrep without file type filter since vue wasn't recognized
rg -l "activation-mode.*manual"
# Also search for general Tabs usage for context
rg "<Tabs" -A 2
Length of output: 3308
Script:
#!/bin/bash
# Let's check the shadcn Tabs component implementation to understand the activation-mode prop
rg -A 10 "interface.*TabsRootProps"
# Also check for any documentation or type definitions related to activation-mode
rg -B 2 -A 5 "activation-mode"
Length of output: 1000
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements