Skip to content

Conversation

@elibosley
Copy link
Member

@elibosley elibosley commented Aug 18, 2025

Adds a new query and subscription to the info resolver for CPU to get CPU utilization on Unraid.

  • Adds cpuUtilization query to get a one-time snapshot of CPU load.
  • Adds cpuUtilizationSubscription to get real-time updates on CPU load.
  • Adds a utilization field to the InfoCpu type.
  • Creates a generic SubscriptionTrackerService to manage polling for subscriptions, ensuring that polling only occurs when there are active subscribers.
  • Creates a request-scoped CpuDataService to cache CPU load data within a single GraphQL request to improve performance.
  • Updates tests to cover the new functionality.
  • Adds detailed documentation to the CpuLoad object type.

Summary by CodeRabbit

  • New Features

    • Added Metrics query and real-time CPU/Memory subscriptions (per-core CPU and memory utilization); Info now exposes richer Info* sections (cpu, memory, devices, display, system, baseboard, os, versions) and machineId.
  • Schema

    • GraphQL surface reorganized to Info*/Metrics types; added CpuLoad/CpuUtilization, MemoryUtilization, Metrics, and enhanced container/port models.
  • Chores

    • Added subscription polling/tracking/helper and new pubsub channels for CPU and memory; API version bumped.
  • Tests

    • Extensive new/updated unit and integration tests for metrics, info, subscriptions, and polling.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 18, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Refactors Info GraphQL types into modular Info* submodels, replaces the monolithic InfoService with per-domain services (Cpu, Memory, Os, Versions, Devices, Display), adds a Metrics subsystem with polling and pubsub-backed subscriptions for CPU/memory, introduces subscription helper/tracker/polling services with tests, updates schema and web typings, and adjusts module wiring and configs.

Changes

Cohort / File(s) Summary
Info model modularization
api/src/unraid-api/graph/resolvers/info/info.model.ts, .../info/cpu/cpu.model.ts, .../info/devices/devices.model.ts, .../info/display/display.model.ts, .../info/memory/memory.model.ts, .../info/os/os.model.ts, .../info/system/system.model.ts, .../info/versions/versions.model.ts
Extracts inline Info types into dedicated Info* model files, adds CpuLoad/CpuUtilization and MemoryLayout/MemoryUtilization, adds descriptions, removes apps field, and changes machineId to ID.
Info resolver & module
api/src/unraid-api/graph/resolvers/info/info.resolver.ts, .../info/info.module.ts, .../info/info.resolver.spec.ts, .../info/info.resolver.integration.spec.ts, .../info/info.service.ts (removed), .../info/info.service.spec.ts (removed)
Replaces InfoService with specialized services (CpuService, MemoryService, OsService, VersionsService, DisplayService); adds machineId ResolveField; time uses GraphQLISODateTime; devices returns an id stub for sub-resolver delegation; removes apps and subscription surface; registers providers via new InfoModule; updates tests and deletes old service/tests.
Devices submodule
.../info/devices/devices.resolver.ts, .../info/devices/devices.model.ts, .../info/devices/devices.service.ts, .../info/devices/devices.resolver.spec.ts, .../info/devices/devices.service.spec.ts, (old .../info/devices.resolver.ts removed)
Moves devices resolver/service into devices subfolder, adopts InfoGpu/InfoNetwork/InfoPci/InfoUsb types, adds generateNetwork, updates import paths and tests.
Display relocation & typing
api/src/unraid-api/graph/resolvers/display/display.resolver.ts, .../display/display.resolver.spec.ts, .../info/display/display.service.ts, .../info/display/display.model.ts, .../info/display/display.service.spec.ts
Repoints display imports to info/display module, introduces InfoDisplay and Temperature enum, service returns typed id'd objects ('info/display', 'display/case'), adds enum validation and config typing, updates tests.
Metrics & subscriptions infra
api/src/unraid-api/graph/resolvers/metrics/*, api/src/unraid-api/graph/services/services.module.ts, .../services/subscription-helper.service.ts, .../services/subscription-tracker.service.ts, .../services/subscription-polling.service.ts, api/src/core/pubsub.ts, packages/unraid-shared/src/pubsub/graphql.pubsub.ts, tests
Adds Metrics model/module/resolver; polls CPU (1s) and memory (2s) and publishes to new PUBSUB channels CPU_UTILIZATION/MEMORY_UTILIZATION; adds SubscriptionHelperService, SubscriptionTrackerService, SubscriptionPollingService with tests; makes createSubscription generic; extends pubsub enum with CPU_UTILIZATION and MEMORY_UTILIZATION.
Schema & web typings
api/generated-schema.graphql, web/composables/gql/graphql.ts
Regenerates GraphQL schema and TypeScript typings: introduces Info* types, Metrics, Container/Port models, telemetry types and subscription changes; removes legacy display/info subscriptions and old types; updates web typings accordingly.
Resolvers module wiring
api/src/unraid-api/graph/resolvers/resolvers.module.ts
Removes direct Display/Devices/Info providers and imports new InfoModule, ServicesModule, MetricsModule.
Subscription tests & utilities
api/src/unraid-api/graph/services/subscription-helper.service.spec.ts, .../subscription-tracker.service.spec.ts
Adds comprehensive tests covering tracked subscriptions, lifecycle, counts, logging, and edge cases.
Config bump & scheduling
api/dev/configs/api.json, api/src/unraid-api/app/app.module.ts, api/src/unraid-api/cron/cron.module.ts
Bumps api.json version 4.12.0 → 4.13.1; adds ScheduleModule.forRoot() to AppModule imports and removes it from CronModule.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant GQL as GraphQL API
  participant MR as MetricsResolver
  participant CPU as CpuService
  participant MEM as MemoryService
  participant TRK as SubscriptionTracker
  participant POLL as SubscriptionPolling
  participant PUB as PubSub

  rect rgb(245,248,255)
  note right of MR: onModuleInit registers polling topics
  MR->>TRK: registerTopic(CPU_UTILIZATION, 1000ms, callback)
  MR->>TRK: registerTopic(MEMORY_UTILIZATION, 2000ms, callback)
  TRK->>POLL: startPolling(name, interval, callback)
  end

  Client->>GQL: subscribe systemMetricsCpu
  GQL->>MR: systemMetricsCpuSubscription()
  MR->>TRK: subscribe(CPU_UTILIZATION)
  MR->>PUB: createTrackedSubscription(CPU_UTILIZATION)
  activate POLL
  loop every 1s
    POLL->>CPU: generateCpuLoad()
    CPU-->>POLL: CpuUtilization
    POLL->>PUB: publish { systemMetricsCpu }
  end
  PUB-->>GQL: payload { systemMetricsCpu }
  GQL-->>Client: CpuUtilization events
Loading
sequenceDiagram
  autonumber
  actor Client
  participant GQL as GraphQL API
  participant IR as InfoResolver
  participant CPU as CpuService
  participant MEM as MemoryService
  participant OS as OsService
  participant VER as VersionsService
  participant DISP as DisplayService
  participant DEV as DevicesResolver

  Client->>GQL: query { info { id time cpu memory os versions display devices { gpu pci usb network } } }
  GQL->>IR: info()
  IR-->>GQL: { id: "info" }
  GQL->>IR: time()
  IR-->>GQL: DateTime
  GQL->>IR: cpu()
  IR->>CPU: generateCpu()
  CPU-->>IR: InfoCpu
  IR-->>GQL: InfoCpu
  GQL->>IR: memory()
  IR->>MEM: generateMemory()
  MEM-->>IR: InfoMemory
  IR-->>GQL: InfoMemory
  GQL->>IR: os()
  IR->>OS: generateOs()
  OS-->>IR: InfoOs
  IR-->>GQL: InfoOs
  GQL->>IR: versions()
  IR->>VER: generateVersions()
  VER-->>IR: InfoVersions
  IR-->>GQL: InfoVersions
  GQL->>IR: display()
  IR->>DISP: getDisplay()
  DISP-->>IR: InfoDisplay
  IR-->>GQL: InfoDisplay
  GQL->>IR: devices()
  IR-->>GQL: { id: "info/devices" }
  GQL->>DEV: gpu/pci/usb/network
  DEV-->>GQL: arrays of Info* devices
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Poem

A rabbit taps the metrics drum,
CPU hums, memory thrums—here they come! 🥁
Info split into tidy parts,
Little nodes with well-trimmed hearts.
Pollers hop and pubs go click—🐇✨

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ca691b7 and ee65e80.

⛔ Files ignored due to path filters (1)
  • api/src/unraid-api/cli/generated/graphql.ts is excluded by !**/generated/**
📒 Files selected for processing (16)
  • api/generated-schema.graphql (5 hunks)
  • api/src/core/utils/validation/enum-validator.ts (1 hunks)
  • api/src/unraid-api/app/app.module.ts (2 hunks)
  • api/src/unraid-api/cron/cron.module.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/display/display.service.ts (5 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.module.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.resolver.integration.spec.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/memory/memory.model.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/memory/memory.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 (1 hunks)
  • api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts (1 hunks)
  • api/src/unraid-api/graph/services/services.module.ts (1 hunks)
  • web/composables/gql/graphql.ts (8 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/cpu-utilization-query

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@claude
Copy link

claude bot commented Aug 18, 2025

Claude finished @elibosley's task —— View job


Code Review - Critical Issues Analysis

Reviewing only the diff/changeset for high-priority issues:

  • Analyze changed lines for critical bugs
  • Check for security vulnerabilities in new code
  • Identify breaking changes
  • Look for data loss risks

🚨 Critical Issues Found

1. Line 183: Runtime crash on undefined access
api/src/unraid-api/graph/resolvers/info/devices/devices.service.ts:183

return id.replace('p', ':');

If regex match fails, id is undefined and id.replace() throws TypeError. Add null check.

2. Line 46: Inverted boolean logic
api/src/unraid-api/graph/resolvers/info/devices/devices.service.ts:46

blacklisted: entry.allowed,

This maps allowed=true to blacklisted=true, which is backwards. Should be !entry.allowed.

3. Lines 34, 44: Unhandled promise rejections
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts:34,44
Async callbacks in polling can throw unhandled rejections that crash the process. Add try/catch.

4. Module structure: Multiple ScheduleModule.forRoot()
Services module and cron module both call ScheduleModule.forRoot() which creates duplicate schedulers.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🔭 Outside diff range comments (1)
api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts (1)

373-382: Add tests for the new CPU query and subscription

This spec doesn’t cover:

  • Query: cpuUtilization
  • Subscription: cpuUtilizationSubscription
  • Field resolver: InfoCpu.utilization

Add targeted tests to validate behavior and interactions with SubscriptionTrackerService and CpuDataService.

Here’s a concise set you can drop in:

describe('cpuUtilization (query)', () => {
  it('returns CpuUtilization snapshot', async () => {
    const res = await resolver.cpuUtilization();
    expect(res).toEqual({ id: 'info/cpu-load', load: 10, cpus: [] });
  });
});

describe('cpuUtilizationSubscription (subscription)', () => {
  it('subscribes/unsubscribes via SubscriptionTrackerService', async () => {
    const { createSubscription } = await import('@app/core/pubsub.js');
    const iterator = { [Symbol.asyncIterator]: vi.fn().mockReturnValue({}), return: vi.fn() };
    vi.mocked(createSubscription).mockReturnValue(iterator as any);

    const asyncIt = await resolver.cpuUtilizationSubscription();

    // Subscribe
    asyncIt[Symbol.asyncIterator]();
    expect(mockSubscriptionTrackerService.subscribe).toHaveBeenCalledWith('CPU_UTILIZATION');

    // Unsubscribe
    await asyncIt.return?.();
    expect(mockSubscriptionTrackerService.unsubscribe).toHaveBeenCalledWith('CPU_UTILIZATION');
  });
});

describe('InfoCpu.utilization (field)', () => {
  it('resolves from CpuDataService cache', async () => {
    // Resolve field via separate resolver if exported, or call directly if available
    // Assuming InfoCpuResolver is provided similarly in the test module:
    const { InfoCpuResolver } = await import('@app/unraid-api/graph/resolvers/info/info.resolver.js');
    const infoCpuResolver = module.get<InstanceType<typeof InfoCpuResolver>>(InfoCpuResolver as any);
    const val = await infoCpuResolver.utilization({} as any);
    expect(val).toBe(10);
  });
});
🧹 Nitpick comments (9)
api/src/unraid-api/graph/services/subscription-tracker.service.ts (1)

19-29: Use nullish coalescing for 0-safe increment

|| 0 is fine here but for symmetry and 0-safety consistency with unsubscribe, prefer ?? 0.

Apply this diff:

-        const currentCount = this.subscriberCounts.get(topic) || 0;
+        const currentCount = this.subscriberCounts.get(topic) ?? 0;
api/src/unraid-api/graph/resolvers/info/info.model.ts (1)

151-155: Add description for CpuUtilization.load for consistency

Per-core CpuLoad.load is described as percent, but the top-level CpuUtilization.load lacks a description. Align docs to avoid ambiguity.

Apply this diff:

-    @Field(() => Float)
+    @Field(() => Float, { description: 'Overall CPU utilization in percent.' })
     load!: number;
api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts (4)

322-327: Use .rejects.toThrow() without asserting message text

Per testing guidelines, avoid brittle assertions on error message strings.

Apply:

-            await expect(resolver.machineId()).rejects.toThrow('Machine ID error');
+            await expect(resolver.machineId()).rejects.toThrow();

385-390: Use .rejects.toThrow() without asserting message text

Avoid asserting on exact error messages unless message format is under test.

-            await expect(resolver.baseboard()).rejects.toThrow('Baseboard error');
+            await expect(resolver.baseboard()).rejects.toThrow();

392-397: Use .rejects.toThrow() without asserting message text

Same guideline applies here.

-            await expect(resolver.system()).rejects.toThrow('System error');
+            await expect(resolver.system()).rejects.toThrow();

399-403: Use .rejects.toThrow() without asserting message text

Same guideline applies here.

-            await expect(resolver.apps()).rejects.toThrow('Service error');
+            await expect(resolver.apps()).rejects.toThrow();
api/src/unraid-api/graph/resolvers/info/info.resolver.ts (3)

60-73: Guard the polling loop and handle errors to avoid overlapping calls and unhandled rejections

setInterval with an async callback can overlap if the operation takes longer than the interval or throw unhandled rejections. Add an in-flight guard and try/catch.

 onModuleInit() {
     this.subscriptionTracker.registerTopic(
         CPU_UTILIZATION,
         () => {
-            this.cpuPollingTimer = setInterval(async () => {
-                const payload = await this.infoService.generateCpuLoad();
-                pubsub.publish(CPU_UTILIZATION, { cpuUtilization: payload });
-            }, 1000);
+            let inFlight = false;
+            this.cpuPollingTimer = setInterval(async () => {
+                if (inFlight) return;
+                inFlight = true;
+                try {
+                    const payload = await this.infoService.generateCpuLoad();
+                    await pubsub.publish(CPU_UTILIZATION, { cpuUtilization: payload });
+                } catch {
+                    // swallow or log; avoid unhandled rejection
+                } finally {
+                    inFlight = false;
+                }
+            }, 1000);
         },
         () => {
             clearInterval(this.cpuPollingTimer);
         }
     );
 }

204-211: Use Float GraphQL type for utilization

CPU load is fractional. Use Float instead of Number to avoid being inferred as Int.

-    @ResolveField(() => Number, {
+    @ResolveField(() => Float, {
         description: 'CPU utilization in percent',
         nullable: true,
     })
     public async utilization(@Parent() cpu: InfoCpu): Promise<number> {
         const { currentLoad } = await this.cpuDataService.getCpuLoad();
         return currentLoad;
     }

And add to imports:

-import {
-    Parent,
-    Query,
-    Resolver,
-    Subscription,
-    ResolveField,
-} from '@nestjs/graphql';
+import { Parent, Query, Resolver, Subscription, ResolveField, Float } from '@nestjs/graphql';

9-9: Unused import: PubSub

PubSub isn’t used here. Consider removing to satisfy lint rules.

-import { PubSub } from 'graphql-subscriptions';
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 541b0ed and 9a35b38.

📒 Files selected for processing (8)
  • api/src/unraid-api/graph/resolvers/info/cpu-data.service.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.model.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts (3 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts (3 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.service.ts (3 hunks)
  • api/src/unraid-api/graph/resolvers/resolvers.module.ts (2 hunks)
  • api/src/unraid-api/graph/services/services.module.ts (1 hunks)
  • api/src/unraid-api/graph/services/subscription-tracker.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

TypeScript source files must use import specifiers with .js extensions for ESM compatibility

Files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
  • api/src/unraid-api/graph/resolvers/info/cpu-data.service.ts
  • api/src/unraid-api/graph/services/services.module.ts
  • api/src/unraid-api/graph/resolvers/info/info.service.ts
  • api/src/unraid-api/graph/resolvers/resolvers.module.ts
  • api/src/unraid-api/graph/services/subscription-tracker.service.ts
  • api/src/unraid-api/graph/resolvers/info/info.model.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts
api/src/unraid-api/**

📄 CodeRabbit Inference Engine (CLAUDE.md)

Place new API source files in api/src/unraid-api/ rather than legacy locations

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.spec.ts
  • api/src/unraid-api/graph/resolvers/info/cpu-data.service.ts
  • api/src/unraid-api/graph/services/services.module.ts
  • api/src/unraid-api/graph/resolvers/info/info.service.ts
  • api/src/unraid-api/graph/resolvers/resolvers.module.ts
  • api/src/unraid-api/graph/services/subscription-tracker.service.ts
  • api/src/unraid-api/graph/resolvers/info/info.model.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Prefer not to mock simple dependencies in API tests

Files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)

Files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
**/*.{ts,tsx,js,jsx,vue}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Avoid unnecessary or obvious comments; only add comments when needed for clarity

