chore: begin moving paths to nest #1329
Conversation
WalkthroughThis PR introduces a centralized configuration management system by adding a new injectable Changes
Sequence Diagram(s)sequenceDiagram
participant Service
participant PathsModule
participant PathsConfig
Service->>PathsModule: Requests configuration
PathsModule->>PathsConfig: Provides singleton instance
Service->>PathsConfig: Accesses file path properties
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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
🔭 Outside diff range comments (2)
api/src/unraid-api/rest/rest.service.ts (1)
41-41:⚠️ Potential issueInconsistent path retrieval method
While the constructor initializes
logPathusing the injectedPathsConfig, thegetLogsmethod still uses the old getter approach withgetters.paths()['log-base']. This creates inconsistency in path management.Apply this change to use the private
logPathproperty:- const logPath = getters.paths()['log-base']; + const logPath = this.logPath;api/src/unraid-api/auth/cookie.service.ts (1)
36-44: 🛠️ Refactor suggestionInconsistency in path retrieval methods.
The
defaultOptsstatic method still usesgetters.paths()['auth-sessions']while the constructor usespaths.authSessions. This could lead to confusion or incorrect behavior if these values diverge.static defaultOpts(): SessionCookieConfig { return { namePrefix: 'unraid_', - sessionDir: getters.paths()['auth-sessions'], + sessionDir: PathsConfig.getInstance().authSessions, secure: true, httpOnly: true, sameSite: 'lax', }; }
🧹 Nitpick comments (25)
api/src/core/utils/misc/parse-state-file.ts (3)
4-7: Consider adding error handlingThe function should include error handling to gracefully manage situations where the file doesn't exist or contains invalid content.
export const parseStateFile = async (path: string) => { + try { const content = await fs.readFile(path, 'utf8'); return parse(content); + } catch (error) { + throw new Error(`Failed to parse state file at ${path}: ${error.message}`); + } };
4-4: Add return type for clarityAdding an explicit return type would make the function signature more informative.
-export const parseStateFile = async (path: string) => { +export const parseStateFile = async (path: string): Promise<Record<string, any>> => {
7-7: Remove trailing whitespaceThere's an extra space after the semicolon.
-}; +};api/src/core/utils/clients/emhttpd.ts (3)
3-5: Consider using dependency injectionRather than using a singleton pattern with
PathsConfig.getInstance(), consider using dependency injection which aligns better with NestJS principles.-export const createEmhttpdClient = () => { - const paths = PathsConfig.getInstance(); +export const createEmhttpdClient = (paths: PathsConfig = PathsConfig.getInstance()) => {
3-3: Add function documentationAdd JSDoc comments to describe the purpose and usage of this function.
+/** + * Creates a client for interacting with the emhttpd service + * @returns An emhttpd client instance + */ export const createEmhttpdClient = () => {
7-7: Remove trailing whitespaceThere's an extra space after the semicolon.
-}; +};api/src/core/utils/clients/ssh.ts (3)
3-5: Consider using dependency injectionRather than using a singleton pattern with
PathsConfig.getInstance(), consider using dependency injection which aligns better with NestJS principles.-export const createSshClient = () => { - const paths = PathsConfig.getInstance(); +export const createSshClient = (paths: PathsConfig = PathsConfig.getInstance()) => {
3-3: Add function documentationAdd JSDoc comments to describe the purpose and usage of this function.
+/** + * Creates a client for SSH connections + * @returns An SSH client instance + */ export const createSshClient = () => {
7-7: Remove trailing whitespaceThere's an extra space after the semicolon.
-}; +};api/src/unraid-api/config/paths.module.ts (1)
9-9: Remove trailing whitespaceThere's an extra space after the closing brace.
-export class PathsModule {} +export class PathsModule {}api/src/config/paths.module.ts (1)
1-9: The module setup looks good but requires a few styling fixes.The implementation of a global PathsModule is a good approach for providing the PathsConfig throughout the application. The module structure follows NestJS best practices with proper provider and export definitions.
According to the static analysis, there are a few styling issues to fix:
import { Global, Module } from '@nestjs/common'; import { PathsConfig } from './paths.config.js'; + @Global() @Module({ providers: [PathsConfig], exports: [PathsConfig], }) -export class PathsModule {} +export class PathsModule {} +Also, consider using absolute imports rather than relative ones:
import { Global, Module } from '@nestjs/common'; -import { PathsConfig } from './paths.config.js'; +import { PathsConfig } from '@app/config/paths.config.js';🧰 Tools
🪛 GitHub Check: Test API
[failure] 2-2:
Insert⏎
[failure] 2-2:
import statements should have an absolute path
[failure] 9-9:
Replace·with⏎
[failure] 9-9:
Newline required at end of file but not found🪛 GitHub Check: Build API
[failure] 2-2:
Insert⏎
[failure] 2-2:
import statements should have an absolute path
[failure] 9-9:
Replace·with⏎
[failure] 9-9:
Newline required at end of file but not foundapi/src/core/types/state.ts (1)
1-18: Well-structured enum for state file keys.The
StateFileKeyenum provides a centralized definition of all state file keys, which is a good practice for maintaining consistency and avoiding string literals throughout the codebase.Add a newline at the end of the file:
SEC = 'sec', NOTIFICATION = 'notification', -} +} +vitest.workspace.js (1)
1-9: Good workspace configuration for multi-project testing.The Vitest workspace configuration correctly includes test configurations from different parts of the project, which will facilitate running tests across multiple packages.
Add a newline at the end of the file:
"./web/vitest.config.mjs", "./api/vite.config.ts", "./unraid-ui/vitest.config.ts" -]) 9 +]) +api/src/core/utils/misc/get-machine-id.ts (1)
4-5:❓ Verification inconclusive
Good refactoring to use PathsConfig instead of getters.
The change to use
PathsConfigis consistent with the centralization of path management described in the PR objectives.Since you're no longer using the
gettersimport, you can remove it:import { FileMissingError } from '@app/core/errors/file-missing-error.js'; -import { getters } from '@app/store/index.js'; import { PathsConfig } from '../../../config/paths.config.js';Consider whether using
PathsConfig.getInstance()aligns with NestJS dependency injection practices. In a NestJS application, you would typically injectPathsConfigrather than using a singleton pattern:Also applies to: 10-11
🏁 Script executed:
#!/bin/bash # Check if PathsConfig is used with dependency injection elsewhere rg "constructor\(.*private.*pathsConfig.*:.*PathsConfig" --type tsLength of output: 68
Attention: Verify the NestJS DI Usage for PathsConfig
- The removal of the unused
gettersimport is a good refactoring move.- Changing to
PathsConfigcentralizes path management as intended.- However, please verify that using
PathsConfig.getInstance()(not shown in the snippet but referenced on lines 10-11) is consistent with NestJS's dependency injection practices, since no evidence of DI usage forPathsConfigwas found in the codebase with the automated search.Could you manually inspect or further search the repository to confirm if dependency injection should be used here instead of the singleton pattern?
api/src/store/store-sync.ts (1)
15-30: Well-structured StoreSyncService implementationThe new StoreSyncService class is a good addition:
- It properly uses dependency injection with PathsConfig
- The syncState method is well-focused and handles a specific responsibility
- The code is clean and easy to understand
One observation: there's duplicate functionality between this new service and the existing
startStoreSyncfunction (lines 53-61). Consider refactoring the existing function to use this service.export const startStoreSync = async () => { // The last state is stored so we don't end up in a loop of writing -> reading -> writing let lastState: RootState | null = null; + const storeSyncService = new StoreSyncService(PathsConfig.getInstance()); // Update cfg when store changes store.subscribe(async () => { const state = store.getState(); // Config dependent options, wait until config loads to execute if (state.config.status === FileLoadStatus.LOADED) { // Update registration await syncRegistration(lastState); // Update docker app counts await syncInfoApps(lastState); } if ( NODE_ENV === 'development' && !isEqual(state, lastState) && state.paths['myservers-config-states'] ) { - writeFileSync(join(state.paths.states, 'config.log'), JSON.stringify(state.config, null, 2)); - writeFileSync( - join(state.paths.states, 'dynamicRemoteAccess.log'), - JSON.stringify(state.dynamicRemoteAccess, null, 2) - ); - writeFileSync( - join(state.paths.states, 'graphql.log'), - JSON.stringify(state.minigraph, null, 2) - ); + storeSyncService.syncState(state); } lastState = state; }); };api/src/config/paths.config.ts (1)
71-77: Fix formatting issuesThe static analysis tools have flagged some formatting issues with this file, including missing newlines.
// Singleton access static getInstance(): PathsConfig { if (!PathsConfig.instance) { PathsConfig.instance = new PathsConfig(); } return PathsConfig.instance; } -} +} +🧰 Tools
🪛 GitHub Check: Test API
[failure] 77-77:
Replace·with⏎
[failure] 77-77:
Newline required at end of file but not found🪛 GitHub Check: Build API
[failure] 77-77:
Replace·with⏎
[failure] 77-77:
Newline required at end of file but not foundapi/src/store/modules/state.ts (1)
3-5: Consider using more specific types instead ofanyThe
StateSliceinterface uses[key: string]: anywhich provides flexibility but reduces type safety. Consider using more specific types if the structure of your state is known or can be constrained.export interface StateSlice { - [key: string]: any; + [key: string]: string | number | boolean | object | null | undefined; }api/src/unraid-api/cli/switch-env.command.ts (1)
62-64: Avoid shadowing class member variables.The local variable
pathsshadows the injected class memberthis.paths. This could lead to confusion for developers maintaining this code in the future.async run(_, options: SwitchEnvOptions): Promise<void> { - const paths = this.paths; const basePath = paths['unraid-api-base']; const envFlashFilePath = paths['myservers-env'];Or if you're trying to create a local reference for convenience:
async run(_, options: SwitchEnvOptions): Promise<void> { - const paths = this.paths; + const { paths } = this; const basePath = paths['unraid-api-base']; const envFlashFilePath = paths['myservers-env'];api/src/store/watch/state-watch.ts (2)
9-18: Consider documenting the purpose of excluded watches.The
excludedWatchesarray is defined but there's no comment explaining why these specific state files are excluded from watching.+// Files excluded from state watching due to being handled by specialized watchers +// or because they change too frequently and don't need realtime updates const excludedWatches: StateFileKey[] = [ StateFileKey.DISPLAY, StateFileKey.DISKS, StateFileKey.DOCKER,
20-35: Good implementation of functional state watching.The function is well-structured and uses the PathsConfig singleton appropriately. Consider adding error handling for file operations.
export const setupStateWatch = () => { const paths = PathsConfig.getInstance(); const statePath = paths.states; // Watch all state files except excluded ones watch(statePath, { persistent: true, ignoreInitial: true, - }).on('change', async (path) => { + }).on('change', async (path) => { const key = path.split('/').pop()?.replace('.ini', '') as StateFileKey; if (key && !excludedWatches.includes(key)) { - const state = await parseStateFile(path); - store.dispatch(updateState({ [key]: state })); + try { + const state = await parseStateFile(path); + store.dispatch(updateState({ [key]: state })); + } catch (error) { + console.error(`Error processing state file ${key}:`, error); + } } }); };api/src/unraid-api/config/paths.config.ts (3)
54-57: Consider consistent path resolution approachUnlike other path properties that use
resolvePath, themyserversKeepaliveproperty directly assigns the path string. For consistency, consider usingresolvePathhere as well.readonly myserversKeepalive = - process.env.PATHS_MY_SERVERS_FB ?? - '/boot/config/plugins/dynamix.my.servers/fb_keepalive'; + resolvePath( + process.env.PATHS_MY_SERVERS_FB ?? + '/boot/config/plugins/dynamix.my.servers/fb_keepalive' + );
61-61: Consider environment variable for unraidLogBaseMost paths support environment variable overrides, but
unraidLogBasedoes not. For consistency and flexibility, consider adding environment variable support for this path as well.- readonly unraidLogBase = resolvePath('/var/log/'); + readonly unraidLogBase = resolvePath( + process.env.PATHS_UNRAID_LOG_BASE ?? '/var/log/' + );
63-63: Apply resolvePath to authSessions pathThe
authSessionsproperty is missingresolvePath, unlike most other path properties in this class. For consistency, consider applyingresolvePathhere as well.- readonly authSessions = process.env.PATHS_AUTH_SESSIONS ?? '/var/lib/php'; + readonly authSessions = resolvePath( + process.env.PATHS_AUTH_SESSIONS ?? '/var/lib/php' + );api/src/unraid-api/cli/report.command.ts (1)
8-8: Consider removing unused importThe import for
gettersfrom@app/store/index.jsappears to no longer be used after the transition toPathsConfig. Consider removing this import to keep the codebase clean.-import { getters } from '@app/store/index.js';api/src/core/modules/docker/get-docker-containers.ts (1)
3-3: Consider removing unused importAfter the changes to the container object construction, the
camelCaseKeysimport appears to be unused. Consider removing this import to keep the codebase clean.-import camelCaseKeys from 'camelcase-keys';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
api/src/config/paths.config.ts(1 hunks)api/src/config/paths.module.ts(1 hunks)api/src/core/modules/docker/get-docker-containers.ts(4 hunks)api/src/core/modules/get-parity-history.ts(2 hunks)api/src/core/types/state.ts(1 hunks)api/src/core/utils/clients/docker.ts(1 hunks)api/src/core/utils/clients/emcmd.ts(2 hunks)api/src/core/utils/clients/emhttpd.ts(1 hunks)api/src/core/utils/clients/ssh.ts(1 hunks)api/src/core/utils/misc/catch-handlers.ts(3 hunks)api/src/core/utils/misc/get-machine-id.ts(1 hunks)api/src/core/utils/misc/parse-state-file.ts(1 hunks)api/src/store/actions/load-dynamix-config-file.ts(2 hunks)api/src/store/index.ts(0 hunks)api/src/store/modules/paths.ts(0 hunks)api/src/store/modules/state.ts(1 hunks)api/src/store/store-sync.ts(2 hunks)api/src/store/watch/dynamix-config-watch.ts(1 hunks)api/src/store/watch/state-watch.ts(1 hunks)api/src/store/watch/var-run-watch.ts(1 hunks)api/src/unraid-api/app/app.module.ts(1 hunks)api/src/unraid-api/auth/api-key.service.ts(2 hunks)api/src/unraid-api/auth/cookie.service.ts(3 hunks)api/src/unraid-api/cli/config.command.ts(1 hunks)api/src/unraid-api/cli/report.command.ts(3 hunks)api/src/unraid-api/cli/switch-env.command.ts(2 hunks)api/src/unraid-api/config/paths.config.ts(1 hunks)api/src/unraid-api/config/paths.module.ts(1 hunks)api/src/unraid-api/cron/write-flash-file.service.ts(2 hunks)api/src/unraid-api/graph/resolvers/display/display.resolver.ts(3 hunks)api/src/unraid-api/graph/resolvers/logs/logs.service.ts(2 hunks)api/src/unraid-api/rest/rest.service.ts(1 hunks)vitest.workspace.js(1 hunks)
💤 Files with no reviewable changes (2)
- api/src/store/index.ts
- api/src/store/modules/paths.ts
🧰 Additional context used
🧬 Code Definitions (14)
api/src/config/paths.module.ts (2)
api/src/unraid-api/config/paths.module.ts (1)
Global(4-9)api/src/unraid-api/app/app.module.ts (1)
Module(19-66)
api/src/unraid-api/config/paths.module.ts (2)
api/src/config/paths.module.ts (1)
Global(4-9)api/src/unraid-api/app/app.module.ts (1)
Module(19-66)
api/src/unraid-api/app/app.module.ts (4)
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.module.ts (1)
Module(5-8)api/src/unraid-api/cli/cli.module.ts (1)
Module(68-102)api/src/unraid-api/graph/graph.module.ts (1)
Module(30-78)api/src/unraid-api/plugin/plugin.module.ts (1)
Module(5-20)
api/src/config/paths.config.ts (5)
api/src/store/store-sync.ts (1)
Injectable(15-30)api/src/unraid-api/cli/config.command.ts (1)
Injectable(10-26)api/src/unraid-api/cron/write-flash-file.service.ts (1)
Injectable(11-56)api/src/unraid-api/graph/resolvers/display/display.resolver.ts (1)
Injectable(63-127)api/src/unraid-api/rest/rest.service.ts (1)
Injectable(18-84)
api/src/unraid-api/cli/config.command.ts (5)
api/src/store/store-sync.ts (1)
Injectable(15-30)api/src/unraid-api/cli/report.command.ts (1)
Injectable(12-88)api/src/unraid-api/cli/switch-env.command.ts (1)
Injectable(18-94)api/src/unraid-api/config/paths.config.ts (1)
Injectable(4-67)api/src/unraid-api/cli/restart.command.ts (1)
Command(7-43)
api/src/store/store-sync.ts (2)
api/src/unraid-api/cli/config.command.ts (1)
Injectable(10-26)api/src/unraid-api/config/paths.config.ts (1)
Injectable(4-67)
api/src/unraid-api/rest/rest.service.ts (4)
api/src/unraid-api/cli/report.command.ts (1)
Injectable(12-88)api/src/unraid-api/cli/log.service.ts (1)
Injectable(6-57)api/src/unraid-api/cli/config.command.ts (1)
Injectable(10-26)api/src/unraid-api/config/paths.config.ts (1)
Injectable(4-67)
api/src/store/watch/state-watch.ts (2)
api/src/core/utils/misc/parse-state-file.ts (1)
parseStateFile(4-7)api/src/store/index.ts (1)
store(15-32)
api/src/unraid-api/config/paths.config.ts (8)
api/src/unraid-api/cli/config.command.ts (1)
Injectable(10-26)api/src/unraid-api/cli/report.command.ts (1)
Injectable(12-88)api/src/unraid-api/auth/cookie.service.ts (1)
Injectable(21-100)api/src/unraid-api/cli/switch-env.command.ts (1)
Injectable(18-94)api/src/unraid-api/graph/resolvers/display/display.resolver.ts (1)
Injectable(63-127)api/src/unraid-api/auth/api-key.service.ts (1)
Injectable(29-349)api/src/unraid-api/rest/rest.service.ts (1)
Injectable(18-84)api/src/unraid-api/graph/resolvers/logs/logs.service.ts (1)
Injectable(27-358)
api/src/core/modules/docker/get-docker-containers.ts (3)
api/src/graphql/generated/api/types.ts (2)
DockerContainer(566-584)ContainerPort(448-454)api/src/core/utils/clients/docker.ts (1)
docker(15-15)api/src/core/utils/misc/catch-handlers.ts (2)
docker(13-23)catchHandlers(12-31)
api/src/unraid-api/cli/report.command.ts (2)
api/src/unraid-api/cli/config.command.ts (1)
Injectable(10-26)api/src/unraid-api/config/paths.config.ts (1)
Injectable(4-67)
api/src/unraid-api/graph/resolvers/display/display.resolver.ts (12)
api/src/unraid-api/cli/report.command.ts (1)
Injectable(12-88)api/src/unraid-api/cli/switch-env.command.ts (1)
Injectable(18-94)api/src/unraid-api/config/paths.config.ts (1)
Injectable(4-67)api/src/unraid-api/rest/rest.service.ts (1)
Injectable(18-84)api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (1)
Resolver(8-35)api/src/unraid-api/graph/resolvers/me/me.resolver.ts (1)
Resolver(9-22)api/src/unraid-api/graph/resolvers/flash/flash.resolver.ts (1)
Resolver(8-25)api/src/unraid-api/graph/resolvers/online/online.resolver.ts (1)
Resolver(7-18)api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts (1)
Resolver(9-43)api/src/unraid-api/graph/resolvers/registration/registration.resolver.ts (1)
Resolver(12-53)api/src/unraid-api/graph/resolvers/servers/server.resolver.ts (1)
Resolver(10-42)api/src/unraid-api/graph/resolvers/vars/vars.resolver.ts (1)
Resolver(8-22)
api/src/core/utils/clients/docker.ts (1)
api/src/core/utils/misc/catch-handlers.ts (1)
docker(13-23)
api/src/unraid-api/cli/switch-env.command.ts (4)
api/src/unraid-api/cli/log.service.ts (1)
Injectable(6-57)api/src/unraid-api/config/paths.config.ts (1)
Injectable(4-67)api/src/unraid-api/cli/start.command.ts (1)
Command(13-61)api/src/unraid-api/cli/stop.command.ts (1)
Command(9-40)
🪛 GitHub Check: Test API
api/src/config/paths.module.ts
[failure] 2-2:
Insert ⏎
[failure] 2-2:
import statements should have an absolute path
[failure] 9-9:
Replace · with ⏎
[failure] 9-9:
Newline required at end of file but not found
api/src/config/paths.config.ts
[failure] 39-39:
Delete ⏎···············
[failure] 42-42:
Replace ⏎············process.env.PATHS_DYNAMIX_CONFIG·??·'/boot/config/plugins/dynamix/dynamix.cfg'⏎········ with process.env.PATHS_DYNAMIX_CONFIG·??·'/boot/config/plugins/dynamix/dynamix.cfg'
[failure] 49-49:
Delete ⏎···········
[failure] 58-58:
Delete ⏎·······
[failure] 77-77:
Replace · with ⏎
[failure] 77-77:
Newline required at end of file but not found
🪛 GitHub Check: Build API
api/src/config/paths.module.ts
[failure] 2-2:
Insert ⏎
[failure] 2-2:
import statements should have an absolute path
[failure] 9-9:
Replace · with ⏎
[failure] 9-9:
Newline required at end of file but not found
api/src/config/paths.config.ts
[failure] 39-39:
Delete ⏎···············
[failure] 42-42:
Replace ⏎············process.env.PATHS_DYNAMIX_CONFIG·??·'/boot/config/plugins/dynamix/dynamix.cfg'⏎········ with process.env.PATHS_DYNAMIX_CONFIG·??·'/boot/config/plugins/dynamix/dynamix.cfg'
[failure] 49-49:
Delete ⏎···········
[failure] 58-58:
Delete ⏎·······
[failure] 77-77:
Replace · with ⏎
[failure] 77-77:
Newline required at end of file but not found
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (52)
api/src/unraid-api/config/paths.module.ts (1)
1-9:❓ Verification inconclusive
Possible module duplication concern
The relevant code snippets indicate there might be another
PathsModuledefined inapi/src/config/paths.module.ts. Please ensure this isn't creating duplicates or confusion in the dependency injection system.
🏁 Script executed:
#!/bin/bash # Check if there are duplicate PathsModule definitions echo "Checking for duplicate PathsModule definitions..." rg --type ts "export class PathsModule" -A 0 -B 0Length of output: 291
Duplicate PathsModule Definitions Detected
Two definitions for
PathsModuleexist in the repository:
api/src/unraid-api/config/paths.module.tsapi/src/config/paths.module.tsPlease verify that both are intentionally maintained and ensure they do not conflict within the dependency injection system.
api/src/unraid-api/app/app.module.ts (2)
17-17: Added PathsModule import for centralized path configurationThe import of
PathsModuleproperly introduces the centralized path configuration system to the application.
21-21: Integrated PathsModule into AppModule importsAdding
PathsModuleto the imports array ensures the centralized path configuration is accessible throughout the application.api/src/store/actions/load-dynamix-config-file.ts (2)
10-10: Added PathsConfig import for direct path accessThe import of
PathsConfigis correctly added to facilitate the transition from dynamic getters to the centralized configuration system.
44-45: Refactored to use PathsConfig singletonThe code now uses the singleton pattern to access path configurations, which aligns with the centralization goal. The
dynamixConfigproperty is properly utilized in thebatchProcessfunction.api/src/unraid-api/rest/rest.service.ts (3)
16-16: Added PathsConfig importThe import of
PathsConfigis correctly added to facilitate dependency injection for path configuration.
22-24: Updated constructor for dependency injectionThe constructor now properly accepts
PathsConfigthrough dependency injection and initializes thelogPathwithpaths.logBase.
26-26: Added private logPath propertyThe private
logPathproperty has been added to store the log base path.api/src/core/modules/get-parity-history.ts (2)
9-9: Added PathsConfig importThe import of
PathsConfigis correctly added to facilitate the transition to the centralized configuration system.
25-26: Refactored to use PathsConfig singletonThe code now properly uses the singleton pattern to obtain the path configuration and directly accesses the
parityChecksproperty, which follows the centralization goal.api/src/store/watch/dynamix-config-watch.ts (2)
4-5: Good move to import the PathsConfig classThis change aligns with the move toward a more centralized path configuration system. Removing the dependency on getters and using a dedicated configuration class improves maintainability.
8-9: Clean implementation of PathsConfig usageYou've correctly implemented the singleton pattern here by using
PathsConfig.getInstance()and accessing thedynamixConfigproperty. This provides better type safety and code organization than the previous approach with getters.api/src/store/store-sync.ts (1)
3-4: Good dependency importsAdding NestJS Injectable decorator and importing PathsConfig for dependency injection follows best practices.
api/src/unraid-api/graph/resolvers/display/display.resolver.ts (3)
5-5: Good importsAdding the Injectable decorator and PathsConfig imports aligns with the dependency injection pattern being implemented.
Also applies to: 13-13
63-66: Well-implemented dependency injectionThe @Injectable decorator and constructor properly set up the resolver for dependency injection. This is a good pattern that improves testability.
78-78: Clean path accessUsing
this.paths.dynamixBasedirectly is cleaner and more maintainable than the previous getter approach. This provides better type safety and makes the code easier to understand.api/src/config/paths.config.ts (4)
1-7: Good class setup with singleton patternThe PathsConfig class is properly set up as an Injectable NestJS service with a singleton pattern. This ensures consistent path configuration throughout the application.
8-21: Well-organized path definitionsThese path definitions are clearly named and properly use environment variables with sensible defaults. The use of resolvePath ensures paths are properly normalized.
23-45: Excellent documentation for dynamixConfigThe detailed comments explaining the configuration order and precedence are very valuable. This makes the code more maintainable and helps future developers understand the design decisions.
🧰 Tools
🪛 GitHub Check: Test API
[failure] 39-39:
Delete⏎···············
[failure] 42-42:
Replace⏎············process.env.PATHS_DYNAMIX_CONFIG·??·'/boot/config/plugins/dynamix/dynamix.cfg'⏎········withprocess.env.PATHS_DYNAMIX_CONFIG·??·'/boot/config/plugins/dynamix/dynamix.cfg'🪛 GitHub Check: Build API
[failure] 39-39:
Delete⏎···············
[failure] 42-42:
Replace⏎············process.env.PATHS_DYNAMIX_CONFIG·??·'/boot/config/plugins/dynamix/dynamix.cfg'⏎········withprocess.env.PATHS_DYNAMIX_CONFIG·??·'/boot/config/plugins/dynamix/dynamix.cfg'
47-68: Comprehensive path definitionsThis section provides a complete set of path configurations for the myservers component, with proper environment variable overrides. The organization is logical and easy to follow.
🧰 Tools
🪛 GitHub Check: Test API
[failure] 49-49:
Delete⏎···········
[failure] 58-58:
Delete⏎·······🪛 GitHub Check: Build API
[failure] 49-49:
Delete⏎···········
[failure] 58-58:
Delete⏎·······api/src/store/modules/state.ts (1)
13-15: LGTM! Clean implementation of state update logicThe implementation using spread operators ensures immutability while merging state objects, which is a good Redux practice.
api/src/core/utils/clients/emcmd.ts (1)
14-15: LGTM! Good use of the new PathsConfig patternThe switch from using getters to PathsConfig.getInstance() is consistent with the PR's objective of centralizing path configuration.
api/src/store/watch/var-run-watch.ts (2)
11-13: LGTM! Good migration to PathsConfigThe change from using getters to PathsConfig.getInstance() improves maintainability by centralizing path configuration.
15-16: LGTM! Improved readability with dot notationThe change from bracket notation (paths['docker-socket']) to dot notation (paths.dockerSocket) improves code readability.
Also applies to: 21-22
api/src/core/utils/clients/docker.ts (2)
4-10: LGTM! Good factory function implementationCreating a factory function for the Docker client makes the code more testable and configurable. This approach aligns well with dependency injection patterns.
15-16: LGTM! Good practice exposing both the instance and factoryExporting both the instantiated client and the factory function provides flexibility for direct usage and for testing.
api/src/unraid-api/cli/config.command.ts (3)
8-8: Added import for PathsConfigThe import of
PathsConfigfrom the relative path is necessary for the dependency injection pattern being implemented across the application.
16-16: Updated constructor signature with PathsConfig dependencyThe constructor now properly accepts the
PathsConfigas a dependency, allowing for better dependency injection and testability. This aligns with the PR objective of centralizing path configuration.
22-22: Replaced path getter with direct property accessThe code now uses
this.paths.myserversConfiginstead ofgetters.paths()['myservers-config'], which provides better type safety and follows the dependency injection pattern.api/src/unraid-api/graph/resolvers/logs/logs.service.ts (3)
11-11: Added import for PathsConfigThe import of
PathsConfigfrom the config directory is necessary to support the dependency injection pattern being implemented.
36-37: Updated constructor with PathsConfig injectionThe constructor now properly injects the
PathsConfigdependency, which aligns with the architectural changes being made throughout the codebase. This eliminates the need for direct access to the global store.
42-42: Updated logBasePath getter implementationThe getter now returns
this.paths.unraidLogBaseinstead of using the global getter function, which aligns with the dependency injection pattern and centralizes path management.api/src/unraid-api/cron/write-flash-file.service.ts (3)
4-4: Added import for PathsConfigThe import of
PathsConfigfrom the config directory is necessary for the dependency injection pattern being implemented across the application.
13-15: Updated constructor with PathsConfig initializationThe constructor now properly accepts
PathsConfigas a dependency and initializes thefileLocationproperty. This is a good change that follows dependency injection principles and centralizes path configuration.
18-18: Updated fileLocation property declarationThe
fileLocationproperty is now declared without initialization, as it's set in the constructor. This is a cleaner approach since the initialization depends on the injected dependency.api/src/unraid-api/auth/api-key.service.ts (2)
27-27: Added import for PathsConfigThe import of
PathsConfigfrom the config directory is necessary to enable dependency injection of path configuration.
36-38: Updated constructor with PathsConfig dependencyThe constructor now accepts
PathsConfigand properly initializes thebasePathproperty withpaths.authKeys. This aligns with the PR's objective to centralize path management and follows good dependency injection practices.api/src/core/utils/misc/catch-handlers.ts (2)
3-3: Good job updating imports to use PathsConfig.This aligns with the PR objective of moving to a centralized path configuration.
14-15: Refactored to use PathsConfig singleton - great!The change from using
getters.paths()['docker-socket']toPathsConfig.getInstance().dockerSocketimproves code maintainability by centralizing path configuration.api/src/unraid-api/auth/cookie.service.ts (3)
8-8: Good job with proper import.This aligns with the PR objective of moving to a centralized path configuration.
24-31: Constructor updated to use dependency injection - great pattern!Using dependency injection for
PathsConfigimproves testability and follows good design principles.
92-98: Updated to use sessionDir property - good!The method is updated to use the instance property, consistent with the dependency injection pattern.
api/src/unraid-api/cli/switch-env.command.ts (1)
5-5: Good job implementing dependency injection.The class is properly decorated with
@Injectable()and dependencies are injected through the constructor, following NestJS best practices.Also applies to: 12-12, 18-31
api/src/store/watch/state-watch.ts (1)
1-8: Good refactoring of imports.Imports have been updated to use the new centralized path configuration and essential store modules.
api/src/unraid-api/config/paths.config.ts (2)
21-43: Excellent documentation of plugin configuration behaviorThe detailed comment explaining how plugin configurations work with default and user-customized settings provides valuable context for maintainers. The array implementation with ordered precedence is a clear representation of the described behavior.
6-6:❓ Verification inconclusive
Verify import.meta.dirname compatibility
This uses
import.meta.dirnamewhich may have compatibility issues in certain environments. Verify that your TypeScript configuration and runtime support this feature.
🏁 Script executed:
#!/bin/bash # Check for any TypeScript configuration that enables or disables this feature grep -r "import.meta" --include="*.json" . # Check for polyfills or utilities that might handle this grep -r "import.meta.dirname" --include="*.{ts,js}" . | grep -v "paths.config.ts"Length of output: 122
Action: Verify Compatibility of
import.meta.dirnameThe current checks did not produce any output, which leaves some uncertainty around whether your TypeScript and runtime configurations fully support using
import.meta.dirname. Please ensure that:
- Your TypeScript configuration (typically in
tsconfig.json) is set to compile using an ES module target (e.g.,"module": "esnext").- Your runtime environment (Node.js or other) supports native ESM features, including
import.meta.dirname.- No polyfills or alternate utilities for
import.meta.dirnameare needed in your deployment.Manual verification of these aspects is required to confirm compatibility.
api/src/unraid-api/cli/report.command.ts (2)
2-3: Clean implementation of dependency injectionThe addition of the
@Injectable()decorator and the injection ofPathsConfigthrough the constructor is a good practice. This approach centralizes path management and follows NestJS best practices.Also applies to: 12-12, 15-18
42-47: Improved error handling for config file readingThe updated implementation provides more descriptive error messages when config files cannot be read. This enhances debuggability by making it clear which specific file is missing, rather than just returning null.
api/src/core/modules/docker/get-docker-containers.ts (4)
1-1: Good updates to importsThe change to use
promises as fsfrom the fs module and the addition ofPathsConfigimport align well with the modernization of the codebase and the centralization of path management.Also applies to: 12-12
22-22: Improved function signatureThe updated function signature with default parameter values directly in the destructuring assignment makes the code more concise and easier to maintain.
31-35: Cleaner path access and error handlingUsing
PathsConfig.getInstance()to access paths centralizes configuration and improves maintainability. The promise-based file reading with.catch()for error handling is a more modern and cleaner approach.
36-55: More explicit container object constructionThe direct mapping of container properties instead of using deep transformation with
camelCaseKeysmakes the code more explicit and potentially more efficient. The type assertions for container state and port types also improve type safety.
| export const createEmhttpdClient = () => { | ||
| const paths = PathsConfig.getInstance(); | ||
| const socketPath = paths.emhttpdSocket; | ||
| // Rest of implementation | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Complete the implementation
The function contains a placeholder comment but lacks actual implementation.
Please complete the implementation of this client function.
| export const createSshClient = () => { | ||
| const paths = PathsConfig.getInstance(); | ||
| const keyPath = paths.keyfileBase; | ||
| // Rest of implementation | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Complete the implementation
The function contains a placeholder comment but lacks actual implementation.
Please complete the implementation of this SSH client function.
| export const createEmcmdClient = () => { | ||
| const paths = PathsConfig.getInstance(); | ||
| const socketPath = paths.emhttpdSocket; | ||
| // Rest of implementation | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Complete the implementation of createEmcmdClient
The function appears to be incomplete with just a comment indicating "Rest of implementation". Complete the function implementation or consider removing it if it's not needed yet.
🏁 Script executed:
#!/bin/bash
# Check if this function is called elsewhere in the codebase
echo "Checking for references to createEmcmdClient..."
rg "createEmcmdClient" --type tsLength of output: 214
Action Required: Implement or remove createEmcmdClient
- The function currently contains only a placeholder comment ("// Rest of implementation") and lacks a complete implementation.
- Our search shows no external references to
createEmcmdClient, suggesting it might not be used elsewhere. - Please either complete the function implementation if it’s needed or remove it to avoid unnecessary code clutter.
| export const handleDockerError = (error: Error) => { | ||
| const paths = PathsConfig.getInstance(); | ||
| const socketPath = paths.dockerSocket; | ||
| // Rest of implementation | ||
| }; |
There was a problem hiding this comment.
Incomplete implementation for handleDockerError.
The function appears to be a stub with "Rest of implementation" as a comment. This should be completed before merging.
export const handleDockerError = (error: Error) => {
const paths = PathsConfig.getInstance();
const socketPath = paths.dockerSocket;
- // Rest of implementation
+ // Apply similar logic as in catchHandlers.docker
+ if (error instanceof Error && 'code' in error && 'address' in error) {
+ const dockerError = error as unknown as DockerError;
+ if (dockerError.code === 'ENOENT' && dockerError.address === socketPath) {
+ throw new AppError('Docker socket unavailable.');
+ }
+ }
+ throw error;
};📝 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.
| export const handleDockerError = (error: Error) => { | |
| const paths = PathsConfig.getInstance(); | |
| const socketPath = paths.dockerSocket; | |
| // Rest of implementation | |
| }; | |
| export const handleDockerError = (error: Error) => { | |
| const paths = PathsConfig.getInstance(); | |
| const socketPath = paths.dockerSocket; | |
| // Apply similar logic as in catchHandlers.docker | |
| if (error instanceof Error && 'code' in error && 'address' in error) { | |
| const dockerError = error as unknown as DockerError; | |
| if (dockerError.code === 'ENOENT' && dockerError.address === socketPath) { | |
| throw new AppError('Docker socket unavailable.'); | |
| } | |
| } | |
| throw error; | |
| }; |
Summary by CodeRabbit
New Features
Refactor