feat: add cpu power query & subscription #1745
Conversation
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
WalkthroughAdds per‑CPU‑package model and telemetry: new GraphQL type CpuPackages, CpuTopologyService that reads /sys for topology/power/temps, CpuModule wiring, InfoCpu.packages and topology fields, a systemMetricsCpuTelemetry subscription, module/test updates, and a deploy script in unraid-shared. Changes
Sequence Diagram(s)sequenceDiagram
participant Sub as Subscriber
participant MR as MetricsResolver
participant CTS as CpuTopologyService
participant Sysfs as /sys
participant Pub as PubSub
Note over MR,CTS: onModuleInit registers CPU_TELEMETRY handler
Sub->>MR: subscribe systemMetricsCpuTelemetry
MR->>Pub: listen CPU_TELEMETRY
loop periodic telemetry
Pub->>CTS: trigger generateTelemetry (handler)
CTS->>Sysfs: read powercap (energy_uj), hwmon (temps), cpu topology
Sysfs-->>CTS: power & temp & topology data
CTS->>CTS: aggregate per-package power/temp, compute totalPower
CTS-->>MR: CpuPackages payload
MR->>Pub: publish CPU_TELEMETRY with payload
Pub-->>Sub: deliver systemMetricsCpuTelemetry
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1745 +/- ##
==========================================
- Coverage 52.69% 52.68% -0.01%
==========================================
Files 865 867 +2
Lines 49343 49582 +239
Branches 4952 4982 +30
==========================================
+ Hits 26000 26121 +121
- Misses 23270 23388 +118
Partials 73 73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
23-33: Guard against NaN for stepping; consider latency impact of power fetch.
- Number(stepping) can yield NaN (GraphQL Int cannot represent NaN). Guard it.
- generateCpuPower() performs a ~200ms sampling; this adds latency to the InfoCpu query. Consider caching, lazy resolution via a separate field resolver, or background sampling.
- power: await this.cpuPowerService.generateCpuPower(), - stepping: Number(stepping), + power: await this.cpuPowerService.generateCpuPower(), + stepping: stepping != null && `${stepping}` !== '' ? Number(stepping) : undefined,
♻️ Duplicate comments (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
2-4: Remove unused imports (CodeQL findings).fsConstants, access, readdir, readFile, basename, join are unused in this file.
-import { constants as fsConstants } from 'node:fs'; -import { access, readdir, readFile } from 'node:fs/promises'; -import { basename, join } from 'node:path';
🧹 Nitpick comments (7)
api/src/unraid-api/graph/resolvers/info/info.module.ts (1)
5-5: Remove unused CpuService import.CpuService is no longer used directly in InfoModule since it's now provided by CpuModule (line 18).
Apply this diff to remove the unused import:
-import { CpuService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.service.js';api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (1)
3-5: Remove unused imports.CpuPowerService and CpuService are imported but not directly used in MetricsModule. Both services are now provided by CpuModule (line 11), making these direct imports unnecessary.
Apply this diff to remove the unused imports:
-import { CpuPowerService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu-power.service.js'; import { CpuModule } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.module.js'; -import { CpuService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.service.js';api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (1)
51-61: Clarify naming vs. semantics (packages/sockets vs. cores).The field name coresPower suggests per-core values, but RAPL reads package/socket power. Either rename the field or update the description to avoid confusion.
Apply this minimal-description fix:
- description: 'CPU power in watts for each physical processor', + description: 'CPU power in watts for each CPU package/socket (physical processor)',api/src/unraid-api/graph/resolvers/info/cpu/cpu-power.service.ts (4)
4-4: Usenode:pathfor ESM consistency.Prefer Node built-in ESM specifier.
-import { basename, join } from 'path'; +import { basename, join } from 'node:path';
46-48: Make sampling window a constant/configurable.Hardcoded 200ms can be a knob for accuracy vs. latency; extract to a const.
- // ~200ms delay to measure energy delta - await new Promise((resolve) => setTimeout(resolve, 200)); + // Sampling window to measure energy delta + const measureWindowMs = 200; + await new Promise((resolve) => setTimeout(resolve, measureWindowMs));
71-78: Handle RAPL energy counter wrap-around.energy_uj can wrap; adjust diff using max_energy_range_uj when negative.
- const diffEnergy = now - prevE; // microjoules + let diffEnergy = now - prevE; // microjoules + if (diffEnergy < 0) { + try { + const maxFile = join(domainPath, 'max_energy_range_uj'); + await access(maxFile, fsConstants.R_OK); + const maxStr = await readFile(maxFile, 'utf8'); + const maxRange = Number.parseInt(maxStr.trim(), 10); + if (Number.isFinite(maxRange) && maxRange > 0) { + diffEnergy = (now + maxRange) - prevE; + } + } catch { + // if wrap-around can't be determined, skip this sample + continue; + } + } const diffTimeNs = Number(tNow - prevT); // nanoseconds if (!(diffTimeNs > 0)) continue;
132-134: Log unexpected errors for observability.Capture and log the error; return safe default.
- } catch { - return { id }; - } + } catch (e) { + this.logger.debug(`generateCpuPower failed: ${String(e)}`); + return { id }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
api/generated-schema.graphql(3 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu-power.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts(2 hunks)api/src/unraid-api/graph/resolvers/info/info.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts(4 hunks)packages/unraid-shared/src/pubsub/graphql.pubsub.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.module.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.module.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu-power.service.tsapi/src/unraid-api/graph/resolvers/info/info.module.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.tspackages/unraid-shared/src/pubsub/graphql.pubsub.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.module.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.module.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu-power.service.tsapi/src/unraid-api/graph/resolvers/info/info.module.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.module.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.module.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu-power.service.tsapi/src/unraid-api/graph/resolvers/info/info.module.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts
🧠 Learnings (2)
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
PR: unraid/api#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/unraid-api/graph/resolvers/info/info.resolver.integration.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
📚 Learning: 2025-09-12T01:36:59.838Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T01:36:59.838Z
Learning: Applies to api/**/*.{test,spec}.{ts,tsx} : API test suite is Vitest; do not use Jest
Applied to files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts
🧬 Code graph analysis (6)
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (3)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts (1)
Module(6-10)api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
Module(37-76)api/src/unraid-api/graph/services/services.module.ts (1)
Module(8-13)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (2)
api/src/unraid-api/graph/resolvers/info/info.model.ts (1)
ObjectType(13-44)api/src/unraid-api/cli/generated/graphql.ts (1)
Node(1371-1373)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts (2)
api/src/unraid-api/graph/resolvers/info/info.module.ts (1)
Module(17-37)api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (1)
Module(10-15)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-power.service.ts (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
Injectable(11-54)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-power.service.ts (1)
Injectable(8-136)
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (2)
api/src/unraid-api/cli/generated/graphql.ts (1)
Subscription(2046-2058)web/composables/gql/graphql.ts (1)
Subscription(2032-2044)
🪛 GitHub Check: CodeQL
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts
[notice] 3-3: Unused variable, import, function or class
Unused import CpuPowerService.
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts
[notice] 2-2: Unused variable, import, function or class
Unused import fsConstants.
[notice] 3-3: Unused variable, import, function or class
Unused imports access, readFile, readdir.
[notice] 4-4: Unused variable, import, function or class
Unused imports basename, join.
🔇 Additional comments (7)
packages/unraid-shared/src/pubsub/graphql.pubsub.ts (1)
8-8: LGTM!The CPU_POWER channel addition is consistent with existing patterns and properly supports the new CPU power metrics feature.
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts (1)
3-3: LGTM!The test properly mocks the CpuPowerService dependency and updates expectations to include the new power field. The mock implementation is appropriately simple for unit testing.
Also applies to: 92-99, 130-130
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts (1)
8-8: LGTM!The integration test correctly includes CpuPowerService in the DI container, enabling proper integration testing of CPU power metrics.
Also applies to: 26-26
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts (1)
6-6: LGTM!The test correctly accommodates the new CpuPowerService dependency and updates the topic registration expectation from 2 to 3 to account for the new CPU_POWER topic.
Also applies to: 22-22, 168-168, 176-176
api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts (1)
1-10: LGTM!The CpuModule is well-structured and follows NestJS conventions. The module properly encapsulates CPU-related services and exports them for use by other modules.
api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.ts (1)
9-9: LGTM!The integration test correctly includes CpuPowerService in the DI container, ensuring the new CPU power functionality is properly tested.
Also applies to: 32-32
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (1)
91-101: Approve CPU power subscription wiringPUBSUB_CHANNEL.CPU_POWER is defined in the shared GRAPHQL_PUBSUB_CHANNEL enum.
| @@ -1,13 +1,15 @@ | |||
| import { Module } from '@nestjs/common'; | |||
|
|
|||
| import { CpuTopologyService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.js'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 days ago
The best way to fix this problem is to delete the unused import line for CpuTopologyService from the module file. Specifically, remove line 3: import { CpuTopologyService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.js'; from api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts. No additional imports, methods, or definitions are needed. This change can be made in isolation and will not affect the current functionality, as the imported symbol was not referenced anywhere in the file.
| @@ -1,6 +1,5 @@ | ||
| import { Module } from '@nestjs/common'; | ||
|
|
||
| import { CpuTopologyService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.js'; | ||
| import { CpuModule } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.module.js'; | ||
| import { CpuService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.service.js'; | ||
| import { MemoryService } from '@app/unraid-api/graph/resolvers/info/memory/memory.service.js'; |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
api/generated-schema.graphql (1)
2669-2671: Regenerate GraphQL client typings for new subscriptions.Clients likely miss
systemMetricsCpuPowerandsystemMetricsCpuTelemetry. Regenerate the codegen outputs across CLI/plugin/web.#!/bin/bash # Locate client graphql typings fd 'graphql.ts' | sed -n '1,200p' # Check presence of new fields rg -n 'systemMetricsCpuPower|systemMetricsCpuTelemetry' -t ts -C2 || true # Run project codegen pnpm run generate || npm run generate || yarn generateapi/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (1)
3-5: Remove unused imports (CodeQL warning).
CpuTopologyServiceandCpuServiceare not referenced in this module.import { Module } from '@nestjs/common'; -import { CpuTopologyService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.js'; -import { CpuModule } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.module.js'; +import { CpuModule } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.module.js'; -import { CpuService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.service.js'; import { MemoryService } from '@app/unraid-api/graph/resolvers/info/memory/memory.service.js'; import { MetricsResolver } from '@app/unraid-api/graph/resolvers/metrics/metrics.resolver.js'; import { ServicesModule } from '@app/unraid-api/graph/services/services.module.js';api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
2-4: Drop unused Node fs/path imports.These are not used and trigger CodeQL “unused import” notices.
-import { constants as fsConstants } from 'node:fs'; -import { access, readdir, readFile } from 'node:fs/promises'; -import { basename, join } from 'node:path';api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (1)
66-74: Comment interval mismatch (should be 5s).The code uses 5000ms; comment says 1 second. Please update.
- // Register CPU power polling with 1 second interval + // Register CPU power polling with 5 second interval
🧹 Nitpick comments (6)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
20-23: Consider removing unused injection until needed.
CpuPowerServiceis injected but unused here; keep only if a field resolver consumes it elsewhere.If unused across the service, remove it from the constructor to avoid dead deps. Otherwise, add a TODO indicating which resolver uses it.
api/generated-schema.graphql (1)
1404-1412: Field naming clarity:coresPowerseems per-package.If values are per physical package, consider renaming to
packagesPowerorperPackagePowerfor clarity before GA.api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (2)
175-177: Harden package-id inference for non-package-Xlabels.Fallback to parsing the domain path (e.g.,
intel-rapl:1) likeCpuPowerServicedoes.- const pkgMatch = label.match(/package-(\d+)/i); - const pkgId = pkgMatch ? Number(pkgMatch[1]) : 0; + const pkgMatch = label.match(/package-(\d+)/i); + let pkgId = pkgMatch ? Number(pkgMatch[1]) : 0; + if (!pkgMatch) { + const base = p.split('/').pop() ?? ''; + const m2 = base.match(/:(\d+)/); + if (m2) pkgId = Number(m2[1]); + }
12-13: Remove unused cache or implement usage.
topologyCacheis defined but never used; either wire it up or drop it.api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (2)
63-74: Name doesn’t match description; clarify per-core vs per-package.
coresPowersuggests per-core, but description says “each physical processor.” Either:
- rename to
packagePower(orprocessorsPower) if per-package; or- fix the description if it’s truly per-core.
export class CpuPower extends Node { @Field(() => Float, { nullable: true, description: 'CPU power in watts' }) totalPower?: number; - @Field(() => [Float], { - nullable: true, - description: 'CPU power in watts for each physical processor', - }) - coresPower?: number[]; + @Field(() => [Float], { + nullable: true, + description: 'CPU power in watts for each physical processor (package)', + }) + packagePower?: number[]; }
131-135: Type alias for readability of triple-nested arrays.Define a named alias (e.g.,
type CpuTopology = number[][][];) and use it here to improve readability and future reuse.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/generated-schema.graphql(3 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts(4 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.module.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.module.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.module.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.module.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.module.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.module.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts
🧬 Code graph analysis (6)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
Injectable(16-78)
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (3)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts (1)
Module(7-11)api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
Module(37-76)api/src/unraid-api/graph/services/services.module.ts (1)
Module(8-13)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (2)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (1)
Injectable(8-208)api/src/unraid-api/graph/resolvers/info/cpu/cpu-power.service.ts (1)
Injectable(8-136)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts (2)
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (1)
Module(10-15)api/src/unraid-api/graph/resolvers/info/info.module.ts (1)
Module(17-37)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (2)
api/src/unraid-api/graph/resolvers/info/info.model.ts (1)
ObjectType(13-44)api/src/unraid-api/cli/generated/graphql.ts (1)
Node(1371-1373)
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (3)
api/src/core/pubsub.ts (1)
pubsub(12-12)api/src/unraid-api/cli/generated/graphql.ts (1)
Subscription(2046-2058)web/composables/gql/graphql.ts (1)
Subscription(2032-2044)
🪛 GitHub Actions: CI - Main (API)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
[error] 199-199: TS2345: Argument of type '{ id: number; power: number; temp: number; }' is not assignable to parameter of type 'never'.
api/generated-schema.graphql
[error] 1-1: Build failed during code generation. GraphQL Code Generator could not load schema from './generated-schema.graphql': Unknown type: 'number'. Command failed: 'tsx ./scripts/build.ts'.
🪛 GitHub Check: CodeQL
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
[notice] 182-182: Unused variable, import, function or class
Unused variable pkgId.
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts
[notice] 3-3: Unused variable, import, function or class
Unused import CpuTopologyService.
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts
[notice] 2-2: Unused variable, import, function or class
Unused import fsConstants.
[notice] 3-3: Unused variable, import, function or class
Unused imports access, readFile, readdir.
[notice] 4-4: Unused variable, import, function or class
Unused imports basename, join.
🔇 Additional comments (4)
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (1)
11-13: Module wiring looks good.Importing
CpuModuleand trimming providers is correct.api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts (1)
7-11: LGTM — module composition is correct.Providers and exports align with consumers; ESM
.jspaths follow our guideline.api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (2)
24-31: Module wiring is correctly configured; no action required.The verification confirms that
MetricsModuleimportsCpuModule(line 11 of metrics.module.ts), which provides bothCpuPowerServiceandCpuTopologyService. Dependency injection will resolve these services correctly in the constructor.
43-64: The review suggestion to changetotalpowertototalPoweris incorrect.The original code is already type-safe. The
CpuPackagesmodel inapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsdefines the property astotalpower(lowercase 'p') at line 45, so the code in metrics.resolver.ts is correct. Changing it tototalPowerwould break the type contract and cause TypeScript errors.The only valid suggestion is a minor comment clarity: line 43 could say "Register CPU telemetry polling" instead of "Register CPU power polling" to align with the channel name
CPU_TELEMETRY, but this is a cosmetic improvement, not a code fix.Likely an incorrect or invalid review comment.
| type CpuPackages implements Node { | ||
| """Total CPU package power draw (W)""" | ||
| totalpower: number | ||
|
|
||
| """Power draw per package (W)""" | ||
| power: [Float!] | ||
|
|
||
| """description: 'Temperature per package (°C)""" | ||
| temp: [Float!] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Schema blocker: invalid type and missing id in CpuPackages.
totalpower: numberis invalid GraphQL (should beFloat).CpuPackages implements Nodebut has noid: PrefixedID!.- Consider casing consistency:
totalpower→totalPower.
Apply these changes in the code-first model (e.g., cpu.model.ts) and regenerate the schema:
type CpuPackages implements Node {
- """Total CPU package power draw (W)"""
- totalpower: number
+ id: PrefixedID!
+ """Total CPU package power draw (W)"""
+ totalPower: Float
"""Power draw per package (W)"""
power: [Float!]
- """description: 'Temperature per package (°C)"""
+ """Temperature per package (°C)"""
temp: [Float!]
}Then re-run codegen to fix the pipeline error.
Verification:
🏁 Script executed:
#!/bin/bash
# Fail build if invalid scalar 'number' remains in schema
rg -n ':[[:space:]]*number\b' api/generated-schema.graphql && exit 1 || echo "OK: no 'number' scalars"
# Regenerate (project-specific)
pnpm run generate || npm run generate || yarn generateLength of output: 25
🏁 Script executed:
#!/bin/bash
# Get full CpuPackages type definition with context
rg -n -A 15 'type CpuPackages' api/generated-schema.graphql
# Check if InfoCpu type exists and its fields
rg -n -A 10 'type InfoCpu' api/generated-schema.graphql
# Search for code-first model files (TypeScript/TS)
fd -e ts -e js --type f | xargs rg -l 'CpuPackages|class.*Cpu' | head -20Length of output: 1000
🏁 Script executed:
#!/bin/bash
# Get full InfoCpu definition (may span multiple lines)
rg -n -A 30 'type InfoCpu implements Node' api/generated-schema.graphql | head -50
# Inspect the code-first model to see source definitions
cat api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsLength of output: 5068
Schema blocker: invalid number scalar and missing id in CpuPackages type.
The code-first model (api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts) has structural issues that cause invalid schema generation:
- Line 1416 in schema:
totalpower: numberis invalid GraphQL (must beFloat). The model uses@Field(() => Float)but TypeScript field isnumber— change to match decorator intent. CpuPackagesimplementsNodein schema but lacksid: PrefixedID!field. Model must extendNodeor include theidfield.- Casing inconsistency:
totalpowershould betotalPower(camelCase convention). - Line 1421 schema docstring malformed:
"""description: 'Temperature...should be"""Temperature....
Update the model:
-@ObjectType()
+@ObjectType({ implements: () => Node })
-export class CpuPackages {
- @Field(() => Float, { description: 'Total CPU package power draw (W)' })
- totalpower?: number;
+export class CpuPackages extends Node {
+ @Field(() => Float, { description: 'Total CPU package power draw (W)' })
+ totalPower?: number;Then regenerate the schema.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/generated-schema.graphql around lines 1414 to 1423 the CpuPackages type
is invalid: change the code-first model
(api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts) so the class
implements Node (or add an id: PrefixedID! field with @Field(() => PrefixedID)
id: PrefixedID), rename the field from totalpower to totalPower to follow
camelCase, ensure the field uses @Field(() => Float) with a TypeScript number
type so the generated schema emits Float (not the literal "number"), and fix the
temp docstring to a plain description ("""Temperature per package (°C)""");
after these edits regenerate the schema.
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
Outdated
Show resolved
Hide resolved
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
Outdated
Show resolved
Hide resolved
| const totalpower = | ||
| Math.round(packageList.reduce((sum, pkg) => sum + (pkg.power ?? 0), 0) * 100) / 100; | ||
|
|
||
| // Build packages object \u2014 plain object matching CpuPackages GraphQL type | ||
| const packages: CpuPackages = { | ||
| totalpower, | ||
| power: packageList.map((pkg) => pkg.power ?? -1), | ||
| temp: packageList.map((pkg) => pkg.temp ?? -1), | ||
| }; |
There was a problem hiding this comment.
Fix total power math; ignore sentinel values.
pkg.power ?? 0 adds -1 (unknown) into the sum, producing incorrect totals.
- const totalpower =
- Math.round(packageList.reduce((sum, pkg) => sum + (pkg.power ?? 0), 0) * 100) / 100;
+ const totalpower =
+ Math.round(
+ packageList.reduce(
+ (sum, pkg) => sum + (typeof pkg.power === 'number' && pkg.power >= 0 ? pkg.power : 0),
+ 0
+ ) * 100
+ ) / 100;
- // Build packages object — plain object matching CpuPackages GraphQL type
- const packages: CpuPackages = {
+ // Build packages object — plain object matching CpuPackages GraphQL type
+ const packages: CpuPackages = {
+ // Ensure schema and model include id if CpuPackages implements Node
+ // id: 'info/cpu-packages',
totalpower,
power: packageList.map((pkg) => pkg.power ?? -1),
temp: packageList.map((pkg) => pkg.temp ?? -1),
};Also ensure the GraphQL model for CpuPackages aligns (field name casing and presence of id). As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const totalpower = | |
| Math.round(packageList.reduce((sum, pkg) => sum + (pkg.power ?? 0), 0) * 100) / 100; | |
| // Build packages object \u2014 plain object matching CpuPackages GraphQL type | |
| const packages: CpuPackages = { | |
| totalpower, | |
| power: packageList.map((pkg) => pkg.power ?? -1), | |
| temp: packageList.map((pkg) => pkg.temp ?? -1), | |
| }; | |
| const totalpower = | |
| Math.round( | |
| packageList.reduce( | |
| (sum, pkg) => sum + (typeof pkg.power === 'number' && pkg.power >= 0 ? pkg.power : 0), | |
| 0 | |
| ) * 100 | |
| ) / 100; | |
| // Build packages object — plain object matching CpuPackages GraphQL type | |
| const packages: CpuPackages = { | |
| // Ensure schema and model include id if CpuPackages implements Node | |
| // id: 'info/cpu-packages', | |
| totalpower, | |
| power: packageList.map((pkg) => pkg.power ?? -1), | |
| temp: packageList.map((pkg) => pkg.temp ?? -1), | |
| }; |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts around lines 35 to
43, the total power calculation currently sums sentinel -1 values and uses a
field casing that may not match the GraphQL model; filter out package.power
values that are null/undefined or equal to the sentinel (-1) before summing and
rounding (i.e., only include valid non-negative numbers in the reduce), and
adjust the packages object to match the GraphQL CpuPackages type: use the
correct field casing (e.g., totalPower if the schema uses camelCase) and include
an id field per package if required by the schema, while still mapping
power/temp using sentinel defaults (or null) consistently with the GraphQL
model.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (2)
42-52: Align GraphQL nullability and casing; make fields required.The TypeScript properties are optional (
?) while GraphQL fields are non-null (nonullable: true), which can cause "Cannot return null for non-nullable field" errors. Additionally,totalpoweruses inconsistent casing compared toCpuPower.totalPower.Apply this diff:
@ObjectType() export class CpuPackages { - @Field(() => Float, { description: 'Total CPU package power draw (W)' }) - totalpower?: number; + @Field(() => Float, { description: 'Total CPU package power draw (W)' }) + totalPower!: number; - @Field(() => [Float], { description: 'Power draw per package (W)' }) - power?: number[]; + @Field(() => [Float], { description: 'Power draw per package (W)' }) + power!: number[]; - @Field(() => [Float], { description: 'Temperature per package (°C)' }) - temp?: number[]; + @Field(() => [Float], { description: 'Temperature per package (°C)' }) + temp!: number[]; }Based on past review comments.
136-137: Non-null GraphQL field should have definite assignment.The
packagesfield is non-null in GraphQL but lacks definite assignment in TypeScript. Use!to satisfy strict property initialization.Apply this diff:
- @Field(() => CpuPackages) - packages: CpuPackages; + @Field(() => CpuPackages) + packages!: CpuPackages;Based on past review comments.
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (2)
36-44: Fix total power calculation; filter sentinel values and align field naming.The calculation
pkg.power ?? 0includes-1sentinel values in the sum, producing incorrect totals. Additionally, the field nametotalpowerdoesn't match the GraphQL model's camelCase conventiontotalPower.Apply this diff:
// Compute total power (2 decimals) const totalpower = - Math.round(packageList.reduce((sum, pkg) => sum + (pkg.power ?? 0), 0) * 100) / 100; + Math.round( + packageList.reduce( + (sum, pkg) => sum + (typeof pkg.power === 'number' && pkg.power >= 0 ? pkg.power : 0), + 0 + ) * 100 + ) / 100; // Build CpuPackages object const packages: CpuPackages = { - totalpower, + totalPower: totalpower, power: packageList.map((pkg) => pkg.power ?? -1), temp: packageList.map((pkg) => pkg.temp ?? -1), };Based on past review comments.
2-4: Remove unused imports.The imports
fsConstants,access,readFile,readdir,basename, andjoinare not used in this file. They appear to be leftover from code that was moved toCpuPowerServiceandCpuTopologyService.Apply this diff:
import { Injectable, Logger } from '@nestjs/common'; -import { constants as fsConstants } from 'node:fs'; -import { access, readdir, readFile } from 'node:fs/promises'; -import { basename, join } from 'node:path'; import { cpu, cpuFlags, currentLoad } from 'systeminformation';
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (1)
45-68: Consolidate duplicate CPU telemetry logic.The telemetry calculation logic (lines 51-59) duplicates similar code in
cpu.service.ts(lines 36-44). Both computetotalpowerfrompackageListand build aCpuPackagesobject. Consider extracting this into a shared method inCpuTopologyService.Additionally, this uses
Number().toFixed(2)whilecpu.service.tsusesMath.round() * 100 / 100for the same purpose—standardize the rounding approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts(4 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts(4 hunks)packages/unraid-shared/src/pubsub/graphql.pubsub.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
packages/unraid-shared/src/pubsub/graphql.pubsub.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:36:59.838Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T01:36:59.838Z
Learning: Applies to api/**/*.{test,spec}.{ts,tsx} : API test suite is Vitest; do not use Jest
Applied to files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts
🧬 Code graph analysis (3)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (2)
api/src/unraid-api/graph/resolvers/info/info.model.ts (1)
ObjectType(13-44)api/src/unraid-api/cli/generated/graphql.ts (1)
Node(1371-1373)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (2)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-power.service.ts (1)
Injectable(8-136)api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (1)
Injectable(8-208)
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (3)
api/src/core/pubsub.ts (1)
pubsub(12-12)api/src/unraid-api/cli/generated/graphql.ts (1)
Subscription(2046-2058)web/composables/gql/graphql.ts (1)
Subscription(2032-2044)
🪛 GitHub Check: CodeQL
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts
[notice] 2-2: Unused variable, import, function or class
Unused import fsConstants.
[notice] 3-3: Unused variable, import, function or class
Unused imports access, readFile, readdir.
[notice] 4-4: Unused variable, import, function or class
Unused imports basename, join.
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (1)
135-157: LGTM: New subscriptions correctly implemented.The
systemMetricsCpuPowerandsystemMetricsCpuTelemetrysubscriptions follow the established pattern with proper permissions, resolve functions, and channel bindings.
| CpuPowerService, | ||
| CpuTopologyService, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Mock CpuPowerService and CpuTopologyService for unit tests.
These services read from the filesystem (/sys/class/powercap, /sys/devices/system/cpu) and are not simple dependencies. In unit tests, they should be mocked to avoid filesystem dependencies and ensure tests run in isolation.
Apply this diff:
- CpuPowerService,
- CpuTopologyService,
+ {
+ provide: CpuPowerService,
+ useValue: {
+ generateCpuPower: vi.fn().mockResolvedValue({
+ id: 'info/cpu-power',
+ totalPower: 65.5,
+ coresPower: [32.5, 33.0],
+ }),
+ },
+ },
+ {
+ provide: CpuTopologyService,
+ useValue: {
+ generateTelemetry: vi.fn().mockResolvedValue([
+ { id: 0, power: 32.5, temp: 45.0 },
+ { id: 1, power: 33.0, temp: 46.0 },
+ ]),
+ generateTopology: vi.fn().mockResolvedValue([
+ [[0, 1], [2, 3]],
+ [[4, 5], [6, 7]],
+ ]),
+ },
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CpuPowerService, | |
| CpuTopologyService, | |
| { | |
| provide: CpuPowerService, | |
| useValue: { | |
| generateCpuPower: vi.fn().mockResolvedValue({ | |
| id: 'info/cpu-power', | |
| totalPower: 65.5, | |
| coresPower: [32.5, 33.0], | |
| }), | |
| }, | |
| }, | |
| { | |
| provide: CpuTopologyService, | |
| useValue: { | |
| generateTelemetry: vi.fn().mockResolvedValue([ | |
| { id: 0, power: 32.5, temp: 45.0 }, | |
| { id: 1, power: 33.0, temp: 46.0 }, | |
| ]), | |
| generateTopology: vi.fn().mockResolvedValue([ | |
| [[0, 1], [2, 3]], | |
| [[4, 5], [6, 7]], | |
| ]), | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts around
lines 23 to 24, CpuPowerService and CpuTopologyService are real
filesystem-reading services and must be replaced with test doubles; update the
test module setup to provide mock implementations (e.g., use providers with
useValue or jest mocks) for CpuPowerService and CpuTopologyService that return
deterministic, minimal data used by the resolver tests, and remove any reliance
on the real filesystem so tests run in isolation.
|
|
||
| const testModule = new MetricsResolver( | ||
| cpuService, | ||
| {} as any, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace {} as any with proper mock.
Using {} as any bypasses type safety and makes tests brittle. Provide a proper mock for CpuPowerService.
Apply this diff:
const testModule = new MetricsResolver(
cpuService,
- {} as any,
+ {
+ generateCpuPower: vi.fn().mockResolvedValue({
+ id: 'info/cpu-power',
+ totalPower: 65.5,
+ coresPower: [32.5, 33.0],
+ }),
+ } as any,
memoryService,
subscriptionTracker as any,
{} as any
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts around
line 170, the test currently uses "{} as any" for the CpuPowerService mock which
disables type checking and makes the test brittle; replace it with a typed mock
object that implements the CpuPowerService interface (create an object with the
exact methods the resolver uses, each set to jest.fn()), set any method return
values or resolved promises the test expects, and pass that mock into the
resolver setup so TypeScript enforces the contract and the test remains stable.
| this.subscriptionTracker.registerTopic( | ||
| PUBSUB_CHANNEL.CPU_POWER, | ||
| async () => { | ||
| const payload = new CpuPower(); |
There was a problem hiding this comment.
Initialize CpuPower with required id field.
CpuPower extends Node, which requires an id field. Creating an instance without setting id will cause GraphQL validation errors.
Apply this diff:
- const payload = new CpuPower();
+ const payload = { id: 'info/cpu-power' } as CpuPower;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const payload = new CpuPower(); | |
| const payload = new CpuPower(); | |
| payload.id = 'info/cpu-power'; |
| const payload = new CpuPower(); | |
| const payload = new CpuPower({ id: 'info/cpu-power' }); |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts around line
74, the code creates a new CpuPower without setting the required Node.id field;
update the instantiation to provide a valid id (for example a generated/derived
string or UUID) when constructing CpuPower so the object includes id before
being returned or used, ensuring GraphQL validation passes.
| ARRAY = "ARRAY", | ||
| CPU_UTILIZATION = "CPU_UTILIZATION", | ||
| CPU_POWER = "CPU_POWER", | ||
| CPY_TELEMETRY = "CPU_TELEMETRY", |
There was a problem hiding this comment.
Fix typo in enum key name.
The enum key is misspelled as CPY_TELEMETRY (missing 'U') while the value correctly uses "CPU_TELEMETRY". This inconsistency can cause confusion and errors when referencing the enum.
Apply this diff:
- CPY_TELEMETRY = "CPU_TELEMETRY",
+ CPU_TELEMETRY = "CPU_TELEMETRY",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CPY_TELEMETRY = "CPU_TELEMETRY", | |
| CPU_TELEMETRY = "CPU_TELEMETRY", |
🤖 Prompt for AI Agents
packages/unraid-shared/src/pubsub/graphql.pubsub.ts around line 9: the enum key
is misspelled as CPY_TELEMETRY while the value is "CPU_TELEMETRY"; rename the
enum key to CPU_TELEMETRY to match the value, and update all references/usages
in the codebase to use the corrected enum key to avoid runtime/compile errors.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/unraid-shared/deploy.sh (2)
40-40: Replace eval with array-based command execution.Using
evalwith constructed commands is error-prone and a security anti-pattern, even with quoted variables. Construct the rsync command as a bash array and invoke directly to avoid shell injection risks and improve reliability.Apply this diff to remove eval:
-# Replace the value inside the rsync command with the user's input -rsync_command="rsync -avz --delete --progress --stats -e ssh \"$source_directory/\" \"root@${server_name}:$destination_directory/\"" - -echo "Executing the following command:" -echo "$rsync_command" - -# Execute the rsync command and capture the exit code -eval "$rsync_command" -exit_code=$? +# Execute rsync using array to avoid eval +rsync_args=( + -avz + --delete + --progress + --stats + -e ssh + "$source_directory/" + "root@${server_name}:$destination_directory/" +) + +echo "Executing: rsync ${rsync_args[*]}" +rsync "${rsync_args[@]}" +exit_code=$?Also applies to: 46-46
56-65: Consider making sound notification optional for automated/CI environments.The sound notifications (lines 56–65) are a nice developer ergonomic feature but may not be appropriate in CI/automated environments. Consider adding an environment variable to suppress them:
if [[ -z "$QUIET_DEPLOY" ]]; then if [[ "$OSTYPE" == "darwin"* ]]; then afplay /System/Library/Sounds/Glass.aiff elif [[ "$OSTYPE" == "linux-gnu" ]]; then paplay /usr/share/sounds/freedesktop/stereo/complete.oga elif [[ "$OSTYPE" == "msys" || "$OSTYPE" == "win32" ]]; then powershell.exe -c "(New-Object Media.SoundPlayer 'C:\Windows\Media\Windows Default.wav').PlaySync()" fi fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/unraid-shared/deploy.sh(1 hunks)packages/unraid-shared/package.json(1 hunks)
🔇 Additional comments (1)
packages/unraid-shared/package.json (1)
24-25: No issues found—deploy.sh has proper executable permissions.The verification confirms that
packages/unraid-shared/deploy.shis tracked in git with mode100755(executable). The script reference in the npm script is correctly configured and ready for use.
| destination_directory="/usr/local/unraid-api/node_modules/@unraid/shared" | ||
|
|
||
| # Create destination directory on remote server | ||
| ssh root@"${server_name}" "mkdir -p $destination_directory" |
There was a problem hiding this comment.
Document SSH key setup and clarify ownership scope.
The script uses root@${server_name} for SSH and recursively changes permissions on the entire @unraid directory. Ensure the following are documented:
- SSH key-based authentication must be configured (passwordless root access).
- Why the full
chown -Ron@unraid/is necessary—can it be narrowed to just thesharedpackage to reduce blast radius? - Root SSH access requirement and any security implications for the deployment environment.
Consider whether changing ownership only on the specific shared package is sufficient:
# Instead of:
ssh root@"${server_name}" "chown -R root:root /usr/local/unraid-api/node_modules/@unraid/"
# Consider:
ssh root@"${server_name}" "chown -R root:root /usr/local/unraid-api/node_modules/@unraid/shared"Do you want me to help refactor this to use a configuration-driven approach or add comprehensive documentation for the deployment prerequisites?
Also applies to: 50-50
Add error handling for SSH commands.
SSH operations on lines 37, 50, and 53 do not check for failure. If any command fails (e.g., connection issue, permission denied), the script silently continues and may deploy a broken state. Add exit checks after each SSH command.
Apply this diff to add error handling:
# Create destination directory on remote server
ssh root@"${server_name}" "mkdir -p $destination_directory"
+if [ $? -ne 0 ]; then
+ echo "Error: Failed to create remote directory!"
+ exit 1
+fi
# (rsync execution here)
# Chown the directory
ssh root@"${server_name}" "chown -R root:root /usr/local/unraid-api/node_modules/@unraid/"
+if [ $? -ne 0 ]; then
+ echo "Error: Failed to change ownership on remote!"
+ exit 1
+fi
# Run unraid-api restart on remote host
ssh root@"${server_name}" 'INTROSPECTION=true LOG_LEVEL=trace unraid-api restart'
+if [ $? -ne 0 ]; then
+ echo "Error: Failed to restart unraid-api on remote!"
+ exit 1
+fiAlso applies to: 50-50, 53-53
🤖 Prompt for AI Agents
In packages/unraid-shared/deploy.sh around lines 37, 50, and 53, the SSH
commands currently execute without checking for failure; update each SSH
invocation to immediately detect and handle errors by appending a failure check
— for example, run the ssh command and if it exits non‑zero log an error to
stderr (including the server_name and the failing command), then exit with a
non‑zero status (or use a conditional || { echo "..."; exit 1; } pattern);
ensure any pipeline/quoted commands preserve exit status so failures are caught
and the script stops instead of continuing in a broken state.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
api/generated-schema.graphql (3)
2659-2659: Regenerate GraphQL client typings for new subscription.
systemMetricsCpuTelemetrywas added. Ensure CLI/web/generated clients are updated so Subscription types include it; otherwise type/runtime mismatches will occur.
1404-1413: Schema blocker: invalid scalar and missingidin CpuPackages.
totalPower: numberis not valid GraphQL; useFloat.CpuPackages implements Nodebut lacksid: PrefixedID!.- Docstring malformed for
temp.Apply in the code-first model (
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts), then regenerate this file:type CpuPackages implements Node { - """Total CPU package power draw (W)""" - totalPower: number + id: PrefixedID! + """Total CPU package power draw (W)""" + totalPower: Float """Power draw per package (W)""" power: [Float!] - """description: 'Temperature per package (°C)""" + """Temperature per package (°C)""" temp: [Float!] }Also ensure the TS class either implements Node or declares
idwith@Field(() => PrefixedID). Regenerate schema and client types afterward.
1460-1462: Expose topology on InfoCpu to match service output.
CpuServicereturnstopology: number[][][]but the schema lacks this field. Add it to the model and regenerate.type InfoCpu implements Node { ... """CPU packages information""" packages: CpuPackages + """Per-package core/thread groupings (package -> cores -> thread IDs)""" + topology: [[[Int!]!]!] ... }api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (3)
2-4: Remove unused imports.
fsConstants,access/readdir/readFile, andbasename/joinare unused here. Clean them up.-import { constants as fsConstants } from 'node:fs'; -import { access, readdir, readFile } from 'node:fs/promises'; -import { basename, join } from 'node:path';
32-33: Exclude sentinel values from total power.Summing
pkg.power ?? 0includes-1(unknown), underreporting totals. Ignore negatives.- const totalPower = - Math.round(packageList.reduce((sum, pkg) => sum + (pkg.power ?? 0), 0) * 100) / 100; + const totalPower = + Math.round( + packageList.reduce( + (sum, pkg) => sum + (typeof pkg.power === 'number' && pkg.power >= 0 ? pkg.power : 0), + 0 + ) * 100 + ) / 100;
36-40: If CpuPackages implements Node, includeid.Once the model is corrected, return an
id(e.g.,'info/cpu-packages') to satisfy the Node contract.- const packages: CpuPackages = { - totalPower, - power: packageList.map((pkg) => pkg.power ?? -1), - temp: packageList.map((pkg) => pkg.temp ?? -1), - }; + const packages: CpuPackages = { + // id required if CpuPackages implements Node + // id: 'info/cpu-packages', + totalPower, + power: packageList.map((pkg) => pkg.power ?? -1), + temp: packageList.map((pkg) => pkg.temp ?? -1), + };api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts (1)
166-172: Replace{}casts with typed stubs.Construct
MetricsResolverwith a real stub forCpuTopologyServiceinstead of{} as any.- const testModule = new MetricsResolver( - cpuService, - {} as any, - memoryService, - subscriptionTracker as any, - {} as any - ); + const cpuTopologyStub = { + generateTelemetry: vi.fn().mockResolvedValue([{ id: 0, power: 10, temp: 40 }]), + generateTopology: vi.fn().mockResolvedValue([[[0, 1]]]), + } as unknown as CpuTopologyService; + const helperStub = { createTrackedSubscription: vi.fn() } as SubscriptionHelperService; + const testModule = new MetricsResolver( + cpuService, + cpuTopologyStub, + memoryService, + subscriptionTracker as any, + helperStub as any + );api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (1)
42-52: Make CpuPackages fields required to match non-nullable GraphQL schema.The TypeScript properties are optional (
?) but the@Fielddecorators don't specifynullable: true, meaning GraphQL expects these fields to always be present. This mismatch can cause runtime errors if any field is omitted.Apply this diff:
@ObjectType() export class CpuPackages { @Field(() => Float, { description: 'Total CPU package power draw (W)' }) - totalPower?: number; + totalPower!: number; @Field(() => [Float], { description: 'Power draw per package (W)' }) - power?: number[]; + power!: number[]; @Field(() => [Float], { description: 'Temperature per package (°C)' }) - temp?: number[]; + temp!: number[]; }
🧹 Nitpick comments (4)
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts (1)
8-8: Mock CpuTopologyService in integration tests to avoid FS flakiness.Provide a lightweight stub via
useValue(return fixed topology/telemetry) instead of the real service. This keeps tests hermetic and faster, especially on CI.- CpuTopologyService, + { + provide: CpuTopologyService, + useValue: { + generateTelemetry: vi.fn().mockResolvedValue([ + { id: 0, power: 32.5, temp: 45.0 }, + { id: 1, power: 33.0, temp: 46.0 }, + ]), + generateTopology: vi.fn().mockResolvedValue([ + [[0, 1], [2, 3]], + [[4, 5], [6, 7]], + ]), + }, + },Also applies to: 26-26
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts (2)
6-6: Provide a stubbed CpuTopologyService in unit tests.Avoid the real FS-backed service; inject a simple stub via
useValue.- CpuTopologyService, + { + provide: CpuTopologyService, + useValue: { + generateTelemetry: vi.fn().mockResolvedValue([ + { id: 0, power: 32.5, temp: 45.0 }, + { id: 1, power: 33.0, temp: 46.0 }, + ]), + generateTopology: vi.fn().mockResolvedValue([ + [[0, 1], [2, 3]], + [[4, 5], [6, 7]], + ]), + }, + },Also applies to: 22-22
176-186: Also assert the CPU telemetry topic name.You expect three registrations; verify the third is the new CPU telemetry channel to prevent regressions.
testModule.onModuleInit(); expect(subscriptionTracker.registerTopic).toHaveBeenCalledTimes(3); expect(subscriptionTracker.registerTopic).toHaveBeenCalledWith( 'CPU_UTILIZATION', expect.any(Function), 1000 ); + expect(subscriptionTracker.registerTopic).toHaveBeenCalledWith( + 'CPU_TELEMETRY', + expect.any(Function), + expect.any(Number) + ); expect(subscriptionTracker.registerTopic).toHaveBeenCalledWith( 'MEMORY_UTILIZATION', expect.any(Function), 2000 );api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (1)
61-61: Remove duplicate debug log.This debug statement is redundant—the same payload was already logged on line 54.
Apply this diff:
pubsub.publish(PUBSUB_CHANNEL.CPU_TELEMETRY, { systemMetricsCpuTelemetry: packages, }); - - this.logger.debug(`CPU_TELEMETRY payload2: ${JSON.stringify(packages)}`); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/generated-schema.graphql(3 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts(4 hunks)packages/unraid-shared/src/pubsub/graphql.pubsub.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.ts
- api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts
- packages/unraid-shared/src/pubsub/graphql.pubsub.ts
🧰 Additional context used
📓 Path-based instructions (6)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts
🧠 Learnings (2)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
PR: unraid/api#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/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
🧬 Code graph analysis (3)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (2)
api/src/unraid-api/graph/resolvers/info/info.model.ts (1)
ObjectType(13-44)api/src/unraid-api/graph/resolvers/metrics/metrics.model.ts (1)
ObjectType(8-21)
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (3)
api/src/core/pubsub.ts (1)
pubsub(12-12)api/src/unraid-api/cli/generated/graphql.ts (1)
Subscription(2046-2058)web/composables/gql/graphql.ts (1)
Subscription(2032-2044)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (2)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (1)
Injectable(8-208)api/src/unraid-api/cli/generated/graphql.ts (1)
InfoCpu(855-892)
🔇 Additional comments (3)
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (2)
39-64: LGTM!The CPU telemetry subscription registration is well-structured: it aggregates package power with appropriate precision, handles missing data with -1 fallbacks, and publishes the payload correctly.
110-120: LGTM!The GraphQL subscription is properly configured with the correct return type, name, resolve function, and permissions.
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (1)
116-122: LGTM!The new
topologyandpackagesfields are correctly declared with definite assignment (!) to match their non-nullable GraphQL schema declarations.
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts (1)
95-99: Fix mock: still has typo and missing required methods.Despite being marked as addressed in a previous commit, the mock still defines
generateCpuTotopology(typo) and lacks the methodsCpuServiceactually calls:generateTelemetry()andgenerateTopology(). This will cause runtime failures.Apply this diff to fix the mock:
- cpuTopologyService = { - generateCpuTotopology: vi.fn().mockResolvedValue({ id: 'info/cpu-topology' }), - } as any; + cpuTopologyService = { + generateTelemetry: vi.fn().mockResolvedValue([ + { id: 0, power: 32.5, temp: 45.0 }, + { id: 1, power: 33.0, temp: 46.0 }, + ]), + generateTopology: vi.fn().mockResolvedValue([ + [[0, 1], [2, 3]], + [[4, 5], [6, 7]], + ]), + } as unknown as CpuTopologyService;Based on learnings
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (1)
195-195: Fix type inference: explicitly typeresultarray.TypeScript infers
resultasnever[], causing the push on line 199 to fail type checking. This is blocking the pipeline.- const result = []; + const result: { id: number; power: number; temp: number }[] = [];api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
27-28: Fix totalPower: exclude sentinel-1values from sum.
pkg.power ?? 0only handles null/undefined. Whenpkg.poweris-1(the sentinel for unavailable data), it gets added to the sum, producing incorrect totals.const totalPower = - Math.round(packageList.reduce((sum, pkg) => sum + (pkg.power ?? 0), 0) * 100) / 100; + Math.round( + packageList.reduce( + (sum, pkg) => sum + (typeof pkg.power === 'number' && pkg.power >= 0 ? pkg.power : 0), + 0 + ) * 100 + ) / 100;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
🧠 Learnings (1)
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
PR: unraid/api#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/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
🧬 Code graph analysis (2)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (3)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (1)
Injectable(8-208)api/src/unraid-api/cli/generated/graphql.ts (1)
InfoCpu(855-892)web/composables/gql/graphql.ts (1)
InfoCpu(842-879)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
Injectable(12-70)
🪛 GitHub Actions: CI - Main (API)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
[error] 199-199: TypeScript error TS2345: Argument of type '{ id: number; power: number; temp: number; }' is not assignable to parameter of type 'never'.
[error] 199-199: Command failed with exit code 1 while running 'pnpm run type-check' (tsc --noEmit).
| packages[pkgId].push(siblings); | ||
| } | ||
| } catch (err) { | ||
| console.warn('Topology read error for', dir, err); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use consistent logging: prefer this.logger.warn.
The rest of the file uses this.logger.warn for consistency. Replace console.warn with the injected logger.
- console.warn('Topology read error for', dir, err);
+ this.logger.warn('Topology read error for', dir, err);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.warn('Topology read error for', dir, err); | |
| this.logger.warn('Topology read error for', dir, err); |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts around
line 53, replace the console.warn call with the injected logger to maintain
consistency: change console.warn('Topology read error for', dir, err) to use
this.logger.warn, passing a clear message and the dir and error (e.g.
this.logger.warn('Topology read error for %s', dir, err) or equivalent
structured logging) so the same logging instance and format are used throughout
the file.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (1)
53-53: Use consistent logging: preferthis.logger.warn.This line still uses
console.warninstead of the injected logger. The rest of the file consistently usesthis.logger.warn.- console.warn('Topology read error for', dir, err); + this.logger.warn('Topology read error for', dir, err);Based on coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/generated-schema.graphql(3 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/generated-schema.graphql
🧰 Additional context used
📓 Path-based instructions (3)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
🧬 Code graph analysis (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
Injectable(12-70)
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
[failure] 5-5: src/unraid-api/app/test/app.module.integration.spec.ts
Error: [vitest] No "promises" export is defined on the "node:fs" mock. Did you forget to return it from "vi.mock"?
If you need to partially mock a module, you can use "importOriginal" helper inside:
vi.mock(import("node:fs"), async (importOriginal) => {
const actual = await importOriginal()
return {
...actual,
// your mocked methods
}
})
❯ src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts:5:39
❯ src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts:3:1
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
Outdated
Show resolved
Hide resolved
| export class CpuTopologyService { | ||
| private readonly logger = new Logger(CpuTopologyService.name); | ||
|
|
||
| private topologyCache: { id: number; cores: number[][] }[] | null = null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused field.
The topologyCache field is declared but never used anywhere in this service.
- private topologyCache: { id: number; cores: number[][] }[] | null = null;
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private topologyCache: { id: number; cores: number[][] }[] | null = null; |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts around
line 12, the private field `topologyCache` is declared but never referenced;
remove this unused field declaration to clean up the class (and run the
linter/TS compiler to ensure no other references exist), or if it was intended
to be used, implement its usage instead of leaving it unused.
| for (const domains of Object.values(results)) { | ||
| const total = Object.values(domains).reduce((a, b) => a + b, 0); | ||
| (domains as any)['total'] = Math.round(total * 100) / 100; | ||
| } |
There was a problem hiding this comment.
Avoid any type: add proper typing for domains.
Line 184 uses (domains as any)['total'] which violates the coding guideline: "Never use the any type; prefer precise typing."
The domains object should be properly typed to allow the total property:
- for (const domains of Object.values(results)) {
+ for (const domains of Object.values(results) as Record<string, number>[]) {
const total = Object.values(domains).reduce((a, b) => a + b, 0);
- (domains as any)['total'] = Math.round(total * 100) / 100;
+ domains['total'] = Math.round(total * 100) / 100;
}Based on coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const domains of Object.values(results)) { | |
| const total = Object.values(domains).reduce((a, b) => a + b, 0); | |
| (domains as any)['total'] = Math.round(total * 100) / 100; | |
| } | |
| for (const domains of Object.values(results) as Record<string, number>[]) { | |
| const total = Object.values(domains).reduce((a, b) => a + b, 0); | |
| domains['total'] = Math.round(total * 100) / 100; | |
| } |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts around
lines 182 to 185, the code uses (domains as any)['total'] which violates the
no-any rule; instead give domains a precise type (for example declare a
DomainMap type = Record<string, number> & { total?: number } or an appropriate
interface) and ensure results is typed so Object.values(results) yields
DomainMap[], then set domains.total = Math.round(total * 100) / 100 without any
casting; update function signatures/variable declarations so TypeScript infers
the correct DomainMap type everywhere.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts (1)
94-113: Consider adding explicit mock clearing for consistency.While the service mock is recreated in
beforeEach, consider adding explicit mock clearing for the module mocks to follow the guideline about clearing mocks between tests.As per coding guidelines.
beforeEach(() => { + vi.clearAllMocks(); + cpuTopologyService = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
PR: unraid/api#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/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts (1)
143-157: LGTM!The test expectations correctly verify the new
packagesandtopologystructure, withtotalPowerproperly computed as the sum of individual package powers and arrays matching the mock telemetry data.
| cpuTopologyService = { | ||
| generateTopology: vi.fn().mockResolvedValue([ | ||
| [ | ||
| [0, 1], | ||
| [2, 3], | ||
| ], | ||
| [ | ||
| [4, 5], | ||
| [6, 7], | ||
| ], | ||
| ]), | ||
| generateTelemetry: vi.fn().mockResolvedValue([ | ||
| { power: 32.5, temp: 45.0 }, | ||
| { power: 33.0, temp: 46.0 }, | ||
| ]), | ||
| } as any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace as any with proper typing.
The coding guideline states to never use the any type. Replace the type assertion with a more precise cast.
As per coding guidelines.
Apply this diff:
cpuTopologyService = {
generateTopology: vi.fn().mockResolvedValue([
[
[0, 1],
[2, 3],
],
[
[4, 5],
[6, 7],
],
]),
generateTelemetry: vi.fn().mockResolvedValue([
{ power: 32.5, temp: 45.0 },
{ power: 33.0, temp: 46.0 },
]),
- } as any;
+ } as unknown as CpuTopologyService;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cpuTopologyService = { | |
| generateTopology: vi.fn().mockResolvedValue([ | |
| [ | |
| [0, 1], | |
| [2, 3], | |
| ], | |
| [ | |
| [4, 5], | |
| [6, 7], | |
| ], | |
| ]), | |
| generateTelemetry: vi.fn().mockResolvedValue([ | |
| { power: 32.5, temp: 45.0 }, | |
| { power: 33.0, temp: 46.0 }, | |
| ]), | |
| } as any; | |
| cpuTopologyService = { | |
| generateTopology: vi.fn().mockResolvedValue([ | |
| [ | |
| [0, 1], | |
| [2, 3], | |
| ], | |
| [ | |
| [4, 5], | |
| [6, 7], | |
| ], | |
| ]), | |
| generateTelemetry: vi.fn().mockResolvedValue([ | |
| { power: 32.5, temp: 45.0 }, | |
| { power: 33.0, temp: 46.0 }, | |
| ]), | |
| } as unknown as CpuTopologyService; |
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts around lines
95 to 110, the test stub uses an unsafe "as any" cast for cpuTopologyService;
replace it with a proper typed mock such as vi.Mocked<CpuTopologyService> (or
Partial<CpuTopologyService> if the full mocked type isn't available): import the
CpuTopologyService type from its module, change the assertion to "as
vi.Mocked<CpuTopologyService>", and ensure vitest's vi.Mocked is used so
generateTopology and generateTelemetry remain correctly typed as mocked
functions.
Signed-off-by: Simon Fairweather <simon.n.fairweather@gmail.com>
Signed-off-by: Simon Fairweather <simon.n.fairweather@gmail.com>
Signed-off-by: Simon Fairweather <simon.n.fairweather@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: Simon Fairweather <simon.n.fairweather@gmail.com>
Signed-off-by: Simon Fairweather <simon.n.fairweather@gmail.com>
Signed-off-by: Simon Fairweather <simon.n.fairweather@gmail.com>
Signed-off-by: Simon Fairweather <simon.n.fairweather@gmail.com>
Signed-off-by: Simon Fairweather <simon.n.fairweather@gmail.com>
Signed-off-by: Simon Fairweather <simon.n.fairweather@gmail.com>
pujitm
force-pushed
the
feat/cpu-power
branch
from
7dd49d1 to
1aa6525
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/generated-schema.graphql (2)
1415-1471: Regenerate schema to add missingtopologyfield to InfoCpu.The model declares
topology!: number[][][];but this field is missing from the InfoCpu type in the generated schema. Regenerate to sync.
2650-2662: Regenerate clients to includesystemMetricsCpuTelemetrysubscription.Verification confirms that
systemMetricsCpuTelemetryis defined inapi/generated-schema.graphqlbut is absent from all generated client type files:
web/src/composables/gql/graphql.tsapi/src/unraid-api/cli/generated/graphql.tspackages/unraid-api-plugin-connect/src/graphql/generated/client/graphql.tsOther subscriptions are present in these files, confirming that codegen has run but is now out of sync with the schema. Run
pnpm codegento regenerate all clients and avoid type/runtime mismatches.
♻️ Duplicate comments (5)
packages/unraid-shared/deploy.sh (3)
37-37: Add error handling for SSH commands.Lines 37 and 50 execute SSH operations without checking for failure. If these commands fail (connection issue, permission denied, or command execution failure), the script silently continues and may deploy a broken state. The issue was flagged in a previous review and remains unresolved.
Apply this diff to add error checking:
# Create destination directory on remote server ssh root@"${server_name}" "mkdir -p $destination_directory" + if [ $? -ne 0 ]; then + echo "Error: Failed to create remote directory on $server_name!" + exit 1 + fi # (... rsync execution ...) # Chown the directory ssh root@"${server_name}" "chown -R root:root /usr/local/unraid-api/node_modules/@unraid/" + if [ $? -ne 0 ]; then + echo "Error: Failed to change ownership on remote $server_name!" + exit 1 + fiAlso applies to: 53-53
Also applies to: 50-50
50-50: Document SSH key setup and narrow ownership scope.The script uses
root@${server_name}for SSH and recursively changes permissions on the entire@unraiddirectory. This was also flagged previously and remains unaddressed. Ensure the following are documented:
- SSH key-based authentication must be configured (passwordless root access).
- Whether the full
chown -Ron the entire@unraid/directory is necessary—consider narrowing to just thesharedpackage to reduce blast radius.- Root SSH access requirement and security implications.
Consider narrowing the chown scope:
- ssh root@"${server_name}" "chown -R root:root /usr/local/unraid-api/node_modules/@unraid/" + ssh root@"${server_name}" "chown -R root:root /usr/local/unraid-api/node_modules/@unraid/shared"
37-53: Fix exit code logic: SSH commands execute after rsync exit code is captured.The script captures the rsync exit code at line 47, but then executes SSH commands on lines 50 and 53. If these SSH commands fail, the script exits with the rsync code (line 68), not the actual failure reason. This masks deployment failures and should be restructured so all critical operations check for success before proceeding.
Consider restructuring to check and exit on each critical operation:
# Create destination directory on remote server ssh root@"${server_name}" "mkdir -p $destination_directory" || { + echo "Error: Failed to create remote directory!" + exit 1 + } # ... rsync and other operations ... # Chown the directory - ssh root@"${server_name}" "chown -R root:root /usr/local/unraid-api/node_modules/@unraid/" + ssh root@"${server_name}" "chown -R root:root /usr/local/unraid-api/node_modules/@unraid/" || { + echo "Error: Failed to change ownership!" + exit 1 + } # Run unraid-api restart on remote host - ssh root@"${server_name}" 'INTROSPECTION=true LOG_LEVEL=trace unraid-api restart' + ssh root@"${server_name}" 'INTROSPECTION=true LOG_LEVEL=trace unraid-api restart' || { + echo "Error: Failed to restart unraid-api!" + exit 1 + } - # Exit with the rsync command's exit code - exit $exit_code + exit 0api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)
26-35: Exclude sentinel values when summing totalPower.
pkg.power ?? 0will add -1, skewing totals. Only include non-negative numbers.- const totalPower = - Math.round(packageList.reduce((sum, pkg) => sum + (pkg.power ?? 0), 0) * 100) / 100; + const totalPower = + Math.round( + packageList.reduce( + (sum, pkg) => sum + (typeof pkg.power === 'number' && pkg.power >= 0 ? pkg.power : 0), + 0, + ) * 100, + ) / 100;api/generated-schema.graphql (1)
1404-1413: Schema invalid:CpuPackages implements Nodebut lacksid.Either add
id: PrefixedID!here or (preferred) update the code-first model to extend/implement Node and regenerate this file.Suggested model fix (then regenerate):
-@ObjectType() -export class CpuPackages { +@ObjectType({ implements: () => Node }) +export class CpuPackages extends Node { @Field(() => Float, { description: 'Total CPU package power draw (W)' }) - totalPower?: number; + totalPower!: number; @Field(() => [Float], { description: 'Power draw per package (W)' }) - power?: number[]; + power!: number[]; @Field(() => [Float], { description: 'Temperature per package (°C)' }) - temp?: number[]; + temp!: number[]; }
🧹 Nitpick comments (8)
packages/unraid-shared/deploy.sh (2)
40-40: Quote the destination path in rsync command.Line 40 expands
$destination_directorywithin a double-quoted eval string. Although the current path is safe, adding quotes prevents potential issues if the path ever contains spaces or special characters.- rsync_command="rsync -avz --delete --progress --stats -e ssh \"$source_directory/\" \"root@${server_name}:$destination_directory/\"" + rsync_command="rsync -avz --delete --progress --stats -e ssh \"$source_directory/\" \"root@${server_name}:${destination_directory}/\""
55-65: Handle missing sound files gracefully.The sound notification uses hard-coded paths that may not exist on all systems (e.g.,
/usr/share/sounds/freedesktop/stereo/complete.ogamay not be present on minimal Linux installations). Consider adding existence checks or usingstderrredirection to suppress errors.if [[ "$OSTYPE" == "darwin"* ]]; then # macOS - afplay /System/Library/Sounds/Glass.aiff + afplay /System/Library/Sounds/Glass.aiff 2>/dev/null || true elif [[ "$OSTYPE" == "linux-gnu" ]]; then # Linux - paplay /usr/share/sounds/freedesktop/stereo/complete.oga + paplay /usr/share/sounds/freedesktop/stereo/complete.oga 2>/dev/null || true elif [[ "$OSTYPE" == "msys" || "$OSTYPE" == "win32" ]]; then # Windows - powershell.exe -c "(New-Object Media.SoundPlayer 'C:\Windows\Media\Windows Default.wav').PlaySync()" + powershell.exe -c "(New-Object Media.SoundPlayer 'C:\Windows\Media\Windows Default.wav').PlaySync()" 2>/dev/null || true fiapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts (2)
95-110: Avoidas anyon the service mock; use a typed Vitest mock.Replace the unsafe cast with a precise mocked type to satisfy our “no any” guideline. Based on learnings.
- cpuTopologyService = { + cpuTopologyService = { generateTopology: vi.fn().mockResolvedValue([ @@ generateTelemetry: vi.fn().mockResolvedValue([ { power: 32.5, temp: 45.0 }, { power: 33.0, temp: 46.0 }, ]), - } as any; + } as unknown as vi.Mocked<Pick<CpuTopologyService, 'generateTopology' | 'generateTelemetry'>>;
161-179: Prefer ts-expect-error overas anyfor partial systeminformation mock.Document the intentional type bypass and drop
as any. Based on learnings.- vi.mocked(cpu).mockResolvedValueOnce({ + // @ts-expect-error test-only partial shape of systeminformation.cpu() + vi.mocked(cpu).mockResolvedValueOnce({ manufacturer: 'Intel', @@ - } as any); + });api/src/unraid-api/graph/resolvers/info/info.module.ts (1)
4-6: Remove unused CpuService import.CpuService is provided via CpuModule; this import is unused.
import { CpuModule } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.module.js'; -import { CpuService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.service.js';api/generated-schema.graphql (1)
1411-1412: Docstring typo.Drop the accidental “description:” prefix.
- """description: 'Temperature per package (°C)""" + """Temperature per package (°C)"""api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (1)
3-6: Remove unused imports (CpuTopologyService, CpuService).They aren’t referenced in this module file.
-import { CpuTopologyService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.js'; import { CpuModule } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.module.js'; -import { CpuService } from '@app/unraid-api/graph/resolvers/info/cpu/cpu.service.js';api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (1)
54-62: Drop duplicate debug log.Two identical logs add noise.
- this.logger.debug(`CPU_TELEMETRY payload: ${JSON.stringify(packages)}`); @@ - this.logger.debug(`CPU_TELEMETRY payload2: ${JSON.stringify(packages)}`); + this.logger.debug(`CPU_TELEMETRY payload: ${JSON.stringify(packages)}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
api/generated-schema.graphql(3 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/info/info.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts(3 hunks)api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts(4 hunks)packages/unraid-shared/deploy.sh(1 hunks)packages/unraid-shared/package.json(1 hunks)packages/unraid-shared/src/pubsub/graphql.pubsub.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts
- api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.integration.spec.ts
- api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts
- api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts
- packages/unraid-shared/src/pubsub/graphql.pubsub.ts
- api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.ts
🧰 Additional context used
📓 Path-based instructions (6)
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/info/info.module.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.module.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/info/info.module.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.module.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.tsapi/src/unraid-api/graph/resolvers/info/info.module.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.resolver.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.model.tsapi/src/unraid-api/graph/resolvers/info/cpu/cpu.service.tsapi/src/unraid-api/graph/resolvers/metrics/metrics.module.ts
🧠 Learnings (2)
📚 Learning: 2025-03-07T19:29:12.540Z
Learnt from: pujitm
PR: unraid/api#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/graph/resolvers/info/cpu/cpu.service.spec.ts
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
PR: unraid/api#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/unraid-api/graph/resolvers/info/cpu/cpu.service.spec.ts
🧬 Code graph analysis (4)
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (3)
api/src/core/pubsub.ts (1)
pubsub(12-12)api/src/unraid-api/cli/generated/graphql.ts (1)
Subscription(2046-2058)web/composables/gql/graphql.ts (1)
Subscription(2032-2044)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (2)
api/src/unraid-api/graph/resolvers/info/info.model.ts (1)
ObjectType(13-44)api/src/unraid-api/graph/resolvers/metrics/metrics.model.ts (1)
ObjectType(8-21)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (3)
api/src/unraid-api/graph/resolvers/info/cpu/cpu-topology.service.ts (1)
Injectable(6-211)api/src/unraid-api/cli/generated/graphql.ts (1)
InfoCpu(855-892)web/composables/gql/graphql.ts (1)
InfoCpu(842-879)
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (2)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.module.ts (1)
Module(6-10)api/src/unraid-api/graph/services/services.module.ts (1)
Module(8-13)
🪛 GitHub Check: CodeQL
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts
[notice] 3-3: Unused variable, import, function or class
Unused import CpuTopologyService.
🔇 Additional comments (5)
packages/unraid-shared/package.json (1)
20-26: Script configuration looks good.The JSON is well-formed, and the new
unraid:deployscript is properly registered. No concerns here.api/src/unraid-api/graph/resolvers/info/info.module.ts (1)
18-18: LGTM on CpuModule wiring.Importing CpuModule here is the right DI boundary.
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (1)
11-13: LGTM on DI: consume CpuService/CpuTopologyService via CpuModule.Providers list is correct.
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (1)
116-123: Definite assignment fortopologyandpackagesis correct.This matches strict initialization and schema.
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (1)
110-120: Subscription wiring looks correct.Field name and payload key match; permissions applied.
| @ObjectType() | ||
| export class CpuPackages { | ||
| @Field(() => Float, { description: 'Total CPU package power draw (W)' }) | ||
| totalPower?: number; | ||
|
|
||
| @Field(() => [Float], { description: 'Power draw per package (W)' }) | ||
| power?: number[]; | ||
|
|
||
| @Field(() => [Float], { description: 'Temperature per package (°C)' }) | ||
| temp?: number[]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make CpuPackages a Node type and align nullability/casing.
Implement Node and make fields definite to match GraphQL expectations; we already return these arrays non-null.
-@ObjectType()
-export class CpuPackages {
- @Field(() => Float, { description: 'Total CPU package power draw (W)' })
- totalPower?: number;
- @Field(() => [Float], { description: 'Power draw per package (W)' })
- power?: number[];
- @Field(() => [Float], { description: 'Temperature per package (°C)' })
- temp?: number[];
-}
+@ObjectType({ implements: () => Node })
+export class CpuPackages extends Node {
+ @Field(() => Float, { description: 'Total CPU package power draw (W)' })
+ totalPower!: number;
+ @Field(() => [Float], { description: 'Power draw per package (W)' })
+ power!: number[];
+ @Field(() => [Float], { description: 'Temperature per package (°C)' })
+ temp!: number[];
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts around lines 42 to
52, change CpuPackages to implement the Node GraphQL type and make all fields
non-optional/non-nullable to match what we actually return: add the Node id
field, mark the class to implement Node (e.g., @ObjectType({ implements: Node
})) and remove the TypeScript optional markers (?) from totalPower, power and
temp; update their @Field decorators to be explicitly non-nullable (set
nullable: false and ensure list fields are non-null lists/values as your GraphQL
decorator supports).
| return { | ||
| id: 'info/cpu', | ||
| ...rest, | ||
| cores: physicalCores, | ||
| threads: cores, | ||
| processors, | ||
| flags, | ||
| stepping: Number(stepping), | ||
| speedmin: speedMin || -1, | ||
| speedmax: speedMax || -1, | ||
| packages, | ||
| topology, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Return shape looks good (processors, packages, topology).
Assuming schema includes packages and topology, this aligns.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verifying generated schema exposes packages and topology..."
rg -n "type InfoCpu" api/generated-schema.graphql -A20 | sed -n '1,60p' || trueLength of output: 450
I need to retrieve the complete type definition. The output was truncated. Let me search for the full schema definition and specifically look for the packages and topology fields.
---
🏁 Script executed:
```shell
#!/bin/bash
set -euo pipefail
echo "Retrieving full InfoCpu type definition..."
rg -n "type InfoCpu" api/generated-schema.graphql -A100 | grep -E "(packages|topology)" || echo "Fields not found in initial search"
echo ""
echo "Full type definition for context:"
rg -n "type InfoCpu" api/generated-schema.graphql -A60 | tail -50
Length of output: 1131
The topology field is missing. Let me confirm it's not present in the schema:
---
🏁 Script executed:
```shell
#!/bin/bash
set -euo pipefail
echo "Searching for topology field in InfoCpu or related types..."
rg -n "topology" api/generated-schema.graphql -B2 -A2
Length of output: 59
Remove or add topology field to schema—currently missing from InfoCpu type definition.
The code returns topology at line 49 of api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts, but the InfoCpu schema in api/generated-schema.graphql does not define this field. The schema includes packages (line 1461) but lacks topology, causing a schema mismatch. Either remove topology from the return object or add it to the schema definition.
🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts around lines 37–49
the returned object includes a topology field that is not present in the
generated GraphQL InfoCpu type, causing a schema mismatch; either remove
topology from the returned object or add topology to
api/generated-schema.graphql under the InfoCpu type (pick an appropriate field
name and GraphQL type that matches the runtime shape), then regenerate
schema/types and update any affected resolvers/TypeScript types so the runtime
return value and GraphQL schema remain consistent.
| // Compute total power with 2 decimals | ||
| const totalPower = Number( | ||
| packageList.reduce((sum, pkg) => sum + (pkg.power ?? 0), 0).toFixed(2) | ||
| ); | ||
|
|
||
| const packages: CpuPackages = { | ||
| totalPower, | ||
| power: packageList.map((pkg) => pkg.power ?? -1), | ||
| temp: packageList.map((pkg) => pkg.temp ?? -1), | ||
| }; |
There was a problem hiding this comment.
Exclude sentinel -1 in totalPower; keep arrays with -1 fallback.
Current sum includes -1. Filter to non-negative numbers.
- const totalPower = Number(
- packageList.reduce((sum, pkg) => sum + (pkg.power ?? 0), 0).toFixed(2)
- );
+ const totalPower = Number(
+ packageList
+ .reduce(
+ (sum, pkg) => sum + (typeof pkg.power === 'number' && pkg.power >= 0 ? pkg.power : 0),
+ 0,
+ )
+ .toFixed(2),
+ );🤖 Prompt for AI Agents
In api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts around lines
44 to 53, the totalPower calculation incorrectly includes sentinel -1 values;
change the reduction to first filter packageList powers to include only values
>= 0, then sum and round to two decimals (keeping the arrays mapping to
pkg.power ?? -1 and pkg.temp ?? -1 unchanged), so totalPower reflects only valid
power readings while the returned arrays still use -1 as fallback.
2 participants
Summary by CodeRabbit
New Features
Chores