Files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
  • api/src/unraid-api/graph/resolvers/info/cpu-data.service.ts
  • api/src/unraid-api/graph/services/services.module.ts
  • api/src/unraid-api/graph/resolvers/info/info.service.ts
  • api/src/unraid-api/graph/resolvers/resolvers.module.ts
  • api/src/unraid-api/graph/services/subscription-tracker.service.ts
  • api/src/unraid-api/graph/resolvers/info/info.model.ts
  • api/src/unraid-api/graph/resolvers/info/info.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.spec.ts
🧠 Learnings (10)
📚 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/info/info.resolver.spec.ts
📚 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.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls

Applied to files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
📚 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.ts,**/__test__/{components,store}/**/*.ts} : Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly

Applied to files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
📚 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.ts,**/__test__/{components,store}/**/*.ts} : Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues

Applied to files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
📚 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.ts,**/__test__/{components,store}/**/*.ts} : Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation

Applied to files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
📚 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__/store/**/*.ts : In store tests, mock external dependencies used by the store and verify interactions with those mocks

Applied to files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
📚 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.ts,**/__test__/{components,store}/**/*.ts} : Use vi.spyOn() to specify return values or behavior of methods under test

Applied to files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
📚 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__/mocks/**/*.ts : Frequently used mocks can also be placed under __test__/mocks

Applied to files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
📚 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 : Use createTestingPinia() to mock Pinia stores when testing Vue components

Applied to files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should mock external dependencies and services

Applied to files:

  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
🧬 Code Graph Analysis (5)
api/src/unraid-api/graph/resolvers/info/cpu-data.service.ts (2)
api/src/unraid-api/graph/resolvers/info/info.service.ts (1)
  • Injectable (29-113)
api/src/unraid-api/graph/services/subscription-tracker.service.ts (1)
  • Injectable (3-44)
api/src/unraid-api/graph/services/services.module.ts (1)
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
  • Module (43-88)
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
api/src/unraid-api/graph/services/services.module.ts (1)
  • Module (4-8)
api/src/unraid-api/graph/resolvers/info/info.model.ts (3)
api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts (3)
  • ObjectType (19-30)
  • ObjectType (32-59)
  • ObjectType (61-66)
api/src/unraid-api/graph/resolvers/config/config.model.ts (1)
  • ObjectType (5-14)
web/composables/gql/graphql.ts (1)
  • Node (1102-1104)
api/src/unraid-api/graph/resolvers/info/info.resolver.ts (2)
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (1)
  • Resolver (24-173)
api/src/core/pubsub.ts (2)
  • pubsub (12-12)
  • createSubscription (18-20)
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/info/cpu-data.service.ts

[failure] 2-2:
Insert

api/src/unraid-api/graph/resolvers/info/info.model.ts

[failure] 145-145:
Replace ·description:·'The·percentage·of·time·the·CPU·spent·servicing·hardware·interrupts.' with ⏎········description:·'The·percentage·of·time·the·CPU·spent·servicing·hardware·interrupts.',⏎···


[failure] 139-139:
Replace ·description:·'The·percentage·of·time·the·CPU·spent·on·low-priority·(niced)·user·space·processes.' with ⏎········description:⏎············'The·percentage·of·time·the·CPU·spent·on·low-priority·(niced)·user·space·processes.',⏎···

api/src/unraid-api/graph/resolvers/info/info.resolver.ts

[failure] 23-23:
Replace ⏎····createSubscription,⏎····PUBSUB_CHANNEL,⏎····pubsub,⏎}·from·'@app/core/pubsub.js';⏎ with ·DisplayService·}·from·'@app/unraid-api/graph/resolvers/display/display.service.js';


[failure] 22-22:
Insert createSubscription,·pubsub,·PUBSUB_CHANNEL·}·from·'@app/core/pubsub.js';⏎import·{·


[failure] 17-17:
Replace ⏎····baseboard·as·getBaseboard,⏎····system·as·getSystem,⏎ with ·PubSub·}·from·'graphql-subscriptions';⏎import·{·baseboard·as·getBaseboard,·system·as·getSystem·


[failure] 2-2:
Replace ⏎····Parent,⏎····Query,⏎····Resolver,⏎····Subscription,⏎····ResolveField,⏎}·from·'@nestjs/graphql';⏎import·{·PubSub·}·from·'graphql-subscriptions with ·Parent,·Query,·ResolveField,·Resolver,·Subscription·}·from·'@nestjs/graphql


[failure] 41-41:
Replace ····CpuUtilization,⏎}·from·'@app/unraid-api/graph/resolvers/info/info.model.js';⏎import·{·CpuDataService·}·from·'@app/unraid-api/graph/resolvers/info/cpu-data.service with }·from·'@app/unraid-api/graph/resolvers/info/info.model

🪛 GitHub Actions: CI - Main (API)
api/src/unraid-api/graph/resolvers/info/cpu-data.service.ts

[error] 2-2: Step 'pnpm run lint' failed: Prettier formatting issue in src/unraid-api/graph/resolvers/info/cpu-data.service.ts. Insert at line 2. (prettier/prettier)

🔇 Additional comments (8)
api/src/unraid-api/graph/resolvers/info/info.model.ts (1)

121-126: LGTM: new utilization field on InfoCpu

Optional Float with clear description; matches the new query/subscription semantics.

api/src/unraid-api/graph/resolvers/info/info.service.ts (1)

104-112: Verify CPU payload structure

I wasn’t able to inspect systeminformation’s type definitions here, so please ensure that each item in cpus matches your GraphQL CpuLoad fields exactly. Confirm you have these properties (or map from snake_case variants):

  • load
  • loadUser (or map from load_user)
  • loadSystem (or map from load_system)
  • loadNice (or map from load_nice)
  • loadIdle (or map from load_idle)
  • loadIrq (or map from load_irq)

If the library returns snake_case keys, transform before returning. For example:

const { currentLoad: load, cpus: rawCpus } = await currentLoad();
const cpus = rawCpus.map((c: any) => ({
  load:        c.load,
  loadUser:    c.loadUser    ?? c.load_user,
  loadSystem:  c.loadSystem  ?? c.load_system,
  loadNice:    c.loadNice    ?? c.load_nice,
  loadIdle:    c.loadIdle    ?? c.load_idle,
  loadIrq:     c.loadIrq     ?? c.load_irq,
}));
return { id: 'info/cpu-load', load, cpus };
api/src/unraid-api/graph/services/services.module.ts (1)

