chore: cli commands for api plugin install & generation #1352
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a robust, extensible configuration state management system for Unraid API plugins, including disk persistence, validation, and migration from legacy formats. It adds new environment variables for configuration directories, ensures their existence at runtime, and provides utilities for config injection and persistence. The CLI gains a comprehensive plugin management command set, enabling installation, removal, and listing of plugins as peer dependencies, with support for bundled and unbundled modes. A new plugin generator CLI scaffolds plugin projects with standardized templates. The Changes
Changes (Grouped Table)
Sequence Diagram(s)sequenceDiagram
participant CLI_User
participant CLI
participant DependencyService
participant PluginService
participant RestartCommand
CLI_User->>CLI: plugin install <package> [--bundled]
CLI->>DependencyService: addPeerDependency(package, bundled)
DependencyService->>DependencyService: Update package.json / run npm install
CLI->>DependencyService: rebuildVendorArchive() (if not bundled)
CLI->>RestartCommand: run()
RestartCommand->>CLI_User: Output restart status
CLI_User->>CLI: plugin remove <package>
CLI->>DependencyService: removePeerDependency(package)
CLI->>RestartCommand: run()
RestartCommand->>CLI_User: Output restart status
CLI_User->>CLI: plugin list
CLI->>PluginService: listPlugins()
PluginService-->>CLI: List of plugins
CLI->>CLI_User: Output plugin list
sequenceDiagram
participant PluginModule
participant ApiStateConfig
participant ConfigPersistenceHelper
participant Disk
PluginModule->>ApiStateConfig: load()
ApiStateConfig->>ConfigPersistenceHelper: parseConfig()
ConfigPersistenceHelper->>Disk: Read config file
Disk-->>ConfigPersistenceHelper: Return data or undefined
ConfigPersistenceHelper-->>ApiStateConfig: Return config or default
ApiStateConfig->>ConfigPersistenceHelper: persist(config)
ConfigPersistenceHelper->>Disk: Write config if changed
Disk-->>ConfigPersistenceHelper: Write result
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 pause |
![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 (9)
api/src/index.ts (2)
6-6: Fix import order according to linting rulesThe static analysis is flagging an ordering issue with the imports.
-import { unlinkSync, mkdirSync } from 'fs'; +import { mkdirSync, unlinkSync } from 'fs';🧰 Tools
🪛 GitHub Check: Build API
[failure] 6-6:
ReplaceunlinkSync,·mkdirwithmkdirSync,·unlink🪛 GitHub Check: Test API
[failure] 6-6:
ReplaceunlinkSync,·mkdirwithmkdirSync,·unlink
17-17: Fix import order according to linting rulesThe static analysis is flagging an ordering issue with the environment imports.
-import { environment, PORT, CONFIG_MODULES_HOME } from '@app/environment.js'; +import { CONFIG_MODULES_HOME, environment, PORT } from '@app/environment.js';🧰 Tools
🪛 GitHub Check: Build API
[failure] 17-17:
Replaceenvironment,·PORT,·CONFIG_MODULES_HOMEwithCONFIG_MODULES_HOME,·environment,·PORT🪛 GitHub Check: Test API
[failure] 17-17:
Replaceenvironment,·PORT,·CONFIG_MODULES_HOMEwithCONFIG_MODULES_HOME,·environment,·PORTapi/src/unraid-api/config/config.interface.ts (1)
1-9: Consider the static analysis suggestion about empty interfaceWhile the empty
ConfigFeaturesinterface serves as a container for type extensions, the static analyzer flags it as suspicious. Since it's meant to be extended, keeping it as an interface is justified, but you could add documentation to clarify this for static analyzers./** * Container record of config names to their types. Used for type completion on registered configs. * Config authors should redeclare/merge this interface with their config names as the keys * and implementation models as the types. + * + * @remarks This is intentionally an empty interface to be extended by other modules. */ export interface ConfigFeatures {};🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
api/src/unraid-api/config/config.registry.ts (1)
14-30: Consider refactoring the static-only classThe static analysis tool flags this class for having only static members. While functionally correct, consider these alternatives:
- Use the class name instead of
thisin static methods- Convert to a namespace or module-level object with exported functions
Option 1: Use class name instead of
this:export class ConfigRegistry { /** A map of config names to their implementation models. */ private static configTypes = new Map<string, string>(); static register(configName: string, configType: string) { - this.configTypes.set(configName, configType); + ConfigRegistry.configTypes.set(configName, configType); } static getConfigType(configName: string) { - return this.configTypes.get(configName); + return ConfigRegistry.configTypes.get(configName); } static getConfigToken(configName: string) { const configType = ConfigRegistry.getConfigType(configName) ?? ''; return makeConfigToken(configName, configType); } }Option 2: Convert to module-level functions:
/** A map of config names to their implementation models. */ const configTypes = new Map<string, string>(); export function registerConfig(configName: string, configType: string) { configTypes.set(configName, configType); } export function getConfigType(configName: string) { return configTypes.get(configName); } export function getConfigToken(configName: string) { const configType = getConfigType(configName) ?? ''; return makeConfigToken(configName, configType); }🧰 Tools
🪛 Biome (1.9.4)
[error] 14-30: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 19-19: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
api/src/unraid-api/config/api-state.service.ts (2)
4-5: Consider using absolute import paths.Static analysis flags these relative imports. If your style guide requires absolute paths, consider updating these imports accordingly.
- import type { ApiStateConfig } from './api-state.model.js'; - import { makeConfigToken } from './config.registry.js'; + import type { ApiStateConfig } from '@app/unraid-api/config/api-state.model.js'; + import { makeConfigToken } from '@app/unraid-api/config/config.registry.js';🧰 Tools
🪛 GitHub Check: Build API
[failure] 4-4:
import statements should have an absolute path🪛 GitHub Check: Test API
[failure] 4-4:
import statements should have an absolute path
57-70: Consider handling multiple invocations of setup().If this method is called more than once, multiple intervals could be registered for the same token, leading to unexpected behavior. An optional improvement is to guard against re-initializing.
setup() { if (this.schedulerRegistry.doesExists('interval', this.token)) { + this.logger.warn(`Persistence already set up for token ${this.token}. Skipping re-initialization.`); return; } ... }api/src/unraid-api/config/api-state.register.ts (2)
4-7: Consider using absolute import paths.Your build pipeline reports warnings for relative paths. Converting them to absolute paths may align with your import style guidelines.
- import type { ApiStateConfigOptions } from './api-state.model.js'; - import type { ApiStateConfigPersistenceOptions } from './api-state.service.js'; - import { ApiStateConfig } from './api-state.model.js'; - import { ScheduledConfigPersistence } from './api-state.service.js'; + import type { ApiStateConfigOptions } from '@app/unraid-api/config/api-state.model.js'; + import type { ApiStateConfigPersistenceOptions } from '@app/unraid-api/config/api-state.service.js'; + import { ApiStateConfig } from '@app/unraid-api/config/api-state.model.js'; + import { ScheduledConfigPersistence } from '@app/unraid-api/config/api-state.service.js';🧰 Tools
🪛 GitHub Check: Build API
[failure] 4-4:
import statements should have an absolute path
[failure] 5-5:
import statements should have an absolute path
[failure] 6-6:
import statements should have an absolute path
[failure] 7-7:
import statements should have an absolute path🪛 GitHub Check: Test API
[failure] 4-4:
import statements should have an absolute path
[failure] 5-5:
import statements should have an absolute path
[failure] 6-6:
import statements should have an absolute path
[failure] 7-7:
import statements should have an absolute path
13-45: Consider refactoring to a plain function rather than a static-only class.A lint rule warns about classes containing only static methods. If instance state is unnecessary, a simpler function can suffice. Alternatively, keep the class if you anticipate adding instance functionality later.
-export class ApiStateConfigModule { - private static readonly logger = new Logger(ApiStateConfigModule.name); - - static async register<ConfigType>( - options: ApiStateRegisterOptions<ConfigType> - ): Promise<DynamicModule> { - ... - return { - module: ApiStateConfigModule, - providers: [ConfigProvider, PersistenceProvider], - exports: [ConfigProvider], - }; - } -} + +export async function registerApiStateConfigModule<ConfigType>( + options: ApiStateRegisterOptions<ConfigType> +): Promise<DynamicModule> { + const logger = new Logger('ApiStateConfigModule'); + ... + return { + module: class {}, + providers: [ConfigProvider, PersistenceProvider], + exports: [ConfigProvider], + }; +}🧰 Tools
🪛 Biome (1.9.4)
[error] 13-45: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
api/src/unraid-api/config/api-state.model.ts (1)
10-10: Consider using absolute import paths.Static analysis warns about these imports. Converting them to absolute references could satisfy your project’s build checks.
- import { ConfigRegistry } from './config.registry.js'; + import { ConfigRegistry } from '@app/unraid-api/config/config.registry.js';🧰 Tools
🪛 GitHub Check: Build API
[failure] 10-10:
import statements should have an absolute path🪛 GitHub Check: Test API
[failure] 10-10:
import statements should have an absolute path
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
api/.env.development(1 hunks)api/.env.production(1 hunks)api/.env.staging(1 hunks)api/.env.test(1 hunks)api/src/environment.ts(1 hunks)api/src/index.ts(3 hunks)api/src/unraid-api/config/api-state.model.ts(1 hunks)api/src/unraid-api/config/api-state.register.ts(1 hunks)api/src/unraid-api/config/api-state.service.ts(1 hunks)api/src/unraid-api/config/config.injection.ts(1 hunks)api/src/unraid-api/config/config.interface.ts(1 hunks)api/src/unraid-api/config/config.registry.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
api/src/unraid-api/config/config.injection.ts (2)
api/src/unraid-api/config/config.interface.ts (1)
ConfigFeatures(8-8)api/src/unraid-api/config/config.registry.ts (1)
ConfigRegistry(14-30)
api/src/index.ts (1)
api/src/environment.ts (1)
CONFIG_MODULES_HOME(89-89)
api/src/unraid-api/config/api-state.register.ts (2)
api/src/unraid-api/config/api-state.model.ts (2)
ApiStateConfigOptions(12-17)ApiStateConfig(19-103)api/src/unraid-api/config/api-state.service.ts (2)
ApiStateConfigPersistenceOptions(7-16)ScheduledConfigPersistence(18-74)
api/src/unraid-api/config/api-state.service.ts (2)
api/src/unraid-api/config/api-state.model.ts (1)
ApiStateConfig(19-103)api/src/unraid-api/config/config.registry.ts (1)
makeConfigToken(10-12)
api/src/unraid-api/config/api-state.model.ts (2)
api/src/unraid-api/config/config.registry.ts (1)
ConfigRegistry(14-30)api/src/environment.ts (1)
CONFIG_MODULES_HOME(89-89)
api/src/unraid-api/config/config.registry.ts (1)
api/src/unraid-api/config/api-state.service.ts (1)
configName(34-36)
🪛 GitHub Check: Build API
api/src/environment.ts
[failure] 89-89:
Insert ⏎
[failure] 89-89:
Newline required at end of file but not found
api/src/index.ts
[failure] 6-6:
Replace unlinkSync,·mkdir with mkdirSync,·unlink
[failure] 17-17:
Replace environment,·PORT,·CONFIG_MODULES_HOME with CONFIG_MODULES_HOME,·environment,·PORT
api/src/unraid-api/config/api-state.register.ts
[failure] 4-4:
import statements should have an absolute path
[failure] 5-5:
import statements should have an absolute path
[failure] 6-6:
import statements should have an absolute path
[failure] 7-7:
import statements should have an absolute path
api/src/unraid-api/config/api-state.service.ts
[failure] 4-4:
import statements should have an absolute path
api/src/unraid-api/config/api-state.model.ts
[failure] 10-10:
import statements should have an absolute path
🪛 GitHub Check: Test API
api/src/environment.ts
[failure] 89-89:
Insert ⏎
[failure] 89-89:
Newline required at end of file but not found
api/src/index.ts
[failure] 6-6:
Replace unlinkSync,·mkdir with mkdirSync,·unlink
[failure] 17-17:
Replace environment,·PORT,·CONFIG_MODULES_HOME with CONFIG_MODULES_HOME,·environment,·PORT
api/src/unraid-api/config/api-state.register.ts
[failure] 4-4:
import statements should have an absolute path
[failure] 5-5:
import statements should have an absolute path
[failure] 6-6:
import statements should have an absolute path
[failure] 7-7:
import statements should have an absolute path
api/src/unraid-api/config/api-state.service.ts
[failure] 4-4:
import statements should have an absolute path
api/src/unraid-api/config/api-state.model.ts
[failure] 10-10:
import statements should have an absolute path
🪛 Biome (1.9.4)
api/src/unraid-api/config/config.interface.ts
[error] 8-8: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
api/src/unraid-api/config/api-state.register.ts
[error] 13-45: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
api/src/unraid-api/config/config.registry.ts
[error] 14-30: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 19-19: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (9)
api/.env.test (1)
12-12: Looks good - new config variable added consistently.The CONFIG_MODULES_HOME variable has been added with a development-appropriate relative path that matches the pattern of other configuration paths in this file.
api/.env.development (1)
12-12: Looks good - new config variable added consistently.The CONFIG_MODULES_HOME variable has been added with a development-appropriate relative path that matches the pattern of other configuration paths in this file.
api/.env.production (1)
5-5: Looks good - new config variable added with production path.The CONFIG_MODULES_HOME variable has been added with an appropriate absolute path for the production environment.
api/.env.staging (1)
5-5: Looks good - new config variable added with staging path.The CONFIG_MODULES_HOME variable has been added with an appropriate absolute path for the staging environment, matching the production path for environment parity.
api/src/unraid-api/config/config.injection.ts (1)
1-13: Well-designed custom decorator for configuration injectionThe
InjectConfigdecorator provides a clean abstraction over NestJS's dependency injection system for configuration settings. Good use of generics to ensure type safety by constraining the feature parameter to keys of theConfigFeaturesinterface.api/src/index.ts (1)
47-48: Good practice ensuring the config directory existsCreating the config modules directory at startup with recursive option is a good practice to prevent file operation errors later in the application lifecycle.
api/src/unraid-api/config/config.interface.ts (1)
11-20: Good interface design for configuration metadataThe
ConfigMetadatainterface provides a clear structure for configuration metadata with well-documented properties. The use of Zod for schema validation is a good practice for runtime type checking.api/src/unraid-api/config/config.registry.ts (1)
10-12: Clean implementation of token creationThe
makeConfigTokenfunction is a simple and effective way to generate unique tokens for configurations.api/src/unraid-api/config/api-state.model.ts (1)
19-29: Nice design for managing default config immutability and logging.Registering the config before assigning a new logger ensures the token is always recognized, and the
structuredCloneusage effectively prevents accidental mutations tooptions.defaultConfig.
✅ Actions performedReviews paused. |
pujitm
force-pushed
the
feat/config-module
branch
from
7c6525e to
86cd4b4
Compare
pujitm
force-pushed
the
feat/config-module
branch
2 times, most recently
from
5267984 to
c9d8377
Compare
pujitm
force-pushed
the
feat/config-module
branch
from
c9d8377 to
db4f475
Compare
pujitm
force-pushed
the
feat/config-module
branch
from
2b37bc4 to
b64d4ea
Compare
pujitm
force-pushed
the
feat/config-module
branch
from
b0fdd31 to
aab0a48
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/unraid-api-plugin-connect/src/config.persistence.ts (1)
24-31: 🛠️ Refactor suggestionError handling needed for required environment variable
The
configPathgetter uses a non-null assertion (!) onPATHS_CONFIG_MODULES, but doesn't handle the case where it might be undefined.get configPath() { + const configModulesPath = this.configService.get("PATHS_CONFIG_MODULES"); + if (!configModulesPath) { + throw new Error("PATHS_CONFIG_MODULES environment variable is not defined"); + } return path.join( - this.configService.get("PATHS_CONFIG_MODULES")!, + configModulesPath, "connect.json" ); }api/src/unraid-api/config/api-state.model.ts (1)
100-104: 🛠️ Refactor suggestionConsider persisting after config update
The
updatemethod modifies the config but doesn't persist it, requiring callers to remember to callpersist()afterward. This could lead to lost changes.-update(config: Partial<T>) { +async update(config: Partial<T>) { const proposedConfig = this.options.parse({ ...this.config, ...config }); this.config = proposedConfig; - return this; + await this.persist(); + return this; }
🧹 Nitpick comments (3)
packages/unraid-api-plugin-connect/src/config.persistence.ts (2)
93-95: Enhance validation with class-validatorThe
validatemethod currently only transforms the object usingplainToInstancebut doesn't perform actual validation with class-validator decorators.private validate(config: object) { - return plainToInstance(MyServersConfig, config); + const configInstance = plainToInstance(MyServersConfig, config); + // Import and use validateSync from class-validator + // const errors = validateSync(configInstance); + // if (errors.length > 0) { + // throw new ValidationError(`Config validation failed: ${errors.toString()}`); + // } + return configInstance; }
64-86: Add comparison check to avoid unnecessary config updatesThe
persistmethod correctly checks if the config has changed before writing, but this comparison is vulnerable to errors or exceptions and doesn't provide a clean fallback.async persist(config = this.configService.get<MyServersConfig>("connect")) { try { + // Extract this logic to a helper method for better readability + const isDifferent = await this.isConfigDifferent(config); + if (!isDifferent) { + this.logger.verbose(`Config is unchanged, skipping persistence`); + return false; + } - if (isEqual(config, await this.loadConfig())) { - this.logger.verbose(`Config is unchanged, skipping persistence`); - return false; - } } catch (error) { this.logger.error(`Error loading config (will overwrite file):`, error); } const data = JSON.stringify(config, null, 2); this.logger.verbose(`Persisting config to ${this.configPath}: ${data}`); try { await writeFile(this.configPath, data); this.logger.verbose(`Config persisted to ${this.configPath}`); return true; } catch (error) { this.logger.error( `Error persisting config to '${this.configPath}':`, error ); return false; } } +// New helper method +private async isConfigDifferent(config: MyServersConfig): Promise<boolean> { + try { + const diskConfig = await this.loadConfig(); + return !isEqual(config, diskConfig); + } catch (error) { + // If we can't load the config, assume it's different + return true; + } +}api/src/unraid-api/config/api-state.model.ts (1)
71-77: Consider handling JSON parse errors explicitlyThe
parseConfigmethod doesn't explicitly handle JSON parsing errors, which could lead to confusing error messages.async parseConfig(opts: { filePath?: string } = {}): Promise<T | undefined> { const { filePath = this.filePath } = opts; if (!(await fileExists(filePath))) return undefined; - const rawConfig = JSON.parse(await readFile(filePath, 'utf8')); - return this.options.parse(rawConfig); + try { + const fileContent = await readFile(filePath, 'utf8'); + const rawConfig = JSON.parse(fileContent); + return this.options.parse(rawConfig); + } catch (error) { + if (error instanceof SyntaxError) { + throw new Error(`Config file at ${filePath} contains invalid JSON: ${error.message}`); + } + throw error; + } }
🛑 Comments failed to post (3)
api/src/index.ts (1)
18-18:
⚠️ Potential issueReorder imports to match project style conventions
The order of
PATHS_CONFIG_MODULES, environmentin the imports should be reversed to match the project's style conventions, as flagged by static analysis tools.-import { PATHS_CONFIG_MODULES, environment, PORT } from '@app/environment.js'; +import { environment, PATHS_CONFIG_MODULES, PORT } from '@app/environment.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { environment, PATHS_CONFIG_MODULES, PORT } from '@app/environment.js';🧰 Tools
🪛 GitHub Check: Build API
[failure] 18-18:
ReplacePATHS_CONFIG_MODULES,·environmentwithenvironment,·PATHS_CONFIG_MODULES🪛 GitHub Check: Test API
[failure] 18-18:
ReplacePATHS_CONFIG_MODULES,·environmentwithenvironment,·PATHS_CONFIG_MODULESpackages/unraid-api-plugin-connect/src/config.persistence.ts (1)
131-134: 🛠️ Refactor suggestion
Use async file operations consistently
The
loadConfigmethod uses synchronous file operations (existsSync,readFileSync) while other methods use asynchronous ones.private async loadConfig(configFilePath = this.configPath) { - if (!existsSync(configFilePath)) throw new Error(`Config file does not exist at '${configFilePath}'`); - return this.validate(JSON.parse(readFileSync(configFilePath, "utf8"))); + const exists = await fileExists(configFilePath); + if (!exists) throw new Error(`Config file does not exist at '${configFilePath}'`); + const content = await readFile(configFilePath, "utf8"); + return this.validate(JSON.parse(content)); }Committable suggestion skipped: line range outside the PR's diff.
api/src/unraid-api/config/api-state.model.ts (1)
45-47: 🛠️ Refactor suggestion
Add error handling for PATHS_CONFIG_MODULES environment variable
The code assumes that
PATHS_CONFIG_MODULESis defined, but should handle the case when it's not.get filePath() { + if (!PATHS_CONFIG_MODULES) { + throw new Error('PATHS_CONFIG_MODULES environment variable is not defined'); + } return join(PATHS_CONFIG_MODULES, this.fileName); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.get filePath() { if (!PATHS_CONFIG_MODULES) { throw new Error('PATHS_CONFIG_MODULES environment variable is not defined'); } return join(PATHS_CONFIG_MODULES, this.fileName); }
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/unraid-api-plugin-connect/src/config.persistence.ts (2)
24-31: 🛠️ Refactor suggestionConsider adding error handling for CONFIG_MODULES_HOME path
The
configPathgetter doesn't include runtime validation for when the environment variable might be undefined. This could lead to unexpected errors during execution.get configPath() { // PATHS_CONFIG_MODULES is a required environment variable. // It is the directory where custom config files are stored. + const configModulesPath = this.configService.get("PATHS_CONFIG_MODULES"); + if (!configModulesPath) { + throw new Error("PATHS_CONFIG_MODULES is not defined"); + } return path.join( - this.configService.get("PATHS_CONFIG_MODULES")!, + configModulesPath, "connect.json" ); }
131-134: 🛠️ Refactor suggestionAdd error handling for JSON parsing
The
loadConfigmethod doesn't handle JSON parsing errors, which could occur if the file content isn't valid JSON.private async loadConfig(configFilePath = this.configPath) { if (!existsSync(configFilePath)) throw new Error(`Config file does not exist at '${configFilePath}'`); - return this.validate(JSON.parse(readFileSync(configFilePath, "utf8"))); + try { + const fileContent = readFileSync(configFilePath, "utf8"); + const parsedConfig = JSON.parse(fileContent); + return this.validate(parsedConfig); + } catch (error) { + throw new Error(`Failed to parse config file at '${configFilePath}': ${error.message}`); + } }
🧹 Nitpick comments (1)
packages/unraid-api-plugin-connect/src/config.persistence.ts (1)
64-69: Improve error handling for config comparisonThe method attempts to compare with a file that might not exist yet, relying on the catch block to handle that scenario. This approach works but could be made more explicit.
async persist(config = this.configService.get<MyServersConfig>("connect")) { try { - if (isEqual(config, await this.loadConfig())) { - this.logger.verbose(`Config is unchanged, skipping persistence`); - return false; - } + if (existsSync(this.configPath)) { + const diskConfig = await this.loadConfig(); + if (isEqual(config, diskConfig)) { + this.logger.verbose(`Config is unchanged, skipping persistence`); + return false; + } + } else { + this.logger.verbose(`Config file doesn't exist yet, will create it`); + } } catch (error) { this.logger.error(`Error loading config (will overwrite file):`, error); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (9)
api/.env.development(2 hunks)api/.env.production(1 hunks)api/.env.staging(1 hunks)api/.env.test(1 hunks)api/src/environment.ts(2 hunks)api/src/index.ts(3 hunks)api/src/unraid-api/config/api-state.model.ts(1 hunks)packages/unraid-api-plugin-connect/src/config.persistence.ts(1 hunks)packages/unraid-api-plugin-generator/src/templates/config.persistence.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- api/.env.production
- api/.env.test
- api/.env.development
- api/.env.staging
- api/src/environment.ts
- api/src/index.ts
- packages/unraid-api-plugin-generator/src/templates/config.persistence.ts
- api/src/unraid-api/config/api-state.model.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Deploy Plugin / Build and Deploy Plugin
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/unraid-api-plugin-connect/src/config.persistence.ts (2)
25-32: 🛠️ Refactor suggestionAdd error handling for missing CONFIG_MODULES_HOME
The configPath getter uses a non-null assertion (
!) which could lead to cryptic errors if the environment variable is undefined.get configPath() { // PATHS_CONFIG_MODULES is a required environment variable. // It is the directory where custom config files are stored. + const configModulesPath = this.configService.get("PATHS_CONFIG_MODULES"); + if (!configModulesPath) { + throw new Error("PATHS_CONFIG_MODULES environment variable is not defined"); + } return path.join( - this.configService.get("PATHS_CONFIG_MODULES")!, + configModulesPath, "connect.json" ); }
139-142: 🛠️ Refactor suggestionAdd try-catch for JSON parsing
The loadConfig method doesn't handle JSON parsing errors, which could occur with malformed config files.
private async loadConfig(configFilePath = this.configPath) { if (!existsSync(configFilePath)) throw new Error(`Config file does not exist at '${configFilePath}'`); - return this.validate(JSON.parse(readFileSync(configFilePath, "utf8"))); + try { + const fileContent = readFileSync(configFilePath, "utf8"); + const configData = JSON.parse(fileContent); + return this.validate(configData); + } catch (error) { + throw new Error(`Failed to parse config file at '${configFilePath}': ${error.message}`); + } }
🧹 Nitpick comments (3)
packages/unraid-api-plugin-connect/src/config.persistence.ts (3)
94-103: Use validateSync for synchronous validationThe validate method is marked as async but validateOrReject returns a Promise that might not need to be awaited in all contexts.
Consider using validateSync for synchronous contexts and keeping validateOrReject for async contexts:
- private async validate(config: object) { + private validate(config: object) { let instance: MyServersConfig; if (config instanceof MyServersConfig) { instance = config; } else { instance = plainToInstance(MyServersConfig, config, { enableImplicitConversion: true }); } - await validateOrReject(instance); + validateSync(instance); return instance; } + private async validateAsync(config: object) { + const instance = this.validate(config); + await validateOrReject(instance); + return instance; + }
152-157: Add comment explaining the demo timestamp purposeThe demo property added in migrateLegacyConfig doesn't have documentation explaining its purpose.
Based on the prior feedback, it's best to document that this is for demonstration purposes only:
private async migrateLegacyConfig() { const legacyConfig = await this.parseLegacyConfig(); this.configService.set("connect", { + // This timestamp is for demonstration purposes only + // It keeps demo environments looking fresh with current dates demo: new Date().toISOString(), ...legacyConfig, }); }
65-87: Extract disk config loading into helper methodThe persist method has logic that could be extracted to improve readability and reusability.
async persist(config = this.configService.get<MyServersConfig>("connect")) { try { - if (isEqual(config, await this.loadConfig())) { + if (await this.isConfigUnchanged(config)) { this.logger.verbose(`Config is unchanged, skipping persistence`); return false; } } catch (error) { this.logger.error(`Error loading config (will overwrite file):`, error); } const data = JSON.stringify(config, null, 2); this.logger.verbose(`Persisting config to ${this.configPath}: ${data}`); try { await writeFile(this.configPath, data); this.logger.verbose(`Config persisted to ${this.configPath}`); return true; } catch (error) { this.logger.error( `Error persisting config to '${this.configPath}':`, error ); return false; } } + + /** + * Checks if the provided config is equal to the one on disk + * @param config The config to compare with the on-disk version + * @returns True if configs are equal, false otherwise + */ + private async isConfigUnchanged(config: MyServersConfig): Promise<boolean> { + try { + const diskConfig = await this.loadConfig(); + return isEqual(config, diskConfig); + } catch (error) { + this.logger.debug(`Failed to compare configs: ${error.message}`); + return false; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/config.persistence.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/unraid-api-plugin-connect/src/config.persistence.ts (1)
Learnt from: pujitm
PR: unraid/api#1352
File: packages/unraid-api-plugin-connect/src/config.persistence.ts:143-149
Timestamp: 2025-04-22T14:41:19.582Z
Learning: The `demo` property in the config.persistence.ts file's migrateLegacyConfig method is used for demonstration purposes, adding a current timestamp to keep demo environments looking fresh.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Refactor
Documentation