chore: add import organizer to prettier config #959
Conversation
as a demonstration
as a demonstration
WalkthroughThe pull request introduces updates to Prettier configuration files in both the 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
api/.prettierrc.cjs (1)
6-10: LGTM! Consider standardizing printWidth.The basic Prettier settings look good. Note that 105 characters for
printWidthis slightly unusual (80 or 100 is more common), but it's not a significant concern if this aligns with your team's preferences.api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (3)
Line range hint
71-73: Incorrect usage ofNotificationType[type]In the
handleNotificationAddmethod, you're usingNotificationType[type]wheretypeis already a value of theNotificationTypeenum. AccessingNotificationType[type]will returnundefined, which may lead to incorrect behavior when loading the notification file.Apply this diff to pass the correct notification type:
-private async handleNotificationAdd(path: string) { +private async handleNotificationAdd(path: string): Promise<void> { // The path looks like /{notification base path}/{type}/{notification id} const type = path.includes('/unread/') ? NotificationType.UNREAD : NotificationType.ARCHIVE; this.logger.debug(`Adding ${type} Notification: ${path}`); - const notification = await this.loadNotificationFile(path, NotificationType[type]); + const notification = await this.loadNotificationFile(path, type); this.increment(notification.importance, NotificationsService.overview[type.toLowerCase()]); this.publishOverview(); pubsub.publish(PUBSUB_CHANNEL.NOTIFICATION_ADDED, { notificationAdded: notification, }); }
Line range hint
123-131: Add error handling forwriteFileoperationIn the
createNotificationmethod, if thewriteFileoperation fails after the legacy notifier script fails, the error will not be caught, potentially causing unhandled exceptions. Consider adding error handling for thewriteFileoperation to ensure any errors are appropriately managed.Apply this diff to handle errors from
writeFile:public async createNotification(data: NotificationData): Promise<Notification> { const id: string = await this.makeNotificationId(data.title); const fileData = this.makeNotificationFileData(data); try { const [command, args] = this.getLegacyScriptArgs(fileData); await execa(command, args); } catch (error) { // Manually write the file if the script fails this.logger.debug(`[createNotification] legacy notifier failed: ${error}`); this.logger.verbose(`[createNotification] Writing: ${JSON.stringify(fileData, null, 4)}`); const path = join(this.paths().UNREAD, id); const ini = encodeIni(fileData); - await writeFile(path, ini); + try { + await writeFile(path, ini); + } catch (writeError) { + this.logger.error(`[createNotification] Failed to write notification file: ${writeError}`); + throw writeError; + } } return this.notificationFileToGqlNotification({ id, type: NotificationType.UNREAD }, fileData); }
Line range hint
155-167: Handle potential errors fromemptyDiroperationsIn the
deleteAllNotificationsmethod, errors fromemptyDirare not being handled. If eitheremptyDircall fails, the notification overview will still be reset, leading to inconsistencies between the file system and the application's state. Consider adding error handling to ensure the overview is only reset when the directories have been successfully emptied.Apply this diff to handle errors appropriately:
public async deleteAllNotifications(): Promise<NotificationOverview> { const { UNREAD, ARCHIVE } = this.paths(); // Ensures the directories exist before deleting - await emptyDir(ARCHIVE); - await emptyDir(UNREAD); + try { + await emptyDir(ARCHIVE); + await emptyDir(UNREAD); + } catch (error) { + this.logger.error(`[deleteAllNotifications] Failed to empty notification directories: ${error}`); + throw error; + } NotificationsService.overview = { unread: { alert: 0, info: 0, warning: 0, total: 0, }, archive: { alert: 0, info: 0, warning: 0, total: 0, }, }; return this.getOverview(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
api/package-lock.jsonis excluded by!**/package-lock.jsonweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
api/.prettierrc.cjs(1 hunks)api/package.json(1 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts(1 hunks)web/.prettierrc.mjs(1 hunks)web/components/Notifications/Indicator.vue(4 hunks)web/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/components/Notifications/Indicator.vue
🔇 Additional comments (9)
web/.prettierrc.mjs (2)
6-6: LGTM!
The quote style change aligns with the singleQuote: true configuration.
7-7: Verify the impact of tabWidth change.
Changing tabWidth from 4 to 2 spaces will affect the entire codebase formatting. While 2 spaces is a common practice in JS/TS projects, this change should be discussed with the team to ensure alignment with coding standards.
api/.prettierrc.cjs (2)
1-4: Well-documented configuration!
The JSDoc comment with documentation link and TypeScript type annotation improves maintainability and provides better IDE support.
11-37: Excellent import organization structure!
The import organization configuration is well-thought-out with several good practices:
- Clear separation between framework, third-party, and application code
- Proper handling of type imports
- Support for decorators which is essential for NestJS
- Good use of empty lines as visual separators
Let's verify if the import plugin is properly added to package.json:
✅ Verification successful
Import organization setup is properly configured and integrated
The verification confirms that @ianvs/prettier-plugin-sort-imports is correctly added as a dev dependency in version ^4.4.0, which aligns with the configuration in .prettierrc.cjs. The import organization setup is complete and ready to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the import plugin is added as a dev dependency
# Test: Check if the plugin is in package.json
jq -r '.devDependencies["@ianvs/prettier-plugin-sort-imports"]' api/package.json
Length of output: 89
web/package.json (1)
32-32: LGTM! The import organizer dependency is correctly added.
The @ianvs/prettier-plugin-sort-imports package is appropriately added to devDependencies and follows proper version constraints.
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (2)
1-17: LGTM! Import organization follows a clear and logical pattern.
The imports are well-organized following a hierarchical structure:
- External dependencies
- Internal types and utilities
- Local module imports
This organization improves code readability and maintainability.
Line range hint 19-156: Implementation remains unchanged and looks good.
The resolver implementation maintains its existing functionality with proper error handling and access control.
api/package.json (1)
134-134: LGTM! Verify prettier config setup.
The addition of @ianvs/prettier-plugin-sort-imports as a dev dependency is appropriate and well-structured.
Let's verify the corresponding prettier configuration:
✅ Verification successful
Prettier import sorting plugin is correctly configured ✅
The plugin is properly set up in api/.prettierrc.cjs:
- Added to the
pluginsarray - Import order configuration is comprehensive and well-structured
- Parser plugins include necessary options for TypeScript and decorators
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify prettier configuration exists and includes the import plugin
# Test: Check if .prettierrc.* exists and contains the plugin configuration
echo "Checking for prettier config file..."
fd -t f "^\.prettierrc\." api/
# Test: Verify plugin configuration in the prettier config
echo -e "\nChecking plugin configuration..."
rg -l "@ianvs/prettier-plugin-sort-imports" api/.prettierrc.*
Length of output: 488
Script:
#!/bin/bash
# Description: Check the content of the prettier config file to verify plugin setup
# Show the content of the prettier config file
echo "Content of api/.prettierrc.cjs:"
cat api/.prettierrc.cjs
Length of output: 1360
api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (1)
Line range hint 235-238: Possible null return value handling
In the parseNotificationDateToIsoDate method, when the input unixStringSeconds is invalid or undefined, the method returns null. Ensure that any code consuming this method can handle null values to prevent potential TypeError exceptions when using the timestamp.
Run the following script to find usages of parseNotificationDateToIsoDate and check for null handling:
✅ Verification successful
Let me gather more information about the schema and validation to understand how null values are handled.
Let me check one more thing to see how the notification schema validation is implemented.
Based on the gathered information, I can now provide the final response:
Null handling for timestamp is properly implemented
The parseNotificationDateToIsoDate method's null return value is safely handled through:
- Schema validation using Zod where
formattedTimestampis defined asz.string().nullish() - Validation of notifications through
NotificationSchema().parse()before usage - The GraphQL resolver wraps the timestamp in
formatTimestamp()before exposing it
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all usages of parseNotificationDateToIsoDate handle null returns.
# Search for all usages of the method
rg 'parseNotificationDateToIsoDate\(' -A 3
# Search for places where the timestamp is used without null checks
rg 'notification\.timestamp' -A 3
Length of output: 2312
Script:
#!/bin/bash
# Check the notification schema definition and validation
ast-grep --pattern 'NotificationSchema = z.$$$'
# Check for any type definitions related to notifications
rg 'interface.*Notification' -A 5
# Check the formatTimestamp implementation
ast-grep --pattern 'function formatTimestamp($$$)'
Length of output: 957
Script:
#!/bin/bash
# Search for notification schema validation and type definitions
rg 'NotificationSchema\(\)' -B 5 -A 5
# Check for any zod schema definitions for notifications
rg 'z\.object\({.*timestamp' -A 5
Length of output: 5214
Yeah! It'd probably involve some setup changes in the dev environment and another pr tho |
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores
@ianvs/prettier-plugin-sort-importsas a development dependency in both API and web projects.