4-8: LGTM: services module wiring

Provider/export for SubscriptionTrackerService looks correct and matches import usage in ResolversModule.

api/src/unraid-api/graph/resolvers/resolvers.module.ts (2)

36-46: LGTM: DI wiring adds ServicesModule (for SubscriptionTrackerService)

Importing ServicesModule ensures the tracker is available in resolver DI. ESM .js specifiers are correct per repo guidelines.


83-85: LGTM: providers registered for CPU data flow

Registering InfoCpuResolver and CpuDataService integrates the new CPU utilization endpoints cleanly.

api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts (2)

192-204: Good DI: targeted mocks for CpuDataService and SubscriptionTrackerService

Nice job providing lightweight, focused mocks for both services. This keeps the resolver tests fast and deterministic while letting you assert interactions where needed.


225-233: Providers wiring looks correct

Registering the new services in the TestingModule aligns with the resolver’s constructor dependencies. This ensures the resolver can be instantiated without leaking real implementations into the unit tests.

api/src/unraid-api/graph/resolvers/info/info.resolver.ts (1)

200-212: No changes needed—InfoCpuResolver and CpuDataService are correctly wired

  • InfoCpuResolver is imported and listed in providers of ResolversModule (api/src/unraid-api/graph/resolvers/resolvers.module.ts:83).
  • CpuDataService is imported, provided (with @Injectable({ scope: Scope.REQUEST })), and listed in the same module (api/src/unraid-api/graph/resolvers/resolvers.module.ts:84).

The new field resolver will be active as expected.

google-labs-jules bot and others added 5 commits August 18, 2025 18:40
Adds a new query and subscription to the info resolver for CPU to get CPU utilization on Unraid.

- Adds `cpuUtilization` query to get a one-time snapshot of CPU load.
- Adds `cpuUtilizationSubscription` to get real-time updates on CPU load.
- Adds a `utilization` field to the `InfoCpu` type.
- Creates a generic `SubscriptionTrackerService` to manage polling for subscriptions, ensuring that polling only occurs when there are active subscribers.
- Creates a request-scoped `CpuDataService` to cache CPU load data within a single GraphQL request to improve performance.
- Updates tests to cover the new functionality.
- Adds detailed documentation to the `CpuLoad` object type.
…ndling

- Consolidated imports in the info resolver and service for better readability.
- Added `CpuDataService` to manage CPU load data more efficiently.
- Updated `InfoResolver` to include `InfoCpuResolver` for improved CPU data handling.
- Enhanced documentation for CPU load fields in the model to improve clarity.
- Streamlined the `SubscriptionTrackerService` for better topic management.
- Updated `CpuDataService` to use nullish coalescing for initializing `cpuLoadData`, enhancing readability and efficiency.
- Ensured `cpuLoadData` can be undefined, allowing for more flexible handling of CPU load data retrieval.
- Changed CPU utilization topic references to use the new PUBSUB_CHANNEL enum for consistency.
- Updated InfoResolver to handle CPU polling timer as potentially undefined, improving type safety.
- Adjusted import statement for SubscriptionTrackerService to include the correct file extension.
- Removed unnecessary imports and parameters in InfoResolver for cleaner code.
- Updated CPU utilization method to directly use infoService for generating CPU load.
- Enhanced SubscriptionTrackerService to improve subscriber count management and added early return for idempotency in unsubscribe method.
@elibosley elibosley force-pushed the feat/cpu-utilization-query branch from 07f9626 to 3afc421 Compare August 18, 2025 22:40
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
api/src/unraid-api/graph/resolvers/info/info.resolver.ts (1)

33-41: Good: Resolver remains singleton (no request-scoped deps injected)

Removing CpuDataService from InfoResolver avoids request-scope escalation and prevents onModuleInit from running per request. This addresses the earlier concern.

🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/info/info.resolver.ts (2)

42-55: Harden polling lifecycle: avoid overlap, handle errors, and clear interval safely

Current setInterval body is async without guarding reentrancy; if a tick runs longer than 1s, calls can overlap. Also, errors in the async body are unhandled, and clearInterval is called with a possibly undefined handle. Recommend the following refinements.

Apply this diff within this block:

     this.subscriptionTracker.registerTopic(
         PUBSUB_CHANNEL.CPU_UTILIZATION,
         () => {
-            this.cpuPollingTimer = setInterval(async () => {
-                const payload = await this.infoService.generateCpuLoad();
-                pubsub.publish(PUBSUB_CHANNEL.CPU_UTILIZATION, { cpuUtilization: payload });
-            }, 1000);
+            let inFlight = false;
+            this.cpuPollingTimer = setInterval(async () => {
+                if (inFlight) return;
+                inFlight = true;
+                try {
+                    const payload = await this.infoService.generateCpuLoad();
+                    await pubsub.publish(PUBSUB_CHANNEL.CPU_UTILIZATION, { cpuUtilization: payload });
+                } catch (err) {
+                    // TODO: optionally log error
+                } finally {
+                    inFlight = false;
+                }
+            }, 1000);
         },
         () => {
-            clearInterval(this.cpuPollingTimer);
+            if (this.cpuPollingTimer) {
+                clearInterval(this.cpuPollingTimer);
+                this.cpuPollingTimer = undefined;
+            }
         }
     );

181-188: Prefer GraphQL Float over Number for clarity

Use Float from @nestjs/graphql to emit a GraphQLFloat. Number works but is less explicit.

Apply this diff here:

