feat: responsive notifications #985
Conversation
elibosley
had a problem deploying
to
production
GitHub Actions
Failure
|
Warning Rate limit exceeded@elibosley has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a series of enhancements to the web application's notification system and components. The changes span multiple files, focusing on improving the user interface, timestamp formatting, and viewport management. Key updates include refactoring the notification sidebar with new tabs, enhancing the bell icon styling, implementing locale-aware timestamp formatting, and adding viewport meta tag management for better responsive design. Changes
Possibly related PRs
Suggested Reviewers
Poem
🪧 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 (
|
elibosley
force-pushed
the
feat/notification-responsive
branch
from
db7fe22 to
2cb9503
Compare
elibosley
had a problem deploying
to
production
GitHub Actions
Failure
elibosley
had a problem deploying
to
production
GitHub Actions
Failure
elibosley
force-pushed
the
feat/dark-mode-fixes
branch
from
8ed9df4 to
1a01a8c
Compare
elibosley
force-pushed
the
feat/notification-responsive
branch
from
7564d67 to
f85c734
Compare
elibosley
had a problem deploying
to
production
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
web/pages/index.vue (1)
16-21:⚠️ Potential issueReconsider fixed viewport width approach
Setting a fixed viewport width of 1300px might cause issues:
- Prevents proper responsive design on smaller devices
- Conflicts with Sheet.vue's dynamic viewport management
- Previous reviewer also questioned this approach
Let's verify the impact on mobile devices:
#!/bin/bash # Search for responsive design related code rg -g '*.vue' -g '*.css' 'media|@media|responsive|mobile|tablet|screen' --type-add 'vue:*.vue'
🧹 Nitpick comments (11)
web/components/shadcn/tabs/TabsContent.vue (2)
6-11: Consider enhancing type safetyWhile the implementation is correct, we could improve type safety.
Consider this typed version:
-const delegatedProps = computed(() => { +const delegatedProps = computed<Omit<TabsContentProps, 'class'>>(() => { const { class: _, ...delegated } = props; return delegated; });
17-25: Consider removing margin from base componentThe implementation is solid with good accessibility features and state management. However, the
mt-2margin in the base component might limit its reusability.Consider moving the margin to the consumer component:
- 'flex mt-2 ring-offset-background focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2', + 'flex ring-offset-background focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2',web/components/shadcn/sheet/Sheet.vue (1)
10-12: Fix typo in commentThe word "oepned" should be "opened".
- * Update meta tags for the root page when oepned + * Update meta tags for the root page when openedweb/pages/index.vue (1)
16-21: Consider using responsive design best practicesInstead of forcing a fixed viewport width, consider:
- Using responsive design with media queries
- Implementing a mobile-first approach
- Using CSS Grid or Flexbox for layout
-useHead({ - meta: [ - { name: 'viewport', - content: 'width=1300', } - ] -}) +useHead({ + meta: [ + { + name: 'viewport', + content: 'width=device-width, initial-scale=1.0' + } + ] +})web/components/Notifications/List.vue (2)
73-73: Consider using overflow-y-auto instead of overflow-y-scrollUsing
overflow-y-autowould only show scrollbars when needed, providing a cleaner UI when content fits within the container.- class="divide-y divide-gray-200 px-7 flex flex-col overflow-y-scroll flex-1 min-h-0" + class="divide-y divide-gray-200 px-7 flex flex-col overflow-y-auto flex-1 min-h-0"
83-85: Consider adding a11y improvements to end messageThe end message could benefit from proper ARIA attributes for screen readers.
- <div v-if="!canLoadMore" class="py-5 grid place-content-center text-gray-500"> - You've reached the end... - </div> + <div + v-if="!canLoadMore" + class="py-5 grid place-content-center text-gray-500" + role="status" + aria-live="polite" + > + You've reached the end of notifications + </div>web/components/Notifications/Sidebar.vue (5)
Line range hint
15-31: Consider enhancing user experience for confirmation dialogs.The native
confirmdialogs could be replaced with a more user-friendly modal component that matches the application's design system.Consider using a custom modal component:
-if (confirm('This will archive all notifications on your Unraid server. Continue?')) { +const confirmed = await openConfirmModal({ + title: 'Archive All Notifications', + message: 'This will archive all notifications on your Unraid server.', + confirmText: 'Archive All', + cancelText: 'Cancel', + variant: 'warning' +}); +if (confirmed) {
52-52: Consider simplifying the class structure.The class string is quite long and could be simplified using Tailwind's built-in screen variants.
-class="w-full max-w-[100vw] sm:max-w-[540px] max-h-screen h-screen min-h-screen px-0 flex flex-col gap-5 pb-0" +class="w-full sm:max-w-[540px] min-h-screen flex flex-col gap-5 px-0 pb-0"
71-92: Improve button positioning and consistency.The archive/delete buttons could be better positioned and styled for consistency.
- Consider moving these action buttons to a more prominent location
- Add loading indicators to provide better feedback
<Button :disabled="loadingArchiveAll" variant="link" size="sm" - class="text-foreground hover:text-destructive transition-none" + class="text-foreground hover:text-destructive transition-none inline-flex items-center gap-2" @click="confirmAndArchiveAll" > + <LoadingSpinner v-if="loadingArchiveAll" class="w-4 h-4" /> Archive All </Button>
94-113: Consider enhancing the filter implementation.The current filter implementation could be improved for better user experience.
- Add a clear visual indication of the active filter
- Consider adding a count next to each filter option
<SelectContent :to="teleportTarget"> <SelectGroup> <SelectLabel>Notification Types</SelectLabel> - <SelectItem value="all">All Types</SelectItem> + <SelectItem value="all"> + All Types + <span v-if="overview" class="ml-2 text-muted-foreground"> + ({{ overview.unread.total + overview.archive.total }}) + </span> + </SelectItem>
Line range hint
1-123: Consider implementing virtual scrolling for large notification lists.Given that this is a notification system that polls every 2 seconds and could potentially handle a large number of notifications, consider these architectural improvements:
- Implement virtual scrolling for better performance with large lists
- Add rate limiting or exponential backoff for the polling interval
- Consider using WebSocket for real-time updates instead of polling
Would you like assistance in implementing any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
web/components/Notifications/Indicator.vue(1 hunks)web/components/Notifications/Item.vue(1 hunks)web/components/Notifications/List.vue(2 hunks)web/components/Notifications/Sidebar.vue(2 hunks)web/components/shadcn/sheet/Sheet.vue(1 hunks)web/components/shadcn/tabs/TabsContent.vue(1 hunks)web/package.json(1 hunks)web/pages/index.vue(1 hunks)
🔇 Additional comments (12)
web/components/shadcn/tabs/TabsContent.vue (2)
2-4: LGTM! Well-organized imports
The imports follow a clean organization pattern: utilities, external components, and Vue imports.
Line range hint 1-30: Verify integration with notification system
The component looks well-structured for the responsive notifications feature. Let's verify its usage in the notification system.
✅ Verification successful
TabsContent component is correctly integrated with notification system
The component is properly integrated into the notification system through web/components/Notifications/Sidebar.vue, where it's used to create a responsive tabbed interface for both unread and archived notifications. The implementation:
- Provides tab panels for "unread" and "archived" notification states
- Maintains proper flex layout structure for responsive design
- Correctly applies the shadcn styling patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the TabsContent component's usage in notification-related components
# Search for TabsContent usage in notification components
rg -l "TabsContent" web/components/ | grep -i "notif"
# Check for any potential accessibility issues in parent components
ast-grep --pattern 'role="tabpanel"' web/components/
Length of output: 147
Script:
#!/bin/bash
# Let's examine the usage context in the Notifications/Sidebar.vue
rg "TabsContent" web/components/Notifications/Sidebar.vue -B 2 -A 2
# Also check if there are any other tab-related components used in the notification system
rg "Tabs[A-Z][a-z]+" web/components/Notifications/Sidebar.vue
# Check the structure of the notification sidebar
cat web/components/Notifications/Sidebar.vue
Length of output: 6123
web/package.json (6)
Line range hint 91-95: Review package override configuration
The overrides section enforces specific versions:
- Latest Vue version
- Specific
@floating-ui/vueversion forradix-vue
This is good practice for maintaining compatibility, but ensure these overrides don't conflict with the notification system's requirements.
34-38: Verify Tailwind and development tools compatibility
The updates to Tailwind-related packages and development tools are significant:
@nuxt/devtools^1.6.4@nuxtjs/tailwindcss^6.12.2@tailwindcss/typography^0.5.15
These updates are important for the responsive design aspects of the notification system.
✅ Verification successful
Tailwind configuration and typography plugin are properly integrated
The verification confirms that:
- The typography plugin is correctly configured in
tailwind.config.tswith proper theme customizations - The plugin is properly imported and used in the configuration
- The package is correctly listed in both
package.jsonandpackage-lock.json
The updated versions are compatible and properly integrated into the project's configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for custom Tailwind configuration
fd -t f "tailwind.config" -x cat {}
# Look for typography plugin usage
rg -l "@tailwindcss/typography"
Length of output: 9310
41-41: Verify compatibility with major Node.js version upgrade
The @types/node package has been upgraded from ^18 to ^22, which represents a major version jump. This could potentially introduce breaking changes in TypeScript type definitions.
✅ Verification successful
Node.js version upgrade is safe to proceed
The upgrade of @types/node to version 22 is compatible with the project's dependencies:
- The project doesn't specify explicit Node.js engine requirements in either package.json
- Key dependencies like Nuxt (^3.14), Vue, and other major packages are all recent versions that support Node.js 22
- TypeScript version (^5.6.3) is also compatible with Node.js 22 type definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Node.js version requirements in other dependencies
rg -l "engines.*node"
Length of output: 23
Script:
#!/bin/bash
# Let's check package.json files for Node.js version requirements and current configurations
fd package.json --exec cat {}
Length of output: 9957
48-48: Verify Nuxt compatibility with other dependencies
The Nuxt upgrade from ^3.11.2 to ^3.14.1592 is significant. Ensure all Nuxt-related packages are compatible with this version.
✅ Verification successful
Based on the output, I can generate the final response as I have enough information about the Nuxt-related dependencies and their versions. The output shows all Nuxt-related packages and their version constraints in the project.
Nuxt upgrade appears compatible with dependencies
The Nuxt upgrade to ^3.14.1592 is safe as all related dependencies are aligned:
- Core dependencies like
@nuxt/kit,@nuxt/schema, and@nuxt/vite-builderare exactly matched at version3.14.1592 - Integration packages have compatible version ranges:
@nuxt/devtools: ^1.6.4@nuxtjs/tailwindcss: ^6.12.2@vueuse/nuxt: ^12.0.0@nuxtjs/color-mode: ^3.5.2@pinia/nuxt: ^0.9.0shadcn-nuxt: ^0.11.3
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Nuxt-related dependencies and their version constraints
rg -l "@nuxt|nuxt" | xargs cat | grep -E "(@nuxt|nuxt)"
Length of output: 8077
74-74: Verify date formatting and security package updates
Critical updates to review:
dayjs^1.11.13 (date formatting)isomorphic-dompurify^2.19.0 (security)marked^12.0.2 (security)wretch^2.11.0 (HTTP client)
These updates are particularly important for the notification system's security and date formatting capabilities.
Also applies to: 80-81, 83-88
60-61: Review UI component library updates for breaking changes
Several UI-related packages have been updated:
@apollo/client^3.12.3@floating-ui/dom^1.6.12@headlessui/vue^1.7.23@heroicons/vue^2.2.0@vueuse/componentsand@vueuse/integrations^12.0.0
These updates align with the PR's focus on responsive notifications, but verify for any breaking changes in the UI components.
Also applies to: 64-70
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient information about the package usage and version changes:
UI components are actively used but updates appear safe
The package updates are minor version bumps with no breaking changes:
@headlessui/vue: 1.7.23 (used in modals, switches, dropdowns)@heroicons/vue: 2.2.0 (used extensively for icons)@floating-ui/dom: 1.6.12 (used as a dependency)@apollo/client: 3.12.3 (used for GraphQL operations)@vueuse: 12.0.0 (used for composables like useToggle, useClipboard)
The CHANGELOG entries show these are routine updates, and the version increments follow semver (no major version changes). The components are used in a standard way across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these UI components in the codebase
ast-grep --pattern 'import { $_ } from "@headlessui/vue"'
ast-grep --pattern 'import { $_ } from "@heroicons/vue"'
ast-grep --pattern 'import { $_ } from "@floating-ui/dom"'
Length of output: 179
Script:
#!/bin/bash
# Let's try with ripgrep to find usages of these packages
echo "=== @headlessui/vue usage ==="
rg -l "@headlessui/vue" --type vue --type js --type ts
echo -e "\n=== @heroicons/vue usage ==="
rg -l "@heroicons/vue" --type vue --type js --type ts
echo -e "\n=== @floating-ui/dom usage ==="
rg -l "@floating-ui/dom" --type vue --type js --type ts
echo -e "\n=== @apollo/client usage ==="
rg -l "@apollo/client" --type vue --type js --type ts
echo -e "\n=== @vueuse usage ==="
rg -l "@vueuse" --type vue --type js --type ts
# Let's also check the previous versions in package.json
echo -e "\n=== Previous versions in package.json ==="
git show HEAD^:web/package.json | grep -E "@apollo/client|@floating-ui/dom|@headlessui/vue|@heroicons/vue|@vueuse"
Length of output: 1246
Script:
#!/bin/bash
# Let's try without file type restrictions since Vue files aren't recognized
echo "=== @headlessui/vue usage ==="
rg "@headlessui/vue" -A 2
echo -e "\n=== @heroicons/vue usage ==="
rg "@heroicons/vue" -A 2
echo -e "\n=== @floating-ui/dom usage ==="
rg "@floating-ui/dom" -A 2
echo -e "\n=== @apollo/client usage ==="
rg "@apollo/client" -A 2
echo -e "\n=== @vueuse usage ==="
rg "@vueuse" -A 2
# Let's also check the changelog or release notes in package-lock.json
echo -e "\n=== Package versions in package-lock.json ==="
rg "\"@apollo/client\"|\"@floating-ui/dom\"|\"@headlessui/vue\"|\"@heroicons/vue\"|\"@vueuse\"" package-lock.json -A 1
Length of output: 34582
web/components/Notifications/Indicator.vue (1)
43-43: Verify color contrast for accessibility
The addition of text-header-text-primary class improves theme consistency. However, ensure the contrast ratio meets WCAG guidelines for accessibility.
✅ Verification successful
Color contrast is properly managed through theme system
The text-header-text-primary color is part of a well-structured theming system that handles contrast appropriately:
- In light mode: text is
#f2f2f2(light) on#1c1b1b(dark) background - In dark mode: text is
#1c1c1c(dark) on#f2f2f2(light) background
Both combinations provide sufficient contrast ratios that meet WCAG guidelines. The color is consistently used across header components and adapts properly to theme changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of text-header-text-primary color to verify contrast
rg -l "text-header-text-primary|header-text-primary" --type css --type html
Length of output: 77
Script:
#!/bin/bash
# Let's try searching in tailwind config files and other potential theme configuration files
fd -e js -e ts -e json "tailwind" --exec rg -l "header-text-primary"
# Also search for any color-related configuration files
fd -e js -e ts -e json "color|theme" --exec rg -l "header-text-primary"
# Search in the entire codebase without type restrictions
rg -l "header-text-primary"
Length of output: 378
Script:
#!/bin/bash
# Let's check the color definition in tailwind config
cat web/tailwind.config.ts
# And check the theme store for any dynamic color settings
cat web/store/theme.ts
# Also check how this color is used in other components for consistency
rg "text-header-text-primary" -A 2 -B 2
Length of output: 15255
web/components/Notifications/Item.vue (2)
82-84: LGTM: Improved responsive layout
The flex layout with gap spacing improves the component's responsiveness and maintainability.
95-99: Verify color contrast for timestamp
Addressing the previous review comment about text-gray-500: ensure this color provides sufficient contrast for accessibility.
#!/bin/bash
# Search for text-gray-500 color definition and usage
rg "text-gray-500" --type css --type htmlweb/components/Notifications/Sidebar.vue (1)
2-2: LGTM!
The import statement is correctly placed and follows the project's import style.
| const emits = defineEmits<DialogRootEmits>() | ||
| const forwarded = useForwardPropsEmits(props, emits) | ||
| const initialViewport = ref(document.querySelector('meta[name="viewport"]')?.getAttribute('content') ?? 'width=1300'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using composable for viewport management
Direct DOM manipulation with document.querySelector should be avoided in Vue components. Consider creating a dedicated composable for viewport management.
-const initialViewport = ref(document.querySelector('meta[name="viewport"]')?.getAttribute('content') ?? 'width=1300');
+import { useViewport } from '~/composables/useViewport'
+const { initialViewport, updateViewport } = useViewport()Committable suggestion skipped: line range outside the PR's diff.
| const reformattedTimestamp = computed(() => { | ||
| const userLocale = navigator.language ?? 'en-US'; // Get the user's browser language (e.g., 'en-US', 'fr-FR') | ||
| const reformattedDate = new Intl.DateTimeFormat(userLocale, { | ||
| month: 'short', | ||
| day: 'numeric', | ||
| hour: '2-digit', | ||
| minute: '2-digit', | ||
| hour12: ['AM', 'PM'].some(period => (props.formattedTimestamp ?? 'AM').includes(period)) | ||
| }).format(new Date(props.timestamp ?? new Date())); | ||
| return reformattedDate; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle edge cases in timestamp formatting
The timestamp formatting has potential issues:
- The null coalescing for userLocale could be moved into the options
- The timestamp null check could lead to unexpected behavior
const reformattedTimestamp = computed(() => {
- const userLocale = navigator.language ?? 'en-US';
+ if (!props.timestamp) return '';
const reformattedDate = new Intl.DateTimeFormat(userLocale, {
+ locale: navigator.language ?? 'en-US',
month: 'short',
day: 'numeric',
hour: '2-digit',
minute: '2-digit',
hour12: ['AM', 'PM'].some(period => (props.formattedTimestamp ?? 'AM').includes(period))
- }).format(new Date(props.timestamp ?? new Date()));
+ }).format(new Date(props.timestamp));
return reformattedDate;
});Committable suggestion skipped: line range outside the PR's diff.
elibosley
had a problem deploying
to
production
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
web/composables/gql/graphql.ts (6)
46-51: Consider adding comments for clarity.
These new input fields look straightforward, but adding doc comments or descriptions could help future maintainers.
53-57: Ensure consistency in naming convention across inputs.
The addition of "AddRoleForApiKeyInput" is clear. Double-check that all inputs follow consistent naming patterns and that confusion is avoided when referencing them in your mutations.
689-689: Confirm removal of roles is properly authorized.
Removing roles from an API key might cause security gaps; ensure the request is authenticated and authorized accordingly.
714-716: Document the addPermission mutation thoroughly.
It’s helpful for future readers to know exactly how permissions are mapped. A doc comment or short inline reference fosters clarity.
718-722: Validate argument naming consistency everywhere.
“MutationaddRoleForApiKeyArgs” is different from typical PascalCase or camelCase patterns. Consider aligning with other types.
1224-1232: Ensure role definitions align with business logic.
The newly introduced roles (admin, guest, my_servers, notifier, upc) should be validated against your business requirements, ensuring no role is redundant or missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
web/composables/gql/graphql.ts(14 hunks)
🔇 Additional comments (8)
web/composables/gql/graphql.ts (8)
58-62: Validate user existence before role assignment.
When adding roles to users, ensure that upstream application code verifies the user exists (and is active) before applying roles.
69-73: Confirm removal of deprecated fields and handle data migrations if necessary.
Replacing “expiresAt” and “scopes” with “id” and “roles” in ApiKey is a significant schema change. Verify that any references to the old fields have been updated and that existing data is handled or migrated properly.
352-356: Check default roles assignment.
When creating new API keys, ensure the roles array is properly defaulted or validated if the user does not provide any roles.
603-603: Ensure handling multiple roles in frontend logic.
Changing the “roles” field from a single value to an array in Me might break older UI or logic. Confirm that all references to Me.roles have been updated.
672-672: Return type changed to ApiKeyWithSecret—ensure consumer usage is updated.
Clients previously expecting an ApiKey might break. Confirm that downstream code is ready to handle the new shape, including the “key” field.
709-710: Check for conflicts with array operations.
If “addDiskToArray” logic changed as well, ensure it’s tested thoroughly, given array modifications can be sensitive in production.
1187-1191: Include admin checks for removing roles.
RemoveRoleFromApiKeyInput looks good, but ensure only privileged users or processes call this mutation.
1192-1223: Double-check resource list coverage.
The Resource enum now enumerates many possible resource domains. Carefully ensure that your permission checks or references align with each resource’s actual usage in the app.
elibosley
had a problem deploying
to
production
GitHub Actions
Failure
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
elibosley
force-pushed
the
feat/notification-responsive
branch
from
02c6334 to
d2f3358
Compare
elibosley
had a problem deploying
to
production
GitHub Actions
Failure
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web/components/shadcn/sheet/Sheet.vue (1)
9-22:⚠️ Potential issueImprove viewport management implementation.
The current implementation has several concerns:
- Direct DOM manipulation in components is an anti-pattern in Vue
- Missing error handling for DOM operations
- Potential race conditions with DOM queries
- Functions should be moved to a composable for better reusability and testing
Consider implementing a composable:
// composables/useViewport.ts export function useViewport() { const getViewport = (): string => { try { return document.querySelector('meta[name="viewport"]')?.getAttribute('content') ?? 'width=1300'; } catch (error) { console.error('Error getting viewport:', error); return 'width=1300'; } }; const updateViewport = (viewport: string): void => { try { const meta = document.querySelector('meta[name="viewport"]'); if (meta) { meta.setAttribute('content', viewport); } else { const meta = document.createElement('meta'); meta.name = 'viewport'; meta.content = viewport; document.head.appendChild(meta); } } catch (error) { console.error('Error updating viewport:', error); } }; const initialViewport = ref(getViewport()); return { getViewport, updateViewport, initialViewport, }; }
🧹 Nitpick comments (1)
web/components/shadcn/sheet/Sheet.vue (1)
24-36: Consider adding error boundaries and state management.While the implementation includes proper cleanup, consider these improvements:
- Add error handling for viewport updates
- Consider using a reactive state to track the dialog's open status
- Add debug logging for development environments
const forwarded = useForwardPropsEmits(props, emits); const initialViewport = ref(getViewport()); +const isOpen = ref(false); + const openListener = (opened: boolean) => { + isOpen.value = opened; if (opened) { - updateViewport(MOBILE_VIEWPORT); + try { + updateViewport(MOBILE_VIEWPORT); + if (import.meta.env.DEV) { + console.debug('Sheet opened: updated viewport to mobile'); + } + } catch (error) { + console.error('Failed to update viewport:', error); + } } else { - updateViewport(initialViewport.value); + try { + updateViewport(initialViewport.value); + if (import.meta.env.DEV) { + console.debug('Sheet closed: restored viewport'); + } + } catch (error) { + console.error('Failed to restore viewport:', error); + } } }; onUnmounted(() => { - updateViewport(initialViewport.value); + if (isOpen.value) { + try { + updateViewport(initialViewport.value); + if (import.meta.env.DEV) { + console.debug('Sheet unmounted: restored viewport'); + } + } catch (error) { + console.error('Failed to restore viewport on unmount:', error); + } + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
web/components/shadcn/sheet/Sheet.vue(1 hunks)
🔇 Additional comments (3)
web/components/shadcn/sheet/Sheet.vue (3)
2-4: LGTM! Well-structured imports and constant definition.
The imports are appropriate and the MOBILE_VIEWPORT constant is well-defined with proper typing.
6-8: LGTM! Clean props and emits setup.
The props and emits are well-typed and properly extend the DialogRoot interfaces.
40-42: LGTM! Clean template implementation.
The template correctly uses DialogRoot with proper prop forwarding and event binding.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/store/theme.ts (1)
Line range hint
13-73: Consider documenting the color system design tokens.The theme configuration is quite extensive with various semantic color tokens. Consider adding documentation about:
- The color system's hierarchy and usage guidelines
- Accessibility requirements for different color combinations
- The relationship between these tokens and UI components
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
web/components/Notifications/Item.vue(1 hunks)web/components/Notifications/List.vue(2 hunks)web/store/theme.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/components/Notifications/Item.vue
- web/components/Notifications/List.vue
🔇 Additional comments (1)
web/store/theme.ts (1)
40-40: Verify contrast ratios for the updated secondary foreground colors.
The changes to --secondary-foreground make the text more muted in both themes:
- Dark theme: 98% → 77% lightness
- Light theme: 9% → 45% lightness
While this creates a more subtle hierarchy, please ensure the new values maintain sufficient contrast ratios for accessibility.
Also applies to: 64-64
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
| </h4> | ||
|
|
||
| <div class="w-full flex flex-row items-center justify-between gap-2"> | ||
| <div class="flex flex-row items-center justify-between gap-2"> |
There was a problem hiding this comment.
Display flex defaults to flex row can remove flex-row.
| class="flex flex-1 flex-col min-h-0" | ||
| aria-label="Notification filters" | ||
| > | ||
| <div class="flex flex-row justify-between items-center flex-wrap gap-5 px-6"> |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Also upgrades dependencies
Summary by CodeRabbit
New Features
Bug Fixes
Chores