feat: make log viewer component dynamic #1242
Conversation
WalkthroughThe update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant LV as LogViewerModification
participant FS as File System
participant Logger as Logger
LV->>FS: Check if config exists (generatePatch)
FS-->>LV: Return existing content or not found
alt Configuration exists
LV->>Logger: Log config already present (shouldApply false)
else Configuration missing
LV->>LV: Initiate rollback to remove previous settings
LV->>FS: Write new log viewer configuration (apply)
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
🪧 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: 1
🧹 Nitpick comments (5)
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts (5)
10-13: Class declaration looks good, but consider adding documentation.The
LogViewerModificationclass is well-defined with clear properties, but adding a class-level JSDoc comment would help other developers understand its purpose and usage.+/** + * Handles the configuration of a log viewer for UNRAID by generating patches, + * applying new configurations, and handling rollbacks. + */ export class LogViewerModification extends FileModification { id: string = 'log-viewer'; public readonly filePath: string = '/usr/local/emhttp/plugins/dynamix.my.servers/LogViewer.page' as const;🧰 Tools
🪛 GitHub Check: Test API
[failure] 12-12:
Insert⏎·······🪛 GitHub Check: Build API
[failure] 12-12:
Insert⏎·······
26-28: Remove unnecessary constructor.This constructor only calls the parent constructor with the logger parameter, which is what would happen by default if the constructor were omitted.
- constructor(logger: Logger) { - super(logger); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 26-28: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
30-40: Consider adding error handling for file operations.The
generatePatchmethod has good logic for checking if the file exists, but it could benefit from additional error handling for file operations.protected async generatePatch(overridePath?: string): Promise<string> { - const currentContent = (await fileExists(this.filePath)) - ? await readFile(this.filePath, 'utf8') - : ''; + let currentContent = ''; + try { + if (await fileExists(this.filePath)) { + currentContent = await readFile(this.filePath, 'utf8'); + } + } catch (error) { + this.logger.warn(`Failed to read file ${this.filePath}: ${error.message}`); + }
50-54: Consider adding error handling to the apply method.The apply method should include try-catch blocks to handle potential errors during file operations.
async apply(): Promise<string> { - await this.rollback(); - await writeFile(this.filePath, this.logViewerConfig, { mode: 0o644 }); - return this.logViewerConfig; + try { + await this.rollback(); + await writeFile(this.filePath, this.logViewerConfig, { mode: 0o644 }); + this.logger.log(`Successfully applied log viewer configuration to ${this.filePath}`); + return this.logViewerConfig; + } catch (error) { + this.logger.error(`Failed to apply log viewer configuration: ${error.message}`); + throw error; + }
56-59: Consider logging rollback actions.Adding logging statements would help with debugging and auditing when rollbacks occur.
async rollback(): Promise<void> { + this.logger.log('Rolling back log viewer configuration...'); await rm(this.getPathToAppliedPatch(), { force: true }); await rm(this.filePath, { force: true }); + this.logger.log('Log viewer configuration rollback completed');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts(1 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/LogViewer.page(0 hunks)
💤 Files with no reviewable changes (1)
- plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/LogViewer.page
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts
[error] 26-28: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🪛 GitHub Check: Test API
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts
[failure] 12-12:
Insert ⏎·······
🪛 GitHub Check: Build API
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts
[failure] 12-12:
Insert ⏎·······
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts (2)
42-48: The shouldApply logic is clear and well-structured.The method correctly checks if the configuration already exists and returns appropriate response objects.
14-24:❓ Verification inconclusive
Ensure i18n is fully supported for the log viewer.
The log viewer template includes an
<unraid-i18n-host>wrapper, which is good for internationalization. Make sure that all displayed text in the log viewer component is properly localized.
🏁 Script executed:
#!/bin/bash # Check if log viewer component has i18n support fd "log-viewer" -e ts -e js -e html --exec cat {} \; | grep -E "i18n|translate"Length of output: 122
i18n Integration in Log Viewer
The<unraid-i18n-host>wrapper is present, indicating basic internationalization support. However, please verify that all user-facing strings – such as the hardcodedTitle="Log Viewer (new)"and any similar text attributes – are fully localized within theunraid-log-viewercomponent (i.e., linked to your i18n system and translation keys).
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts (1)
27-29: Remove unnecessary constructor.This constructor doesn't add any functionality beyond calling the parent constructor and can be safely removed.
- constructor(logger: Logger) { - super(logger); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 27-29: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts
[error] 27-29: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
api/src/unraid-api/unraid-file-modifier/modifications/log-viewer.modification.ts (6)
10-13: Good implementation of the LogViewerModification class.The class extends FileModification correctly and provides proper identification through the id property. The filePath constant is well-defined and accurately points to the target file location.
15-25: Configuration template looks good.The log viewer configuration includes appropriate menu categorization, title, icon, and tag settings. The HTML template uses unraid-specific custom elements which aligns with the modern component approach. The use of
trimStart()is a good practice to remove unwanted leading whitespace.
31-41: Well-implemented patch generation logic.The method correctly checks for file existence before reading content and creates an appropriate patch by comparing current and desired configurations. The error handling is robust with good fallback to empty string when file doesn't exist.
43-49: Good implementation of shouldApply method.The method properly determines if the modification should be applied based on whether the configuration file already exists, with clear and descriptive reason messages.
51-55: Robust apply method implementation.The method follows a good practice by calling rollback first to ensure a clean state before writing the new configuration. The file permissions (0o644) are explicitly set, which is a security best practice.
57-60: Thorough rollback implementation.The method ensures proper cleanup by removing both the applied patch and the configuration file, with the
force: trueoption to handle cases where files might not exist.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new Log Viewer configuration component to enhance the management and application of log settings. - **Chores** - Removed the legacy log viewer interface to streamline log management and improve the overall user experience. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [4.4.0](v4.3.1...v4.4.0) (2025-03-25) ### Features * add ReplaceKey functionality to plugin ([#1264](#1264)) ([4aadcef](4aadcef)) * downgrade page replace key check ([#1263](#1263)) ([8d56d12](8d56d12)) * make log viewer component dynamic ([#1242](#1242)) ([e6ec110](e6ec110)) * ReplaceKey functionality in Registration and Update pages ([#1246](#1246)) ([04307c9](04307c9)) * UnraidCheckExec for Check OS Updates via UPC dropdown ([#1265](#1265)) ([5935a3b](5935a3b)) ### Bug Fixes * **deps:** update all non-major dependencies ([#1236](#1236)) ([7194f85](7194f85)) * **deps:** update all non-major dependencies ([#1247](#1247)) ([20b0aeb](20b0aeb)) * **deps:** update all non-major dependencies ([#1251](#1251)) ([33a1a1d](33a1a1d)) * **deps:** update all non-major dependencies ([#1253](#1253)) ([53fec0e](53fec0e)) * **deps:** update dependency @nestjs/passport to v11 ([#1244](#1244)) ([edc93a9](edc93a9)) * **deps:** update dependency graphql-subscriptions to v3 ([#1209](#1209)) ([c14c85f](c14c85f)) * **deps:** update dependency ini to v5 ([#1217](#1217)) ([f27660f](f27660f)) * **deps:** update dependency jose to v6 ([#1248](#1248)) ([42e3d59](42e3d59)) * **deps:** update dependency marked to v15 ([#1249](#1249)) ([2b6693f](2b6693f)) * **deps:** update dependency pino-pretty to v13 ([#1250](#1250)) ([85fb910](85fb910)) * **deps:** update dependency pm2 to v6 ([#1258](#1258)) ([04ad2bc](04ad2bc)) * **deps:** update dependency shadcn-vue to v1 ([#1259](#1259)) ([1a4fe8f](1a4fe8f)) * **deps:** update dependency vue-i18n to v11 ([#1261](#1261)) ([0063286](0063286)) * **deps:** update vueuse monorepo to v13 (major) ([#1262](#1262)) ([94caae3](94caae3)) * make scripts executable when building the plugin ([#1255](#1255)) ([e237f38](e237f38)) * node installation not persisting across reboots ([#1256](#1256)) ([0415cf1](0415cf1)) * update configValid state to ineligible in var.ini and adjust rel… ([#1268](#1268)) ([ef8c954](ef8c954)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Chores