-    @ResolveField(() => Number, {
+    @ResolveField(() => Float, {
         description: 'CPU utilization in percent',
         nullable: true,
     })

And update the import at the top of this file:

// Add Float to the imports from @nestjs/graphql
import { Query, ResolveField, Resolver, Subscription, Float } from '@nestjs/graphql';
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9a35b38 and 3afc421.

📒 Files selected for processing (9)
  • api/src/unraid-api/graph/resolvers/info/cpu-data.service.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.model.ts (1 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts (3 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts (4 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.service.ts (2 hunks)
  • api/src/unraid-api/graph/resolvers/resolvers.module.ts (3 hunks)
  • api/src/unraid-api/graph/services/services.module.ts (1 hunks)
  • api/src/unraid-api/graph/services/subscription-tracker.service.ts (1 hunks)
  • packages/unraid-shared/src/pubsub/graphql.pubsub.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • api/src/unraid-api/graph/resolvers/info/cpu-data.service.ts
  • api/src/unraid-api/graph/resolvers/info/info.service.ts
  • api/src/unraid-api/graph/services/services.module.ts
  • api/src/unraid-api/graph/services/subscription-tracker.service.ts
  • api/src/unraid-api/graph/resolvers/resolvers.module.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.spec.ts
  • api/src/unraid-api/graph/resolvers/info/info.model.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

TypeScript source files must use import specifiers with .js extensions for ESM compatibility

Files:

  • packages/unraid-shared/src/pubsub/graphql.pubsub.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts
**/*.{ts,tsx,js,jsx,vue}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Avoid unnecessary or obvious comments; only add comments when needed for clarity

Files:

  • packages/unraid-shared/src/pubsub/graphql.pubsub.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts
api/src/unraid-api/**

📄 CodeRabbit Inference Engine (CLAUDE.md)

Place new API source files in api/src/unraid-api/ rather than legacy locations

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.ts
🧬 Code Graph Analysis (1)
api/src/unraid-api/graph/resolvers/info/info.resolver.ts (3)
api/src/unraid-api/graph/resolvers/display/display.resolver.ts (1)
  • Resolver (14-37)
api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1)
  • Resolver (14-37)
api/src/core/pubsub.ts (2)
  • pubsub (12-12)
  • createSubscription (18-20)
🔇 Additional comments (2)
packages/unraid-shared/src/pubsub/graphql.pubsub.ts (1)

7-9: New CPU_UTILIZATION pubsub channel looks good

Enum entry is consistent with existing channels and unblocks the resolver/subscription wiring that targets this topic.

api/src/unraid-api/graph/resolvers/info/info.resolver.ts (1)

142-150: Query cpuUtilization wired correctly to InfoService

Returning CpuUtilization via InfoService keeps resolver singleton and matches the subscription payload shape.

…ion management

- Added `SubscriptionHelperService` to streamline the creation of tracked GraphQL subscriptions with automatic cleanup.
- Updated `InfoResolver` to utilize the new service for managing CPU utilization subscriptions, enhancing code clarity and maintainability.
- Introduced unit tests for `SubscriptionHelperService` and `SubscriptionTrackerService` to ensure robust functionality and reliability.
- Refactored `SubscriptionTrackerService` to include logging for subscription events, improving observability.
- Removed deprecated types and methods related to previous subscription handling, simplifying the codebase.
…ote access and connection settings

- Introduced new types including `AccessUrl`, `Cloud`, `RemoteAccess`, and `Connect` to enhance remote access capabilities.
- Added enums for `URL_TYPE`, `WAN_ACCESS_TYPE`, and `WAN_FORWARD_TYPE` to standardize access configurations.
- Implemented new input types for connection settings and remote access setup, improving API usability.
- Updated the `Query` and `Mutation` types to include new fields for managing remote access and connection settings.
- Bumped API version to 4.13.1 in configuration.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
api/src/unraid-api/graph/resolvers/info/info.resolver.ts (2)

12-12: Good: central PUBSUB channel + ESM import extensions are correctly used

Using PUBSUB_CHANNEL.CPU_UTILIZATION with .js import specifiers aligns with our ESM and central-channel guidelines. Thanks for addressing earlier feedback.


144-152: Query uses InfoService (singleton) — avoids request-scope escalation

Routing cpuUtilization through InfoService keeps InfoResolver singleton and prevents OnModuleInit from running per request. This resolves the prior concern.

🧹 Nitpick comments (9)
api/src/unraid-api/graph/services/subscription-helper.service.ts (2)

19-21: Simplify iterator creation (avoid unnecessary cast and symbol lookup)

createSubscription already returns an AsyncIterableIterator. No need to cast to AsyncIterable and then call Symbol.asyncIterator. This also reduces the chance of subtle iterator shape mismatches.

Apply this diff:

-        const iterator = createSubscription(topic) as AsyncIterable<T>;
-        const innerIterator = iterator[Symbol.asyncIterator]();
+        const innerIterator = createSubscription(topic) as AsyncIterableIterator<T>;

26-28: Ensure cleanup on next() errors or iterator completion

If innerIterator.next() throws or resolves with done: true, the tracker never unsubscribes, potentially leaking a "subscriber" count. Wrap next() to unsubscribe on error or completion.

Apply this diff:

-        const proxyIterator: AsyncIterableIterator<T> = {
-            next: () => innerIterator.next(),
+        const proxyIterator: AsyncIterableIterator<T> = {
+            next: async () => {
+                try {
+                    const result = await innerIterator.next();
+                    if (result?.done) {
+                        // Cleanup on natural completion
+                        this.subscriptionTracker.unsubscribe(topic);
+                    }
+                    return result;
+                } catch (err) {
+                    // Cleanup on error during next()
+                    this.subscriptionTracker.unsubscribe(topic);
+                    throw err;
+                }
+            },
api/generated-schema.graphql (1)

2239-2239: Align subscription field naming for consistency

The Subscription type currently mixes suffixed and non-suffixed field names:

• Suffix present:

  • displaySubscription
  • infoSubscription
  • ownerSubscription
  • serversSubscription
  • parityHistorySubscription
  • arraySubscription

• Suffix missing:

  • cpuUtilization
  • logFile
  • notificationAdded
  • notificationsOverview
  • upsUpdates

To keep a clear, uniform convention (and avoid confusing GraphQL clients), consider appending Subscription to each non-suffixed field:

• cpuUtilization → cpuUtilizationSubscription
• logFile → logFileSubscription
• notificationAdded → notificationAddedSubscription
• notificationsOverview → notificationsOverviewSubscription
• upsUpdates → upsUpdatesSubscription

Locations to update:

  • Schema (api/generated-schema.graphql): rename fields under type Subscription.
  • Resolvers (e.g., info.resolver.ts, logs.resolver.ts, ups.resolver.ts, etc.): adjust @Subscription({ name: '…' }) decorators and/or method names to match the new GraphQL names.

This refactor will ensure all subscription endpoints follow the same naming pattern.

api/src/unraid-api/graph/services/subscription-tracker.service.spec.ts (2)

67-75: Relax log assertions to avoid brittle tests

Asserting exact log strings is fragile. Prefer expect.stringContaining() to focus on behavior over wording per our testing guidelines.

Apply these diffs:

@@
-            expect(loggerSpy).toHaveBeenCalledWith(
-                "Subscription added for topic 'TEST_TOPIC': 1 active subscriber(s)"
-            );
+            expect(loggerSpy).toHaveBeenCalledWith(
+                expect.stringContaining("Subscription added for topic 'TEST_TOPIC'")
+            );
@@
-            expect(loggerSpy).toHaveBeenCalledWith(
-                "Subscription added for topic 'TEST_TOPIC': 2 active subscriber(s)"
-            );
+            expect(loggerSpy).toHaveBeenCalledWith(
+                expect.stringContaining("Subscription added for topic 'TEST_TOPIC'")
+            );
@@
-            expect(loggerSpy).toHaveBeenCalledWith("Starting topic 'TEST_TOPIC' (first subscriber)");
+            expect(loggerSpy).toHaveBeenCalledWith(
+                expect.stringContaining("Starting topic 'TEST_TOPIC'")
+            );
@@
-            expect(loggerSpy).toHaveBeenCalledWith(
-                "Unsubscribe called for topic 'TEST_TOPIC' but no active subscribers (idempotent)"
-            );
+            expect(loggerSpy).toHaveBeenCalledWith(
+                expect.stringContaining("no active subscribers (idempotent)")
+            );
@@
-            expect(loggerSpy).toHaveBeenCalledWith(
-                "Subscription removed for topic 'TEST_TOPIC': 1 active subscriber(s) remaining"
-            );
+            expect(loggerSpy).toHaveBeenCalledWith(
+                expect.stringContaining("Subscription removed for topic 'TEST_TOPIC'")
+            );
@@
-            expect(loggerSpy).toHaveBeenCalledWith(
-                "Subscription removed for topic 'TEST_TOPIC': 0 active subscriber(s) remaining"
-            );
+            expect(loggerSpy).toHaveBeenCalledWith(
+                expect.stringContaining("Subscription removed for topic 'TEST_TOPIC'")
+            );
@@
-            expect(loggerSpy).toHaveBeenCalledWith(
-                "Stopping topic 'TEST_TOPIC' (last subscriber removed)"
-            );
+            expect(loggerSpy).toHaveBeenCalledWith(
+                expect.stringContaining("Stopping topic 'TEST_TOPIC'")
+            );

Also applies to: 84-85, 137-139, 149-156, 169-171


9-9: Type the spy for clarity

Prefer a specific spy type instead of any.

Apply this diff:

-    let loggerSpy: any;
+    let loggerSpy: vi.SpyInstance;
api/src/unraid-api/graph/services/subscription-helper.service.spec.ts (2)

55-67: Use .rejects.toThrow() for async error assertion

Aligns with our testing guideline to assert thrown errors succinctly.

Apply this diff:

-            try {
-                await iterator.throw?.(new Error('Test error'));
-            } catch (e) {
-                // Expected to throw
-            }
+            await expect(iterator.throw?.(new Error('Test error'))).rejects.toThrow();

168-183: Simplify error test with .rejects.toThrow() and avoid matching specific error object

Focus on behavior, not exact error identity, to reduce brittleness.

Apply this diff:

-            const testError = new Error('Test error');
-
-            try {
-                await iterator.throw?.(testError);
-                expect.fail('Should have thrown');
-            } catch (error) {
-                expect(error).toBe(testError);
-            }
+            await expect(iterator.throw?.(new Error('Test error'))).rejects.toThrow();
api/src/unraid-api/graph/resolvers/info/info.resolver.ts (2)

44-57: Make polling start/stop idempotent and handle errors inside the interval

Guard against multiple intervals, clear safely, and prevent unhandled rejections in the async interval callback.

Apply this diff:

-    onModuleInit() {
-        this.subscriptionTracker.registerTopic(
-            PUBSUB_CHANNEL.CPU_UTILIZATION,
-            () => {
-                this.cpuPollingTimer = setInterval(async () => {
-                    const payload = await this.infoService.generateCpuLoad();
-                    pubsub.publish(PUBSUB_CHANNEL.CPU_UTILIZATION, { cpuUtilization: payload });
-                }, 1000);
-            },
-            () => {
-                clearInterval(this.cpuPollingTimer);
-            }
-        );
-    }
+    onModuleInit() {
+        this.subscriptionTracker.registerTopic(
+            PUBSUB_CHANNEL.CPU_UTILIZATION,
+            () => {
+                if (this.cpuPollingTimer) {
+                    // Already polling
+                    return;
+                }
+                this.cpuPollingTimer = setInterval(async () => {
+                    try {
+                        const payload = await this.infoService.generateCpuLoad();
+                        pubsub.publish(PUBSUB_CHANNEL.CPU_UTILIZATION, { cpuUtilization: payload });
+                    } catch (_err) {
+                        // Swallow or log as needed to avoid unhandled rejection noise
+                    }
+                }, 1000);
+            },
+            () => {
+                if (this.cpuPollingTimer) {
+                    clearInterval(this.cpuPollingTimer);
+                    this.cpuPollingTimer = undefined;
+                }
+            }
+        );
+    }

Optionally, consider adding OnModuleDestroy to clear any leftover interval on shutdown.


172-179: Prefer GraphQL Float for utilization scalar

Number works but Float communicates intent and aligns with GraphQL scalar mappings in Nest.

Apply this diff in-place:

-    @ResolveField(() => Number, {
+    @ResolveField(() => Float, {
         description: 'CPU utilization in percent',
         nullable: true,
     })

Also add the import (outside this hunk):

// at the top import from @nestjs/graphql
import { Query, ResolveField, Resolver, Subscription, Float } from '@nestjs/graphql';
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3afc421 and 573b048.

📒 Files selected for processing (8)
  • api/dev/configs/api.json (1 hunks)
  • api/generated-schema.graphql (3 hunks)
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts (4 hunks)
  • api/src/unraid-api/graph/services/services.module.ts (1 hunks)
  • api/src/unraid-api/graph/services/subscription-helper.service.spec.ts (1 hunks)
  • api/src/unraid-api/graph/services/subscription-helper.service.ts (1 hunks)
  • api/src/unraid-api/graph/services/subscription-tracker.service.spec.ts (1 hunks)
  • api/src/unraid-api/graph/services/subscription-tracker.service.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • api/dev/configs/api.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • api/src/unraid-api/graph/services/subscription-tracker.service.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

TypeScript source files must use import specifiers with .js extensions for ESM compatibility

Files:

  • api/src/unraid-api/graph/services/services.module.ts
  • api/src/unraid-api/graph/services/subscription-tracker.service.spec.ts
  • api/src/unraid-api/graph/services/subscription-helper.service.spec.ts
  • api/src/unraid-api/graph/services/subscription-helper.service.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts
api/src/unraid-api/**

📄 CodeRabbit Inference Engine (CLAUDE.md)

Place new API source files in api/src/unraid-api/ rather than legacy locations

Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code

Files:

  • api/src/unraid-api/graph/services/services.module.ts
  • api/src/unraid-api/graph/services/subscription-tracker.service.spec.ts
  • api/src/unraid-api/graph/services/subscription-helper.service.spec.ts
  • api/src/unraid-api/graph/services/subscription-helper.service.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts
**/*.{ts,tsx,js,jsx,vue}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Avoid unnecessary or obvious comments; only add comments when needed for clarity

Files:

  • api/src/unraid-api/graph/services/services.module.ts
  • api/src/unraid-api/graph/services/subscription-tracker.service.spec.ts
  • api/src/unraid-api/graph/services/subscription-helper.service.spec.ts
  • api/src/unraid-api/graph/services/subscription-helper.service.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Prefer not to mock simple dependencies in API tests

Files:

  • api/src/unraid-api/graph/services/subscription-tracker.service.spec.ts
  • api/src/unraid-api/graph/services/subscription-helper.service.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)

Files:

  • api/src/unraid-api/graph/services/subscription-tracker.service.spec.ts
  • api/src/unraid-api/graph/services/subscription-helper.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/services/subscription-tracker.service.spec.ts
  • api/src/unraid-api/graph/services/subscription-helper.service.spec.ts
🧠 Learnings (4)
📚 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.ts,**/__test__/{components,store}/**/*.ts} : Focus tests on observable behavior and outcomes, not implementation details such as exact error messages

Applied to files:

  • api/src/unraid-api/graph/services/subscription-tracker.service.spec.ts
📚 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.ts,**/__test__/{components,store}/**/*.ts} : Use vi.spyOn() to specify return values or behavior of methods under test

Applied to files:

  • api/src/unraid-api/graph/services/subscription-tracker.service.spec.ts
📚 Learning: 2025-07-22T13:29:32.175Z
Learnt from: elibosley
PR: unraid/api#1526
File: api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts:82-85
Timestamp: 2025-07-22T13:29:32.175Z
Learning: In GraphQL subscriptions using the graphql-subscriptions PubSub, the correct method is `asyncIterableIterator()`, not `asyncIterator()`. The `asyncIterableIterator` method returns an `AsyncIterableIterator` that conforms to both `AsyncIterable` and `AsyncIterator` interfaces, which is required for GraphQL subscription resolvers. Using `asyncIterator()` would cause TypeScript type errors as it only returns an `AsyncIterator` that doesn't satisfy the `AsyncIterable` interface requirement.

Applied to files:

  • api/src/unraid-api/graph/services/subscription-helper.service.spec.ts
  • api/src/unraid-api/graph/services/subscription-helper.service.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts
📚 Learning: 2025-07-22T13:29:32.175Z
Learnt from: elibosley
PR: unraid/api#1526
File: api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts:82-85
Timestamp: 2025-07-22T13:29:32.175Z
Learning: In GraphQL subscriptions using the graphql-subscriptions PubSub, the correct method to create an async iterator is `asyncIterableIterator()`, not `asyncIterator()`. This is the modern/current method used in the graphql-subscriptions library.

Applied to files:

  • api/src/unraid-api/graph/services/subscription-helper.service.ts
  • api/src/unraid-api/graph/resolvers/info/info.resolver.ts
🧬 Code Graph Analysis (4)
api/src/unraid-api/graph/services/services.module.ts (1)
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
  • Module (42-87)
api/src/unraid-api/graph/services/subscription-helper.service.spec.ts (1)
api/src/core/pubsub.ts (1)
  • pubsub (12-12)
api/src/unraid-api/graph/services/subscription-helper.service.ts (1)
api/src/core/pubsub.ts (1)
  • createSubscription (18-20)
api/src/unraid-api/graph/resolvers/info/info.resolver.ts (1)
api/src/core/pubsub.ts (1)
  • pubsub (12-12)
🔇 Additional comments (8)
api/src/unraid-api/graph/services/services.module.ts (1)

6-10: Module wiring looks correct and ESM-compatible

Providers/exports are properly declared, imports use .js extensions per our ESM guideline, and the file lives under api/src/unraid-api/. This should integrate cleanly with ResolversModule.

api/src/unraid-api/graph/services/subscription-helper.service.ts (1)

22-24: Good ordering to avoid initial publish race

Creating the async iterator before calling subscribe() helps ensure listeners are attached before onStart-triggered polling/publishing begins.

api/generated-schema.graphql (4)

982-984: InfoCpu.utilization addition looks good

Optional utilization field is a sensible extension and aligns with the PR’s goals.


986-1007: CpuLoad type is well-documented and clear

The per-core load fields and descriptions look consistent and useful for clients.


1009-1013: CpuUtilization Node type LGTM

Non-null id and top-level load with per-core cpus array matches expected usage.


1955-1955: Query.cpuUtilization resolver is correctly implemented and wired

  • A @Query(() => CpuUtilization) method named cpuUtilization exists in InfoResolver (lines 150–152 of info.resolver.ts) and returns this.infoService.generateCpuLoad().
  • The CpuUtilization GraphQL type is defined in info.model.ts as
    @ObjectType({ implements: () => Node })
    extending Node, so it inherits a @Field(() => PrefixedID) id: string field.
  • Both InfoResolver (with cpuUtilization) and InfoCpuResolver are listed under providers in ResolversModule, and ServicesModule is imported.

No further changes are needed.

api/src/unraid-api/graph/resolvers/info/info.resolver.ts (2)

168-171: Ok to inject request-scoped CpuDataService in the field resolver

Keeping request-scoped caching at InfoCpuResolver while InfoResolver remains singleton is the right split.


154-165: SubscriptionHelper returns AsyncIterableIterator & manages lifecycle correctly

The createTrackedSubscription method in subscription-helper.service.ts has a return type of AsyncIterableIterator<T> and uses Symbol.asyncIterator. It also calls this.subscriptionTracker.subscribe(topic) when the iterator starts and unsubscribe(topic) on both return and error, fulfilling the expected subscribe/unsubscribe behavior.

…etrics resolver (#1594)

- also enables subscription resolution for CPU and memory usage
- Updated `createSubscription` and `createTrackedSubscription` methods to include generic type parameters, improving type safety and ensuring correct async iterable handling.
…ption handling

- Removed legacy polling methods from `MetricsResolver` and integrated polling logic into `SubscriptionPollingService` for better separation of concerns.
- Updated `SubscriptionTrackerService` to support polling configuration directly, allowing for cleaner topic registration.
- Adjusted unit tests to accommodate changes in the subscription handling and polling logic.
- Introduced `SubscriptionPollingService` to manage polling intervals and ensure efficient execution of polling tasks.
…ptionPollingService

- Integrated `ScheduleModule` to manage polling intervals effectively.
- Updated tests to utilize `SubscriptionPollingService` for CPU and memory polling, ensuring single execution during concurrent attempts.
- Improved error handling in polling tests to verify graceful error logging.
- Ensured proper cleanup of polling subscriptions and timers during module destruction.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
api/src/unraid-api/graph/resolvers/info/devices/devices.service.ts (1)

103-108: Missing bus and device fields for USB devices.

The generateUsb() method only maps id and name fields, but the InfoUsb type also includes optional bus and device fields. The private getSystemUSBDevices() returns a UsbDevice type that doesn't include these fields.

Consider extracting bus and device information from the USB device data. The lsusb output typically includes bus and device numbers that could be parsed and included:

-    async generateUsb(): Promise<InfoUsb[]> {
+    async generateUsb(): Promise<InfoUsb[]> {
         try {
             const usbDevices = await this.getSystemUSBDevices();
             return usbDevices.map((device) => ({
                 id: `usb/${device.id}`,
                 name: device.name,
+                bus: device.bus,
+                device: device.device,
             }));
         } catch (error: unknown) {

This would also require updating the UsbDevice interface and the parsing logic in getSystemUSBDevices().

api/src/unraid-api/graph/resolvers/info/info.resolver.ts (1)

75-79: Fix GraphQL type mismatch: machineId should be ID, not String.

The Info model declares machineId as ID; resolver annotates it as String. Align resolver decorator to ID to match the generated schema and avoid confusion.

-    @ResolveField(() => String, { nullable: true })
+    @ResolveField(() => ID, { nullable: true })
     public async machineId(): Promise<string | undefined> {
         return getMachineId();
     }

Note: Also import ID from @nestjs/graphql (see diff below).

🧹 Nitpick comments (29)
api/src/unraid-api/graph/resolvers/info/cpu/cpu.model.ts (1)

88-90: Prefer unknown over any for JSON payloads

Tighten typing for the JSON cache to avoid any leakage.

-    cache?: Record<string, any>;
+    cache?: Record<string, unknown>;
api/src/unraid-api/graph/resolvers/info/display/display.model.ts (1)

1-1: Remove unused Float import

Float isn’t used in this file.

-import { Field, Float, Int, ObjectType, registerEnumType } from '@nestjs/graphql';
+import { Field, Int, ObjectType, registerEnumType } from '@nestjs/graphql';
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (1)

8-12: Avoid duplicating CpuService/CpuDataService/MemoryService providers across modules to preserve request-scoped caching

MetricsModule re-declares CpuService, CpuDataService, and MemoryService, which are also provided by InfoModule. For request-scoped CpuDataService, this results in separate instances per module per request, defeating the goal of sharing cached CPU data across resolvers within a single GraphQL request. Centralize these providers in one module and re-use via exports/imports.

Recommended approach:

  • Export CpuService, CpuDataService, and MemoryService from InfoModule.
  • Import InfoModule in MetricsModule and remove the duplicate providers.

Apply this diff to MetricsModule:

 import { Module } from '@nestjs/common';

 import { CpuDataService, 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';
+import { InfoModule } from '@app/unraid-api/graph/resolvers/info/info.module.js';

 @Module({
-    imports: [ServicesModule],
-    providers: [MetricsResolver, CpuService, CpuDataService, MemoryService],
+    imports: [ServicesModule, InfoModule],
+    providers: [MetricsResolver],
     exports: [MetricsResolver],
 })
 export class MetricsModule {}

And update InfoModule exports as suggested in the InfoModule review comment.

api/src/unraid-api/graph/resolvers/info/info.module.ts (2)

14-34: Module wiring looks solid; consider exporting CPU/memory services for reuse

InfoModule correctly registers the Info resolver and sub-services. To prevent duplicate provider instances and to share request-scoped CpuDataService across resolvers (including Metrics), export CpuService, CpuDataService, and MemoryService here and have MetricsModule import InfoModule instead of re-providing them.

Apply this diff to export the services:

 @Module({
     imports: [ConfigModule, ServicesModule],
     providers: [
         // Main resolver
         InfoResolver,

         // Sub-resolvers
         DevicesResolver,

         // Services
         CpuService,
         CpuDataService,
         MemoryService,
         DevicesService,
         OsService,
         VersionsService,
         DisplayService,
     ],
-    exports: [InfoResolver, DevicesResolver, DisplayService],
+    exports: [
+        InfoResolver,
+        DevicesResolver,
+        DisplayService,
+        CpuService,
+        CpuDataService,
+        MemoryService,
+    ],
 })
 export class InfoModule {}

32-32: Optional: avoid exporting resolvers unless another module consumes them

Exporting resolvers is usually unnecessary unless another module injects them. You can simplify the surface by only exporting services that are re-used elsewhere.

packages/unraid-shared/src/pubsub/graphql.pubsub.ts (1)

11-11: Add brief docs for new channels to aid discoverability

Consider documenting payload shape or linking to the CpuLoad type where appropriate. This helps consumers of the shared package know what data to expect.

Example:

/** Publishes CpuLoad snapshots/stream data; see CpuLoad GraphQL type for fields */
CPU_UTILIZATION = "CPU_UTILIZATION",
/** Publishes memory utilization snapshots/stream data */
MEMORY_UTILIZATION = "MEMORY_UTILIZATION",
api/src/core/pubsub.ts (1)

18-21: Prefer unknown over any as the default generic

This strengthens typing without affecting call sites that explicitly specify T.

Apply:

-export const createSubscription = <T = any>(
+export const createSubscription = <T = unknown>(
     channel: GRAPHQL_PUBSUB_CHANNEL
 ): AsyncIterableIterator<T> => {
     return pubsub.asyncIterableIterator<T>(channel);
 };
api/src/unraid-api/graph/resolvers/info/os/os.service.ts (1)

14-19: Optional: make hostname resolution resilient with a fallback

If getters.emhttp().var.name is ever unset, we can fall back to the system hostname. This avoids returning undefined and better matches the schema’s intent.

Apply:

@@
-import { osInfo } from 'systeminformation';
+import { osInfo } from 'systeminformation';
+import { hostname as getHostname } from 'node:os';
@@
-            hostname: getters.emhttp().var.name,
+            hostname: getters.emhttp()?.var?.name ?? getHostname(),
             uptime: bootTimestamp.toISOString(),
api/src/unraid-api/graph/resolvers/info/display/display.service.spec.ts (1)

40-40: Optional: reduce reliance on “dev files” in tests

The test mentions using actual dev files for configuration merging. That can be brittle across environments. Prefer deterministic fixtures (e.g., fixtures/default.cfg and fixtures/dynamix.cfg) and mock fs.readFile to return those contents. This keeps behavior-focused tests stable and faster.

I can extract those contents into fixtures and wire up the vi.mock readFile to use them if you want.

api/src/unraid-api/graph/resolvers/info/os/os.model.ts (2)

46-48: Unify nullable boolean typing (avoid boolean | null union)

For consistency with other fields that use nullable: true, prefer uefi?: boolean instead of boolean | null.

-    @Field(() => Boolean, { nullable: true, description: 'OS started via UEFI' })
-    uefi?: boolean | null;
+    @Field(() => Boolean, { nullable: true, description: 'OS started via UEFI' })
+    uefi?: boolean;

37-38: Clarify uptime description (name vs. meaning)

Field name suggests "time since boot", but description says "Boot time ISO string". Recommend clarifying to avoid confusion.

-    @Field(() => String, { nullable: true, description: 'Boot time ISO string' })
+    @Field(() => String, { nullable: true, description: 'System uptime as ISO-8601 duration string (e.g., PT1H35M)' })

If the value is actually the boot timestamp, consider renaming the field to bootTime in a follow-up breaking-change PR.

api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts (3)

1-11: Use topic constants instead of raw strings

Import the shared pubsub topic constants to avoid string drift and improve refactor safety.

 import { SubscriptionHelperService } from '@app/unraid-api/graph/services/subscription-helper.service.js';
 import { SubscriptionTrackerService } from '@app/unraid-api/graph/services/subscription-tracker.service.js';
 
+import { CPU_UTILIZATION, MEMORY_UTILIZATION } from '@unraid/shared/pubsub/graphql.pubsub.js';

123-127: Avoid asserting exact error messages in tests

Per the testing guidelines, use .rejects.toThrow() without arguments to prevent brittle tests tied to message wording.

-            await expect(resolver.cpu()).rejects.toThrow('CPU error');
+            await expect(resolver.cpu()).rejects.toThrow();

-            await expect(resolver.memory()).rejects.toThrow('Memory error');
+            await expect(resolver.memory()).rejects.toThrow();

Also applies to: 151-155


175-183: Replace hardcoded topic names with constants

Use imported CPU_UTILIZATION and MEMORY_UTILIZATION to keep tests aligned with production topics.

-            expect(subscriptionTracker.registerTopic).toHaveBeenCalledWith(
-                'CPU_UTILIZATION',
+            expect(subscriptionTracker.registerTopic).toHaveBeenCalledWith(
+                CPU_UTILIZATION,
                 expect.any(Function),
                 1000
             );
-            expect(subscriptionTracker.registerTopic).toHaveBeenCalledWith(
-                'MEMORY_UTILIZATION',
+            expect(subscriptionTracker.registerTopic).toHaveBeenCalledWith(
+                MEMORY_UTILIZATION,
                 expect.any(Function),
                 2000
             );
api/src/unraid-api/graph/resolvers/info/memory/memory.service.ts (2)

16-22: Prefer stable IDs for memory layout entries over index-based IDs

Index-based IDs can shuffle across reboots or hardware changes, making caching/normalization brittle. Derive IDs from stable properties when available (e.g., serialNum/partNum/bank/size), falling back to index only if necessary.

-                dims.map(
-                    (dim, index) =>
-                        ({
-                            ...dim,
-                            id: `memory-layout-${index}`,
-                        }) as MemoryLayout
-                )
+                dims.map((dim, index) => {
+                    const stableKey =
+                        [dim.serialNum, dim.partNum, dim.bank, dim.size].filter(Boolean).join(':') ||
+                        String(index);
+                    return { ...dim, id: `memory-layout-${stableKey}` } as MemoryLayout;
+                })

32-49: Harden memory load collection with error handling and bounded percentages

If systeminformation.mem() throws (e.g., permissions or unsupported platform in CI), the resolver will reject and bubble errors. Also, percent calculations can produce out-of-range results on corner values. Add a try/catch and clamp to [0, 100].

-    async generateMemoryLoad(): Promise<MemoryUtilization> {
-        const memInfo = await mem();
+    async generateMemoryLoad(): Promise<MemoryUtilization> {
+        let memInfo;
+        try {
+            memInfo = await mem();
+        } catch {
+            return {
+                id: 'memory-utilization',
+                total: 0,
+                used: 0,
+                free: 0,
+                available: 0,
+                active: 0,
+                buffcache: 0,
+                usedPercent: 0,
+                swapTotal: 0,
+                swapUsed: 0,
+                swapFree: 0,
+                swapUsedPercent: 0,
+            };
+        }
 
         return {
             id: 'memory-utilization',
             total: Math.floor(memInfo.total),
             used: Math.floor(memInfo.used),
             free: Math.floor(memInfo.free),
             available: Math.floor(memInfo.available),
             active: Math.floor(memInfo.active),
             buffcache: Math.floor(memInfo.buffcache),
-            usedPercent:
-                memInfo.total > 0 ? ((memInfo.total - memInfo.available) / memInfo.total) * 100 : 0,
+            usedPercent:
+                memInfo.total > 0
+                    ? Math.min(100, Math.max(0, ((memInfo.total - memInfo.available) / memInfo.total) * 100))
+                    : 0,
             swapTotal: Math.floor(memInfo.swaptotal),
             swapUsed: Math.floor(memInfo.swapused),
             swapFree: Math.floor(memInfo.swapfree),
-            swapUsedPercent: memInfo.swaptotal > 0 ? (memInfo.swapused / memInfo.swaptotal) * 100 : 0,
+            swapUsedPercent:
+                memInfo.swaptotal > 0
+                    ? Math.min(100, Math.max(0, (memInfo.swapused / memInfo.swaptotal) * 100))
+                    : 0,
         };
     }
api/src/unraid-api/graph/resolvers/info/system/system.model.ts (1)

46-50: Optional: Consider Int for slot counts if schema allows

If memSlots is semantically an integer, using @Field(() => Int) can better convey intent. This would require aligning the generated schema (currently Float) and any frontend types first.

api/src/unraid-api/graph/resolvers/info/display/display.service.ts (2)

147-164: Avoid NaN creeping into numeric config fields

Number.parseInt(...) can yield NaN for missing/invalid values, which then propagates to the API as NaN. Coerce to undefined so the defaults in generateDisplay() apply cleanly.

-        return {
-            ...display,
-            theme: theme as ThemeName,
-            unit: unit as Temperature,
-            scale: toBoolean(display.scale),
-            tabs: toBoolean(display.tabs),
-            resize: toBoolean(display.resize),
-            wwn: toBoolean(display.wwn),
-            total: toBoolean(display.total),
-            usage: toBoolean(display.usage),
-            text: toBoolean(display.text),
-            warning: Number.parseInt(display.warning, 10),
-            critical: Number.parseInt(display.critical, 10),
-            hot: Number.parseInt(display.hot, 10),
-            max: Number.parseInt(display.max, 10),
-            locale: display.locale || 'en_US',
-        };
+        const toIntOrUndef = (v: unknown) => {
+            const n = Number.parseInt(String(v ?? ''), 10);
+            return Number.isFinite(n) ? n : undefined;
+        };
+        return {
+            ...display,
+            theme: theme as ThemeName,
+            unit: unit as Temperature,
+            scale: toBoolean(display.scale),
+            tabs: toBoolean(display.tabs),
+            resize: toBoolean(display.resize),
+            wwn: toBoolean(display.wwn),
+            total: toBoolean(display.total),
+            usage: toBoolean(display.usage),
+            text: toBoolean(display.text),
+            warning: toIntOrUndef(display.warning),
+            critical: toIntOrUndef(display.critical),
+            hot: toIntOrUndef(display.hot),
+            max: toIntOrUndef(display.max),
+            locale: display.locale || 'en_US',
+        };

132-165: Validate incoming enum values (ThemeName, Temperature) defensively

Casting theme/unit blindly may accept invalid values from config. Optionally, validate against the enum keys and fall back to defaults.

Example (outside this hunk):

const isTheme = (t: unknown): t is ThemeName => Object.values(ThemeName).includes(t as ThemeName);
const isTemp = (u: unknown): u is Temperature => Object.values(Temperature).includes(u as Temperature);
...
theme: isTheme(theme) ? theme : ThemeName.white,
unit: isTemp(unit) ? unit : Temperature.CELSIUS,
api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)

19-35: Consider making the ID format consistent across services.

The generateCpu() method returns id: 'info/cpu' while generateCpuLoad() returns id: 'info/cpu-load'. Consider standardizing the ID format for consistency - either use hyphens or forward slashes consistently.

Apply this diff to align ID format:

     async generateCpuLoad(): Promise<CpuUtilization> {
         const { currentLoad: load, cpus } = await currentLoad();
 
         return {
-            id: 'info/cpu-load',
+            id: 'info/cpu/load',
             load,
             cpus,
         };
     }
api/src/unraid-api/graph/resolvers/info/devices/devices.service.ts (1)

87-99: Consider implementing network device generation or documenting the placeholder.

The generateNetwork() method returns an empty array with a comment about future implementation. Consider either implementing basic network interface detection or adding a TODO comment to track this.

Would you like me to implement basic network interface detection using the systeminformation library's networkInterfaces() function, or would you prefer to create a tracking issue for this?

api/src/unraid-api/graph/services/subscription-tracker.service.ts (4)

37-40: Prefer nullish-coalescing-safe default for onStop (avoid logical OR on functions).

Use an explicit typeof check to avoid accidental fallback and for clarity.

-            this.topicHandlers.set(topic, {
-                onStart: callbackOrOnStart,
-                onStop: intervalMsOrOnStop || (() => {}),
-            });
+            this.topicHandlers.set(topic, {
+                onStart: callbackOrOnStart,
+                onStop: typeof intervalMsOrOnStop === 'function' ? intervalMsOrOnStop : () => {},
+            });

51-56: Guard onStart with try/catch to prevent unhandled exceptions from breaking subscription flows.

Starting polling can throw (e.g., scheduler issues). Log and continue.

-            const handlers = this.topicHandlers.get(topic);
-            if (handlers?.onStart) {
-                handlers.onStart();
-            }
+            const handlers = this.topicHandlers.get(topic);
+            if (handlers?.onStart) {
+                try {
+                    handlers.onStart();
+                } catch (err) {
+                    this.logger.error(
+                        `Error in onStart handler for topic '${topic}': ${
+                            err instanceof Error ? err.message : String(err)
+                        }`
+                    );
+                }
+            }

100-104: Guard onStop with try/catch to ensure cleanup doesn’t crash the caller.

Stopping polling should never throw back to subscribers. Log and proceed.

-            const handlers = this.topicHandlers.get(topic);
-            if (handlers?.onStop) {
-                handlers.onStop();
-            }
+            const handlers = this.topicHandlers.get(topic);
+            if (handlers?.onStop) {
+                try {
+                    handlers.onStop();
+                } catch (err) {
+                    this.logger.error(
+                        `Error in onStop handler for topic '${topic}': ${
+                            err instanceof Error ? err.message : String(err)
+                        }`
+                    );
+                }
+            }

19-42: Tighten topic typing to PUBSUB_CHANNEL for end-to-end type safety

Currently, SubscriptionTrackerService uses string for its topic keys and method signatures, which leaves us open to typos and gaps in compile-time coverage. Since PUBSUB_CHANNEL is already the source-of-truth in SubscriptionHelperService and our resolvers, update this service to use PUBSUB_CHANNEL instead.

Required refactors:

  • Import the enum at the top of api/src/unraid-api/graph/services/subscription-tracker.service.ts:
    import type { PUBSUB_CHANNEL } from '@app/core/pubsub.js';
  • Change your maps:
    private subscriberCounts = new Map<PUBSUB_CHANNEL, number>();
    private topicHandlers = new Map<PUBSUB_CHANNEL, { onStart: () => void; onStop: () => void }>();
  • Update all method signatures:
    registerTopic(topic: PUBSUB_CHANNEL, …)
    subscribe(topic: PUBSUB_CHANNEL)
    unsubscribe(topic: PUBSUB_CHANNEL)
    getSubscriberCount(topic: PUBSUB_CHANNEL)
  • Adjust any internal calls (e.g. this.topicHandlers.set(topic, …)) to compile against the new enum-typed keys.

This alignment will ensure end-to-end type safety across tracker, helper service, and resolvers.

api/src/unraid-api/graph/services/subscription-polling.service.ts (3)

41-45: Log error message and stack clearly for easier debugging.

Current Logger.error usage passes the whole error object. Prefer message + stack.

-            } catch (error) {
-                this.logger.error(`Error in polling task '${name}'`, error);
+            } catch (error) {
+                this.logger.error(
+                    `Error in polling task '${name}': ${
+                        error instanceof Error ? error.message : String(error)
+                    }`,
+                    error instanceof Error ? error.stack : undefined
+                );

52-57: Prime the poller immediately so first subscribers get data without waiting a full interval.

Kick off one run right after registration.

         // Create and register the interval
         const interval = setInterval(pollFunction, intervalMs);
         this.schedulerRegistry.addInterval(name, interval);

+        // Prime once immediately
+        void pollFunction();
+
         this.logger.debug(`Started polling for '${name}' every ${intervalMs}ms`);

24-31: Validate interval input to avoid misconfiguration (non-positive/NaN).

Defensive guard prevents scheduling nonsense intervals.

     startPolling(config: PollingConfig): void {
         const { name, intervalMs, callback } = config;

+        if (!Number.isFinite(intervalMs) || intervalMs <= 0) {
+            this.logger.warn(`Skipping polling for '${name}': invalid interval ${intervalMs}`);
+            return;
+        }
+
         // Clean up any existing interval
         this.stopPolling(name);
api/src/unraid-api/graph/resolvers/info/info.resolver.ts (1)

1-1: Add ID to imports for resolver decorator above.

-import { GraphQLISODateTime, Query, ResolveField, Resolver } from '@nestjs/graphql';
+import { GraphQLISODateTime, ID, Query, ResolveField, Resolver } from '@nestjs/graphql';

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
api/src/unraid-api/graph/resolvers/info/display/display.service.ts (1)

132-165: NaN integers can leak to GraphQL (will throw at runtime)

Number.parseInt(...) can return NaN (e.g., undefined, empty string). Using ?? later won’t catch NaN, so fields like warning, critical, hot, and max can be NaN, which is invalid for GraphQL Int and will cause runtime errors.

Return undefined on invalid parses so the defaults in generateDisplay apply cleanly.

Apply this diff to sanitize numeric and enum parsing:

-        const { theme, unit, ...display } = state.display;
-        return {
-            ...display,
-            theme: theme as ThemeName,
-            unit: unit as Temperature,
-            scale: toBoolean(display.scale),
-            tabs: toBoolean(display.tabs),
-            resize: toBoolean(display.resize),
-            wwn: toBoolean(display.wwn),
-            total: toBoolean(display.total),
-            usage: toBoolean(display.usage),
-            text: toBoolean(display.text),
-            warning: Number.parseInt(display.warning, 10),
-            critical: Number.parseInt(display.critical, 10),
-            hot: Number.parseInt(display.hot, 10),
-            max: Number.parseInt(display.max, 10),
-            locale: display.locale || 'en_US',
-        };
+        const { theme, unit, ...display } = state.display;
+        const parseIntOrUndef = (v: unknown): number | undefined => {
+            const n = Number.parseInt(String(v ?? ''), 10);
+            return Number.isNaN(n) ? undefined : n;
+        };
+        const validTheme = Object.values(ThemeName).includes(theme as ThemeName)
+            ? (theme as ThemeName)
+            : undefined;
+        const validUnit = Object.values(Temperature).includes(unit as Temperature)
+            ? (unit as Temperature)
+            : undefined;
+        return {
+            ...display,
+            theme: validTheme,
+            unit: validUnit,
+            scale: toBoolean(display.scale),
+            tabs: toBoolean(display.tabs),
+            resize: toBoolean(display.resize),
+            wwn: toBoolean(display.wwn),
+            total: toBoolean(display.total),
+            usage: toBoolean(display.usage),
+            text: toBoolean(display.text),
+            warning: parseIntOrUndef(display.warning),
+            critical: parseIntOrUndef(display.critical),
+            hot: parseIntOrUndef(display.hot),
+            max: parseIntOrUndef(display.max),
+            locale: display.locale || 'en_US',
+        };

Follow-up: with this change, the defaults in generateDisplay will kick in as intended:

  • warning: config.warning ?? 60
  • critical: config.critical ?? 80
  • hot: config.hot ?? 90
  • max: config.max (now undefined when invalid)
api/src/unraid-api/graph/resolvers/info/devices/devices.service.ts (2)

42-52: Fix inverted GPU 'blacklisted' mapping (and verify filter semantics)

blacklisted is currently set from entry.allowed, which inverts the intended meaning. If allowed means "not blacklisted", then blacklisted should be !entry.allowed. Also, double-check whether the !device.allowed filter is intended (return only blacklisted GPUs) or if you meant to return all GPUs and annotate their blacklist status.

Apply minimal fix to correct the mapping:

-                        blacklisted: entry.allowed,
+                        blacklisted: !entry.allowed,

Optionally, if you intend to return all GPUs (recommended), drop the !device.allowed predicate:

-                .filter((device) => device.class === 'vga' && !device.allowed)
+                .filter((device) => device.class === 'vga')

179-186: Prevent runtime error when parsing USB hubs (id may be undefined)

If match fails, id is undefined, and id.replace('p', ':') will throw. Add a guard and filter nulls.

-        const usbHubs = await execa('cat /sys/bus/usb/drivers/hub/*/modalias', { shell: true })
-            .then(({ stdout }) =>
-                stdout.split('\n').map((line) => {
-                    const [, id] = line.match(/usb:v(\w{9})/) ?? [];
-                    return id.replace('p', ':');
-                })
-            )
+        const usbHubs = await execa('cat /sys/bus/usb/drivers/hub/*/modalias', { shell: true })
+            .then(({ stdout }) =>
+                stdout
+                    .split('\n')
+                    .map((line) => {
+                        const [, id] = line.match(/usb:v(\w{9})/) ?? [];
+                        return id ? id.replace('p', ':') : null;
+                    })
+                    .filter((v): v is string => v !== null)
+            )
             .catch(() => [] as string[]);
api/generated-schema.graphql (1)

2310-2326: Add top-level cpuUtilization query for convenience

The schema currently exposes Query.metrics.cpu, but not a direct Query.cpuUtilization as stated in the PR objective. To satisfy client expectations (and keep the change backward-compatible), add this field:

type Query {
  …
  docker: Docker!
  disks: [Disk!]!
  disk(id: PrefixedID!): Disk!
  rclone: RCloneBackupSettings!
  info: Info!
+  """One-time snapshot of CPU utilization"""
+  cpuUtilization: CpuUtilization
  settings: Settings!
  …
  metrics: Metrics!
  …
}

– Confirmed type Metrics already defines

"""Current CPU utilization metrics"""
cpu: CpuUtilization

– No existing queries, docs or tests reference Query.cpuUtilization; safe to add.

🧹 Nitpick comments (35)
api/src/core/pubsub.ts (1)

18-22: Optional: strengthen type-safety by mapping channels to payloads

If GRAPHQL_PUBSUB_CHANNEL has stable, known payloads per channel, consider overloads or a mapped type (Record<Channel, Payload>) so createSubscription infers T from channel at call sites. This prevents accidental mismatches between channel and payload.

api/src/unraid-api/graph/resolvers/info/display/display.model.ts (1)

80-82: Note: TS aliases don’t create GraphQL schema aliases

Re-exporting InfoDisplay as Display is a TypeScript alias only; it won’t add a “Display” GraphQL type. Ensure all GraphQL clients (schema, codegen, queries) are aligned to InfoDisplay/InfoDisplayCase names.

api/src/unraid-api/graph/resolvers/info/display/display.service.ts (1)

124-129: Minor duplication: id is already provided by states.default

You spread states.default (which includes id: 'display/case') and then set id again. Safe but redundant.

-        return {
-            ...states.default,
-            id: 'display/case',
-            icon: serverCase,
-        };
+        return {
+            ...states.default,
+            icon: serverCase,
+        };
api/src/unraid-api/graph/resolvers/info/memory/memory.model.ts (1)

6-40: MemoryLayout: consider bigint typing for GraphQLBigInt fields

size is exposed as GraphQLBigInt but typed as number. If you anticipate very large values, consider using bigint (or string per your scalar config) to avoid precision loss.

api/src/unraid-api/graph/resolvers/display/display.resolver.ts (1)

34-36: Remove unnecessary async from subscription method.

The method returns the subscription immediately without awaiting; returning a Promise is unnecessary. Prefer returning the AsyncIterable directly.

-    public async displaySubscription() {
+    public displaySubscription() {
         return createSubscription(PUBSUB_CHANNEL.DISPLAY);
     }
api/src/unraid-api/graph/resolvers/info/devices/devices.resolver.spec.ts (1)

70-71: Potential type mismatch for blacklisted field in test data.

'blacklisted' appears as a string ('false') instead of a boolean. If the GraphQL type expects a boolean, this can mask type issues in tests.

If the schema defines this field as boolean, apply:

-                    blacklisted: 'false',
+                    blacklisted: false,
api/src/unraid-api/graph/resolvers/metrics/metrics.module.ts (1)

9-11: Avoid duplicating CpuService/CpuDataService/MemoryService providers across modules.

These services are also provided in InfoModule. Registering the same class token in multiple modules produces separate singleton instances, which can fragment caches (especially if CpuDataService is request-scoped in one place and used cross-module). Centralize their provision in a single module and export them for reuse.

Option A (export from InfoModule, consume here):

- @Module({
-     imports: [ServicesModule],
-     providers: [MetricsResolver, CpuService, CpuDataService, MemoryService],
-     exports: [MetricsResolver],
- })
+ @Module({
+     imports: [ServicesModule, 
+       // Reuse CPU/Memory services from InfoModule
+       // so the same provider instances are injected app-wide
+       ] // add InfoModule import after applying the InfoModule diff below
+     providers: [MetricsResolver],
+     exports: [MetricsResolver],
+ })

And in MetricsModule header add:

+import { InfoModule } from '@app/unraid-api/graph/resolvers/info/info.module.js';

Option B (move CPU/Memory services to ServicesModule) is also viable if you prefer to keep InfoModule light; export them there and import only ServicesModule here.

I can generate full diffs for both modules once you pick an option.

api/src/unraid-api/graph/resolvers/info/info.module.ts (1)

32-33: Export CPU/Memory services to prevent duplicate registrations in other modules.

Since MetricsModule also registers CpuService, CpuDataService, and MemoryService, prefer exporting them from one place (e.g., InfoModule) and reusing them to avoid multiple singleton instances and duplicated caches.

Apply this diff to InfoModule:

 @Module({
     imports: [ConfigModule, ServicesModule],
     providers: [
       // ...omitted for brevity...
       DisplayService,
     ],
-    exports: [InfoResolver, DevicesResolver, DisplayService],
+    exports: [
+      InfoResolver,
+      DevicesResolver,
+      DisplayService,
+      CpuService,
+      CpuDataService,
+      MemoryService,
+    ],
 })
 export class InfoModule {}

Then update MetricsModule to import InfoModule and remove local CPU/Memory providers (see the paired suggestion in metrics.module.ts).

api/src/unraid-api/graph/resolvers/info/os/os.service.ts (1)

17-17: Avoid potential runtime crash when store is not initialized; add safe fallback for hostname.

If getters.emhttp() or its nested fields are undefined during early startup or tests, this will throw. Use optional chaining and fall back to os.hostname.

-            hostname: getters.emhttp().var.name,
+            hostname: getters.emhttp?.()?.var?.name ?? os.hostname,
api/src/unraid-api/graph/resolvers/info/display/display.service.spec.ts (2)

20-29: Restore spies/mocks after each test to prevent cross-test pollution.

vi.clearAllMocks() resets call history but doesn’t restore overridden implementations. Add vi.restoreAllMocks() in an afterEach.

-import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
@@
     beforeEach(async () => {
         // Reset all mocks
         vi.clearAllMocks();
@@
         service = module.get<DisplayService>(DisplayService);
     });
+
+    afterEach(() => {
+        vi.restoreAllMocks();
+    });

99-119: Tests rely on “dev files” content; consider using controlled fixtures to reduce brittleness.

Asserting exact values like theme/unit from dev configs couples tests to non-test assets. Prefer dedicated fixtures or focused behavior assertions so changes to dev files don’t break tests.

api/src/unraid-api/graph/resolvers/info/versions/versions.service.ts (1)

13-13: Prefer null/undefined over sentinel strings for unknown values.

Returning 'unknown' conflates “missing” with a concrete value, and makes filtering harder. Use null/undefined to align with GraphQL Maybe semantics.

-        const unraid = this.configService.get<string>('store.emhttp.var.version') || 'unknown';
+        const unraid = this.configService.get<string>('store.emhttp.var.version') ?? undefined;
api/src/unraid-api/graph/resolvers/info/system/system.model.ts (2)

46-50: Make nullability handling consistent for memMax (avoid redundant | null)

You already mark the field as nullable: true in the GraphQL decorator, so ?: plus | null is redundant and inconsistent with the other fields. Prefer a single convention (optional or | null, not both).

-    @Field(() => Number, { nullable: true, description: 'Maximum memory capacity in bytes' })
-    memMax?: number | null;
+    @Field(() => Number, { nullable: true, description: 'Maximum memory capacity in bytes' })
+    memMax?: number;

49-50: Double-check scalar choice for memSlots (Int vs Float)

A slot count is an integer conceptually. If the schema allows it, consider using the Int scalar for better semantic accuracy.

If acceptable, this change aligns the TypeScript type and GraphQL scalar:

-import { Field, ObjectType } from '@nestjs/graphql';
+import { Field, ObjectType, Int } from '@nestjs/graphql';
@@
-    @Field(() => Number, { nullable: true, description: 'Number of memory slots' })
-    memSlots?: number;
+    @Field(() => Int, { nullable: true, description: 'Number of memory slots' })
+    memSlots?: number;

Note: The generated CLI types you shared currently expose Float for memSlots. If that contract is relied upon, keep as-is to avoid breaking clients.

api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)

39-39: Consider removing direct ServicesModule import (if already provided transitively)

If both InfoModule and MetricsModule already import ServicesModule (they do), you may not need to import it here as well. Not harmful, but slightly redundant.

api/src/unraid-api/graph/services/subscription-helper.service.ts (1)

25-27: Optionally auto-unsubscribe when iteration ends (done: true) from next()

Most GraphQL subscriptions don’t complete naturally, but guarding this path makes the helper more robust and prevents leaks if a producer completes.

-        const proxyIterator: AsyncIterableIterator<T> = {
-            next: () => innerIterator.next(),
+        const proxyIterator: AsyncIterableIterator<T> = {
+            next: async () => {
+                const res = await innerIterator.next();
+                if (res?.done) {
+                    // Ensure tracker is cleaned up if iterator completes naturally
+                    this.subscriptionTracker.unsubscribe(topic);
+                }
+                return res as IteratorResult<T>;
+            },
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.spec.ts (4)

123-127: Follow testing guideline: avoid asserting exact error message text

Use .rejects.toThrow() without message arguments to keep tests less brittle.

-            await expect(resolver.cpu()).rejects.toThrow('CPU error');
+            await expect(resolver.cpu()).rejects.toThrow();

151-155: Follow testing guideline: avoid asserting exact error message text

Same rationale for memory error path.

-            await expect(resolver.memory()).rejects.toThrow('Memory error');
+            await expect(resolver.memory()).rejects.toThrow();

71-83: Minimize unnecessary mocks in unit tests

SubscriptionTrackerService and SubscriptionHelperService aren’t exercised in this suite. Removing them simplifies the test bed and matches the guideline to avoid over-mocking.


124-124: Type-safety of vi.mocked(...) on service methods

Depending on tsconfig, vi.mocked(cpuService.generateCpuLoad) and vi.mocked(memoryService.generateMemoryLoad) may need casting. If you see TS friction, prefer casting to vi.Mock:

Example:

( cpuService.generateCpuLoad as unknown as vi.Mock ).mockRejectedValueOnce(new Error('CPU error'));

If your setup already infers correct mocked types, ignore this.

Also applies to: 152-152

api/src/unraid-api/graph/services/subscription-polling.service.ts (1)

42-45: Improve Logger.error payload to capture stack traces

Nest’s Logger.error expects a message and optionally a stack trace. Passing the raw error object can omit useful details.

-            } catch (error) {
-                this.logger.error(`Error in polling task '${name}'`, error);
+            } catch (error) {
+                const err = error as Error;
+                this.logger.error(`Error in polling task '${name}': ${err.message}`, err?.stack);
api/src/unraid-api/graph/resolvers/info/memory/memory.service.ts (1)

35-49: Clamp and normalize usedPercent; consider consistent precision

The usedPercent calculation can exceed bounds in edge cases or produce overly precise floats. Clamp to [0, 100] and optionally normalize precision to reduce noisy diffs/outputs.

-            usedPercent:
-                memInfo.total > 0 ? ((memInfo.total - memInfo.available) / memInfo.total) * 100 : 0,
+            usedPercent:
+                memInfo.total > 0
+                    ? Math.min(
+                          100,
+                          Math.max(
+                              0,
+                              Number((((memInfo.total - memInfo.available) / memInfo.total) * 100).toFixed(2))
+                          )
+                      )
+                    : 0,

Additionally, verify that the GraphQL BigInt scalar accepts JS numbers for total/used/free on your target platforms. If your scalar expects bigint/string, we should convert accordingly (e.g., BigInt(memInfo.total)).

api/src/unraid-api/graph/resolvers/info/cpu/cpu.service.ts (1)

7-15: Import Systeminformation as a type-only import

Systeminformation’s “Systeminformation” is a type. Mark it as a type-only import to avoid any chance of runtime named import emission under ESM.

-import { cpu, cpuFlags, currentLoad, Systeminformation } from 'systeminformation';
+import { cpu, cpuFlags, currentLoad } from 'systeminformation';
+import type { Systeminformation } from 'systeminformation';
api/src/unraid-api/graph/resolvers/metrics/metrics.resolver.ts (1)

29-49: Wrap polling callbacks with try/catch to avoid unhandled rejections

If generateCpuLoad()/generateMemoryLoad() throws, the unhandled rejection could bubble out of the poller. Log and swallow to keep the polling loop healthy (and to match the previous behavior your tests expected).

         this.subscriptionTracker.registerTopic(
             PUBSUB_CHANNEL.CPU_UTILIZATION,
-            async () => {
-                const payload = await this.cpuService.generateCpuLoad();
-                pubsub.publish(PUBSUB_CHANNEL.CPU_UTILIZATION, { systemMetricsCpu: payload });
-            },
+            async () => {
+                try {
+                    const payload = await this.cpuService.generateCpuLoad();
+                    await pubsub.publish(PUBSUB_CHANNEL.CPU_UTILIZATION, { systemMetricsCpu: payload });
+                } catch (err) {
+                    // Keep polling resilient
+                    console.error('Error polling CPU utilization:', err);
+                }
+            },
             1000
         );
 
         // Register memory polling with 2 second interval
         this.subscriptionTracker.registerTopic(
             PUBSUB_CHANNEL.MEMORY_UTILIZATION,
-            async () => {
-                const payload = await this.memoryService.generateMemoryLoad();
-                pubsub.publish(PUBSUB_CHANNEL.MEMORY_UTILIZATION, { systemMetricsMemory: payload });
-            },
+            async () => {
+                try {
+                    const payload = await this.memoryService.generateMemoryLoad();
+                    await pubsub.publish(PUBSUB_CHANNEL.MEMORY_UTILIZATION, { systemMetricsMemory: payload });
+                } catch (err) {
+                    console.error('Error polling memory utilization:', err);
+                }
+            },
             2000
         );
api/src/unraid-api/graph/resolvers/info/devices/devices.service.ts (3)

67-77: Guard vendorid derivation to avoid crashes on malformed typeid

Deriving vendorid via substring(0, 4) will throw if typeid is undefined/null. While unlikely, a defensive guard here avoids hard-to-diagnose crashes.

-                vendorid: device.typeid.substring(0, 4), // Extract vendor ID from type ID
+                vendorid: device.typeid?.slice(0, 4) ?? '', // Extract vendor ID from type ID

55-61: Reduce CI noise: downgrade expected environment failures to warnings

In CI (and non-Unraid/dev machines), PCI/USB discovery often fails and is expected. Logging these as errors clutters CI output. Consider logging at warn level for these code paths.

-            this.logger.error(
+            this.logger.warn(
                 `Failed to generate GPU devices: ${error instanceof Error ? error.message : String(error)}`,
                 error instanceof Error ? error.stack : undefined
             );
-            this.logger.error(
+            this.logger.warn(
                 `Failed to generate PCI devices: ${error instanceof Error ? error.message : String(error)}`,
                 error instanceof Error ? error.stack : undefined
             );

Also applies to: 79-83


87-99: Network stub is fine as a placeholder

Returning an empty array with proper error handling is acceptable until implementation lands. Add a TODO with the intended source (e.g., systeminformation) to make next steps clear.

api/src/unraid-api/graph/services/subscription-tracker.service.ts (1)

9-12: Broaden handler types and prefer nullish coalescing

Allow async lifecycle handlers in type signatures, and prefer ?? over || for the onStop fallback.

-    private topicHandlers = new Map<string, { onStart: () => void; onStop: () => void }>();
+    private topicHandlers = new Map<string, { onStart: () => void | Promise<void>; onStop: () => void | Promise<void> }>();
@@
-                onStop: intervalMsOrOnStop || (() => {}),
+                onStop: intervalMsOrOnStop ?? (() => {}),

Also applies to: 37-40

api/src/unraid-api/graph/resolvers/info/info.resolver.ts (2)

53-57: Optional: Avoid casting and explicitly map baseboard fields from systeminformation

Casting the entire result can hide shape drift. Explicitly pick fields we expose to the client to make the mapping robust and self-documenting.

Apply this diff:

-    public async baseboard(): Promise<InfoBaseboard> {
-        const baseboard = await getBaseboard();
-        return { id: 'info/baseboard', ...baseboard } as InfoBaseboard;
-    }
+    public async baseboard(): Promise<InfoBaseboard> {
+        const {
+            manufacturer,
+            model,
+            version,
+            serial,
+            assetTag,
+            memMax,
+            memSlots,
+        } = await getBaseboard();
+        return {
+            id: 'info/baseboard',
+            manufacturer,
+            model,
+            version,
+            serial,
+            assetTag,
+            memMax,
+            memSlots,
+        };
+    }

90-94: Optional: Same explicit field-pick for system() mapping

Mirror the baseboard suggestion for system information to avoid broad casts.

Apply this diff:

-    public async system(): Promise<InfoSystem> {
-        const system = await getSystem();
-        return { id: 'info/system', ...system } as InfoSystem;
-    }
+    public async system(): Promise<InfoSystem> {
+        const { manufacturer, model, serial, sku, uuid, version, virtual } = await getSystem();
+        return {
+            id: 'info/system',
+            manufacturer,
+            model,
+            serial,
+            sku,
+            uuid,
+            version,
+            virtual,
+        };
+    }
api/generated-schema.graphql (5)

943-959: Doc inconsistency: Port scalar says “TCP” but used for UDP too

ContainerPortType includes UDP while Port’s doc references TCP only. Suggest generalizing the description to “valid port (0–65535)”.

Apply this diff to update the description:

-"""
-A field whose value is a valid TCP port within the range of 0 to 65535: https://en.wikipedia.org/wiki/Transmission_Control_Protocol#TCP_ports
-"""
+"""
+A field whose value is a valid port number within the range 0 to 65535.
+"""
 scalar Port

1239-1270: Consider clarifying CPU load semantics and add a timestamp

Docs are clear on per-field percentages, but clients often need:

  • Whether “load” ≈ 100 - loadIdle (expected), and whether values are 0–100 bounded.
  • A timestamp to correlate/dedupe samples.

Possible schema enhancement:

 type CpuLoad {
   """The total CPU load on a single core, in percent."""
   load: Float!
+  """ISO timestamp when this sample was taken"""
+  timestamp: DateTime
   ...
 }

Optionally document that load + loadIdle ~ 100 and values are percentages in [0, 100].


1272-1325: PR objective mentions InfoCpu.utilization, but the field isn’t present

The PR description says a utilization field is added to InfoCpu, but InfoCpu here only holds static CPU info. If the intent is to expose utilization under Info.cpu.utilization for convenience, add the field (even if Query.metrics.cpu exists).

Apply this diff to add the field:

 type InfoCpu implements Node {
   id: PrefixedID!
   ...
   """CPU feature flags"""
   flags: [String!]
+  
+  """Realtime CPU utilization snapshot for this CPU"""
+  utilization: CpuUtilization
 }

If intentionally omitted (preferring Query.metrics), consider updating the PR description and any docs/tests expecting InfoCpu.utilization.


1660-1669: Nullability of Metrics fields

Metrics implements Node and returns cpu/memory as nullable. If these are always resolvable on supported platforms, prefer non-null to tighten the contract. If they can legitimately be unavailable, keep as nullable but document conditions (e.g., unsupported platform/permission issues).


2565-2576: Subscription naming consistency and alias for PR objective

Subscriptions expose systemMetricsCpu/systemMetricsMemory, while Query uses metrics. For symmetry or to match the PR’s “cpuUtilizationSubscription” goal, consider exposing additional alias fields (keep existing for compatibility):

 type Subscription {
   ...
-  systemMetricsCpu: CpuUtilization!
-  systemMetricsMemory: MemoryUtilization!
+  systemMetricsCpu: CpuUtilization!
+  systemMetricsMemory: MemoryUtilization!
+  """Realtime CPU utilization updates"""
+  cpuUtilization: CpuUtilization!
+  """Realtime memory utilization updates"""
+  memoryUtilization: MemoryUtilization!
 }

Optionally deprecate the “systemMetrics*” names later, but keep them now to avoid breaking clients.

- Added `ScheduleModule` to the main application module for managing scheduled tasks.
- Removed redundant `ScheduleModule` imports from `CronModule` and `ServicesModule` to streamline module dependencies.
- Added a null check for the `module` before calling `close()` in the `afterEach` hook to prevent potential errors during test teardown.
- Renamed CPU and memory metric fields for consistency, changing `load` to `percentTotal`, `loadUser` to `percentUser`, and similar adjustments for other fields.
- Updated integration tests to reflect the new naming conventions, ensuring accurate property checks for CPU and memory utilization.
- Enhanced the `CpuService` and `MemoryService` to return the updated metric names, improving clarity in the API response.
- Renamed CPU and memory metric fields for consistency, changing `load` to `percentTotal`, `loadUser` to `percentUser`, and similar adjustments for other fields across the GraphQL schema and resolvers.
- Updated integration tests to reflect the new naming conventions, ensuring accurate property checks for CPU and memory utilization.
- Enhanced the `CpuService` and `MemoryService` to return the updated metric names, improving clarity in the API response.
- Renamed `percentSwapUsed` to `percentSwapTotal` and `percentUsed` to `percentTotal` in the GraphQL schema to align with recent naming conventions.
- Adjusted export statements in the index file for consistency in quotation style.
- Introduced `isValidEnumValue` and `validateEnumValue` functions to validate enum values, enhancing type safety in the application.
- Updated `DisplayService` to utilize `validateEnumValue` for theme and unit properties, ensuring only valid enum values are assigned.
- Eliminated the `CpuDataService` from the `info.module.ts`, `metrics.module.ts`, and related integration tests, streamlining the CPU service implementation.
- Updated imports and provider lists to reflect the removal, ensuring the application remains functional without the redundant service.
@github-actions
Copy link
Contributor

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL:

https://preview.dl.unraid.net/unraid-api/tag/PR1590/dynamix.unraid.net.plg

@elibosley elibosley merged commit 2b4c2a2 into main Aug 19, 2025
12 of 15 checks passed
@elibosley elibosley deleted the feat/cpu-utilization-query branch August 19, 2025 18:44
@elibosley elibosley mentioned this pull request Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants