feat: style improvements #1077
feat: style improvements #1077
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request encompasses a series of modifications across multiple files in the Unraid project, focusing on logging, SSO management, UI styling, and theme configuration. The changes include updates to logging imports, SSO removal logic, PM2 update process, and UI component styling. The modifications introduce subtle refinements to error handling, logging messages, and visual presentation, with an emphasis on border styling and theme color adjustments. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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 (
|
![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
🧹 Nitpick comments (6)
api/src/unraid-api/cli/sso/add-sso-user.command.ts (3)
43-44: LGTM! Good UX improvement.Adding a delay before restart gives users time to read the message and prepare for the API restart. The implementation uses a standard Promise-based setTimeout pattern.
However, consider extracting the delay duration to a constant for better maintainability:
+const RESTART_DELAY_MS = 5000; + @Injectable() @SubCommand({ name: 'add-user', aliases: ['add', 'a'], description: 'Add a user for SSO', }) export class AddSSOUserCommand extends CommandRunner { // ... async run(_input: string[], options: AddSSOUserCommandOptions): Promise<void> { try { // ... if (shouldRestart) { - this.logger.info('Restarting the Unraid API in 5 seconds to enable the SSO button'); - await new Promise(resolve => setTimeout(resolve, 5000)); + this.logger.info(`Restarting the Unraid API in ${RESTART_DELAY_MS/1000} seconds to enable the SSO button`); + await new Promise(resolve => setTimeout(resolve, RESTART_DELAY_MS)); await this.restartCommand.run([]); }
Line range hint
47-53: Enhance error handling with more context.The current error handling could be improved to provide more context for debugging:
} catch (e: unknown) { if (e instanceof Error) { - this.logger.error('Error adding user: ' + e.message); + this.logger.error(`Error adding user: ${e.message}`, { error: e }); } else { - this.logger.error('Error adding user'); + this.logger.error('Error adding user: Unknown error type', { error: e }); } }
Line range hint
82-82: Fix typo in error message.- throw new Error('Please answer the diclaimer with (y/n)\n'); + throw new Error('Please answer the disclaimer with (y/n)\n');api/src/unraid-api/cli/start.command.ts (1)
20-30: Consider adding timeout handling and retries.The PM2 update process could potentially hang or fail temporarily due to network issues.
Consider implementing a retry mechanism with timeouts:
private async retryWithTimeout<T>( operation: () => Promise<T>, retries = 3, timeout = 30000 ): Promise<T> { for (let attempt = 1; attempt <= retries; attempt++) { try { return await Promise.race([ operation(), new Promise((_, reject) => setTimeout(() => reject(new Error('Operation timed out')), timeout) ) ]); } catch (error) { if (attempt === retries) throw error; this.logger.warn(`Attempt ${attempt} failed, retrying...`); await new Promise(resolve => setTimeout(resolve, 1000 * attempt)); } } throw new Error('Should not reach here'); }Then use it in your update logic:
await this.retryWithTimeout( () => execa(PM2_PATH, ['update']), 3, // 3 retries 30000 // 30 second timeout );unraid-ui/src/components/common/sheet/sheet.variants.ts (1)
4-4: LGTM! Consider improving readability.The addition of
border-borderclass is consistent with the theme configuration. However, the long string of utility classes could be split for better readability.Consider splitting the classes for better maintainability:
- "fixed z-50 bg-background gap-4 shadow-lg transition ease-in-out data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:duration-300 data-[state=open]:duration-500 border-border", + [ + "fixed z-50 bg-background gap-4 shadow-lg", + "transition ease-in-out", + "data-[state=open]:animate-in data-[state=closed]:animate-out", + "data-[state=closed]:duration-300 data-[state=open]:duration-500", + "border-border" + ].join(" "),unraid-ui/src/theme/preset.ts (1)
17-19: LGTM! Consider documenting the border color system.The addition of borderColor configuration with HSL variables is consistent with the theme system.
Consider adding a comment to document the border color usage:
borderColor: { + // Uses HSL variables for consistent theming across light/dark modes DEFAULT: "hsl(var(--border))", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (10)
api/src/core/sso/sso-remove.ts(2 hunks)api/src/index.ts(0 hunks)api/src/unraid-api/cli/sso/add-sso-user.command.ts(1 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)unraid-ui/src/components/common/sheet/sheet.variants.ts(1 hunks)unraid-ui/src/theme/preset.ts(1 hunks)web/assets/main.css(0 hunks)web/components/Notifications/List.vue(1 hunks)web/components/shadcn/sheet/index.ts(1 hunks)web/store/theme.ts(2 hunks)
💤 Files with no reviewable changes (2)
- web/assets/main.css
- api/src/index.ts
✅ Files skipped from review due to trivial changes (3)
- api/src/core/sso/sso-remove.ts
- web/components/Notifications/List.vue
- web/components/shadcn/sheet/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
web/store/theme.ts (3)
35-35: LGTM! Border color adjustment improves visibility.The increase in border lightness from 14.9% to 20% in dark mode enhances contrast while maintaining the dark theme aesthetic.
86-90: LGTM! Type safety in theme check.The computed property correctly uses type assertion to ensure type safety when checking theme names against DARK_THEMES array.
77-78: Verify the impact of theme change on existing UPC components.The replacement of 'azure' with 'gray' in DARK_THEMES could affect UPC text color in existing implementations.
Run this script to check for potential impacts:
| const { stderr: updateErr, stdout: updateOut } = await execa(`${PM2_PATH} update`); | ||
| if (updateOut) { | ||
| this.logger.log(updateOut); | ||
| } | ||
| if (updateErr) { | ||
| this.logger.error('PM2 Update Error: ' + updateErr); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for PM2 update.
The current implementation has several potential issues:
- No try-catch around execa which could throw if PM2 is not found
- PM2 might write non-fatal messages to stderr
- Immediate process.exit() could leave resources unclosed
Consider this improved implementation:
- const { stderr: updateErr, stdout: updateOut } = await execa(`${PM2_PATH} update`);
- if (updateOut) {
- this.logger.log(updateOut);
- }
- if (updateErr) {
- this.logger.error('PM2 Update Error: ' + updateErr);
- process.exit(1);
- }
+ try {
+ const { stderr: updateErr, stdout: updateOut } = await execa(PM2_PATH, ['update'], {
+ timeout: 30000, // 30 second timeout
+ });
+
+ if (updateOut) {
+ this.logger.log(updateOut);
+ }
+
+ // Only treat stderr as error if it contains specific error patterns
+ if (updateErr && /error|fail|exception/i.test(updateErr)) {
+ this.logger.error('PM2 Update Error: ' + updateErr);
+ await this.cleanup();
+ process.exit(1);
+ } else if (updateErr) {
+ // Log non-fatal stderr messages as warnings
+ this.logger.warn('PM2 Update Warning: ' + updateErr);
+ }
+ } catch (error) {
+ this.logger.error('Failed to execute PM2 update:', error);
+ await this.cleanup();
+ process.exit(1);
+ }You'll need to implement the cleanup method:
private async cleanup(): Promise<void> {
// Add cleanup logic here (close connections, etc.)
this.logger.info('Cleaning up before exit...');
}
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
| this.logger.info('Restarting the Unraid API in 5 seconds to enable the SSO button'); | ||
| await new Promise(resolve => setTimeout(resolve, 5000)); |
There was a problem hiding this comment.
what's the reason behind this wait?
There was a problem hiding this comment.
The API wouldn't sync the config internally, and then wrote over the change.
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
Styling Updates
Logging Improvements
Chores