feat(web): implement notification filtering #943
Conversation
WalkthroughThe changes involve updates to the notification handling components in a web application. In Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sidebar
participant NotificationsList
participant API
User->>Sidebar: Select importance level
Sidebar->>Sidebar: Update importance reference
Sidebar->>NotificationsList: Pass importance as prop
NotificationsList->>API: Fetch notifications with importance filter
API-->>NotificationsList: Return filtered notifications
NotificationsList-->>User: Display notifications
📜 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 (
|
pujitm
force-pushed
the
feat/filter-notifications
branch
from
1115db2 to
fdd56ae
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
web/components/Notifications/List.vue (1)
18-18: Consider adding JSDoc for the importance prop
While the implementation is correct, adding documentation for the importance prop would improve maintainability.
defineProps<{
type: NotificationType;
pageSize?: number;
+ /** Filter notifications by their importance level. If undefined, no importance filtering is applied. */
importance?: Importance;
}>(),Also applies to: 22-22
web/components/Notifications/Sidebar.vue (1)
Line range hint 62-76: Consider adding type validation in the update handler
While using enum values is more type-safe, the type casting in the update handler could be made more robust.
Consider adding type validation:
-@update:model-value="(val) => {importance = val as Importance}"
+@update:model-value="(val) => {
+ if (Object.values(Importance).includes(val)) {
+ importance = val as Importance
+ }
+}"web/composables/gql/graphql.ts (2)
824-824: Document the format and purpose of formattedTimestamp
The new formattedTimestamp field lacks documentation about its format and purpose. While the existing timestamp field is documented as an ISO timestamp, it's unclear how formattedTimestamp differs.
Add a comment to clarify:
+ /** Human-friendly formatted version of the timestamp */
formattedTimestamp?: Maybe<Scalars['String']['output']>;824-824: Consider using a computed field instead of storing formatted timestamp
Having both timestamp and formattedTimestamp fields could lead to data inconsistency. Consider using a computed field or resolver to format the timestamp on-demand instead of storing it separately.
Example resolver approach:
@Field(() => String, { nullable: true })
get formattedTimestamp(): string | null {
return this.timestamp
? format(new Date(this.timestamp), 'MMM d, yyyy h:mm a')
: null;
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- web/components/Notifications/List.vue (3 hunks)
- web/components/Notifications/Sidebar.vue (3 hunks)
- web/composables/gql/graphql.ts (2 hunks)
🔇 Additional comments (8)
web/components/Notifications/List.vue (4)
6-6: LGTM: Type import is correctly placed
The Importance type import is properly added alongside other GraphQL types.
58-58: LGTM: Consistent filtering in pagination
The importance filter is correctly maintained in the fetchMore implementation, ensuring consistent filtering across paginated results.
26-31: Verify reactive behavior and error handling
While the implementation looks correct, please verify:
- The list updates reactively when importance filter changes
- Invalid importance values are handled gracefully
Also applies to: 58-58
✅ Verification successful
Importance filtering is well-implemented with proper validation and testing
The implementation is thoroughly tested and properly handled:
- Importance is a strictly typed enum, preventing invalid values
- Comprehensive test coverage exists for importance filtering:
- Tests verify reactive filtering behavior with different importance values
- Tests cover edge cases including archiving/unarchiving by importance
- GraphQL schema properly validates importance as a required enum field
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error handling in the notification query
# Test 1: Look for validation or error handling of importance values
rg -A 5 "importance.*validation|handleError.*importance" --type ts
# Test 2: Check for tests covering importance filtering
rg -A 10 "test.*importance.*filter|describe.*importance" --type spec
Length of output: 288
Script:
#!/bin/bash
# Let's try a broader search for importance-related code and tests
# Search for importance in Vue/TS/JS files
rg -t vue -t ts -t js -A 5 "importance.*filter|importance.*query|importance.*prop"
# Search for test files specifically
fd ".(spec|test)\.(js|ts|vue)$" --exec rg -A 10 "importance"
# Look for GraphQL schema definitions related to importance
fd ".(graphql|gql)$" --exec rg -A 3 "importance"
# Check for validation utilities or types related to importance
rg -t ts -t js "enum.*Importance|type.*Importance|interface.*Importance"
Length of output: 10290
26-33: Verify GraphQL query structure
The importance filter is correctly added to the query filter object. Let's verify the GraphQL query structure matches the schema.
✅ Verification successful
GraphQL query structure is correct and matches schema
The GraphQL schema explicitly defines importance as a valid field in the NotificationFilter input type, and the query implementation correctly uses this filter. The schema shows that:
NotificationFilteracceptsimportance: ImportancefieldImportanceenum includesALERT,INFO, andWARNINGvalues- The query uses the filter correctly through
notifications.list(filter: $filter)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the GraphQL query structure includes importance filter
# Test: Search for the query definition to ensure it accepts importance parameter
rg -A 10 "getNotifications\s*=\s*gql|query\s+getNotifications" --type graphql
Length of output: 111
Script:
#!/bin/bash
# Let's try to find the GraphQL query definition without file type restriction
rg -A 10 "getNotifications\s*=\s*gql|query\s+getNotifications"
Length of output: 64
Script:
#!/bin/bash
# Search for .graphql or .gql files that might contain the query
fd "\.(graphql|gql)$"
Length of output: 2039
Script:
#!/bin/bash
# Look for the query in JavaScript/TypeScript/Vue files
rg -t js -t ts -t vue -A 10 "getNotifications\s*=\s*gql|export\s+const\s+getNotifications"
Length of output: 120
Script:
#!/bin/bash
# Let's check the notifications GraphQL schema file
cat api/src/graphql/schema/types/notifications/notifications.graphql
Length of output: 2122
Script:
#!/bin/bash
# Search for the query definition without file type restrictions, focusing on web directory
rg -g 'web/**' "getNotifications\s*=\s*gql|export\s+const\s+getNotifications" -A 10
Length of output: 958
web/components/Notifications/Sidebar.vue (4)
12-12: LGTM: Clean type import and ref implementation
The importance type import and reactive reference are well-typed and properly implemented to support optional filtering.
Also applies to: 20-20
83-86: LGTM: Clean prop implementation for unread notifications
The importance prop is correctly passed to the NotificationsList component while maintaining existing functionality.
90-93: LGTM: Consistent implementation for archived notifications
The importance filtering is consistently implemented across both unread and archived notifications lists.
83-86: Verify NotificationsList importance prop implementation
Let's ensure the NotificationsList component properly implements the importance prop type and handling.
Also applies to: 90-93
✅ Verification successful
Let me gather more information about the NotificationsList component implementation.
Let me try a different approach to find the component implementation.
Let me check the List.vue component since it appears to be the actual implementation of NotificationsList.
NotificationsList component properly implements the importance prop
The implementation in web/components/Notifications/List.vue correctly:
- Defines the
importanceprop with proper typing asImportance - Sets a default value of
undefined - Properly uses the prop in GraphQL queries for filtering notifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NotificationsList component definition and its props
ast-grep --pattern 'defineProps<{
$$$
importance?: $_;
$$$
}>()'
# Backup: Search for any file containing NotificationsList
rg -l "NotificationsList"
Length of output: 139
Script:
#!/bin/bash
# Search for NotificationsList component file
fd NotificationsList
# Search for the component's content and prop usage
rg -A 10 "NotificationsList" --type vue
Length of output: 90
Script:
#!/bin/bash
# Search for files with .vue extension that might contain NotificationsList
fd -e vue
# Search for NotificationsList in all files without file type restriction
rg -A 10 "NotificationsList"
Length of output: 6093
Script:
#!/bin/bash
# Get the content of the List component
cat "web/components/Notifications/List.vue"
# Search for importance prop usage in List.vue
rg -A 5 "importance" "web/components/Notifications/List.vue"
Length of output: 2501
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/components/Notifications/List.vue (1)
67-73: Consider enhancing the empty state messageWhile the empty state UI is well-implemented, the message could be more informative and helpful to users.
Consider making the message more specific:
- No notifications to see here! + No ${props.type.toLowerCase()} notifications${props.importance ? ` with ${props.importance.toLowerCase()} importance` : ''} to displayThis would provide users with more context about their current filter settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/components/Notifications/List.vue(3 hunks)web/components/Notifications/Sidebar.vue(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/components/Notifications/Sidebar.vue
🔇 Additional comments (2)
web/components/Notifications/List.vue (2)
6-6: LGTM: Props and imports are well-structured
The addition of the importance prop with proper TypeScript typing and a sensible default value follows Vue.js best practices.
Also applies to: 19-19, 23-23
27-34: Verify GraphQL schema support for importance filtering
Let's ensure the GraphQL schema properly supports the importance filter across all notification types.
✅ Verification successful
The previous attempt failed because the file type was not recognized. Let's try a broader search to locate the GraphQL schema files first, and then search for notification-related definitions across all files.
GraphQL schema fully supports importance filtering
The GraphQL schema properly supports the importance filter for notifications. The schema defines:
- An
Importanceenum with values:ALERT,INFO,WARNING - A
NotificationFilterinput type that includes an optionalimportancefield - The
Notificationtype includesimportanceas a required field of typeImportance
The implementation in List.vue correctly uses these schema definitions for filtering notifications by importance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GraphQL schema definitions related to notifications and importance
rg -t graphql "type Notification|input NotificationFilter|enum Importance" -A 5
Length of output: 113
Script:
#!/bin/bash
# Search for GraphQL schema files with common extensions
fd "\.(gql|graphql)$"
# Also search for schema definitions in other file types
rg "type Notification|input NotificationFilter|enum Importance" -A 5
Length of output: 20950
Summary by CodeRabbit
New Features
importancefilter for notifications, allowing users to sort and view notifications based on their importance level.Bug Fixes
Documentation
importancefiltering capability and formatted timestamp inclusion.