refactor(api): use nestjs modules as the api plugin interface #1321
Conversation
WalkthroughThis pull request refines the plugin system for the API by adding a new dependency ( Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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: 4
🧹 Nitpick comments (5)
api/src/unraid-api/plugin/plugin.interface.ts (1)
41-41: Config property implementation looks good but consider type safety.Adding the optional config function aligns with the PR objectives. The implementation matches how it's used in the health plugin.
Consider defining a more specific schema for the configuration return type instead of using
z.array(z.any()). This would improve type safety while still allowing flexibility.- config: z.function().returns(z.array(z.any())).optional(), + config: z.function().returns(z.array(z.union([z.string(), z.record(z.string(), z.any())]))).optional(),The suggested change would enforce the pattern seen in the health plugin implementation where the first element is a string identifier and the second element is a configuration object.
packages/unraid-api-plugin-connect/tsconfig.json (1)
1-19: TypeScript configuration looks appropriate for a NestJS plugin.The configuration enables the necessary features for NestJS development, including decorators and metadata emission. The target and module settings are reasonable for compatibility.
Consider evaluating if ESM would be a better fit for your module system since it's the more modern approach and aligns with the
"type": "module"setting in the API package.json:- "module": "commonjs", + "module": "esnext",Also ensure you're consistent with trailing newlines at the end of files:
- } + }packages/unraid-api-plugin-connect/index.js (1)
43-46: Consider using only the logger for consistency.While the initialization message is helpful, using both
this.logger.logandconsole.logfor similar purposes creates inconsistency.onModuleInit() { this.logger.log("Connect plugin initialized"); - console.log("Connect plugin initialized", this.configService.get('connect')); + this.logger.log(`Connect plugin config: ${JSON.stringify(this.configService.get('connect'))}`); }api/src/unraid-api/plugin/plugin.module.ts (1)
2-2: Importing ConfigModule but not using it.The ConfigModule is imported but not used in the current implementation.
Either use the ConfigModule or remove the unused import.
packages/unraid-api-plugin-connect/package.json (1)
7-7: Improve the test script.The current test script just outputs an error. Consider implementing actual tests or using a placeholder that doesn't exit with an error code.
- "test": "echo \"Error: no test specified\" && exit 1" + "test": "echo \"No tests implemented yet\""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
api/package.json(3 hunks)api/src/unraid-api/plugin/plugin.interface.ts(1 hunks)api/src/unraid-api/plugin/plugin.module.ts(2 hunks)api/src/unraid-api/plugin/plugin.service.ts(2 hunks)packages/unraid-api-plugin-connect/index.js(1 hunks)packages/unraid-api-plugin-connect/package.json(1 hunks)packages/unraid-api-plugin-connect/tsconfig.json(1 hunks)packages/unraid-api-plugin-health/index.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/unraid-api-plugin-connect/index.js (1)
api/src/unraid-api/plugin/plugin.module.ts (1)
Module(6-27)
api/src/unraid-api/plugin/plugin.service.ts (1)
packages/unraid-api-plugin-connect/index.js (1)
config(24-26)
🪛 GitHub Check: Test API
api/src/unraid-api/plugin/plugin.service.ts
[failure] 169-169:
Replace ·module.registerGraphQLTypeDefs() with ⏎············module.registerGraphQLTypeDefs()⏎········
🪛 GitHub Check: Build API
api/src/unraid-api/plugin/plugin.service.ts
[failure] 169-169:
Replace ·module.registerGraphQLTypeDefs() with ⏎············module.registerGraphQLTypeDefs()⏎········
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (14)
packages/unraid-api-plugin-health/index.js (1)
8-10: Implementation of the new config method looks good.The addition of the config method aligns with the plugin interface changes. It returns an array with a string identifier and a configuration object.
Minor suggestion: add a space after the comma in the returned array for better readability.
- return ['health', { demo: true}] + return ['health', { demo: true }]api/package.json (3)
65-65: Adding @nestjs/config dependency is appropriate.This dependency will provide configuration management capabilities for the NestJS application, which aligns with the PR's goal of implementing a plugin configuration interface.
135-135: New plugin dependency looks good.Adding the connect plugin as a dependency makes sense for the feature being implemented. Using workspace reference ensures the local version is used during development.
197-197: Health plugin added as a dev dependency.Adding the health plugin as a dev dependency is appropriate since it appears to be used for demonstration or testing purposes.
packages/unraid-api-plugin-connect/index.js (7)
1-4: Import structure looks good.The necessary NestJS modules and decorators are properly imported for creating a NestJS-based plugin module.
5-5: Good use of an adapter type constant.This constant clearly identifies the plugin adapter type as 'nestjs', which will be useful for the plugin system to understand how to integrate with this module.
7-13: GraphQL type definitions look correct.The
registerGraphQLTypeDefsfunction correctly returns a GraphQL schema string with a health query.
15-22: Health resolver implementation is straightforward.The GraphQL resolver correctly implements a simple health check endpoint that returns a string.
24-26: Configuration is properly defined.Using
registerAsis the correct approach for isolating the plugin's configuration under the "connect" namespace.
29-32: Module structure follows NestJS best practices.The module correctly imports ConfigModule and provides the HealthResolver.
33-41: Constructor properly injects the ConfigService.The dependency injection is correctly set up with the appropriate Inject decorator.
api/src/unraid-api/plugin/plugin.service.ts (1)
229-233: Good implementation of plugin configuration retrieval.The
getConfigmethod correctly loads all plugins and filters out undefined configurations. This is a clean approach to gather configurations from multiple plugins.packages/unraid-api-plugin-connect/package.json (2)
1-12: Package metadata looks good.The basic package configuration with name, version, entry point, and type is well-defined.
20-26: Peer dependencies match dev dependencies correctly.The peer dependencies correctly mirror the dev dependencies, which is a good practice for plugin modules.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
pujitm
changed the title
pujitm
force-pushed
the
feat/config-extraction
branch
from
6805a4b to
0d57c26
Compare
pujitm
changed the title
pujitm
changed the title
pujitm
changed the title
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
pujitm
force-pushed
the
feat/config-extraction
branch
from
d6ad894 to
0d57c26
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/unraid-api-plugin-connect/tsconfig.json (1)
16-17: Consider expanding the include pattern.The include pattern is currently limited to only "index.ts". This works if the package is simple, but might be too restrictive if you add more files in the future.
- "include": ["index.ts"], + "include": ["*.ts", "src/**/*.ts"],packages/unraid-api-plugin-health/index.ts (1)
15-17: Consider making the health check more meaningfulThis implementation returns a static string. For a more useful health check, consider adding actual system health verification logic.
@Query(() => String) health() { - return 'OK'; + // Check any necessary system components + const systemHealthy = true; // Replace with actual checks + return systemHealthy ? 'OK' : 'System degraded'; }packages/unraid-api-plugin-connect/index.ts (1)
42-43: Remove redundant loggingThere's duplicate logging with both Logger.log and console.log. Prefer using only the Logger service for consistency.
onModuleInit() { this.logger.log("Connect plugin initialized"); - console.log("Connect plugin initialized", this.configService.get('connect')); + this.logger.log(`Connect config: ${JSON.stringify(this.configService.get('connect'))}`); }packages/unraid-api-plugin-connect/package.json (2)
10-10: Consider adding a proper test script.Currently, the test script logs an error and exits. Adding tests or removing the placeholder script can improve clarity.
17-17: Description mismatch.This mentions "Example Health plugin" while the plugin name is "unraid-api-plugin-connect." Confirm that the description is accurate or consider updating it for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
api/package.json(1 hunks)api/src/unraid-api/app/app.module.ts(1 hunks)api/src/unraid-api/cli/cli.module.ts(3 hunks)api/src/unraid-api/graph/graph.module.ts(2 hunks)api/src/unraid-api/plugin/plugin.interface.ts(2 hunks)api/src/unraid-api/plugin/plugin.module.ts(1 hunks)api/src/unraid-api/plugin/plugin.service.ts(1 hunks)packages/unraid-api-plugin-connect/index.ts(1 hunks)packages/unraid-api-plugin-connect/package.json(1 hunks)packages/unraid-api-plugin-connect/tsconfig.json(1 hunks)packages/unraid-api-plugin-health/index.js(0 hunks)packages/unraid-api-plugin-health/index.ts(1 hunks)packages/unraid-api-plugin-health/package.json(1 hunks)packages/unraid-api-plugin-health/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/unraid-api-plugin-health/index.js
✅ Files skipped from review due to trivial changes (1)
- packages/unraid-api-plugin-health/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- api/package.json
🧰 Additional context used
🧬 Code Definitions (2)
api/src/unraid-api/graph/graph.module.ts (1)
api/src/graphql/schema/loadTypesDefs.ts (1)
loadTypeDefs(5-28)
api/src/unraid-api/plugin/plugin.service.ts (3)
api/src/unraid-api/plugin/plugin.interface.ts (2)
ApiNestPluginDefinition(66-66)apiNestPluginSchema(32-64)api/src/utils.ts (1)
batchProcess(60-73)api/src/environment.ts (1)
getPackageJson(36-42)
🪛 Biome (1.9.4)
api/src/unraid-api/plugin/plugin.module.ts
[error] 30-47: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cloudflare Pages
- GitHub Check: Cloudflare Pages
🔇 Additional comments (21)
packages/unraid-api-plugin-health/package.json (5)
4-4: Updated main entry point to compiled output.The main entry point has been correctly changed to point to the compiled TypeScript output.
6-8: Appropriate files configuration for npm packaging.The "files" array properly restricts the package to only include the compiled output, which is a best practice for TypeScript packages.
10-12: Good build workflow implementation.The build and prepare scripts ensure that TypeScript code is properly compiled before packaging, following npm best practices.
26-32: Appropriate peer dependencies configuration.Peer dependencies are correctly specified, ensuring that consuming projects have compatible NestJS versions.
18-25:❓ Verification inconclusive
NestJS dependencies properly declared.
The devDependencies section correctly includes all necessary NestJS packages for development.
One observation - TypeScript version ^5.8.2 appears to be a very recent version. Please verify this is the intended version and is compatible with your environment.
🌐 Web query:
What is the latest stable version of TypeScript as of April 2024?💡 Result:
TypeScript 5.4 was the latest stable version as of April 2024, released on March 6, 2024[2][7]. This version introduced features like preserved narrowing in closures after last assignments, the
NoInferutility type, support for ECMAScript'sObject.groupByandMap.groupBy, and improvements to module resolution settings[2][7].TypeScript 5.5 entered beta in April 2024 with enhancements like inferred type predicates and regular expression syntax checking, but its stable release occurred later on June 20, 2024[1][4].
Citations:
- 1: https://effectivetypescript.com/2024/07/02/ts-55/
- 2: https://laststance.io/articles/All-TypeScript-release-blog-list
- 3: https://devblogs.microsoft.com/typescript/announcing-typescript-5-8/
- 4: https://www.allthingstypescript.dev/p/april-2024-monthly-newsletter-typescript
- 5: https://github.com/microsoft/typescript/wiki/roadmap
- 6: https://blog.jetbrains.com/webstorm/2024/02/js-and-ts-trends-2024/
- 7: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-4.html
- 8: https://devblogs.microsoft.com/typescript/announcing-typescript-5-5/
- 9: https://devblogs.microsoft.com/typescript/category/typescript/page/2/
- 10: https://devblogs.microsoft.com/typescript/
Action Required: Confirm TypeScript Version Compatibility
The devDependencies in
packages/unraid-api-plugin-health/package.jsonnow properly list the required NestJS packages. However, the declared TypeScript version (^5.8.2) is noticeably newer than the latest stable release (5.4) as of April 2024. Please verify that using TypeScript 5.8.2 is intentional and fully compatible with your environment and dependencies.api/src/unraid-api/app/app.module.ts (1)
50-50: Updated plugin registration method.Changed from
registerPlugins()toregister(), which aligns with the new NestJS module-based plugin system.packages/unraid-api-plugin-connect/tsconfig.json (1)
1-15: Appropriate TypeScript configuration for NestJS.The TypeScript configuration is well-structured for a NestJS module with:
- Modern JavaScript support (ESNext)
- Necessary decorator settings for NestJS
- Strict type checking enabled
- Declaration files and source maps generated
api/src/unraid-api/plugin/plugin.interface.ts (3)
26-29: Good utility for NestJS module validation.The
isClassutility function provides a simple but effective way to validate that a value is a class constructor, which is essential for NestJS modules.
31-64: Well-designed NestJS plugin schema.The new plugin schema effectively enforces the NestJS module structure with:
- Clear adapter specification
- Optional ApiModule and CliModule properties with proper validation
- Optional GraphQL schema extension
- Custom validation rules to ensure valid configurations
The validation logic is particularly good, ensuring that at least one module type is defined and that GraphQL extensions have an ApiModule.
66-66: Clean type export for plugin definition.The exported type properly maps to the Zod schema, providing type safety for plugin implementations.
api/src/unraid-api/graph/graph.module.ts (2)
35-36: The change to static method call improves module loadingThe refactoring from injected PluginService to a static method call streamlines the GraphQL module configuration and reduces dependencies.
52-52: Schema integration looks goodThe approach for integrating plugin schemas is clean and maintainable.
packages/unraid-api-plugin-health/index.ts (1)
1-31: Good implementation of the NestJS module approachThe health plugin effectively demonstrates the new NestJS module structure for plugins. It provides a simple health check endpoint via GraphQL and properly initializes with logging.
api/src/unraid-api/cli/cli.module.ts (2)
1-1: Good simplification of importsThe imports have been cleaned up to only include what's necessary.
Also applies to: 27-27
59-63: Cleaner module structureThe refactored CLI module is now more declarative and follows NestJS best practices by using the module system for plugin registration.
packages/unraid-api-plugin-connect/index.ts (2)
1-47: Good implementation of the NestJS module with configurationThe Connect plugin effectively demonstrates the new NestJS module structure with proper configuration integration and dependency injection.
13-20: Health resolver seems duplicatedThere's a health resolver in both this plugin and the health plugin. Consider differentiating them or using a more specific name for this one.
Are both health resolvers intentional? If so, they should have different GraphQL field names to avoid conflicts.
api/src/unraid-api/plugin/plugin.module.ts (2)
10-17: Looks good for plugin registration.This approach properly logs the discovered modules, and the usage of
PluginService.getPlugins()is consistent.
29-47: Update the returned 'module' property and consider flattening static members.It appears that
PluginCliModule.register()is returning a dynamic module withmodule: PluginModule. This likely should bemodule: PluginCliModuleto avoid confusion. Also, the static analysis warns about classes containing only static members; converting this class to simple functions or merging it with an existing module might be more idiomatic.[flag_critical_issue, suggest_optional_refactor]
Apply the following changes to fix the mismatch:- module: PluginModule, + module: PluginCliModule,🧰 Tools
🪛 Biome (1.9.4)
[error] 30-47: 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/plugin/plugin.service.ts (2)
20-41: GraphQL schema parsing is well-implemented.Using
parse()ensures immediate detection of invalid syntax, and the logging is appropriately detailed. Good job.
43-81: Batch plugin import logic.The
importPluginsandlistPluginsflow is clean, with robust error handling and partial success/failure tracking. Looks good overall.
pujitm
force-pushed
the
feat/config-extraction
branch
from
0d57c26 to
f7678fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/unraid-api-plugin-health/package.json (2)
17-17: Fix JSON formattingThe description field shouldn't end with a comma.
- "description": "Example Health plugin for Unraid API", + "description": "Example Health plugin for Unraid API"
18-32: Consider using version ranges for peerDependenciesWhile the current setup works, it's common practice to use version ranges (e.g.,
"^11.0.0"instead of"^11.0.11") for peerDependencies to provide more flexibility for consumers of your package.api/src/unraid-api/plugin/plugin.service.ts (3)
13-13: Initializepluginscautiously.
Storing a lazy-initialized promise for plugin imports is a reasonable approach. However, if the import fails or only partially succeeds, subsequent calls reuse the same rejected or partial promise. Consider whether a re-initialization or error recovery strategy is needed.
20-41: Potential single point of failure in GraphQL schema import.
A single invalid GraphQL schema extension throws an error for all schemas. Consider handling partial failures more gracefully by skipping invalid plugins if that aligns with your project’s requirements.
64-79: Refine the condition for empty plugin lists.
Because an empty array is truthy,if (!plugins)never executes. Use a length check if the intent is to log a warning when no plugins are found.Below is an example fix to log a warning for an empty list:
-if (!plugins) { +if (!plugins.length) { PluginService.logger.warn('Could not load dependencies from the Unraid-API package.json'); return []; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
api/package.json(1 hunks)api/src/unraid-api/app/app.module.ts(1 hunks)api/src/unraid-api/cli/cli.module.ts(3 hunks)api/src/unraid-api/graph/graph.module.ts(1 hunks)api/src/unraid-api/plugin/plugin.interface.ts(2 hunks)api/src/unraid-api/plugin/plugin.module.ts(1 hunks)api/src/unraid-api/plugin/plugin.service.ts(1 hunks)packages/unraid-api-plugin-connect/index.ts(1 hunks)packages/unraid-api-plugin-connect/package.json(1 hunks)packages/unraid-api-plugin-connect/tsconfig.json(1 hunks)packages/unraid-api-plugin-health/index.js(0 hunks)packages/unraid-api-plugin-health/index.ts(1 hunks)packages/unraid-api-plugin-health/package.json(1 hunks)packages/unraid-api-plugin-health/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/unraid-api-plugin-health/index.js
🚧 Files skipped from review as they are similar to previous changes (8)
- api/package.json
- api/src/unraid-api/app/app.module.ts
- packages/unraid-api-plugin-connect/tsconfig.json
- packages/unraid-api-plugin-health/index.ts
- packages/unraid-api-plugin-health/tsconfig.json
- api/src/unraid-api/graph/graph.module.ts
- packages/unraid-api-plugin-connect/index.ts
- packages/unraid-api-plugin-connect/package.json
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/plugin/plugin.service.ts (1)
api/src/unraid-api/plugin/plugin.interface.ts (2)
ApiNestPluginDefinition(66-66)apiNestPluginSchema(32-64)
🪛 Biome (1.9.4)
api/src/unraid-api/plugin/plugin.module.ts
[error] 30-47: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (12)
api/src/unraid-api/plugin/plugin.interface.ts (3)
26-29: Good implementation for class detectionThe
isClassfunction correctly detects class constructors by checking if a value is a function and if its string representation starts with 'class'.
31-64: Well-designed schema with thorough validationThe new
apiNestPluginSchemaproperly validates NestJS modules with clear validation rules:
- Enforces 'nestjs' adapter type
- Validates that ApiModule and CliModule are class constructors
- Requires at least one of ApiModule or CliModule
- Ensures ApiModule is defined when graphqlSchemaExtension is provided
The validation logic is robust and provides helpful error messages.
66-66: LGTM: Type definition looks goodThe exported type correctly infers the schema definition.
api/src/unraid-api/cli/cli.module.ts (3)
1-1: LGTM: Imports simplifiedUnnecessary imports have been removed, which helps reduce clutter.
27-27: LGTM: Updated import for new module structureThe import of
PluginCliModulealigns with the new plugin architecture.
59-63: LGTM: Simplified module structureThe module definition now properly delegates plugin registration to the dedicated
PluginCliModule, which aligns with the separation of API and CLI concerns.api/src/unraid-api/plugin/plugin.module.ts (1)
10-26: LGTM: Improved plugin registration logicThe renamed
registermethod effectively filters and imports API modules with good logging to track loaded plugins.packages/unraid-api-plugin-health/package.json (2)
4-8: LGTM: Updated package entry point and distribution configurationThe changes correctly update the main entry point to the compiled output and specify the distribution files.
10-13: LGTM: Added build scriptsThe new scripts properly set up TypeScript compilation and prepare hooks.
api/src/unraid-api/plugin/plugin.service.ts (3)
5-7: Good usage of typed imports.
These imports correctly reference the schema and type definitions, ensuring type safety for plugins.
16-18: Lazy-loading plugins.
This pattern ensures plugins are only imported once. Just be mindful of scenarios where plugins fail midway and whether retry logic or error handling is desired in subsequent calls.
43-62: Validation and logging are well-handled.
This function neatly imports each plugin and applies schema validation, logging any errors without halting the entire import process. This design seems robust for plugin-based architectures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/unraid-api-plugin-connect/index.ts (4)
17-19: Consider implementing actual health check logic.The current implementation uses a static string as a placeholder. For a more useful health check, consider implementing logic that verifies system status or connectivity.
- health() { - // You can replace the return value with your actual health check logic - return 'I am healthy!'; - } + health() { + try { + // Perform actual health check logic here + return 'I am healthy!'; + } catch (error) { + return `Health check failed: ${error.message}`; + } + }
43-43: Use logger consistently instead of console.log.Using direct console.log statements is inconsistent with the logger pattern established earlier in the file.
- console.log("Connect plugin initialized", this.configService.get('connect')); + this.logger.log(`Connect plugin initialized ${JSON.stringify(this.configService.get('connect'))}`);
34-36: Simplify JSDoc comment.The type annotation in the JSDoc is redundant since TypeScript already provides type information through the parameter declaration.
- /** - * @param {ConfigService} configService - */ + /** + * @param configService The configuration service + */
22-24: Consider documenting the purpose of the config properties.Adding documentation for the configuration properties would help other developers understand their purpose and usage.
const config = registerAs("connect", () => ({ - demo: true, + /** + * Flag indicating if this is a demo instance + */ + demo: true, }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
packages/unraid-api-plugin-connect/index.ts(1 hunks)packages/unraid-api-plugin-health/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/unraid-api-plugin-health/index.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/unraid-api-plugin-connect/index.ts (2)
packages/unraid-api-plugin-health/index.ts (4)
adapter(4-4)graphqlSchemaExtension(6-10)Resolver(12-18)ApiModule(31-31)api/src/unraid-api/cli/cli.module.ts (1)
Module(59-63)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
packages/unraid-api-plugin-connect/index.ts (2)
5-11: Health check endpoint matches the pattern in unraid-api-plugin-health.The adapter and GraphQL schema structure aligns well with the health plugin pattern seen in other parts of the application.
47-48: Export naming follows the expected plugin pattern.The export of
ApiModulecorrectly follows the convention established in other plugins, making it consistent for the plugin system to register.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/src/unraid-api/plugin/plugin.service.ts (3)
13-13: Please update the type declaration to be more preciseThe type declaration for
pluginscould be more specific. Consider usingPromise<ApiNestPluginDefinition[]> | undefinedinstead of justPromise<ApiNestPluginDefinition[]>.- private static plugins: Promise<ApiNestPluginDefinition[]> | undefined; + private static plugins: Promise<ApiNestPluginDefinition[]> | undefined;
20-41: Consider avoiding non-null assertions when possibleIn line 27 and 35, you're using non-null assertions (
!). While they're likely safe due to your filtering and schema validation, it's better to avoid them when possible.- const schema = await plugin.graphqlSchemaExtension!(); + // We've already filtered for plugins with graphqlSchemaExtension + const schema = await plugin.graphqlSchemaExtension(); - `Error parsing GraphQL schema from ${plugin.ApiModule!.name}: ${JSON.stringify(error, null, 2)}` + `Error parsing GraphQL schema from ${plugin.ApiModule?.name || 'unknown'}: ${JSON.stringify(error, null, 2)}`
1-80: Consider design implications of using static methodsThe service uses static methods and a static property for plugins, which is unusual for a NestJS injectable service. While this might be intentional for global state management, consider the implications:
- Harder to test due to shared state
- Potential issues with multiple instances in different contexts
- Makes dependency injection less effective
Consider refactoring to use instance methods with a singleton pattern or a properly injected configuration.
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-80: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/unraid-api/graph/graph.module.ts(1 hunks)api/src/unraid-api/plugin/plugin.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/graph.module.ts
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/plugin/plugin.service.ts (3)
api/src/unraid-api/plugin/plugin.interface.ts (2)
ApiNestPluginDefinition(66-66)apiNestPluginSchema(32-64)api/src/utils.ts (1)
batchProcess(60-73)api/src/environment.ts (1)
getPackageJson(36-42)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
api/src/unraid-api/plugin/plugin.service.ts (1)
43-46:⚠️ Potential issueFix the early return logic in importPlugins
The early return logic has an issue:
PluginService.pluginsis a Promise, but you're returning it directly without awaiting it.- private static async importPlugins() { - if (PluginService.plugins) { - return PluginService.plugins; + private static async importPlugins() { + if (PluginService.plugins) { + return await PluginService.plugins;Likely an incorrect or invalid review comment.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/unraid-api/plugin/plugin.service.ts (1)
34-36: Consider using a helper method instead of non-null assertionRather than using the non-null assertion (
!) forplugin.ApiModule, consider creating a helper method to filter plugins that have bothgraphqlSchemaExtensionandApiModuleproperties. This would make the code more type-safe.- PluginService.logger.error( - `Error parsing GraphQL schema from ${plugin.ApiModule!.name}: ${JSON.stringify(error, null, 2)}` - ); + PluginService.logger.error( + `Error parsing GraphQL schema from ${plugin.ApiModule.name}: ${JSON.stringify(error, null, 2)}` + );You could add a filter function like:
function pluginHasApiModuleAndSchema( plugin: ApiNestPluginDefinition ): plugin is SetRequired<ApiNestPluginDefinition, 'graphqlSchemaExtension' | 'ApiModule'> { return plugin.graphqlSchemaExtension !== undefined && plugin.ApiModule !== undefined; }
🧹 Nitpick comments (3)
api/src/unraid-api/plugin/plugin.service.ts (3)
44-47: Remove redundant plugins checkThe check for
PluginService.pluginshere is redundant sincegetPlugins()already performs this check. This method is only called fromgetPlugins()which already has the nullish coalescing assignment.private static async importPlugins() { - if (PluginService.plugins) { - return PluginService.plugins; - } const pluginPackages = await PluginService.listPlugins();
59-60: Fix typo in variable nameThere's a typo in the property name
errorOccured. It should beerrorOccurred.- if (plugins.errorOccured) { + if (plugins.errorOccurred) { PluginService.logger.warn(`Failed to load ${plugins.errors.length} plugins. Ignoring them.`); }
69-73: Remove unnecessary null checkThe
getPackageJson()function already ensures thatdependenciesis present through its return typeSetRequired<PackageJson, 'version' | 'dependencies'>. This check will never be true.const { dependencies } = getPackageJson(); - if (!dependencies) { - PluginService.logger.warn('Unraid-API dependencies not found; skipping plugins.'); - return []; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/unraid-api/graph/graph.module.ts(1 hunks)api/src/unraid-api/plugin/plugin.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/graph.module.ts
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/plugin/plugin.service.ts (3)
api/src/unraid-api/plugin/plugin.interface.ts (2)
ApiNestPluginDefinition(66-66)apiNestPluginSchema(32-64)api/src/utils.ts (1)
batchProcess(60-73)api/src/environment.ts (1)
getPackageJson(36-42)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Deploy Plugin / Build and Deploy Plugin
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
api/src/unraid-api/plugin/plugin.service.ts (2)
21-42: Great implementation of GraphQL schema collectionThe method effectively handles schema collection from plugins with appropriate error handling and validation. The use of
batchProcessfor concurrent processing improves performance.
65-79: Good plugin discovery mechanismThe implementation of
listPlugins()provides a clean way to discover plugins by looking at package dependencies. The type predicate ensures type safety.
Changes plugin interface to expect Nest modules as opposed to a custom plain JS object.
Summary by CodeRabbit
New Features
Refactor
CliModuleandPluginServiceto reduce complexity and enhance maintainability.Chores
package.jsonandtsconfig.jsonfiles for theunraid-api-plugin-connectandunraid-api-plugin-healthprojects.