feat: swap to nodemon instead of PM2 #1798
Conversation
WalkthroughReplaces PM2 with nodemon across the API: removes PM2 config, service, helpers and tests; adds nodemon config, NodemonService and tests; updates environment paths, CLI wiring, GraphQL types, packaging, build script, and removes PM2 IPC ready signalling. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Nod as NodemonService
participant FS as Filesystem (pid/log)
participant Child as nodemon process
rect `#ebf5ff`
Note over CLI,Nod: Start flow (new)
CLI->>Nod: ensureNodemonDependencies()
Nod->>FS: mkdir logs & pid dirs (recursive)
CLI->>Nod: start({ env })
Nod->>Child: execa nodemon (detached, stdio piped)
Child->>FS: write logfile, write PID -> nodemon.pid
end
rect `#e9ffeb`
Note over CLI,Nod: Status / Stop / Restart
CLI->>Nod: status()
Nod->>FS: read nodemon.pid
Nod->>Nod: process.kill(pid, 0) => alive? -> boolean
CLI->>Nod: stop({ force? })
Nod->>Nod: send SIGTERM or SIGKILL, remove pid file
CLI->>Nod: restart()
Nod->>Nod: stop() then start()
end
rect `#fff3e6`
Note over CLI,Nod: Logs retrieval
CLI->>Nod: logs(lines)
Nod->>Child: execa tail -n lines logfile
Child->>Nod: return log output -> CLI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-04-21T18:25:16.491ZApplied to files:
📚 Learning: 2025-03-07T19:29:12.540ZApplied to files:
📚 Learning: 2025-04-21T18:25:16.491ZApplied to files:
📚 Learning: 2025-03-13T16:17:55.820ZApplied to files:
📚 Learning: 2024-11-05T14:49:07.308ZApplied to files:
📚 Learning: 2025-02-06T17:25:45.397ZApplied to files:
📚 Learning: 2025-02-05T14:43:48.568ZApplied to files:
🧬 Code graph analysis (2)api/src/unraid-api/cli/nodemon.service.spec.ts (1)
api/src/unraid-api/cli/nodemon.service.ts (3)
🔇 Additional comments (2)
Comment |
elibosley
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
api/scripts/build.ts (1)
10-10: Build script now correctly packagesnodemon.jsonIncluding
nodemon.jsonin the pack directory ensures nodemon-managed deployments get the config in the release artifacts, which matches the PR goal. ThegetDeploymentVersionimport via the@app/../scriptspath is acceptable given the existing aliasing, though long‑term you may want a dedicated path alias for scripts to avoid the../hop.Also applies to: 95-98
api/src/unraid-api/cli/__test__/report.command.test.ts (1)
29-34: Mocks point to the right module, but last test contains redundant scenario
- The
vi.mock('@app/core/utils/process/unraid-api-running.js', …)wiring and the reset ofmockIsUnraidApiRunninginbeforeEachlook correct and consistent with the new process-utils location and ESM style.- In
it('should pass correct apiRunning parameter to ApiReportService', …), the second block (Lines 153‑157) stubsmockIsUnraidApiRunningtotrueand assertsgenerateReport(true)again, which is effectively identical to the first part of the test and doesn’t add coverage. If you intended to cover a different status (e.g.,falseor some alternate path), consider adjusting the stub accordingly; otherwise, you can drop the redundant block to keep the test focused.Also applies to: 53-55, 153-157
api/legacy/generated-schema-legacy.graphql (1)
1244-1271: Legacy Versions schema pm2→nodemon rename is consistentReplacing the
pm2field withnodemononVersionsmatches the rest of the nodemon migration (models, resolvers, web schema). Just ensure any consumers of the legacy schema are updated to readnodemoninstead ofpm2.api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts (1)
6-6: Nodemon version wiring into PackageVersions is correctUsing
getPackageJson().dependencies?.nodemonto populate thenodemonfield aligns with the newPackageVersionsmodel and schema, and gracefully returnsnullif the dependency is missing.If this resolver is called frequently, consider reading and caching
getPackageJson()once at module init rather than re-readingpackage.jsonon every request.Also applies to: 34-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
api/ecosystem.config.json(0 hunks)api/generated-schema.graphql(1 hunks)api/legacy/generated-schema-legacy.graphql(1 hunks)api/nodemon.json(1 hunks)api/package.json(1 hunks)api/scripts/build.ts(2 hunks)api/src/__test__/core/utils/pm2/dummy-process.js(0 hunks)api/src/__test__/core/utils/pm2/unraid-api-running.integration.test.ts(0 hunks)api/src/__test__/core/utils/process/unraid-api-running.integration.test.ts(1 hunks)api/src/core/log.ts(1 hunks)api/src/core/utils/pm2/unraid-api-running.ts(0 hunks)api/src/core/utils/process/unraid-api-running.ts(1 hunks)api/src/environment.ts(1 hunks)api/src/unraid-api/cli/__test__/report.command.test.ts(3 hunks)api/src/unraid-api/cli/cli-services.module.ts(2 hunks)api/src/unraid-api/cli/cli.module.ts(2 hunks)api/src/unraid-api/cli/logs.command.ts(2 hunks)api/src/unraid-api/cli/nodemon.service.spec.ts(1 hunks)api/src/unraid-api/cli/nodemon.service.ts(1 hunks)api/src/unraid-api/cli/pm2.service.spec.ts(0 hunks)api/src/unraid-api/cli/pm2.service.ts(0 hunks)api/src/unraid-api/cli/report.command.ts(1 hunks)api/src/unraid-api/cli/restart.command.ts(2 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/cli/status.command.ts(1 hunks)api/src/unraid-api/cli/stop.command.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/versions/versions.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts(2 hunks)api/src/unraid-api/main.ts(0 hunks)web/composables/gql/graphql.ts(1 hunks)web/src/composables/gql/graphql.ts(5 hunks)web/src/composables/gql/index.ts(1 hunks)
💤 Files with no reviewable changes (7)
- api/src/unraid-api/main.ts
- api/src/unraid-api/cli/pm2.service.ts
- api/src/test/core/utils/pm2/unraid-api-running.integration.test.ts
- api/src/unraid-api/cli/pm2.service.spec.ts
- api/src/core/utils/pm2/unraid-api-running.ts
- api/src/test/core/utils/pm2/dummy-process.js
- api/ecosystem.config.json
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: mdatelle
Repo: unraid/api PR: 1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
📚 Learning: 2025-03-12T13:48:14.850Z
Learnt from: pujitm
Repo: unraid/api PR: 1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Applied to files:
web/src/composables/gql/index.tsweb/src/composables/gql/graphql.ts
📚 Learning: 2025-05-23T21:59:29.632Z
Learnt from: elibosley
Repo: unraid/api PR: 1408
File: web/components/ApiKey/PermissionCounter.vue:6-6
Timestamp: 2025-05-23T21:59:29.632Z
Learning: This codebase uses ESM (ECMAScript Modules) and requires .js extensions in import statements, even when importing from TypeScript files, as the imports refer to the compiled JavaScript output.
Applied to files:
web/src/composables/gql/index.ts
📚 Learning: 2024-12-11T17:20:09.604Z
Learnt from: pujitm
Repo: unraid/api PR: 978
File: web/helpers/apollo-cache/index.ts:133-157
Timestamp: 2024-12-11T17:20:09.604Z
Learning: In `web/helpers/apollo-cache/index.ts`, when updating the cache in the `cache.updateQuery` function, it's sufficient to check if `data` is present, as type checking inside `data` is handled at the GraphQL layer.
Applied to files:
web/src/composables/gql/index.ts
📚 Learning: 2025-09-14T14:07:28.265Z
Learnt from: elibosley
Repo: unraid/api PR: 1702
File: web/eslint.config.mjs:106-106
Timestamp: 2025-09-14T14:07:28.265Z
Learning: The unraid/api codebase specifically requires Node 20+ as a minimum version, so modern Node.js features like import.meta.dirname (available since Node v20.11.0) can be used without compatibility fallbacks.
Applied to files:
api/src/unraid-api/graph/resolvers/info/versions/versions.model.tsapi/src/core/utils/process/unraid-api-running.tsapi/src/unraid-api/cli/cli-services.module.tsapi/package.jsonapi/src/environment.tsapi/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts
📚 Learning: 2025-02-04T13:48:45.521Z
Learnt from: pujitm
Repo: unraid/api PR: 1104
File: api/src/unraid-api/cli/stop.command.ts:14-18
Timestamp: 2025-02-04T13:48:45.521Z
Learning: The PM2Service's `run` method automatically handles logging when `raw: true` is not set in the context argument. Explicit logging is not needed in such cases.
Applied to files:
api/src/unraid-api/cli/logs.command.tsapi/src/core/log.ts
📚 Learning: 2025-03-27T13:34:53.438Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Applied to files:
api/src/unraid-api/cli/report.command.ts
📚 Learning: 2025-04-02T21:21:29.168Z
Learnt from: elibosley
Repo: unraid/api PR: 1308
File: unraid-ui/src/components/common/loading/Error.vue:2-2
Timestamp: 2025-04-02T21:21:29.168Z
Learning: Components in the unraid-ui folder require explicit imports and are not autoloaded, unlike other parts of the project that may use Nuxt.js autoloading features.
Applied to files:
api/src/unraid-api/cli/cli-services.module.ts
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
Repo: unraid/api PR: 1575
File: packages/unraid-shared/src/services/socket-config.service.spec.ts:10-13
Timestamp: 2025-08-09T01:03:29.676Z
Learning: Vitest is used for all testing across all repositories in the unraid organization, not Jest. Always use `vi` for mocking utilities, not `jest`.
Applied to files:
api/src/__test__/core/utils/process/unraid-api-running.integration.test.tsapi/src/unraid-api/cli/__test__/report.command.test.tsapi/src/unraid-api/cli/nodemon.service.spec.ts
📚 Learning: 2025-03-04T14:55:00.903Z
Learnt from: elibosley
Repo: unraid/api PR: 1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Applied to files:
api/src/environment.ts
📚 Learning: 2025-03-26T15:18:27.452Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: plugin/docker-compose.yml:15-15
Timestamp: 2025-03-26T15:18:27.452Z
Learning: The files in the `api/deploy` directory (including `packed-pnpm-store.txz`) are gitignored build artifacts that are generated when the API is built. Building the API is a prerequisite before using the plugin builder container, as it needs access to these artifacts.
Applied to files:
api/scripts/build.ts
📚 Learning: 2025-02-06T17:25:45.397Z
Learnt from: mdatelle
Repo: unraid/api PR: 1122
File: web/package.json:16-16
Timestamp: 2025-02-06T17:25:45.397Z
Learning: The build script in web/package.json should include type-check command (`npm run type-check`) even when NODE_ENV is production, as it provides value for local builds by catching type errors before pushing to CI/CD.
Applied to files:
api/scripts/build.ts
📚 Learning: 2025-02-04T16:42:21.782Z
Learnt from: elibosley
Repo: unraid/api PR: 1105
File: api/scripts/build.mjs:0-0
Timestamp: 2025-02-04T16:42:21.782Z
Learning: In Node.js build scripts, avoid manually copying node_modules as a cache layer. Instead, rely on npm's built-in caching mechanism and use direct `npm install` commands, as this ensures dependency consistency and proper version resolution while still maintaining good performance through npm's cache.
Applied to files:
api/scripts/build.ts
🧬 Code graph analysis (13)
api/src/unraid-api/cli/logs.command.ts (4)
api/src/unraid-api/cli/restart.command.ts (1)
Command(21-54)api/src/unraid-api/cli/start.command.ts (1)
Command(11-33)api/src/unraid-api/cli/status.command.ts (1)
Command(5-13)api/src/unraid-api/cli/stop.command.ts (1)
Command(8-28)
api/src/unraid-api/cli/status.command.ts (4)
api/src/unraid-api/cli/logs.command.ts (1)
Command(9-25)api/src/unraid-api/cli/restart.command.ts (1)
Command(21-54)api/src/unraid-api/cli/start.command.ts (1)
Command(11-33)api/src/unraid-api/cli/stop.command.ts (1)
Command(8-28)
api/src/core/utils/process/unraid-api-running.ts (2)
packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)api/src/environment.ts (1)
NODEMON_PID_PATH(114-114)
api/src/unraid-api/cli/report.command.ts (1)
api/src/core/utils/process/unraid-api-running.ts (1)
isUnraidApiRunning(6-23)
web/src/composables/gql/graphql.ts (1)
web/composables/gql/graphql.ts (2)
Node(1357-1359)Scalars(11-75)
web/composables/gql/graphql.ts (1)
web/src/composables/gql/graphql.ts (2)
Maybe(3-3)Scalars(11-75)
api/src/__test__/core/utils/process/unraid-api-running.integration.test.ts (1)
api/src/core/utils/process/unraid-api-running.ts (1)
isUnraidApiRunning(6-23)
api/src/unraid-api/cli/start.command.ts (4)
api/src/unraid-api/cli/logs.command.ts (1)
Command(9-25)api/src/unraid-api/cli/restart.command.ts (2)
Command(21-54)LogLevelOptions(9-11)api/src/unraid-api/cli/status.command.ts (1)
Command(5-13)api/src/unraid-api/cli/stop.command.ts (1)
Command(8-28)
api/src/unraid-api/cli/nodemon.service.ts (3)
api/src/environment.ts (6)
PATHS_LOGS_DIR(101-102)NODEMON_PID_PATH(114-114)PATHS_LOGS_FILE(103-103)NODEMON_PATH(105-112)NODEMON_CONFIG_PATH(113-113)UNRAID_API_CWD(115-115)api/src/unraid-api/cli/log.service.ts (1)
error(65-69)packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)
api/src/core/log.ts (1)
api/src/environment.ts (2)
SUPPRESS_LOGS(94-94)LOG_TYPE(88-88)
api/src/unraid-api/cli/stop.command.ts (4)
api/src/unraid-api/cli/logs.command.ts (1)
Command(9-25)api/src/unraid-api/cli/restart.command.ts (1)
Command(21-54)api/src/unraid-api/cli/start.command.ts (1)
Command(11-33)api/src/unraid-api/cli/status.command.ts (1)
Command(5-13)
api/src/unraid-api/cli/nodemon.service.spec.ts (1)
packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)
api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts (1)
api/src/environment.ts (1)
getPackageJson(49-53)
🪛 GitHub Actions: CI - Main (API)
api/src/unraid-api/cli/nodemon.service.spec.ts
[error] 1-1: vitest error: No 'SUPPRESS_LOGS' export is defined on the '@app/environment.js' mock in nodemon.service.spec.ts. Ensure environment mock exports SUPPRESS_LOGS.
🔇 Additional comments (18)
web/src/composables/gql/index.ts (1)
1-2: ESM re-exports are consistent and correctThe barrel re-exports look good and align with the existing ESM/import style in this repo; no changes needed. Based on learnings.
api/src/core/log.ts (1)
20-29: Updated logging comments accurately describe behaviorThe revised comments now correctly describe that output goes directly to a log file and why colorization is disabled; they match the existing
colorize: false/destination: logDestinationconfiguration without changing behavior.api/package.json (1)
140-140: Nodemon 3.1.10 is secure and appropriate for runtime deploymentVerification confirms no npm security advisories affecting version 3.1.10. The latest stable release is 3.1.11, but Renovate will handle future updates automatically. The move to
dependenciesaligns with your new process manager strategy.api/nodemon.json (1)
1-18: Nodemon config looks sound and matches build layoutWatching
dist/main.jsandmyservers.cfg, ignoringnode_modules/src/.env.*, and executingnode ./dist/main.jswithNODE_ENV=productionall look consistent with the packaged API. No changes needed.api/src/unraid-api/cli/report.command.ts (1)
33-40: Dynamic import and log message now correctly reference nodemon-based checkImporting
@app/core/utils/process/unraid-api-running.jsand updating the debug message to "failed to check nodemon state" cleanly rewire the report command onto the new process-based liveness check without altering behavior.web/composables/gql/graphql.ts (1)
1511-1528: Frontend PackageVersions type correctly adds nodemonAdding the nullable
nodemon?: Maybe<Scalars['String']['output']>field with the corresponding docstring keeps the web GraphQL typings in sync with the server schema and versions resolver.api/generated-schema.graphql (1)
1666-1690: Generated schema PackageVersions now exposes nodemonThe
"""nodemon version"""docstring andnodemon: Stringfield correctly reflect the server-side change and stay consistent with the TS model and client typings. No issues here as long as this file continues to be generated from the updated schema sources.api/src/unraid-api/graph/resolvers/info/versions/versions.model.ts (1)
17-42: PackageVersions model nodemon field matches schema & resolverAdding the nullable
nodemonfield with the proper description keeps the GraphQL model in sync with both the generated schema andVersionsResolver.packages(). Looks good.api/src/unraid-api/cli/cli.module.ts (1)
16-16: LGTM! Clean service replacement.The NodemonService import and provider registration are correctly implemented, aligning with the PM2-to-Nodemon migration strategy.
Also applies to: 67-67
api/src/unraid-api/cli/restart.command.ts (1)
5-7: LGTM! Restart command correctly migrated to Nodemon.The service replacement is consistent with the broader migration, and the log level uppercasing aligns with environment variable conventions.
Also applies to: 23-35
api/src/unraid-api/cli/cli-services.module.ts (1)
7-7: LGTM! Service wiring correctly updated.The NodemonService replacement in the CLI services module is consistent with the migration pattern.
Also applies to: 24-24
api/src/unraid-api/cli/nodemon.service.spec.ts (1)
49-98: Well-structured test suite.The test cases properly cover key NodemonService functionality with appropriate mocking and lifecycle management. Good use of Vitest patterns.
api/src/unraid-api/cli/status.command.ts (1)
3-3: LGTM! Status command correctly migrated.The service replacement follows the same clean pattern as other commands in this migration.
Also applies to: 7-7, 11-11
api/src/__test__/core/utils/process/unraid-api-running.integration.test.ts (1)
7-53: LGTM! Comprehensive integration test with proper lifecycle management.The test suite covers all critical scenarios (missing pid file, live pid, invalid pid) with appropriate setup/teardown and dynamic module mocking. Good use of Vitest patterns.
api/src/unraid-api/cli/logs.command.ts (1)
3-3: LGTM! Logs command successfully migrated.The NodemonService integration is clean and consistent with the rest of the migration.
Also applies to: 11-11, 23-23
api/src/unraid-api/cli/stop.command.ts (1)
1-28: LGTM!The migration from PM2Service to NodemonService is clean and well-structured. The semantic change from the
--deleteflag to--forceis appropriate—in the PM2 context, "delete" removed the process from PM2's process list, whereas in the Nodemon context, "force" appropriately indicates using SIGKILL instead of SIGTERM for process termination.web/src/composables/gql/graphql.ts (1)
562-571: Auto-generated GraphQL types reflect schema changes correctly.These changes are auto-generated by GraphQL CodeGen and correctly reflect the backend schema migration from PM2 to Nodemon. The
nodemonfield now replacespm2inPackageVersions, and the newCpuPackagestype with telemetry support has been added.Based on learnings.
Also applies to: 883-883, 900-901, 1548-1549, 2070-2070
api/src/unraid-api/cli/start.command.ts (1)
1-33: LGTM! Clean migration to Nodemon.The PM2-based startup flow with its lifecycle management has been cleanly replaced with a simple delegation to
NodemonService.start(). The log level configuration is properly preserved and passed through environment variables. This simplification removes complexity while maintaining the same functionality.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1798 +/- ##
==========================================
+ Coverage 52.04% 52.19% +0.15%
==========================================
Files 874 873 -1
Lines 50372 50460 +88
Branches 5017 5050 +33
==========================================
+ Hits 26214 26337 +123
+ Misses 24083 24048 -35
Partials 75 75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/core/log.ts (1)
119-132: Arrays insanitizeParamscan leak sensitive values
sanitizeParamsrecurses only into plain objects and skips arrays, so something like{ users: [{ password: 'x' }] }will log the raw password. Given this is core logging, this is a non-trivial privacy/security risk.Consider extending masking to arrays of objects, e.g. by mapping over arrays and calling
sanitizeParamson object elements before logging.
🧹 Nitpick comments (9)
api/src/core/log.ts (2)
20-21: Clarify environment assumptions for disabling color and consider configurabilityThe comment and
colorize: false/colorizeObjects: falseenforce non-colored pretty output in all contexts, not only when logs are truly being redirected to a file. If you expect interactive/dev runs where stdout is a TTY, you might want a config flag (e.g.,LOG_COLORIZE) or a TTY check to re-enable color there, while keeping it off for file-piped production output.Also applies to: 25-32
18-19: Normalize SUPPRESS_LOGS handling to the imported constant
logDestinationchecksprocess.env.SUPPRESS_LOGS === 'true'directly whilestreamuses the importedSUPPRESS_LOGSflag. This duplication is easy to forget if env parsing changes and can also diverge ifprocess.envis mutated at runtime. Prefer routing both throughSUPPRESS_LOGSfor a single source of truth.Also applies to: 22-24
api/src/environment.ts (1)
105-116: Nodemon path/env constants look correct; ensure PID directory existsThe
NODEMON_PATH,NODEMON_CONFIG_PATH, andUNRAID_API_CWDcalculations fromimport.meta.dirnameand../../look consistent with both the monorepo layout and the packeddist + node_modulesrelease layout. With Node 20+ andimport.meta.dirnamethis should be robust.One thing to confirm:
NODEMON_PID_PATHdefaults to/var/run/unraid-api/nodemon.pid. Make sure whatever writes the PID file creates the/var/run/unraid-apidirectory (or its configured parent) before writing, otherwise you’ll getENOENTon first write on a clean system.Based on learnings.
api/scripts/build.ts (1)
10-10: Alias import for getDeploymentVersion is a bit subtle; consider simplifyingUsing
@app/../scripts/get-deployment-version.jsties this script to the@app/*path mapping and relies on the loader rewriting it to something like./src/../scripts/get-deployment-version.js. That’s a bit non-obvious for future readers and could break if the alias ever changes.Unless there’s a specific reason to route this through
@app, consider reverting to a plain relative import from the same directory, which is loader‑agnostic:-import { getDeploymentVersion } from '@app/../scripts/get-deployment-version.js'; +import { getDeploymentVersion } from './get-deployment-version.js';In any case, please run the build script (
pnpm --filter @unraid/api build:release) on a clean checkout to confirm this alias-based import resolves as expected.api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts (1)
38-38: Consider whether to return the dependency version range or the actual installed version.The current implementation returns
getPackageJson().dependencies?.nodemon, which may include a version range (e.g.,^3.0.0or~3.1.4) rather than the actual installed version. This is consistent with how dependency information is typically stored in package.json, but differs from other version fields that return actual running versions (e.g.,softwareVersions.nodereturns the runtime version like20.11.0).If you want to display the exact installed version, consider using
nodemon --versionor reading fromnode_modules/nodemon/package.json.api/src/unraid-api/cli/nodemon.service.ts (3)
45-50: Handle PID file races and read errors more defensively
getStoredPiddoes an existence check thenreadFile, which can still throw (e.g. if the file is removed between the two calls or unreadable). Right now any such error would bubble up intostop/status.Consider collapsing this into a single
readFilewith a catch and treating failures as “no PID”, e.g.:private async getStoredPid(): Promise<number | null> { try { const contents = (await readFile(NODEMON_PID_PATH, 'utf-8')).trim(); const pid = Number.parseInt(contents, 10); return Number.isNaN(pid) ? null : pid; } catch { return null; } }This makes
stop/statusresilient to transient file races or permissions issues.
87-105: Be cautious about removing PID file when kill fails
stopalways deletes the PID file in thefinallyblock, even ifprocess.killthrows for reasons other than “no such process” (e.g. permission issues). That can leave a still-running nodemon process without a PID file, making later status/stop operations think nothing is running.If you care about this edge case, consider:
- Only removing the PID file when
killsucceeds orisPidRunning(pid)returns false, or- Logging more explicitly when you remove the PID file after a failed kill so operators can track potential strays.
Not a blocker, but worth considering for robustness.
129-132: Logs command behavior is reasonable; consider friendlier error handlingUsing
execa('tail', ['-n',${lines}, PATHS_LOGS_FILE])is straightforward and leverages the system tail. If you expect this to run on systems wheretailor the log file might be missing, optionally wrap this in try/catch and translate failures into a clearer message like “No logs available yet” instead of surfacing a raw execa error.api/src/unraid-api/cli/start.command.ts (1)
20-32: Hardenrunerror handling and normalizeLOG_LEVELenvVerification confirms that
@OptiondefaultValuedoes not guarantee the property will be set in all invocation paths (e.g., programmatic calls bypassing commander parsing), so your concern aboutoptions.logLevelbeing undefined is valid.Two improvements are recommended:
- Wrap the call in a try/catch like
RestartCommand.runso CLI failures are logged cleanly and exit with a non‑zero code.- Normalize/fallback the env value so
LOG_LEVELis always a string:async run(_: string[], options: LogLevelOptions): Promise<void> { this.logger.info('Starting the Unraid API'); const logLevel = (options.logLevel ?? LOG_LEVEL).toUpperCase(); try { await this.nodemon.start({ env: { LOG_LEVEL: logLevel } }); } catch (error) { if (error instanceof Error) { this.logger.error(error.message); } else { this.logger.error('Unknown error occurred'); } process.exit(1); } }This keeps behavior consistent with
restartand avoids passing an undefined env value toexeca.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
api/ecosystem.config.json(0 hunks)api/generated-schema.graphql(1 hunks)api/legacy/generated-schema-legacy.graphql(1 hunks)api/nodemon.json(1 hunks)api/package.json(1 hunks)api/scripts/build.ts(2 hunks)api/src/__test__/core/utils/pm2/dummy-process.js(0 hunks)api/src/__test__/core/utils/pm2/unraid-api-running.integration.test.ts(0 hunks)api/src/__test__/core/utils/process/unraid-api-running.integration.test.ts(1 hunks)api/src/__test__/graphql/resolvers/rclone-api.service.test.ts(1 hunks)api/src/core/log.ts(1 hunks)api/src/core/utils/pm2/unraid-api-running.ts(0 hunks)api/src/core/utils/process/unraid-api-running.ts(1 hunks)api/src/environment.ts(1 hunks)api/src/unraid-api/cli/__test__/report.command.test.ts(3 hunks)api/src/unraid-api/cli/cli-services.module.ts(2 hunks)api/src/unraid-api/cli/cli.module.ts(2 hunks)api/src/unraid-api/cli/logs.command.ts(2 hunks)api/src/unraid-api/cli/nodemon.service.spec.ts(1 hunks)api/src/unraid-api/cli/nodemon.service.ts(1 hunks)api/src/unraid-api/cli/pm2.service.spec.ts(0 hunks)api/src/unraid-api/cli/pm2.service.ts(0 hunks)api/src/unraid-api/cli/report.command.ts(1 hunks)api/src/unraid-api/cli/restart.command.ts(2 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/cli/status.command.ts(1 hunks)api/src/unraid-api/cli/stop.command.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/versions/versions.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts(2 hunks)api/src/unraid-api/main.ts(0 hunks)web/composables/gql/graphql.ts(1 hunks)web/src/composables/gql/graphql.ts(5 hunks)web/src/composables/gql/index.ts(1 hunks)
💤 Files with no reviewable changes (7)
- api/src/unraid-api/main.ts
- api/src/unraid-api/cli/pm2.service.spec.ts
- api/src/core/utils/pm2/unraid-api-running.ts
- api/src/unraid-api/cli/pm2.service.ts
- api/src/test/core/utils/pm2/dummy-process.js
- api/src/test/core/utils/pm2/unraid-api-running.integration.test.ts
- api/ecosystem.config.json
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2025-09-14T14:07:28.265Z
Learnt from: elibosley
Repo: unraid/api PR: 1702
File: web/eslint.config.mjs:106-106
Timestamp: 2025-09-14T14:07:28.265Z
Learning: The unraid/api codebase specifically requires Node 20+ as a minimum version, so modern Node.js features like import.meta.dirname (available since Node v20.11.0) can be used without compatibility fallbacks.
Applied to files:
api/src/core/utils/process/unraid-api-running.tsapi/src/unraid-api/cli/report.command.tsapi/src/environment.tsapi/src/unraid-api/graph/resolvers/info/versions/versions.resolver.tsapi/src/unraid-api/cli/cli-services.module.tsapi/package.json
📚 Learning: 2025-01-27T14:57:46.617Z
Learnt from: elibosley
Repo: unraid/api PR: 1063
File: web/_data/serverState.ts:137-147
Timestamp: 2025-01-27T14:57:46.617Z
Learning: The values in `web/_data/serverState.ts` are used for testing purposes and should remain as hardcoded mock data to facilitate testing different scenarios.
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-02-03T18:57:28.442Z
Learnt from: elibosley
Repo: unraid/api PR: 1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts:55-56
Timestamp: 2025-02-03T18:57:28.442Z
Learning: Using console.log is acceptable in test files, especially for debugging test execution. The logger instances in test files are often used as mocks/dependencies for the classes being tested, not for actual logging.
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-03-27T13:34:53.438Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Applied to files:
api/src/unraid-api/cli/report.command.ts
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
Repo: unraid/api PR: 1575
File: packages/unraid-shared/src/services/socket-config.service.spec.ts:10-13
Timestamp: 2025-08-09T01:03:29.676Z
Learning: Vitest is used for all testing across all repositories in the unraid organization, not Jest. Always use `vi` for mocking utilities, not `jest`.
Applied to files:
api/src/__test__/core/utils/process/unraid-api-running.integration.test.tsapi/src/unraid-api/cli/nodemon.service.spec.tsapi/src/unraid-api/cli/__test__/report.command.test.ts
📚 Learning: 2025-03-04T14:55:00.903Z
Learnt from: elibosley
Repo: unraid/api PR: 1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Applied to files:
api/src/environment.ts
📚 Learning: 2025-03-12T13:48:14.850Z
Learnt from: pujitm
Repo: unraid/api PR: 1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Applied to files:
web/composables/gql/graphql.tsweb/src/composables/gql/index.ts
📚 Learning: 2025-02-06T17:25:45.397Z
Learnt from: mdatelle
Repo: unraid/api PR: 1122
File: web/package.json:16-16
Timestamp: 2025-02-06T17:25:45.397Z
Learning: The build script in web/package.json should include type-check command (`npm run type-check`) even when NODE_ENV is production, as it provides value for local builds by catching type errors before pushing to CI/CD.
Applied to files:
api/scripts/build.ts
📚 Learning: 2025-03-26T15:18:27.452Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: plugin/docker-compose.yml:15-15
Timestamp: 2025-03-26T15:18:27.452Z
Learning: The files in the `api/deploy` directory (including `packed-pnpm-store.txz`) are gitignored build artifacts that are generated when the API is built. Building the API is a prerequisite before using the plugin builder container, as it needs access to these artifacts.
Applied to files:
api/scripts/build.ts
📚 Learning: 2025-02-04T16:42:21.782Z
Learnt from: elibosley
Repo: unraid/api PR: 1105
File: api/scripts/build.mjs:0-0
Timestamp: 2025-02-04T16:42:21.782Z
Learning: In Node.js build scripts, avoid manually copying node_modules as a cache layer. Instead, rely on npm's built-in caching mechanism and use direct `npm install` commands, as this ensures dependency consistency and proper version resolution while still maintaining good performance through npm's cache.
Applied to files:
api/scripts/build.ts
📚 Learning: 2025-04-02T21:21:29.168Z
Learnt from: elibosley
Repo: unraid/api PR: 1308
File: unraid-ui/src/components/common/loading/Error.vue:2-2
Timestamp: 2025-04-02T21:21:29.168Z
Learning: Components in the unraid-ui folder require explicit imports and are not autoloaded, unlike other parts of the project that may use Nuxt.js autoloading features.
Applied to files:
api/src/unraid-api/cli/cli-services.module.ts
📚 Learning: 2025-05-23T21:59:29.632Z
Learnt from: elibosley
Repo: unraid/api PR: 1408
File: web/components/ApiKey/PermissionCounter.vue:6-6
Timestamp: 2025-05-23T21:59:29.632Z
Learning: This codebase uses ESM (ECMAScript Modules) and requires .js extensions in import statements, even when importing from TypeScript files, as the imports refer to the compiled JavaScript output.
Applied to files:
web/src/composables/gql/index.ts
📚 Learning: 2024-12-11T17:20:09.604Z
Learnt from: pujitm
Repo: unraid/api PR: 978
File: web/helpers/apollo-cache/index.ts:133-157
Timestamp: 2024-12-11T17:20:09.604Z
Learning: In `web/helpers/apollo-cache/index.ts`, when updating the cache in the `cache.updateQuery` function, it's sufficient to check if `data` is present, as type checking inside `data` is handled at the GraphQL layer.
Applied to files:
web/src/composables/gql/index.ts
📚 Learning: 2025-03-12T13:35:43.900Z
Learnt from: pujitm
Repo: unraid/api PR: 1211
File: web/codegen.ts:14-14
Timestamp: 2025-03-12T13:35:43.900Z
Learning: The JSON scalar type in web/codegen.ts was temporarily changed from 'string' to 'any' for compatibility with JsonForms integration. This change facilitates the implementation of the Connect settings web component.
Applied to files:
web/src/composables/gql/index.ts
📚 Learning: 2025-02-04T13:48:45.521Z
Learnt from: pujitm
Repo: unraid/api PR: 1104
File: api/src/unraid-api/cli/stop.command.ts:14-18
Timestamp: 2025-02-04T13:48:45.521Z
Learning: The PM2Service's `run` method automatically handles logging when `raw: true` is not set in the context argument. Explicit logging is not needed in such cases.
Applied to files:
api/src/unraid-api/cli/logs.command.ts
🧬 Code graph analysis (13)
api/src/core/utils/process/unraid-api-running.ts (2)
packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)api/src/environment.ts (1)
NODEMON_PID_PATH(114-114)
api/src/unraid-api/cli/report.command.ts (1)
api/src/core/utils/process/unraid-api-running.ts (1)
isUnraidApiRunning(6-23)
api/src/__test__/core/utils/process/unraid-api-running.integration.test.ts (1)
api/src/core/utils/process/unraid-api-running.ts (1)
isUnraidApiRunning(6-23)
api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts (1)
api/src/environment.ts (1)
getPackageJson(49-53)
api/src/unraid-api/cli/nodemon.service.spec.ts (1)
packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)
web/composables/gql/graphql.ts (1)
web/src/composables/gql/graphql.ts (2)
Maybe(3-3)Scalars(11-75)
api/src/unraid-api/cli/status.command.ts (4)
api/src/unraid-api/cli/logs.command.ts (1)
Command(9-25)api/src/unraid-api/cli/restart.command.ts (1)
Command(21-54)api/src/unraid-api/cli/start.command.ts (1)
Command(11-33)api/src/unraid-api/cli/stop.command.ts (1)
Command(8-28)
api/src/unraid-api/cli/stop.command.ts (4)
api/src/unraid-api/cli/logs.command.ts (1)
Command(9-25)api/src/unraid-api/cli/restart.command.ts (1)
Command(21-54)api/src/unraid-api/cli/start.command.ts (1)
Command(11-33)api/src/unraid-api/cli/status.command.ts (1)
Command(5-13)
api/src/unraid-api/cli/logs.command.ts (4)
api/src/unraid-api/cli/restart.command.ts (1)
Command(21-54)api/src/unraid-api/cli/start.command.ts (1)
Command(11-33)api/src/unraid-api/cli/status.command.ts (1)
Command(5-13)api/src/unraid-api/cli/stop.command.ts (1)
Command(8-28)
api/src/core/log.ts (1)
api/src/environment.ts (2)
SUPPRESS_LOGS(94-94)LOG_TYPE(88-88)
api/src/unraid-api/cli/nodemon.service.ts (3)
api/src/environment.ts (6)
PATHS_LOGS_DIR(101-102)NODEMON_PID_PATH(114-114)PATHS_LOGS_FILE(103-103)NODEMON_PATH(105-112)NODEMON_CONFIG_PATH(113-113)UNRAID_API_CWD(115-115)api/src/unraid-api/cli/log.service.ts (1)
error(65-69)packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)
api/src/unraid-api/cli/start.command.ts (4)
api/src/unraid-api/cli/logs.command.ts (1)
Command(9-25)api/src/unraid-api/cli/restart.command.ts (2)
Command(21-54)LogLevelOptions(9-11)api/src/unraid-api/cli/status.command.ts (1)
Command(5-13)api/src/unraid-api/cli/stop.command.ts (1)
Command(8-28)
web/src/composables/gql/graphql.ts (2)
api/src/unraid-api/cli/generated/graphql.ts (4)
CpuPackages(562-571)Node(1385-1387)Scalars(11-75)Maybe(3-3)web/composables/gql/graphql.ts (3)
Node(1357-1359)Scalars(11-75)Maybe(3-3)
🔇 Additional comments (26)
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts (1)
54-55: LGTM! Environment mock standardization.The addition of
SUPPRESS_LOGSandLOG_LEVELto the environment mock aligns with the broader PM2-to-nodemon migration and standardizes the test configuration. Since the Logger is already mocked at lines 65-76, these additions don't affect test behavior but provide consistency with other test files.api/package.json (1)
140-140: Confirm nodemon is intended as a runtime dependencyAdding
nodemontodependenciesmakes sense if deployments rely on it as the process manager (mirroring how PM2 was treated). Just double‑check there are no usage contexts where@unraid/apiis consumed purely as a library and this heavier runtime dependency would be undesirable, and that release builds indeed neednodemonavailable at runtime.api/scripts/build.ts (1)
97-97: Including nodemon.json in the release pack matches nodemon-managed deploymentsUpdating the pack step to
cp -r dist README.md .env.* nodemon.json ./deploy/pack/ensures the nodemon configuration is shipped with the release, which aligns with switching process management from PM2 to nodemon.Just confirm that
api/nodemon.jsonis always present in the repo (and in any CI/build contexts), since a missing file here will now cause the build to fail at the copy step.web/src/composables/gql/index.ts (1)
1-2: Stylistic quote change only; exports remain the sameOnly the quote style changed here; the exported modules and paths are unchanged, so there’s no behavioral impact.
api/legacy/generated-schema-legacy.graphql (1)
1260-1260: Renaming Versions.pm2 → Versions.nodemon is a schema-breaking changeUpdating the
Versionstype to exposenodemon: Stringinstead ofpm2aligns with the runtime change, but it is a breaking change for any clients that still querypm2against the legacy schema.If all consumers (including anything relying on this “legacy” schema) are updated in lockstep with this PR, this is fine. Otherwise, consider temporarily exposing both fields, or at least coordinating a versioned rollout so older clients don’t start failing GraphQL queries unexpectedly.
api/src/unraid-api/cli/report.command.ts (1)
36-40: LGTM!The import path and error message have been correctly updated to reflect the migration from PM2 to nodemon-based process management.
api/src/core/utils/process/unraid-api-running.ts (1)
6-23: LGTM!The implementation correctly checks if the Unraid API process is running by:
- Verifying the PID file exists
- Reading and validating the PID
- Using
process.kill(pid, 0)to check process liveness without sending a signalThis is a standard and reliable approach for process health checking.
api/generated-schema.graphql (1)
1676-1677: LGTM!The GraphQL schema has been correctly updated to replace the
pm2field withnodemonin thePackageVersionstype. This is an auto-generated file that reflects the changes in the underlying model.api/src/unraid-api/cli/__test__/report.command.test.ts (1)
29-34: LGTM!The test mocks have been correctly updated to reference the new process-based utility instead of PM2-specific code.
api/src/unraid-api/cli/cli-services.module.ts (2)
7-7: LGTM!The import has been correctly updated to use
NodemonServiceinstead ofPM2Service, aligning with the broader migration to nodemon-based process management.
24-24: LGTM!The providers array has been correctly updated to register
NodemonServiceinstead ofPM2Service.api/src/unraid-api/graph/resolvers/info/versions/versions.model.ts (1)
28-29: LGTM!The model has been correctly updated to replace the
pm2field withnodemon, consistent with the broader migration to nodemon-based process management.api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts (1)
6-6: LGTM!The import of
getPackageJsonis correctly added to support retrieving the nodemon version from package.json.api/src/__test__/core/utils/process/unraid-api-running.integration.test.ts (1)
7-54: Excellent test coverage for nodemon PID detection.The integration test is well-structured with proper setup/teardown lifecycle management and covers the key scenarios (missing file, valid PID, invalid PID). The use of dynamic imports with mocking ensures proper isolation between test cases.
api/src/unraid-api/cli/nodemon.service.spec.ts (1)
29-101: Comprehensive unit tests for NodemonService.The test suite provides excellent coverage of the service's key functionality: dependency setup, process starting, and status checking. The mocking strategy effectively isolates the service behavior, and the assertions verify all critical interactions with the filesystem and process management.
api/src/unraid-api/cli/status.command.ts (1)
3-11: Clean migration to NodemonService.The status command has been successfully migrated from PM2 to nodemon with a simplified implementation. The changes are consistent with the broader migration pattern across all CLI commands.
api/src/unraid-api/cli/logs.command.ts (1)
3-23: Successful migration to NodemonService.The logs command has been cleanly migrated to use NodemonService while maintaining the same functionality for viewing logs with configurable line counts.
web/composables/gql/graphql.ts (1)
1526-1527: GraphQL types updated to reflect nodemon migration.The generated types correctly reflect the schema change from
pm2tonodemonversion tracking. This aligns with the broader migration from PM2 to nodemon process management.api/src/unraid-api/cli/cli.module.ts (1)
16-67: Dependency injection updated for NodemonService.The CLI module has been correctly updated to provide NodemonService instead of PM2Service, ensuring all commands receive the new service through dependency injection.
api/src/unraid-api/cli/restart.command.ts (1)
5-35: Restart command successfully migrated to nodemon.The restart command has been cleanly migrated to use NodemonService with proper environment variable handling (uppercasing LOG_LEVEL). Error handling and logging remain consistent.
api/src/unraid-api/cli/stop.command.ts (1)
3-27: Excellent migration with improved naming.The stop command has been successfully migrated to NodemonService with a notable improvement: renaming the option from
--deleteto--forceprovides better semantic clarity for forcefully stopping the API process. The simplified implementation is cleaner and easier to maintain.web/src/composables/gql/graphql.ts (4)
562-571: CpuPackages type matches backend schema and Node interfaceThe new
CpuPackagesNode type and its fields (power/temp arrays plus totalPower) look consistent with the backend GraphQL snippet and existingNodeusage. No issues from the client typing perspective.
1548-1550: PackageVersions nodemon field aligns with pm2→nodemon migrationAdding
nodemon?: Maybe<Scalars['String']['output']>;toPackageVersions(and dropping pm2 on the API side) is consistent with the process manager swap. This keeps the web types in sync with the API’s version surface.
2069-2071: New CPU telemetry subscription field is consistent with CpuPackages
systemMetricsCpuTelemetry: CpuPackagescleanly reuses the new type for per-package telemetry and coexists withsystemMetricsCpu: CpuUtilization. Typing here looks correct; just ensure any consumers are updated when they start using this subscription.
883-901: Schema verification confirms code alignment – no action neededThe GraphQL schema in
api/generated-schema.graphql(lines 1474–1475) explicitly declares both fields as non-nullable:
topology: [[[Int!]!]!]!packages: CpuPackages!The
!suffix in GraphQL enforces non-nullability, meaning the server is required to return these fields. The TypeScript code correctly reflects this by not wrapping them inMaybe<...>. The code is properly aligned with the schema guarantees.Likely an incorrect or invalid review comment.
api/src/unraid-api/cli/nodemon.service.ts (1)
112-127: Status semantics and stale PID cleanup look good
statuscorrectly reports running/not-running and eagerly cleans up a stale PID file when the process is gone. Returning the boolean while still logging is useful for both CLI and programmatic callers; the implementation here looks solid.
elibosley
force-pushed
the
codex/switch-api-process-management-to-nodemon
branch
from
c6cb6d2 to
c1f8224
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts (1)
6-43: Nodemon version resolution is correct; consider caching for perfUsing
getPackageJson().dependencies?.nodemoninsidepackages()correctly reports the nodemon version and is safely wrapped in the existing try/catch. The only downside is a synchronouspackage.jsonread on each field resolution; ifinfo.versions.packagesis queried frequently, you might later introduce a simple in-memory cache forgetPackageJson()to avoid repeated disk I/O.api/src/unraid-api/cli/restart.command.ts (1)
23-36: Consider handlingLOG_LEVELfallback whenoptions.logLevelis absent
envis built as{ LOG_LEVEL: options.logLevel?.toUpperCase() }and then passed intothis.nodemon.restart({ env }). Ifrunis ever invoked programmatically without CLI option parsing (e.g.,restart.run()orrestart.run([], {})),options.logLevelwill beundefined, soLOG_LEVELwill also beundefinedin the env passed to NodemonService.Depending on how
NodemonService.restartusesenv, this could either:
- Silently override a default
LOG_LEVELwithundefined, or- Cause issues if child process spawning expects string env values.
To make this more robust, consider falling back to the configured default or omitting the key when it’s unset, e.g.:
- const env = { LOG_LEVEL: options.logLevel?.toUpperCase() }; - await this.nodemon.restart({ env }); + const logLevel = options.logLevel ?? LOG_LEVEL.toLowerCase(); + const env = { LOG_LEVEL: logLevel.toUpperCase() }; + await this.nodemon.restart({ env });(or filter out undefined keys inside
NodemonService).api/src/unraid-api/cli/start.command.ts (1)
8-8: AlignLOG_LEVELenv handling with a clear defaultSimilar to
RestartCommand,StartCommandbuildsenvas{ LOG_LEVEL: options.logLevel?.toUpperCase() }before callingthis.nodemon.start(...). While Nest Commander will typically supply a value due to the@Optiondefault, this method can still be called directly in code with an options object that omitslogLevel, which would propagateLOG_LEVEL: undefinedinto the NodemonService layer.To make behavior explicit and safer, consider normalizing the log level with a fallback to
LOG_LEVELand always passing a concrete string:- async run(_: string[], options: LogLevelOptions): Promise<void> { - this.logger.info('Starting the Unraid API'); - await this.nodemon.start({ env: { LOG_LEVEL: options.logLevel?.toUpperCase() } }); - } + async run(_: string[], options: LogLevelOptions): Promise<void> { + this.logger.info('Starting the Unraid API'); + const logLevel = options.logLevel ?? LOG_LEVEL.toLowerCase(); + await this.nodemon.start({ env: { LOG_LEVEL: logLevel.toUpperCase() } }); + }This mirrors the intended default and avoids leaking
undefinedinto the child process environment.Also applies to: 13-23
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
api/ecosystem.config.json(0 hunks)api/generated-schema.graphql(1 hunks)api/legacy/generated-schema-legacy.graphql(1 hunks)api/nodemon.json(1 hunks)api/package.json(1 hunks)api/scripts/build.ts(2 hunks)api/src/__test__/core/utils/pm2/dummy-process.js(0 hunks)api/src/__test__/core/utils/pm2/unraid-api-running.integration.test.ts(0 hunks)api/src/__test__/core/utils/process/unraid-api-running.integration.test.ts(1 hunks)api/src/__test__/graphql/resolvers/rclone-api.service.test.ts(1 hunks)api/src/core/log.ts(1 hunks)api/src/core/utils/pm2/unraid-api-running.ts(0 hunks)api/src/core/utils/process/unraid-api-running.ts(1 hunks)api/src/environment.ts(1 hunks)api/src/unraid-api/cli/__test__/report.command.test.ts(3 hunks)api/src/unraid-api/cli/cli-services.module.ts(2 hunks)api/src/unraid-api/cli/cli.module.ts(2 hunks)api/src/unraid-api/cli/logs.command.ts(2 hunks)api/src/unraid-api/cli/nodemon.service.spec.ts(1 hunks)api/src/unraid-api/cli/nodemon.service.ts(1 hunks)api/src/unraid-api/cli/pm2.service.spec.ts(0 hunks)api/src/unraid-api/cli/pm2.service.ts(0 hunks)api/src/unraid-api/cli/report.command.ts(1 hunks)api/src/unraid-api/cli/restart.command.ts(2 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/cli/status.command.ts(1 hunks)api/src/unraid-api/cli/stop.command.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/versions/versions.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts(2 hunks)api/src/unraid-api/main.ts(0 hunks)web/composables/gql/graphql.ts(1 hunks)web/src/composables/gql/graphql.ts(5 hunks)web/src/composables/gql/index.ts(1 hunks)
💤 Files with no reviewable changes (7)
- api/src/unraid-api/main.ts
- api/src/test/core/utils/pm2/unraid-api-running.integration.test.ts
- api/src/test/core/utils/pm2/dummy-process.js
- api/src/core/utils/pm2/unraid-api-running.ts
- api/ecosystem.config.json
- api/src/unraid-api/cli/pm2.service.ts
- api/src/unraid-api/cli/pm2.service.spec.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/composables/gql/index.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- api/package.json
- api/src/unraid-api/cli/cli-services.module.ts
- api/src/unraid-api/cli/report.command.ts
- api/scripts/build.ts
- api/src/unraid-api/graph/resolvers/info/versions/versions.model.ts
- api/src/unraid-api/cli/test/report.command.test.ts
- api/src/test/core/utils/process/unraid-api-running.integration.test.ts
- web/composables/gql/graphql.ts
- api/nodemon.json
- api/src/unraid-api/cli/status.command.ts
- api/src/unraid-api/cli/stop.command.ts
- api/src/unraid-api/cli/nodemon.service.spec.ts
- api/src/unraid-api/cli/nodemon.service.ts
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-02-03T18:57:28.442Z
Learnt from: elibosley
Repo: unraid/api PR: 1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts:55-56
Timestamp: 2025-02-03T18:57:28.442Z
Learning: Using console.log is acceptable in test files, especially for debugging test execution. The logger instances in test files are often used as mocks/dependencies for the classes being tested, not for actual logging.
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-03-04T14:55:00.903Z
Learnt from: elibosley
Repo: unraid/api PR: 1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Applied to files:
api/src/environment.ts
📚 Learning: 2025-02-05T14:43:48.568Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/package.json:0-0
Timestamp: 2025-02-05T14:43:48.568Z
Learning: In Node.js projects, npm scripts should be organized with clear namespacing (e.g., build:*, docker:*, env:*) and include proper environment validation and error handling. Each script should follow the single responsibility principle.
Applied to files:
api/src/environment.ts
📚 Learning: 2025-09-14T14:07:28.265Z
Learnt from: elibosley
Repo: unraid/api PR: 1702
File: web/eslint.config.mjs:106-106
Timestamp: 2025-09-14T14:07:28.265Z
Learning: The unraid/api codebase specifically requires Node 20+ as a minimum version, so modern Node.js features like import.meta.dirname (available since Node v20.11.0) can be used without compatibility fallbacks.
Applied to files:
api/src/environment.tsapi/src/core/utils/process/unraid-api-running.tsapi/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts
📚 Learning: 2025-03-27T13:34:53.438Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Applied to files:
api/src/core/utils/process/unraid-api-running.ts
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
Repo: unraid/api PR: 942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/core/utils/process/unraid-api-running.ts
📚 Learning: 2025-01-23T21:26:17.766Z
Learnt from: elibosley
Repo: unraid/api PR: 1063
File: api/src/unraid-api/cli/sso/validate-token.command.ts:76-81
Timestamp: 2025-01-23T21:26:17.766Z
Learning: In the unraid/api codebase, store dispatches using Redux Toolkit's `unwrap()` method already handle errors appropriately and don't require additional try-catch blocks.
Applied to files:
api/src/core/utils/process/unraid-api-running.ts
📚 Learning: 2025-02-05T14:43:25.062Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/package.json:1-8
Timestamp: 2025-02-05T14:43:25.062Z
Learning: The repository uses Renovate for automated dependency updates, making strict version pinning in package.json less critical as updates are handled automatically through PRs.
Applied to files:
api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts
📚 Learning: 2025-02-04T13:48:45.521Z
Learnt from: pujitm
Repo: unraid/api PR: 1104
File: api/src/unraid-api/cli/stop.command.ts:14-18
Timestamp: 2025-02-04T13:48:45.521Z
Learning: The PM2Service's `run` method automatically handles logging when `raw: true` is not set in the context argument. Explicit logging is not needed in such cases.
Applied to files:
api/src/unraid-api/cli/logs.command.ts
🧬 Code graph analysis (6)
api/src/core/log.ts (1)
api/src/environment.ts (2)
SUPPRESS_LOGS(94-94)LOG_TYPE(88-88)
api/src/core/utils/process/unraid-api-running.ts (2)
packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)api/src/environment.ts (1)
NODEMON_PID_PATH(114-114)
api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts (1)
api/src/environment.ts (1)
getPackageJson(49-53)
api/src/unraid-api/cli/logs.command.ts (4)
api/src/unraid-api/cli/restart.command.ts (1)
Command(21-54)api/src/unraid-api/cli/start.command.ts (1)
Command(11-33)api/src/unraid-api/cli/status.command.ts (1)
Command(5-13)api/src/unraid-api/cli/stop.command.ts (1)
Command(8-28)
web/src/composables/gql/graphql.ts (2)
api/src/unraid-api/cli/generated/graphql.ts (4)
CpuPackages(562-571)Node(1385-1387)Scalars(11-75)Maybe(3-3)web/composables/gql/graphql.ts (3)
Node(1357-1359)Scalars(11-75)Maybe(3-3)
api/src/unraid-api/cli/start.command.ts (4)
api/src/unraid-api/cli/logs.command.ts (1)
Command(9-25)api/src/unraid-api/cli/restart.command.ts (2)
Command(21-54)LogLevelOptions(9-11)api/src/unraid-api/cli/status.command.ts (1)
Command(5-13)api/src/unraid-api/cli/stop.command.ts (1)
Command(8-28)
🔇 Additional comments (14)
api/src/core/log.ts (1)
20-28: LGTM! Documentation updates align with nodemon migration.The comment updates correctly reflect the change from PM2-managed stdout to direct file piping. Disabling colorization (
colorize: false) remains appropriate to prevent ANSI escape codes from being written to the log file.api/src/__test__/graphql/resolvers/rclone-api.service.test.ts (1)
54-55: LGTM! Environment mock extended appropriately.The additions of
SUPPRESS_LOGSandLOG_LEVELto the environment mock are reasonable and align with standard testing practices. These logging configuration values are likely required by the code under test or its dependencies following the nodemon migration.api/generated-schema.graphql (1)
1676-1677: Schema nodemon version field looks consistentThe new
nodemon: Stringfield and its description align with the rest ofPackageVersionsand with the pm2→nodemon migration; no issues from the schema side.api/legacy/generated-schema-legacy.graphql (1)
1244-1271: Legacy Versions now exposes nodemon version correctlyAdding
nodemon: Stringto the legacyVersionstype keeps it in sync with the new schema and avoids divergence between legacy and current APIs.web/src/composables/gql/graphql.ts (4)
562-571: CpuPackages client type matches server schemaThe new
CpuPackagestype (Node +power,temp,totalPower) matches the schema and CLI-generated types; no adjustments needed.
883-901: InfoCpu packages/topology fields correctly modeledAdding
packages: CpuPackagesand the strongly-typedtopology: Array<Array<Array<Scalars['Int']['output']>>>matches the non-null schema definitions and should work as long as the backend resolvers always populate them.If not already covered, double-check the InfoCpu resolver/model always sets both fields to avoid
Cannot return null for non-nullable fielderrors under load.
1548-1549: PackageVersions nodemon field aligns with backendReplacing the pm2 field with
nodemon?: Maybe<Scalars['String']['output']>and its docstring keeps the frontend typings in sync with the updatedPackageVersionsschema.
2061-2071: Subscription wiring for systemMetricsCpuTelemetry looks correct
systemMetricsCpuTelemetry: CpuPackagesonSubscriptionmirrors the server-side addition and reuses the new CpuPackages type as expected.api/src/environment.ts (1)
105-115: Nodemon path and CWD constants now match repo layoutThe new
NODEMON_PATH,NODEMON_CONFIG_PATH,NODEMON_PID_PATH, andUNRAID_API_CWDvalues correctly resolve to root-levelnode_modules/nodemon,api/nodemon.json, the PID file, and the API working directory, fixing the earlier off-by-one issue while remaining overrideable via env vars.api/src/core/utils/process/unraid-api-running.ts (1)
1-23: Process-running check is robust and race-safe
isUnraidApiRunningcorrectly guards all I/O andprocess.killcalls in a try/catch, handles missing/invalid PID values, and safely returnsfalseon any error or race with the pidfile. This gives a clean boolean API for callers without leaking low-level errors.api/src/unraid-api/cli/logs.command.ts (1)
3-24: Logs command correctly migrated to NodemonServiceInjecting
NodemonServiceand delegatingrun()tothis.nodemon.logs(lines)aligns this command with the rest of the nodemon-based CLI lifecycle commands; option parsing semantics are unchanged.api/src/unraid-api/cli/cli.module.ts (1)
16-71: CLI module wiring for NodemonService is correctAdding
NodemonServiceto the imports andDEFAULT_PROVIDERSensures all lifecycle commands (start/stop/status/restart/logs/report) can inject it without additional module changes; the rest of the provider list is untouched.api/src/unraid-api/cli/restart.command.ts (1)
5-5: Consistent use ofLOG_LEVELfor CLI defaults looks goodImporting
LOG_LEVELfrom@app/environment.jsand reusing it as thedefaultValuefor the--log-leveloption keeps CLI defaults aligned with the runtime environment and avoids magic strings. Combined withparseLogLevelOption, this keeps validation centralized.Also applies to: 46-53
api/src/unraid-api/cli/start.command.ts (1)
6-6: Nice reuse of shared log-level parsing and defaultsUsing
LOG_LEVEL.toLowerCase()as thedefaultValueand delegating validation toparseLogLevelOptionreused fromrestart.command.tskeeps CLI behavior consistent acrossstartandrestartand avoids duplicating allowed-level logic.Also applies to: 25-32
elibosley
force-pushed
the
codex/switch-api-process-management-to-nodemon
branch
from
c1f8224 to
8f2c826
Compare
…demonService - Updated nodemon.json to remove unnecessary watch entry. - Adjusted NODEMON_CONFIG_PATH and UNRAID_API_CWD paths for better structure. - Enhanced error handling in isUnraidApiRunning and start methods of NodemonService to ensure proper logging and resource management. - Added tests for error scenarios in NodemonService to ensure robustness.
- Introduced UNRAID_API_ROOT to streamline path definitions for nodemon. - Replaced direct usage of import.meta.dirname with UNRAID_API_ROOT in NODEMON_PATH and UNRAID_API_CWD for improved clarity and maintainability. - Added dirname import to facilitate the new path structure.
elibosley
force-pushed
the
codex/switch-api-process-management-to-nodemon
branch
from
8f2c826 to
b35da13
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/src/core/log.ts (1)
20-28: LGTM! Comment updates align with the PM2-to-nodemon migration.The comment changes appropriately remove PM2-specific references and make the rationale more generic. The logic remains unchanged, and the comments accurately reflect the deployment context where stdout is redirected to log files.
Optional refinement for clarity:
Consider being more explicit that stdout is redirected rather than stating logs are written directly to file. For example:
-// Since process output is piped directly to the log file, we should not colorize stdout +// Since stdout is redirected to a log file in production, we should not colorize outputand
- colorize: false, // No colors since logs are written directly to file + colorize: false, // No colors since stdout is redirected to fileThis would more precisely reflect that
pino.destination()writes to stdout by default, with external redirection handling the file output.api/src/unraid-api/cli/nodemon.service.ts (2)
55-98: Consider handling execa’s promise to avoid unhandled rejections
start()intentionally treats nodemon as a background daemon and never awaits theexecapromise, but when nodemon fails very early (spawn error or immediate non‑zero exit), that promise can reject without a.catch, potentially surfacing as an unhandled‑rejection warning even though you also throw a synchronousErrorhere.You can keep the fire‑and‑forget behavior but explicitly observe the promise, for example:
- nodemonProcess = execa(NODEMON_PATH, ['--config', NODEMON_CONFIG_PATH, '--quiet'], { + nodemonProcess = execa(NODEMON_PATH, ['--config', NODEMON_CONFIG_PATH, '--quiet'], { cwd: UNRAID_API_CWD, env, detached: true, stdio: ['ignore', 'pipe', 'pipe'], }); + + // Prevent unhandled rejection noise for this long-running daemon. + nodemonProcess.catch(() => { + // Intentionally ignored; lifecycle is managed via PID + status(). + });This keeps the external behavior the same while hardening against noisy unhandled rejections.
100-118: Stop semantics are acceptable but always clear the PID
stop()is straightforward and robust against missing PID files; note that it removes the PID file even whenprocess.killthrows (e.g., ESRCH). That’s arguably desirable cleanup, but if you ever need to debug “couldn’t signal process X”, consider logging at warn/error as you already do and possibly guarding the PID removal by error type in a follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
api/ecosystem.config.json(0 hunks)api/generated-schema.graphql(1 hunks)api/legacy/generated-schema-legacy.graphql(1 hunks)api/nodemon.json(1 hunks)api/package.json(1 hunks)api/scripts/build.ts(2 hunks)api/src/__test__/core/utils/pm2/dummy-process.js(0 hunks)api/src/__test__/core/utils/pm2/unraid-api-running.integration.test.ts(0 hunks)api/src/__test__/core/utils/process/unraid-api-running.integration.test.ts(1 hunks)api/src/__test__/graphql/resolvers/rclone-api.service.test.ts(1 hunks)api/src/core/log.ts(1 hunks)api/src/core/utils/pm2/unraid-api-running.ts(0 hunks)api/src/core/utils/process/unraid-api-running.ts(1 hunks)api/src/environment.ts(3 hunks)api/src/unraid-api/cli/__test__/report.command.test.ts(3 hunks)api/src/unraid-api/cli/cli-services.module.ts(2 hunks)api/src/unraid-api/cli/cli.module.ts(2 hunks)api/src/unraid-api/cli/logs.command.ts(2 hunks)api/src/unraid-api/cli/nodemon.service.spec.ts(1 hunks)api/src/unraid-api/cli/nodemon.service.ts(1 hunks)api/src/unraid-api/cli/pm2.service.spec.ts(0 hunks)api/src/unraid-api/cli/pm2.service.ts(0 hunks)api/src/unraid-api/cli/report.command.ts(1 hunks)api/src/unraid-api/cli/restart.command.ts(2 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/cli/status.command.ts(1 hunks)api/src/unraid-api/cli/stop.command.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/versions/versions.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts(2 hunks)api/src/unraid-api/main.ts(0 hunks)web/composables/gql/graphql.ts(1 hunks)web/src/composables/gql/graphql.ts(5 hunks)web/src/composables/gql/index.ts(1 hunks)
💤 Files with no reviewable changes (7)
- api/src/unraid-api/cli/pm2.service.spec.ts
- api/src/test/core/utils/pm2/unraid-api-running.integration.test.ts
- api/src/unraid-api/main.ts
- api/src/core/utils/pm2/unraid-api-running.ts
- api/src/test/core/utils/pm2/dummy-process.js
- api/src/unraid-api/cli/pm2.service.ts
- api/ecosystem.config.json
✅ Files skipped from review due to trivial changes (1)
- web/src/composables/gql/index.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- api/src/unraid-api/cli/cli-services.module.ts
- api/src/test/core/utils/process/unraid-api-running.integration.test.ts
- api/src/unraid-api/cli/nodemon.service.spec.ts
- api/src/unraid-api/cli/test/report.command.test.ts
- api/src/unraid-api/cli/report.command.ts
- api/src/unraid-api/cli/start.command.ts
- api/src/unraid-api/cli/cli.module.ts
- api/nodemon.json
- api/package.json
- api/generated-schema.graphql
🧰 Additional context used
🧠 Learnings (21)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
📚 Learning: 2025-02-04T13:48:45.521Z
Learnt from: pujitm
Repo: unraid/api PR: 1104
File: api/src/unraid-api/cli/stop.command.ts:14-18
Timestamp: 2025-02-04T13:48:45.521Z
Learning: The PM2Service's `run` method automatically handles logging when `raw: true` is not set in the context argument. Explicit logging is not needed in such cases.
Applied to files:
api/src/unraid-api/cli/logs.command.ts
📚 Learning: 2025-03-12T13:48:14.850Z
Learnt from: pujitm
Repo: unraid/api PR: 1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Applied to files:
web/composables/gql/graphql.ts
📚 Learning: 2025-09-14T14:07:28.265Z
Learnt from: elibosley
Repo: unraid/api PR: 1702
File: web/eslint.config.mjs:106-106
Timestamp: 2025-09-14T14:07:28.265Z
Learning: The unraid/api codebase specifically requires Node 20+ as a minimum version, so modern Node.js features like import.meta.dirname (available since Node v20.11.0) can be used without compatibility fallbacks.
Applied to files:
api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.tsapi/src/unraid-api/graph/resolvers/info/versions/versions.model.tsapi/src/core/utils/process/unraid-api-running.tsapi/src/environment.ts
📚 Learning: 2025-03-27T13:34:53.438Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Applied to files:
api/src/core/utils/process/unraid-api-running.tsapi/src/environment.ts
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
Repo: unraid/api PR: 942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/core/utils/process/unraid-api-running.tsapi/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-01-23T21:26:17.766Z
Learnt from: elibosley
Repo: unraid/api PR: 1063
File: api/src/unraid-api/cli/sso/validate-token.command.ts:76-81
Timestamp: 2025-01-23T21:26:17.766Z
Learning: In the unraid/api codebase, store dispatches using Redux Toolkit's `unwrap()` method already handle errors appropriately and don't require additional try-catch blocks.
Applied to files:
api/src/core/utils/process/unraid-api-running.ts
📚 Learning: 2025-02-03T18:57:28.442Z
Learnt from: elibosley
Repo: unraid/api PR: 1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts:55-56
Timestamp: 2025-02-03T18:57:28.442Z
Learning: Using console.log is acceptable in test files, especially for debugging test execution. The logger instances in test files are often used as mocks/dependencies for the classes being tested, not for actual logging.
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-02-06T17:25:45.397Z
Learnt from: mdatelle
Repo: unraid/api PR: 1122
File: web/package.json:16-16
Timestamp: 2025-02-06T17:25:45.397Z
Learning: The build script in web/package.json should include type-check command (`npm run type-check`) even when NODE_ENV is production, as it provides value for local builds by catching type errors before pushing to CI/CD.
Applied to files:
api/scripts/build.tsapi/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-03-26T15:18:27.452Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: plugin/docker-compose.yml:15-15
Timestamp: 2025-03-26T15:18:27.452Z
Learning: The files in the `api/deploy` directory (including `packed-pnpm-store.txz`) are gitignored build artifacts that are generated when the API is built. Building the API is a prerequisite before using the plugin builder container, as it needs access to these artifacts.
Applied to files:
api/scripts/build.ts
📚 Learning: 2025-02-04T16:42:21.782Z
Learnt from: elibosley
Repo: unraid/api PR: 1105
File: api/scripts/build.mjs:0-0
Timestamp: 2025-02-04T16:42:21.782Z
Learning: In Node.js build scripts, avoid manually copying node_modules as a cache layer. Instead, rely on npm's built-in caching mechanism and use direct `npm install` commands, as this ensures dependency consistency and proper version resolution while still maintaining good performance through npm's cache.
Applied to files:
api/scripts/build.ts
📚 Learning: 2025-03-04T14:55:00.903Z
Learnt from: elibosley
Repo: unraid/api PR: 1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Applied to files:
api/src/environment.ts
📚 Learning: 2025-02-05T14:43:48.568Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/package.json:0-0
Timestamp: 2025-02-05T14:43:48.568Z
Learning: In Node.js projects, npm scripts should be organized with clear namespacing (e.g., build:*, docker:*, env:*) and include proper environment validation and error handling. Each script should follow the single responsibility principle.
Applied to files:
api/src/environment.tsapi/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
api/src/environment.ts
📚 Learning: 2025-05-23T21:59:29.632Z
Learnt from: elibosley
Repo: unraid/api PR: 1408
File: web/components/ApiKey/PermissionCounter.vue:6-6
Timestamp: 2025-05-23T21:59:29.632Z
Learning: This codebase uses ESM (ECMAScript Modules) and requires .js extensions in import statements, even when importing from TypeScript files, as the imports refer to the compiled JavaScript output.
Applied to files:
api/src/environment.ts
📚 Learning: 2025-04-23T20:22:36.001Z
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: api/src/unraid-api/app/pubsub.module.ts:24-28
Timestamp: 2025-04-23T20:22:36.001Z
Learning: In the Unraid API project, string literals are intentionally used for dependency injection tokens instead of Symbols to enable "collision" behavior across modules that don't directly import from each other. This is a temporary approach during refactoring until a shared library for constants is implemented.
Applied to files:
api/src/environment.ts
📚 Learning: 2025-09-02T19:35:14.427Z
Learnt from: elibosley
Repo: unraid/api PR: 1639
File: web/components/standalone-mount.ts:38-54
Timestamp: 2025-09-02T19:35:14.427Z
Learning: In the unraid/api project, exposing Apollo client and GraphQL utilities (apolloClient, gql, graphqlParse) on the global window object is intentional to allow developers to interact with GraphQL from the browser console without needing to be inside Vue components. Developer experience is prioritized over minimizing global exposure in this context.
Applied to files:
api/src/environment.ts
📚 Learning: 2025-04-21T18:25:16.491Z
Learnt from: pujitm
Repo: unraid/api PR: 1352
File: api/src/unraid-api/cli/dependency.service.ts:114-117
Timestamp: 2025-04-21T18:25:16.491Z
Learning: Error masking through try/catch blocks is not desirable for the `npmInstall` method in `DependencyService` as errors should propagate to the caller.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-03-07T19:29:12.540Z
Learnt from: pujitm
Repo: unraid/api PR: 1219
File: api/src/unraid-api/main.ts:18-25
Timestamp: 2025-03-07T19:29:12.540Z
Learning: When handling type mismatches between dependencies in TypeScript (such as between NestJS and Fastify), prefer using `ts-expect-error` with a detailed explanation comment instead of `as any` type assertions. This better documents the reason for the type bypass and makes the issue more visible for future maintenance.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-04-21T18:25:16.491Z
Learnt from: pujitm
Repo: unraid/api PR: 1352
File: api/src/unraid-api/cli/dependency.service.ts:114-117
Timestamp: 2025-04-21T18:25:16.491Z
Learning: Methods in `DependencyService` like `npmInstall` and `rebuildVendorArchive` intentionally propagate errors from execa calls upward rather than handling them internally, allowing callers to implement appropriate error handling. This follows the pattern documented in the `rebuildVendorArchive` method's docstring.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-03-13T16:17:55.820Z
Learnt from: elibosley
Repo: unraid/api PR: 1211
File: api/src/unraid-api/graph/connect/connect-settings.service.ts:48-53
Timestamp: 2025-03-13T16:17:55.820Z
Learning: In NestJS services, dynamic imports should be wrapped in try/catch blocks with proper error logging and fallback handling to prevent unhandled rejections from crashing the application.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
🧬 Code graph analysis (9)
api/src/unraid-api/cli/logs.command.ts (4)
api/src/unraid-api/cli/restart.command.ts (1)
Command(21-54)api/src/unraid-api/cli/start.command.ts (1)
Command(11-33)api/src/unraid-api/cli/status.command.ts (1)
Command(5-13)api/src/unraid-api/cli/stop.command.ts (1)
Command(8-28)
web/composables/gql/graphql.ts (1)
web/src/composables/gql/graphql.ts (2)
Maybe(3-3)Scalars(11-75)
api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts (1)
api/src/environment.ts (1)
getPackageJson(49-53)
api/src/unraid-api/cli/status.command.ts (4)
api/src/unraid-api/cli/logs.command.ts (1)
Command(9-25)api/src/unraid-api/cli/restart.command.ts (1)
Command(21-54)api/src/unraid-api/cli/start.command.ts (1)
Command(11-33)api/src/unraid-api/cli/stop.command.ts (1)
Command(8-28)
api/src/core/log.ts (1)
api/src/environment.ts (2)
SUPPRESS_LOGS(95-95)LOG_TYPE(89-89)
api/src/core/utils/process/unraid-api-running.ts (2)
packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)api/src/environment.ts (1)
NODEMON_PID_PATH(108-108)
web/src/composables/gql/graphql.ts (1)
api/src/unraid-api/cli/generated/graphql.ts (3)
CpuPackages(562-571)Node(1385-1387)Scalars(11-75)
api/src/unraid-api/cli/stop.command.ts (4)
api/src/unraid-api/cli/logs.command.ts (1)
Command(9-25)api/src/unraid-api/cli/restart.command.ts (1)
Command(21-54)api/src/unraid-api/cli/start.command.ts (1)
Command(11-33)api/src/unraid-api/cli/status.command.ts (1)
Command(5-13)
api/src/unraid-api/cli/nodemon.service.ts (3)
api/src/environment.ts (6)
PATHS_LOGS_DIR(102-103)NODEMON_PID_PATH(108-108)PATHS_LOGS_FILE(104-104)NODEMON_PATH(106-106)NODEMON_CONFIG_PATH(107-107)UNRAID_API_CWD(109-109)packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)api/src/unraid-api/cli/log.service.ts (1)
error(65-69)
🔇 Additional comments (20)
api/scripts/build.ts (1)
97-97: LGTM! Nodemon config copy aligns with PM2 migration.The change from copying
ecosystem.config.jsontonodemon.jsoncorrectly reflects the migration from PM2 to nodemon process management, ensuring the nodemon configuration is packaged in the release artifacts.api/legacy/generated-schema-legacy.graphql (1)
1260-1260: LGTM! GraphQL schema updated for nodemon migration.The field change from
pm2: Stringtonodemon: Stringin theVersionstype correctly reflects the transition from PM2 to nodemon process management in the GraphQL API surface.api/src/unraid-api/graph/resolvers/info/versions/versions.model.ts (1)
28-29: LGTM! Model definition updated consistently with schema.The field change from
pm2?: stringtonodemon?: stringwith the updated description properly reflects the nodemon migration and maintains consistency with the GraphQL schema changes.web/composables/gql/graphql.ts (1)
1526-1527: LGTM! Generated types updated from GraphQL schema.The
PackageVersionstype change frompm2tonodemonis auto-generated by GraphQL CodeGen and correctly reflects the updated schema. As learnings indicate, the generated code structure is intentional for proper type system function.api/src/environment.ts (1)
5-5: LGTM! Clean path derivation using UNRAID_API_ROOT.The introduction of
UNRAID_API_ROOTas a base for nodemon-related paths is a solid improvement over the previous approach. Usingdirname(getPackageJsonPath())provides a reliable anchor point for:
NODEMON_CONFIG_PATH→api/nodemon.jsonNODEMON_PATH→api/node_modules/nodemon/bin/nodemon.js(valid in pnpm workspaces)UNRAID_API_CWD→api/(correct working directory)This approach is more maintainable and addresses the path construction issues noted in previous reviews.
Also applies to: 68-68, 106-109
api/src/unraid-api/graph/resolvers/info/versions/versions.resolver.ts (1)
6-6: Clarify nodemon version format - dependency declaration vs installed version.The code now reads the nodemon version from
package.jsondependencies usinggetPackageJson().dependencies?.nodemon. This will return the dependency declaration (e.g.,"^3.0.0") rather than the actual installed version.Is it acceptable to show the semver range specification, or should this display the resolved installed version instead? If the latter, consider using
await versions()from systeminformation (which is already imported) or reading fromnode_modules/nodemon/package.json.Current behavior: Shows
"^3.0.0"(declared dependency)
Alternative: Show"3.1.4"(installed version)Also applies to: 38-38
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts (1)
54-55: LGTM! Test mock extended with logging controls.Adding
SUPPRESS_LOGSandLOG_LEVELto the environment mock provides proper test-level control over logging behavior, aligning with the logging controls used in the nodemon-based process management workflow.api/src/core/utils/process/unraid-api-running.ts (1)
1-23: LGTM! Robust nodemon process check with proper error handling.The implementation correctly verifies if the Unraid API is running under nodemon by:
- Checking PID file existence at
NODEMON_PID_PATH- Reading and parsing the PID
- Verifying the process is alive using
process.kill(pid, 0)The comprehensive try-catch wrapper properly handles all error cases (missing file, I/O errors, invalid PID, dead process) by returning
false, addressing the concerns raised in previous reviews. This provides a clean boolean interface without error leakage.web/src/composables/gql/graphql.ts (4)
562-571: CpuPackages type matches backend schemaThe
CpuPackagesnode shape (id/power/temp/totalPower) aligns with the CLI-generated GraphQL types and looks correct for client usage.
883-901: InfoCpu extensions are consistent and well-documentedAdding
packages: CpuPackagesand thetopologytriple-nested array with an example docstring makes the per-package CPU view clear and type-safe.
1548-1549: nodemon version field fits PackageVersions modelIntroducing the optional
nodemonversion alongside existing package fields cleanly reflects the runtime process manager swap.
2070-2070: CPU telemetry subscription wiring looks correct
systemMetricsCpuTelemetry: CpuPackagesmatches the newCpuPackagesnode and should integrate cleanly with telemetry consumers.api/src/unraid-api/cli/nodemon.service.ts (4)
34-44: PID file handling helper is reasonable
ensureNodemonDependencies+getStoredPid/parseIntgive a clear, small surface for PID management; letting mkdir errors propagate is appropriate so callers can abort early.
46-53: Process liveness check is simple and idiomaticUsing
process.kill(pid, 0)with a try/catch to probe liveness is a standard, low-overhead approach here.
125-140: Status correctly reconciles stale PID files
status()returning a boolean and cleaning up stale PID files when the process is no longer running provides a good single source of truth for higher-level commands.
142-160: Logs helper behavior is caller-friendly
logs()tails viaexeca('tail', ...), logs the content, and returns it, while gracefully handling file-missing/errors and emitting clear messages. Returning an empty string on failure is a reasonable, non-throwing contract for CLI callers.api/src/unraid-api/cli/logs.command.ts (1)
3-24: LogsCommand–NodemonService integration looks cleanConstructor injection of
NodemonServiceand delegatinglogs(lines)after simple, defensive parsing of the--linesoption is straightforward and keeps CLI logic thin.api/src/unraid-api/cli/status.command.ts (1)
3-12: StatusCommand correctly delegates to NodemonServiceSwapping to
NodemonServiceand simply awaitingnodemon.status()keeps the command minimal and aligns with the new process-manager abstraction.api/src/unraid-api/cli/restart.command.ts (1)
5-53: Restart flow via NodemonService is consistent and robustDelegating restarts to
nodemon.restart({ env })with normalizedLOG_LEVEL, plus structured try/catch that logs a clear message and exits with code 1 on failure, gives a clean and predictable restart UX in line with the start command.api/src/unraid-api/cli/stop.command.ts (1)
3-27: StopCommand force option and NodemonService wiring make intent clearInjecting
NodemonServiceand exposing a-f, --forceflag that maps directly to{ force }fornodemon.stopmakes the stop behavior explicit (SIGTERM vs SIGKILL) while keeping the command logic minimal.
| import { $, cd } from 'zx'; | ||
|
|
||
| import { getDeploymentVersion } from './get-deployment-version.js'; | ||
| import { getDeploymentVersion } from '@app/../scripts/get-deployment-version.js'; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the import path change for getDeploymentVersion.
The import path changed from './get-deployment-version.js' (resolving to api/scripts/get-deployment-version.js) to '@app/../scripts/get-deployment-version.js' (resolving to scripts/get-deployment-version.js at repo root). This suggests the file was moved from the api/scripts directory to a root-level scripts directory.
Run the following script to verify the file exists at the expected location:
🏁 Script executed:
#!/bin/bash
# Description: Verify get-deployment-version.js exists at the new location
echo "Checking for get-deployment-version.js..."
if [ -f "scripts/get-deployment-version.js" ]; then
echo "✓ Found at scripts/get-deployment-version.js"
echo "File contents preview:"
head -10 scripts/get-deployment-version.js
elif [ -f "api/scripts/get-deployment-version.js" ]; then
echo "⚠ File still exists at old location: api/scripts/get-deployment-version.js"
echo "The import path may be incorrect"
else
echo "✗ File not found at either location"
fiLength of output: 77
The file doesn't exist at either location. Let me search the entire repository to determine where this file actually is:
🏁 Script executed:
#!/bin/bash
# Search for any files with get-deployment-version in the name
echo "Searching for get-deployment-version files in repo..."
fd "get-deployment-version" --type f
echo -e "\n---"
echo "Checking if file exists anywhere with different extension..."
fd "get-deployment-version" --type f -e ts -e js -e mjs
echo -e "\n---"
echo "Searching for references to getDeploymentVersion..."
rg "getDeploymentVersion" -n --type ts --type js 2>/dev/null | head -20Length of output: 632
Let me verify the module alias configuration to understand how @app resolves:
🏁 Script executed:
#!/bin/bash
# Find tsconfig or path alias configuration
echo "Looking for tsconfig.json files..."
fd "tsconfig" --type f
echo -e "\n---"
echo "Checking for path alias configuration in tsconfig..."
rg '"@app"' --type json -A2 -B2
echo -e "\n---"
echo "Checking import in build.ts context..."
rg -n -B2 -A5 "import.*getDeploymentVersion" git/api/scripts/build.tsLength of output: 885
Let me check the specific tsconfig.json in the api directory where build.ts is located:
🏁 Script executed:
#!/bin/bash
# Check the api tsconfig.json for @app alias
echo "Contents of api/tsconfig.json:"
cat git/api/tsconfig.json
echo -e "\n---"
echo "Checking for package.json in api directory:"
cat git/api/package.json 2>/dev/null | head -40
echo -e "\n---"
echo "Double-checking - what is the correct relative import from build.ts to get-deployment-version.ts?"
echo "build.ts location: api/scripts/build.ts"
echo "get-deployment-version.ts location: api/scripts/get-deployment-version.ts"
echo "Correct import should be: './get-deployment-version.js' or './get-deployment-version.ts'"Length of output: 4417
Fix broken import path and extension in build.ts line 10.
The import path is incorrect on multiple levels:
@app/../scripts/get-deployment-version.jsresolves to./scripts/get-deployment-version.js(repo root) via the@appalias mapping ("@app/*": ["./src/*"]inapi/tsconfig.json)- The actual file is located at
api/scripts/get-deployment-version.ts(same directory asbuild.ts) - The file does not exist at the resolved path
Change line 10 to:
import { getDeploymentVersion } from './get-deployment-version.js';🤖 Prompt for AI Agents
In api/scripts/build.ts around line 10, the import path uses the @app alias and
points to @app/../scripts/get-deployment-version.js which resolves incorrectly
to the repo root and the target file doesn't exist there; update the import to
reference the local sibling module by changing it to import from
'./get-deployment-version.js' so it correctly loads
api/scripts/get-deployment-version.ts (compiled .js at runtime).
… handling - Introduced a new test file to validate nodemon path configurations, ensuring they anchor to the package root by default. - Enhanced the NodemonService to throw an error when nodemon exits immediately after starting, improving robustness. - Added tests to cover scenarios where nodemon fails to start, ensuring proper logging and resource cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/__test__/environment.nodemon-paths.test.ts (1)
1-28: Solid test for nodemon path derivation; consider adding a second case for explicit CWD.This test is well-structured: it safely snapshots/restores
UNRAID_API_CWD, resets modules before each run so@app/environment.jsre-evaluates with a clean env, and asserts the nodemon paths are anchored toUNRAID_API_ROOTas expected. No functional issues spotted.As a small enhancement, you might add a second test that sets
process.env.UNRAID_API_CWDbefore importing@app/environment.jsand asserts thatUNRAID_API_CWDreflects the override whileNODEMON_CONFIG_PATHandNODEMON_PATHstill anchor toUNRAID_API_ROOT. That would lock in both default and overridden behavior for the new env wiring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/src/__test__/environment.nodemon-paths.test.ts(1 hunks)api/src/unraid-api/cli/nodemon.service.spec.ts(1 hunks)api/src/unraid-api/cli/nodemon.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/unraid-api/cli/nodemon.service.spec.ts
- api/src/unraid-api/cli/nodemon.service.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
🧬 Code graph analysis (1)
api/src/__test__/environment.nodemon-paths.test.ts (1)
api/src/environment.ts (5)
environment(73-75)UNRAID_API_CWD(109-109)UNRAID_API_ROOT(68-68)NODEMON_CONFIG_PATH(107-107)NODEMON_PATH(106-106)
- Updated the return type of killSpy in nodemon.service.spec.ts to directly return a boolean instead of casting it, improving code clarity and maintainability.
- Implemented stopPm2IfRunning method to stop any running pm2-managed instances of unraid-api before starting nodemon. - Added findMatchingNodemonPids method to identify existing nodemon processes, improving resource management. - Updated start method to handle scenarios where a stored pid is running or dead, ensuring proper cleanup and logging. - Introduced new unit tests to validate the new functionality and ensure robustness in process handling.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/src/unraid-api/cli/nodemon.service.ts (3)
4-5: Remove unused imports flagged by static analysis
homedir(Line 4) andjoin(Line 5) are imported but never used. Please drop them to keep the module clean and satisfy the CodeQL notice.-import { homedir } from 'node:os'; -import { dirname, join } from 'node:path'; +import { dirname } from 'node:path';
35-38: Optionally add explicit return type to clarify intent
ensureNodemonDependenciescurrently relies on inference. For a public-ish service API, giving it an explicitPromise<void>return type makes the contract clearer and consistent with other async methods:- async ensureNodemonDependencies() { + async ensureNodemonDependencies(): Promise<void> { await mkdir(PATHS_LOGS_DIR, { recursive: true }); await mkdir(dirname(NODEMON_PID_PATH), { recursive: true }); }
140-182: Avoid double‑closinglogStreamand verify long‑running log behaviorTwo related points in
start:
- Double
logStream.close()on error paths
logStream.close()is called inside theif (!nodemonProcess.pid)andif (!stillRunning)branches (Lines 160 and 172) and then again in thecatchblock (Line 179). This is redundant and can lead to awkward double‑close behavior.You can rely on the
catchblock to close the stream and remove the inner closes:- if (!nodemonProcess.pid) { - logStream.close(); - throw new Error('Failed to start nodemon: process spawned but no PID was assigned'); - } + if (!nodemonProcess.pid) { + throw new Error('Failed to start nodemon: process spawned but no PID was assigned'); + } @@ - if (!stillRunning) { - const recentLogs = await this.logs(50); - await rm(NODEMON_PID_PATH, { force: true }); - logStream.close(); - const logMessage = recentLogs ? ` Recent logs:\n${recentLogs}` : ''; - throw new Error(`Nodemon exited immediately after start.${logMessage}`); - } + if (!stillRunning) { + const recentLogs = await this.logs(50); + await rm(NODEMON_PID_PATH, { force: true }); + const logMessage = recentLogs ? ` Recent logs:\n${recentLogs}` : ''; + throw new Error(`Nodemon exited immediately after start.${logMessage}`); + }
- Confirm that logs continue after the CLI process exits
Right now, nodemon’s
stdout/stderrare piped into acreateWriteStream(PATHS_LOGS_FILE)owned by this process. If the CLI process exits shortly afterstartreturns (which is typical for a detached daemon pattern), any further logging from nodemon will no longer go through this stream.If PATHS_LOGS_FILE is intended to capture long‑running API logs via this pipeline, consider instead wiring the log file directly via
stdioFDs in theexecaoptions so the child owns its log descriptors even after the CLI exits. For example (sketch only, adjust to your conventions):// Before spawning: const outFd = fs.openSync(PATHS_LOGS_FILE, 'a'); const errFd = fs.openSync(PATHS_LOGS_FILE, 'a'); const nodemonProcess = execa(NODEMON_PATH, ['--config', NODEMON_CONFIG_PATH, '--quiet'], { cwd: UNRAID_API_CWD, env, detached: true, stdio: ['ignore', outFd, errFd], });Then you can drop the
logStreamand explicit piping altogether.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/unraid-api/cli/nodemon.service.spec.ts(1 hunks)api/src/unraid-api/cli/nodemon.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/cli/nodemon.service.spec.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
📚 Learning: 2025-04-21T18:25:16.491Z
Learnt from: pujitm
Repo: unraid/api PR: 1352
File: api/src/unraid-api/cli/dependency.service.ts:114-117
Timestamp: 2025-04-21T18:25:16.491Z
Learning: Error masking through try/catch blocks is not desirable for the `npmInstall` method in `DependencyService` as errors should propagate to the caller.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-03-07T19:29:12.540Z
Learnt from: pujitm
Repo: unraid/api PR: 1219
File: api/src/unraid-api/main.ts:18-25
Timestamp: 2025-03-07T19:29:12.540Z
Learning: When handling type mismatches between dependencies in TypeScript (such as between NestJS and Fastify), prefer using `ts-expect-error` with a detailed explanation comment instead of `as any` type assertions. This better documents the reason for the type bypass and makes the issue more visible for future maintenance.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-04-21T18:25:16.491Z
Learnt from: pujitm
Repo: unraid/api PR: 1352
File: api/src/unraid-api/cli/dependency.service.ts:114-117
Timestamp: 2025-04-21T18:25:16.491Z
Learning: Methods in `DependencyService` like `npmInstall` and `rebuildVendorArchive` intentionally propagate errors from execa calls upward rather than handling them internally, allowing callers to implement appropriate error handling. This follows the pattern documented in the `rebuildVendorArchive` method's docstring.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-03-13T16:17:55.820Z
Learnt from: elibosley
Repo: unraid/api PR: 1211
File: api/src/unraid-api/graph/connect/connect-settings.service.ts:48-53
Timestamp: 2025-03-13T16:17:55.820Z
Learning: In NestJS services, dynamic imports should be wrapped in try/catch blocks with proper error logging and fallback handling to prevent unhandled rejections from crashing the application.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
Repo: unraid/api PR: 942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-02-06T17:25:45.397Z
Learnt from: mdatelle
Repo: unraid/api PR: 1122
File: web/package.json:16-16
Timestamp: 2025-02-06T17:25:45.397Z
Learning: The build script in web/package.json should include type-check command (`npm run type-check`) even when NODE_ENV is production, as it provides value for local builds by catching type errors before pushing to CI/CD.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
📚 Learning: 2025-02-05T14:43:48.568Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/package.json:0-0
Timestamp: 2025-02-05T14:43:48.568Z
Learning: In Node.js projects, npm scripts should be organized with clear namespacing (e.g., build:*, docker:*, env:*) and include proper environment validation and error handling. Each script should follow the single responsibility principle.
Applied to files:
api/src/unraid-api/cli/nodemon.service.ts
🧬 Code graph analysis (1)
api/src/unraid-api/cli/nodemon.service.ts (3)
api/src/environment.ts (6)
PATHS_LOGS_DIR(102-103)NODEMON_PID_PATH(108-108)NODEMON_CONFIG_PATH(107-107)PATHS_LOGS_FILE(104-104)NODEMON_PATH(106-106)UNRAID_API_CWD(109-109)packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)api/src/unraid-api/cli/log.service.ts (1)
error(65-69)
🪛 GitHub Check: CodeQL
api/src/unraid-api/cli/nodemon.service.ts
[notice] 4-4: Unused variable, import, function or class
Unused import homedir.
[notice] 5-5: Unused variable, import, function or class
Unused import join.
🔇 Additional comments (2)
api/src/unraid-api/cli/nodemon.service.ts (2)
70-84: PID file/liveness helpers look solid
getStoredPidandisPidRunningare straightforward and robust (NaN‑safe parsing andkill(pid, 0)check), which should work well with the rest of the lifecycle methods.
227-245: Nice, logs() now handles missing file/errors gracefullyWrapping the
tailcall in try/catch and distinguishing ENOENT from other failures yields much clearer diagnostics while returning a safe fallback string for callers.
- Implemented findDirectMainPids and terminatePids methods to identify and terminate existing unraid-api processes before starting nodemon. - Enhanced the start method to include checks for running processes, ensuring proper cleanup and logging. - Updated unit tests to validate the new process management functionality, improving overall robustness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/cli/nodemon.service.spec.ts (1)
63-64: Simplify mock return values.The explicit type casting is unnecessary since
mockResolvedValue(undefined)correctly types toPromise<void>.Apply this diff to simplify:
- mockWriteFile.mockResolvedValue(undefined as unknown as void); - mockRm.mockResolvedValue(undefined as unknown as void); + mockWriteFile.mockResolvedValue(undefined); + mockRm.mockResolvedValue(undefined);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/unraid-api/cli/nodemon.service.spec.ts(1 hunks)api/src/unraid-api/cli/nodemon.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/cli/nodemon.service.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
🧬 Code graph analysis (1)
api/src/unraid-api/cli/nodemon.service.spec.ts (1)
packages/unraid-shared/src/util/file.ts (1)
fileExists(12-15)
🔇 Additional comments (4)
api/src/unraid-api/cli/nodemon.service.spec.ts (4)
18-27: Environment mock is now complete.The addition of
SUPPRESS_LOGSresolves the previous pipeline failure. The mock now correctly mirrors all necessary exports fromenvironment.ts.
91-312: Comprehensive start method test coverage.The test suite thoroughly covers the
startmethod's behavior across multiple scenarios:
- ✅ Successful start with proper setup and PID file management
- ✅ Error handling with appropriate cleanup (closing streams, removing stale files)
- ✅ Edge cases (already running, stale PID files, immediate exit detection)
- ✅ Process discovery and adoption
- ✅ Termination of conflicting direct main.js processes
The tests properly verify that cleanup occurs on failures and that diagnostic information is collected when nodemon exits unexpectedly.
314-362: Well-tested status and logs methods.The tests for
statusandlogsmethods properly verify:
- Correct status reporting when PID file is absent
- Successful log retrieval via tail command
- Graceful error handling for missing log files (ENOENT)
- Proper error logging for tail command failures
1-363: Excellent test suite with thorough coverage.This test file demonstrates strong testing practices:
- ✅ Comprehensive coverage of all public methods
- ✅ Proper mock setup and isolation via
beforeEach- ✅ Both success and failure paths tested
- ✅ Error handling and cleanup verified
- ✅ Edge cases covered (stale PIDs, already running, immediate exit)
- ✅ Appropriate assertions for behavior and logging
The test suite provides confidence that NodemonService will handle various runtime scenarios correctly.
…n NodemonService - Added waitForNodemonExit method to ensure nodemon processes are fully terminated before restarting. - Updated restart method to call waitForNodemonExit, improving process management during restarts. - Introduced a new unit test to validate the behavior of the restart method, ensuring proper sequence of stop, wait, and start operations.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary
Testing
Codex Task
Summary by CodeRabbit
Refactor
New Features
Updates
Tests
✏️ Tip: You can customize this high-level summary in your review settings.