feat(api): pull date & time format from notification settings #967
Conversation
Pulled from `/boot/config/plugins/dynamix/dynamix.cfg` by changing the relevant notification settings (display, date, time).
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces modifications across several files, primarily focusing on enhancing date and time formatting capabilities in the application. Key changes include the addition of a Changes
Possibly related PRs
Suggested reviewers
🪧 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. |
pujitm
force-pushed
the
feat/notif-settings
branch
from
55779e1 to
f53f664
Compare
|
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. |
|
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: 6
🧹 Outside diff range and nitpick comments (7)
api/dev/dynamix/dynamix.cfg (1)
3-3: Add documentation for time format configurationsConsider adding comments to explain:
- The purpose of different time formats in display vs. notify sections
- The expected behavior when these formats are used
- The relationship between these settings and the notification system
Example documentation:
[display] date="%c" +# Format for displaying time in the UI (12-hour format with AM/PM) time="%I:%M %p"api/src/core/types/ini.ts (1)
48-50: Fix typo in time format documentationThere's a typo in the time format documentation.
* Time format: - * - `hi: A` => 12 hr + * - `h:i A` => 12 hr * - `H:i` => 24 hr (default)api/src/__test__/utils.test.ts (2)
8-34: Consider adding error handling test cases.While the existing test cases thoroughly cover valid formatting scenarios, consider adding tests for:
- Invalid date input
- Invalid format strings
- Null/undefined inputs
Example test cases to add:
it('handles invalid date input gracefully', () => { expect(() => formatDatetime(new Date('invalid'))).toThrow(); }); it('handles invalid format strings gracefully', () => { expect(() => formatDatetime(testDate, { dateFormat: '%invalid' })).toThrow(); }); it('handles null/undefined inputs gracefully', () => { expect(() => formatDatetime(null as any)).toThrow(); expect(() => formatDatetime(undefined as any)).toThrow(); });
35-71: Consider adding more diverse test scenarios.The current implementation provides excellent coverage of format combinations. To make it even more robust, consider:
- Add test cases for different locales:
it('handles different locales correctly', () => { const locales = ['en-US', 'fr-FR', 'ja-JP']; locales.forEach(locale => { const result = formatDatetime(testDate, { locale }); expect(result).toMatch(/^[A-Za-z].*2024/); }); });
- Add edge cases for dates:
const edgeCases = [ new Date('2024-01-01T00:00:00'), // New Year new Date('2024-12-31T23:59:59'), // Year end new Date('2024-02-29T12:00:00'), // Leap year ];web/components/Notifications/Item.vue (1)
87-87: Consider adding a loading state for timestamp formattingSince the timestamp is now being formatted based on user settings, it might be beneficial to handle potential loading states or formatting errors.
- <p class="text-12px opacity-75">{{ formattedTimestamp }}</p> + <p class="text-12px opacity-75"> + <span v-if="formattedTimestamp">{{ formattedTimestamp }}</span> + <span v-else class="opacity-50">Formatting...</span> + </p>api/package.json (1)
117-117: Consider modern date/time libraries as alternatives.While strftime is lightweight and matches Unix-style format strings, consider these alternatives for better features and maintenance:
- date-fns: Modular and functional approach
- Luxon: Modern successor to Moment.js
- Day.js: Lightweight alternative with similar API to Moment.js
These libraries offer:
- Better timezone handling
- Immutable operations
- Modern ES6+ features
- Regular updates and active maintenance
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (1)
709-728: Improve readability with early returnsThe error handling logic can be simplified using early returns to reduce nesting and improve readability.
Apply this diff:
private formatTimestamp(timestamp: string) { const { display: settings } = getters.dynamix(); const date = this.parseNotificationDateToIsoDate(timestamp); if (!settings) { this.logger.warn( '[formatTimestamp] Dynamix display settings not found. Cannot apply user settings.' ); return timestamp; - } else if (!date) { + } + + if (!date) { this.logger.warn(`[formatTimestamp] Could not parse date from timestamp: ${date}`); return timestamp; } + this.logger.debug(`[formatTimestamp] ${settings.date} :: ${settings.time} :: ${date}`); return formatDatetime(date, { dateFormat: settings.date, timeFormat: settings.time, omitTimezone: true, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
api/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
api/dev/dynamix/dynamix.cfg(1 hunks)api/package.json(2 hunks)api/src/__test__/utils.test.ts(1 hunks)api/src/core/types/ini.ts(3 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts(3 hunks)api/src/utils.ts(2 hunks)web/components/Notifications/Item.vue(1 hunks)web/components/Notifications/graphql/notification.query.ts(1 hunks)web/composables/gql/gql.ts(2 hunks)web/composables/gql/graphql.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts
[error] 703-703: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (14)
api/src/utils.ts (1)
1-1: Verify the inclusion and compatibility of the strftime module
Ensure that the strftime module and its types (@types/strftime) are correctly added to the project's dependencies to prevent any runtime or type-checking issues.
api/dev/dynamix/dynamix.cfg (1)
3-3: Consider aligning time formats between display and notify sections
There's an inconsistency in time formats:
- Display section: 12-hour format ("%I:%M %p")
- Notify section: 24-hour format ("H:i")
This might lead to user confusion when times are displayed in different formats across the application.
Let's verify the format handling across the codebase:
web/components/Notifications/graphql/notification.query.ts (1)
Line range hint 26-33: Verify query response handling in consuming components.
The NotificationFragment with the new formattedTimestamp field is used in both getNotifications and archiveNotification operations. Ensure that components consuming these queries handle the new field appropriately.
Let's check the component implementations:
Also applies to: 35-41
✅ Verification successful
Let's gather more specific information about how these components handle notification data.
The formattedTimestamp field is properly handled in the consuming components
The verification shows that the NotificationFragment with the new formattedTimestamp field is correctly integrated:
- The
Item.vuecomponent renders the timestamp in the header section using{{ formattedTimestamp }} - The
List.vuecomponent correctly passes all notification properties toNotificationsItemusingv-bind="notification" - The fragment usage is properly set up through
useFragment(NOTIFICATION_FRAGMENT, result.value?.notifications.list)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find components using these queries and verify response handling
# Find components using the queries
rg -l "getNotifications|archiveNotification"
# Look for timestamp rendering in components
ast-grep --pattern 'formattedTimestamp'
Length of output: 631
Script:
#!/bin/bash
# Check the implementation of notification components
cat web/components/Notifications/Item.vue web/components/Notifications/List.vue
# Look for timestamp-related rendering or processing
rg -A 5 "timestamp" web/components/Notifications/
Length of output: 7264
api/src/core/types/ini.ts (1)
45-52: LGTM! Well-structured type definitions with clear documentation
The changes effectively capture the date and time format requirements with type-safe string literals and comprehensive documentation.
api/src/__test__/utils.test.ts (1)
1-7: LGTM! Well-structured test setup.
The test setup follows best practices with proper imports and a well-defined test date constant.
web/components/Notifications/Item.vue (1)
87-87: Verify the formattedTimestamp prop exists in NotificationFragmentFragment
The change from timestamp to formattedTimestamp aligns with the PR objective. However, we should verify that this field is properly defined in the GraphQL fragment.
✅ Verification successful
formattedTimestamp field is properly defined in the GraphQL fragment
The search results confirm that formattedTimestamp is correctly defined in the NotificationFragment GraphQL fragment, both in the generated code (web/composables/gql/gql.ts) and the source query file (web/components/Notifications/graphql/notification.query.ts). The change from timestamp to formattedTimestamp is valid and properly supported by the GraphQL schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NotificationFragment definition to verify formattedTimestamp field
rg -t ts "fragment NotificationFragment" -A 10
Length of output: 5098
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (1)
48-48: LGTM! Good architectural improvement.
Moving the timestamp formatting responsibility to the service layer is a good separation of concerns. This allows the resolver to focus on GraphQL operations while the service handles data transformation and formatting based on user settings.
Let's verify the service implementation:
✅ Verification successful
Timestamp formatting is properly implemented in the service layer
The service implementation correctly handles timestamp formatting:
NotificationsServiceincludes theformatTimestampmethod that processes timestamps- Both raw
timestampandformattedTimestampfields are included in the GraphQL fragments - The service maintains backward compatibility by providing both formatted and raw timestamp values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that NotificationsService properly implements the timestamp formatting
# Check if the service implements the formatting logic
ast-grep --pattern 'class NotificationsService {
$$$
getNotifications($$$) {
$$$
formattedTimestamp
$$$
}
$$$
}'
# Check for any GraphQL fragments that might expect the old timestamp format
rg -A 2 'fragment.*Notification.*on.*Notification.*{'
Length of output: 340
Script:
#!/bin/bash
# Let's try a different approach to verify the service implementation
# Check the NotificationsService implementation
rg -A 10 "class NotificationsService"
# Look for timestamp formatting in the service
rg -l "formatTimestamp.*notification"
# Check GraphQL fragments with proper syntax
rg "fragment.*on Notification"
# Look for any timestamp-related fields in notification types/schemas
rg "timestamp|datetime" --type ts
Length of output: 14455
api/package.json (1)
117-117: LGTM! Verify the strftime dependency.
The addition of strftime and its type definitions looks good. Let's verify there are no known vulnerabilities.
Also applies to: 159-159
✅ Verification successful
The strftime dependency is safe to use
The verification shows:
- No security vulnerabilities reported in GitHub's advisory database
- Package is actively maintained with latest version 0.10.3 released in June 2024
- Long history of stable releases since 2010
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and alternatives for the strftime package
# Check npm for latest versions and security advisories
npm view strftime time
npm audit strftime
# Check for GitHub security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "strftime") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1992
web/composables/gql/gql.ts (2)
17-17: LGTM! Verify schema definition for formattedTimestamp.
The addition of formattedTimestamp to the NotificationFragment is consistent and well-structured. Let's verify the schema definition to ensure the field type matches expectations.
Also applies to: 52-52
✅ Verification successful
LGTM! Schema definition for formattedTimestamp is correct
The formattedTimestamp field is properly defined in the schema at api/src/graphql/schema/types/notifications/notifications.graphql with type String, which aligns with the fragment usage in the generated code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the schema definition for formattedTimestamp field
# Expected: Field should be defined in the Notification type with String type
# Search for Notification type definition and formattedTimestamp field
fd -e graphql -e gql . | xargs rg "type Notification.*?{.*?formattedTimestamp.*?}" -U
Length of output: 237
Script:
#!/bin/bash
# Let's try a different approach to find the schema definition
# First, find GraphQL schema files
fd -e graphql -e gql . | while read -r file; do
echo "=== $file ==="
# Look for Notification type definition
rg "type\s+Notification\s*{[^}]*}" -U "$file"
done
# Also search for any field named formattedTimestamp
echo -e "\nSearching for formattedTimestamp field:"
fd -e graphql -e gql . | xargs rg "formattedTimestamp"
Length of output: 14718
Line range hint 1-1: Verify code generation process.
This file appears to be auto-generated. Let's ensure the changes were made through the proper code generation process rather than manual edits.
✅ Verification successful
Let me gather more information about the specific file in question to verify if it's part of the generated code.
Let me check one more thing to verify if there's another codegen configuration file that might be responsible for generating this file.
Let me check the web/codegen.ts file to confirm if it's responsible for generating these files.
The file is correctly managed by the code generation process
The file web/composables/gql/gql.ts is indeed auto-generated as confirmed by:
- The presence of
web/codegen.tsconfiguration file that explicitly generates files incomposables/gql/directory - The configuration matches the file content with the same
/* eslint-disable */header - The presence of other typical generated files in the same directory (graphql.ts, fragment-masking.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the presence of code generation configuration and scripts
# Expected: Find codegen config files and generation scripts
# Look for GraphQL codegen configuration
fd "codegen.ya?ml|codegen.json" .
# Look for generation scripts in package.json
fd "package.json" . --exec rg "codegen|generate"
Length of output: 1368
Script:
#!/bin/bash
# Check if the file is mentioned in codegen config
cat ./api/codegen.yml
# Look for any generation output path patterns that might match our file
rg -l "web/composables/gql" ./api/codegen.yml
# Check if the file contains typical codegen header/footer
head -n 5 ./web/composables/gql/gql.ts
Length of output: 2423
Script:
#!/bin/bash
# Look for any other codegen configuration files
fd "codegen.ts" .
# Check if there are any other files in the same directory
ls -la ./web/composables/gql/
Length of output: 489
Script:
#!/bin/bash
# Check the web codegen configuration
cat ./web/codegen.ts
# Also check if the file has the typical generated file markers
grep -A 2 "Generated file" ./web/composables/gql/gql.ts
Length of output: 1060
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (2)
30-30: LGTM!
The import of formatDatetime is correctly added to support the new timestamp formatting functionality.
675-676: LGTM!
The changes correctly implement:
- ISO string format for consistent timestamp representation
- User-friendly formatted timestamp based on notification settings
web/composables/gql/graphql.ts (2)
1660-1660: LGTM: Addition of formattedTimestamp field
The new optional field is properly typed and aligns with the PR objective to support formatted timestamps from notification settings.
1753-1757: LGTM: GraphQL document updates
The auto-generated GraphQL documents have been properly updated to include the new formattedTimestamp field consistently across all relevant operations.
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
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
api/src/utils.ts (1)
138-168: Consider extracting format strings as constants.The format strings could be moved to named constants at the module level for better maintainability and reusability.
+const DATE_TIME_FORMATS = { + SYSTEM_TIME: '%c', + DEFAULT_TIME: '%I:%M %p', + TIMEZONE_FREE: '%a %d %b %Y %I:%M:%S %p' +} as const; export function formatDatetime( date: Date, options: Partial<{ dateFormat: string; timeFormat: string; omitTimezone?: boolean }> = {} ): string { - const { dateFormat = '%c', timeFormat = '%I:%M %p', omitTimezone = true } = options; + const { dateFormat = DATE_TIME_FORMATS.SYSTEM_TIME, timeFormat = DATE_TIME_FORMATS.DEFAULT_TIME, omitTimezone = true } = options; let formatted = strftime(dateFormat, date); - if (dateFormat === '%c') { + if (dateFormat === DATE_TIME_FORMATS.SYSTEM_TIME) { if (omitTimezone) { - const timezoneFreeFormat = '%a %d %b %Y %I:%M:%S %p'; - formatted = strftime(timezoneFreeFormat, date); + formatted = strftime(DATE_TIME_FORMATS.TIMEZONE_FREE, date); } } else {api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (2)
702-707: Fix typo in comment.The implementation looks good, but there's a small typo in the comment.
- // i.e. if unixStringSeconds is an empty string or represents a non-numberS + // i.e. if unixStringSeconds is an empty string or represents a non-number
711-730: Consider improving error handling and documentation.The implementation has a few areas that could be enhanced:
- The fallback to raw timestamp in error cases might not be user-friendly
- The debug log could be noisy in production
- Missing JSDoc documentation for the method
Consider these improvements:
+ /** + * Formats a timestamp string according to user's display settings. + * @param timestamp Unix timestamp in seconds + * @returns Formatted date string based on user preferences, or a fallback format if settings unavailable + */ private formatTimestamp(timestamp: string) { const { display: settings } = getters.dynamix(); const date = this.parseNotificationDateToIsoDate(timestamp); if (!settings) { this.logger.warn( '[formatTimestamp] Dynamix display settings not found. Cannot apply user settings.' ); - return timestamp; + return new Date(Number(timestamp) * 1000).toLocaleString(); // More user-friendly fallback } else if (!date) { this.logger.warn(`[formatTimestamp] Could not parse date from timestamp: ${date}`); - return timestamp; + return new Date(Number(timestamp) * 1000).toLocaleString(); // More user-friendly fallback } - this.logger.debug(`[formatTimestamp] ${settings.date} :: ${settings.time} :: ${date}`); + this.logger.verbose(`[formatTimestamp] ${settings.date} :: ${settings.time} :: ${date}`); return formatDatetime(date, { dateFormat: settings.date, timeFormat: settings.time, omitTimezone: true, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
api/src/core/types/ini.ts(2 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts(3 hunks)api/src/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/core/types/ini.ts
🔇 Additional comments (4)
api/src/utils.ts (2)
1-1: LGTM! Good choice of date formatting library.
The strftime library is a solid choice for date formatting, providing robust locale support and a wide range of formatting options.
108-131: LGTM! Well-documented implementation.
The JSDoc documentation is comprehensive, including clear parameter descriptions and practical examples.
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (2)
675-676: LGTM! Clean implementation of timestamp formatting.
The changes properly handle null cases and ensure consistent ISO string formatting while adding the new formatted timestamp field.
675-676: Verify GraphQL schema updates and consumers.
The addition of formattedTimestamp field requires verification of schema updates and consumers.
✅ Verification successful
GraphQL schema and consumers are properly updated for the timestamp changes
The verification shows that:
- The GraphQL schema (
api/src/graphql/generated/api/types.ts) includes bothtimestampandformattedTimestampfields in the Notification type - The field is properly propagated to client-side types (
web/composables/gql/graphql.ts) - The
NotificationFragmentFragmentin the client includes both timestamp fields - Only one direct usage of
notification.timestampfound in a test file, which is expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the GraphQL schema includes the new field
rg -A 5 "type Notification"
# Check for potential consumers of the Notification type
ast-grep --pattern 'interface Notification {
$$$
}'
# Check for direct uses of the timestamp field
rg "notification\.timestamp"
Length of output: 16406
pujitm
force-pushed
the
feat/notif-settings
branch
from
2f75a11 to
abce38e
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
2 participants
Pulled from
/boot/config/plugins/dynamix/dynamix.cfgby changing the relevant notification settings (display, date, time).Summary by CodeRabbit
Release Notes
New Features
formattedTimestampfor improved timestamp display in notifications.Bug Fixes
Documentation
Tests
These changes enhance the user experience by providing clearer and more customizable notification timestamps.