feat: basic vm controls #1293

elibosley · 2025-03-28T20:35:53Z

Summary by CodeRabbit

New Features
- Introduced new GraphQL operations for comprehensive virtual machine control (start, stop, pause, resume, force stop, reboot, reset).
- Enhanced API authentication and authorization with standardized roles and permission checks.
- Added a configuration template that streamlines server setup and improves remote access and parity management.
- New functionality for managing parity checks within the array service, including state validation and conditional command execution.
- New types and mutations for array and virtual machine management in the GraphQL schema.
- Added a new directive for authorization control within the GraphQL schema.
- Introduced a new utility for generating authentication enum type definitions.
- Added a new configuration file template for server access and authentication details.
- Updated the configuration file version to reflect the latest changes.
Improvements
- Upgraded core dependencies for better stability and performance.
- Refined notification handling and error feedback for a more responsive user experience.
- Improved error handling and logging for API key management and validation processes.
- Updated configuration versioning for enhanced compatibility.

coderabbitai · 2025-03-28T20:36:00Z

Walkthrough

This pull request implements extensive modifications across the codebase. It updates the GitHub Actions workflow to configure the libvirt environment, refines GraphQL schema generation by adding new types, directives, and mutations (especially for virtual machine and array management), and upgrades dependency versions. The changes also restructure and deprecate legacy VM and array functions while introducing new services and resolvers for VM operations. Additionally, authentication and authorization components have been enhanced with better error handling and updated guard classes, and several configuration files, test setups, and unraid file modifier templates have been updated accordingly.

Changes

File(s)	Change Summary
`.github/workflows/main.yml`	Added a new "Setup libvirt" step with caching for `libvirt-daemon-system`, group/user creation, directory setup, and service verification; minor formatting adjustments.
`api/codegen.ts` `api/src/unraid-api/graph/utils/auth-enum.utils.ts`	Introduced new import and usage of `getAuthEnumTypeDefs()` in schema generation to include authentication enum definitions.
`api/dev/Unraid.net/myservers.cfg` `api/dev/Unraid.net/myservers.example.cfg`	Updated API version from 4.4.1 to 4.6.6; added a new example config template with `[api]`, `[local]`, and `[remote]` sections.
`api/dev/keys/*.json`	Changed role value from `"connect"` to `"CONNECT"` to enforce uniform casing.
`api/generated-schema.graphql`	Added a comprehensive GraphQL schema with new types, inputs, enums, mutations (e.g. VmMutations), and an `@auth` directive for fine-grained access control.
`api/package.json`	Upgraded `@unraid/libvirt` dependency from `^1.1.3` to `^2.1.0`.
`api/src/__test__/graphql/resolvers/subscription/network.test.ts` `api/src/__test__/store/modules/paths.test.ts`	Updated inline snapshots for URL lists and error messages; added new key `"libvirt-pid"` in store paths.
Core modules in `api/src/core/modules/vms/` `api/src/core/utils/vms/` `api/src/core/modules/array/*`	Removed exports and functions for VM domain retrieval and array management (e.g. `getDomains`, `parseDomain`, `updateArray`, `add-disk-to-array`, `remove-disk-from-array`, `update-parity-check`).
`api/src/unraid-api/auth/*` (including auth.service, auth.interceptor, authentication.guard, casbin model & policy, auth-related tests)	Replaced `GraphqlAuthGuard` with `AuthenticationGuard`; updated logger references; enhanced API key error logging through a new helper method; refined Casbin matcher and ADMIN policy; adjusted auth tests to use constants (e.g. `AuthActionVerb.READ`).
GraphQL schema files in `api/src/graphql/schema/types/*`	Lowercased enum values updated to uppercase; added new `@auth` directive in base types; introduced new VmMutations type with multiple VM operations (start, stop, pause, etc.).
GraphQL resolvers in `api/src/unraid-api/graph/**`	Added new `VmMutationsResolver` and `VmsService` classes to handle VM operations; restructured `ResolversModule` providers; updated tests for these resolvers and services.
`web/codegen.ts` `web/composables/gql/graphql.ts`	Modified schema source from a remote URL to a local file; added new types and mutations for array, Docker, and VM operations.
Test and setup files: `api/src/__test__/setup/config-setup.ts` `api/src/unraid-api/graph/directives/auth.directive.spec.ts` `api/vite.config.ts`	Added a config setup script to copy example config to production file; introduced tests for auth directive transformations; included the setup file in Vite’s test configuration.
Unraid file modifier changes (`api/src/unraid-api/unraid-file-modifier/modifications/**`)	Updated last-download-time numeric values; enhanced DefaultPageLayout by adding a monitoring method (`NchanSubscriber.prototype.monitor`) for live update pausing/resuming and replaced legacy notification system with a `<uui-toaster>` element.

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant GR as VmMutationsResolver
    participant VS as VmsService
    participant H as Hypervisor (Libvirt API)

    C->>GR: Request startVm(id)
    GR->>VS: Call startVm(id)
    VS->>H: Execute start command via libvirt
    H-->>VS: Return success
    VS-->>GR: Respond with boolean result
    GR-->>C: Return operation result

sequenceDiagram
    participant Req as Request
    participant AI as AuthInterceptor
    participant App as Application

    Req->>AI: Intercept request
    AI->>Req: Check for user authentication
    alt Authenticated
       AI->>App: Pass request for processing
    else Not Authenticated
       AI-->>Req: Throw "Unauthorized" error
    end

Possibly related PRs

feat: generate key one time #992: The changes in the main PR, which involve updates to the myservers.cfg configuration file, are related to the retrieved PR as both involve modifications to the same configuration file, specifically the myservers.cfg, although they address different aspects of the configuration.

Poem

In circuits deep, our code takes flight,
Libvirt rises in the glow of CI light.
GraphQL dreams with mutations anew,
Auth guards keep our requests ever true.
Code flows like verses in a digital night!
🚀💻🌙

🪧 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.
- Generate unit testing code for this file.
- 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. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
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 src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai plan to trigger planning for file edits and PR creation.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

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

Documentation and Community

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.

coderabbitai

Actionable comments posted: 8

🧹 Nitpick comments (14)

api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (1)
1-74: Consider adding tests for additional VM operations

The VmMutationsResolver contains several other methods (pauseVm, resumeVm, forceStopVm, rebootVm, resetVm) that should also be tested for complete coverage.

You could add similar test blocks for each operation. For example:
describe('pauseVm', () => {
    it('should call service.pauseVm with the provided id', async () => {
        const vmId = 'test-vm-id';
        vi.mocked(vmsService.pauseVm).mockResolvedValue(true);

        const result = await resolver.pauseVm(vmId);

        expect(result).toBe(true);
        expect(vmsService.pauseVm).toHaveBeenCalledWith(vmId);
    });

    it('should propagate errors from the service', async () => {
        const vmId = 'test-vm-id';
        const error = new Error('Failed to pause VM');
        vi.mocked(vmsService.pauseVm).mockRejectedValue(error);

        await expect(resolver.pauseVm(vmId)).rejects.toThrow('Failed to pause VM');
    });
});
Don't forget to update the mocked service object in the beforeEach setup:
{
    provide: VmsService,
    useValue: {
        startVm: vi.fn(),
        stopVm: vi.fn(),
        pauseVm: vi.fn(),
        resumeVm: vi.fn(),
        forceStopVm: vi.fn(),
        rebootVm: vi.fn(),
        resetVm: vi.fn(),
    },
},
api/src/unraid-api/graph/directives/auth.directive.ts (3)

7-61: Comprehensive resolver transformation logic.
The transformResolvers function correctly wraps resolver calls with permission checks. As a minor suggestion, consider removing or swapping console.log (lines 47-50) for a structured logger if you intend to keep logs in production.

63-109: Schema transformer handles directive metadata effectively.
Appending permission information to field descriptions and storing them in extensions is a neat approach. If you wish to enforce correctness, consider throwing an error instead of just a console.warn when required directive arguments (action, resource, possession) are missing.

149-168: Generic enum conversion helper is nicely implemented.
The createGraphQLEnumFromTSEnum approach neatly skips numeric keys and supports string-based enum values. If you foresee usage beyond authentication, you might integrate it more centrally for broader code reusability.

api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (1)

108-114: Subscriber registration logic.
Defining NchanSubscriber.prototype.monitor and the global subscribers array is straightforward. Just ensure that extending prototypes won’t conflict with any external libraries. Also, watch out for potential global namespace collisions with var subscribers = [].

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

23-35: Consider more comprehensive environment checks.

While this method checks for a PID file to confirm libvirt is running, there may be additional edge cases (e.g., containerized or ephemeral environments) where this file-based check might be insufficient. Consider adding more robust health checks or re-checking connectivity during startup.

97-108: Validate domain transitions for RUNNING state.

When the current domain state is VmState.SHUTOFF, the flow calls domain.create() to power on the domain, which is valid. However, ensure there's no race condition if the domain has just been created or is mid-transition. Logging is helpful, but an additional post-state verification could improve reliability in large-scale or high-load scenarios.

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

9-42: Add error handling wrappers for each VM operation.

All resolve fields directly return the result from the service, which already throws a GraphQLError when failures occur. This is acceptable, but adding an explicit try/catch in the resolver could provide more granular control (e.g., custom logging or metrics) before rethrowing. This is optional unless you need specialized logging or metrics in the resolver layer.

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

262-301: Optimize repeated polling for VM state changes.

These loops poll VM states with sleeps. While effective, it may be more robust to implement an event-driven or callback-based approach, if the hypervisor supports it. This eliminates guesswork around timing and reduces test flakiness. Alternatively, you can reduce the overall time spent in polling by halving the sleep intervals or introducing a backoff strategy.

api/src/graphql/schema/types/vms/domain.graphql (1)

11-26: Consider returning richer data from VM mutations.
Each mutation currently returns a Boolean!, which is sufficient if you only need success/failure. However, returning specific details such as an updated VM status or an error message can provide more clarity to the client.

web/composables/gql/graphql.ts (1)

236-247: Solid definition for ArrayMutations.
The fields addDiskToArray, clearArrayDiskStatistics, mountArrayDisk, etc., encapsulate array-related operations well. Consider adding doc comments above each field if you want them to appear in the schema documentation.

api/generated-schema.graphql (3)

937-1033: Notifications, OS, and Partition Details

The Notifications, Os, Owner, ParityCheck, and Partition types are detailed and should cover a wide range of system reporting needs. The Partition type, in particular, includes many fields; consider whether some of these could be grouped or abstracted for maintainability.

1381-1444: Unassigned Device, Uptime, and USB Types

The UnassignedDevice type is detailed and may benefit from refactoring if many fields share similarities with other device types. The Uptime and Usb types are concise and clear.

1467-1647: System Variables (Vars Type)

The Vars type is extremely comprehensive, covering many system and configuration parameters. While this provides a one-stop view for system state, consider whether some fields might be grouped into nested types to enhance maintainability and clarity.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 48c6ad7 and f3514b1.

⛔ Files ignored due to path filters (3)

api/src/graphql/generated/api/operations.ts is excluded by !**/generated/**
api/src/graphql/generated/api/types.ts is excluded by !**/generated/**
pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (46)

.github/workflows/main.yml (3 hunks)
api/codegen.ts (4 hunks)
api/dev/Unraid.net/myservers.cfg (1 hunks)
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json (1 hunks)
api/generated-schema.graphql (1 hunks)
api/package.json (1 hunks)
api/src/__test__/common/allowed-origins.test.ts (0 hunks)
api/src/__test__/graphql/resolvers/subscription/network.test.ts (1 hunks)
api/src/__test__/store/modules/paths.test.ts (1 hunks)
api/src/core/modules/index.ts (0 hunks)
api/src/core/modules/vms/get-domains.ts (0 hunks)
api/src/core/modules/vms/index.ts (0 hunks)
api/src/core/utils/vms/get-hypervisor.ts (0 hunks)
api/src/core/utils/vms/index.ts (0 hunks)
api/src/core/utils/vms/parse-domain.ts (0 hunks)
api/src/core/utils/vms/parse-domains.ts (0 hunks)
api/src/graphql/schema/types/api-key/roles.graphql (1 hunks)
api/src/graphql/schema/types/base.graphql (1 hunks)
api/src/graphql/schema/types/vms/domain.graphql (1 hunks)
api/src/store/modules/paths.ts (1 hunks)
api/src/unraid-api/app/app.module.ts (2 hunks)
api/src/unraid-api/auth/api-key.service.spec.ts (3 hunks)
api/src/unraid-api/auth/api-key.service.ts (5 hunks)
api/src/unraid-api/auth/auth.service.spec.ts (3 hunks)
api/src/unraid-api/auth/authentication.guard.ts (1 hunks)
api/src/unraid-api/auth/casbin/model.ts (1 hunks)
api/src/unraid-api/auth/casbin/policy.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.ts (1 hunks)
api/src/unraid-api/graph/graph.module.ts (3 hunks)
api/src/unraid-api/graph/resolvers/online/online.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/resolvers.module.ts (2 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (2 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (3 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (3 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (4 hunks)
web/codegen.ts (1 hunks)
web/composables/gql/graphql.ts (12 hunks)

💤 Files with no reviewable changes (8)

api/src/core/modules/index.ts
api/src/core/modules/vms/index.ts
api/src/test/common/allowed-origins.test.ts
api/src/core/utils/vms/index.ts
api/src/core/utils/vms/parse-domains.ts
api/src/core/utils/vms/get-hypervisor.ts
api/src/core/modules/vms/get-domains.ts
api/src/core/utils/vms/parse-domain.ts

🧰 Additional context used

🧠 Learnings (2)

api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (6)

Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:30-54
Timestamp: 2025-03-28T19:43:21.758Z
Learning: The removal of jGrowl notifications from DefaultPageLayout.php is intentional as notifications are now handled on a separate page as part of the architectural design to override existing Unraid pages.

Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:6-20
Timestamp: 2025-03-28T19:43:21.758Z
Learning: The default-page-layout.patch removes the old jGrowl notification system and is complemented by the unraid-toaster component implementation. The new system is added through the DefaultPageLayout modification which inserts the toaster component with proper position configuration based on user preferences.

Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:6-20
Timestamp: 2025-03-28T19:43:21.758Z
Learning: The default-page-layout.patch is used to remove the old jGrowl notification system from Unraid pages, as notifications are handled by a new system implemented on a different page.

Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:24-27
Timestamp: 2025-03-28T19:43:21.758Z
Learning: The Unraid UI uses a modern notification system with a custom `unraid-toaster` component replacing the legacy jGrowl notifications. The system is backed by a comprehensive GraphQL API with real-time subscription support for notification updates.

Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-03-28T19:43:21.758Z
Learning: The project uses patches to override existing Unraid pages rather than modifying them directly.

Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-03-28T19:43:21.758Z
Learning: Notifications are implemented on a separate page rather than in the DefaultPageLayout.php file.

api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (2)

Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-03-28T19:43:21.758Z
Learning: Notifications are implemented on a separate page rather than in the DefaultPageLayout.php file.

Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-03-28T19:43:21.758Z
Learning: The project uses patches to override existing Unraid pages rather than modifying them directly.

🧬 Code Definitions (7)

api/codegen.ts (1)

api/src/unraid-api/graph/directives/auth.directive.ts (1)

getAuthEnumTypeDefs (114-143)

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

api/src/unraid-api/graph/directives/auth.directive.ts (3)

getAuthEnumTypeDefs (114-143)

transformResolvers (7-61)

authSchemaTransformer (63-109)

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

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

Resolver (5-43)

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

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

domain (21-30)

api/src/unraid-api/graph/resolvers/vms/vms.service.ts (3)

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

domain (21-30)

web/composables/gql/graphql.ts (1)

VmDomain (1867-1874)

api/src/graphql/generated/api/types.ts (1)

VmDomain (1839-1846)

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

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

Module (6-10)

api/src/unraid-api/auth/auth.module.ts (1)

Module (16-71)

web/composables/gql/graphql.ts (1)

api/src/graphql/generated/api/types.ts (22)

ArrayDiskInput (194-199)

Scalars (14-26)

InputMaybe (6-6)

ArrayMutations (233-244)

Maybe (5-5)

ArrayType (112-131)

Disk (494-515)

ArrayMutationsaddDiskToArrayArgs (247-249)

ArrayMutationsclearArrayDiskStatisticsArgs (252-254)

ArrayMutationsmountArrayDiskArgs (257-259)

ArrayMutationsremoveDiskFromArrayArgs (262-264)

ArrayMutationssetStateArgs (267-269)

ArrayStateInput (312-315)

ArrayMutationsunmountArrayDiskArgs (272-274)

VmMutations (1848-1864)

VmMutationsforceStopVmArgs (1867-1869)

VmMutationspauseVmArgs (1872-1874)

VmMutationsrebootVmArgs (1877-1879)

VmMutationsresetVmArgs (1882-1884)

VmMutationsresumeVmArgs (1887-1889)

VmMutationsstartVmArgs (1892-1894)

VmMutationsstopVmArgs (1897-1899)

🪛 GitHub Actions: CI - Main (API)

api/src/unraid-api/auth/auth.service.spec.ts

[error] 168-168: Failed to sync roles for API key: Error: Failed to delete role

[error] 179-179: Failed to sync roles for API key: Error: Failed to add role

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

[error] 13-13: Type 'AuthPossession.ANY' is not assignable to type 'AuthPossession | undefined'. Did you mean 'AuthPossession.ANY'?

api/src/__test__/graphql/resolvers/subscription/network.test.ts

[error] 194-194: Snapshot integration test, loading nginx ini and generating all URLs 1 mismatched

api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts

[error] 304-304: expected [Function] to throw error including 'Failed to start VM' but got 'Failed to set VM state: Invalid UUID'

🪛 GitHub Check: Test API

api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts

[failure] 304-304: src/unraid-api/graph/resolvers/vms/vms.service.spec.ts > VmsService > should handle errors when VM is not available
AssertionError: expected [Function] to throw error including 'Failed to start VM' but got 'Failed to set VM state: Invalid UUID'

Expected: "Failed to start VM"
Received: "Failed to set VM state: Invalid UUID"

❯ src/unraid-api/graph/resolvers/vms/vms.service.spec.ts:304:9

⏰ Context from checks skipped due to timeout of 90000ms (1)

GitHub Check: Cloudflare Pages

🔇 Additional comments (107)

api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time (1)

1-1: Timestamp update noted.

This is a simple timestamp update to reflect the latest download time of the file.

api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time (1)

1-1: Timestamp update noted.

This is a simple timestamp update to reflect the latest download time of the file.

api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time (1)

1-1: Timestamp update noted.

This is a simple timestamp update to reflect the latest download time of the file.

api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time (1)

1-1: Timestamp update noted.

This is a simple timestamp update to reflect the latest download time of the file.

api/dev/Unraid.net/myservers.cfg (1)

2-2: Version update aligns with new VM features.

The API version has been updated from 4.1.3 to 4.4.1, which corresponds with the addition of virtual machine control functionality and the libvirt dependency upgrade noted in other files.
api/package.json (1)

74-74:
❓ Verification inconclusive

Update to major version of libvirt dependency.

The dependency @unraid/libvirt has been upgraded from version ^1.1.3 to ^2.1.0, which is a major version increment. Major version upgrades typically include breaking changes.

Let's verify that any code using this library has been updated to accommodate potential breaking changes:

🏁 Script executed:
#!/bin/bash
# Look for imports/usages of the @unraid/libvirt package
rg -A 3 "from '@unraid/libvirt'" --type ts
rg -A 3 "require\$['\"\\s]*@unraid/libvirt['\"]\$" --type ts
rg -A 3 "import.*@unraid/libvirt" --type ts
Length of output: 2154
Major version update for @unraid/libvirt – Manual verification needed

The dependency in api/package.json was upgraded from ^1.1.3 to ^2.1.0, which may introduce breaking changes. A search of the codebase shows that both production (api/src/unraid-api/graph/resolvers/vms/vms.service.ts) and test files (api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts) import and use this library. Please ensure that all API calls in these files are fully compatible with the new version and that the test suite covers any breaking changes introduced in v2.1.0.
api/src/store/modules/paths.ts (1)

70-70: Added path for libvirt PID file.

A new entry has been added to track the location of the libvirt daemon's PID file, which is necessary for VM control functionality.

api/src/graphql/schema/types/base.graphql (1)

8-9: Added auth directive for GraphQL field access control.

A new directive @auth has been added that can be applied to field definitions in the GraphQL schema. This directive requires three parameters:

action: The type of action being performed (AuthActionVerb)

resource: The resource being accessed (Resource)

possession: The possession relationship (AuthPossession)

This addition enables more granular access control within the GraphQL API, which will be important for securing VM management operations.
api/src/unraid-api/auth/casbin/policy.ts (1)

7-7:
❓ Verification inconclusive

Verify admin permissions scope after policy change

The admin role policy has been reduced from 5 parameters to 3, removing two wildcard matchers. This change may restrict admin permissions. Ensure this change maintains the intended permission scope for admin users.

🏁 Script executed:
#!/bin/bash
# Search for any permission checks that might expect the old 5-parameter format
# This would help identify if there are any places in the code that rely on the previous policy structure

echo "Searching for auth permission checks in the codebase..."
rg -t ts "AuthAction" --files-with-matches | xargs -I{} rg -A 3 -B 3 "Role\.ADMIN" {}
Length of output: 3885
Attention: Verify Admin Permissions Scope

The admin role policy in api/src/unraid-api/auth/casbin/policy.ts now uses the pattern
p, ${Role.ADMIN}, *, *
which reduces the parameters (and thus the wildcard matchers) compared to the previous 5-parameter format.

Automated searches did not reveal any parts of the code implying an expectation for a 5-parameter admin policy.

However, references to Role.ADMIN in test cases indicate that permissions are being exercised in several areas. Please manually verify that these tests—and any other relevant authorization checks—continue to allow the intended scope of admin access with the updated policy format.
api/src/__test__/store/modules/paths.test.ts (1)

7-34: Updated snapshot to include libvirt-pid path

The test snapshot has been correctly updated to include the new "libvirt-pid" path entry. This is consistent with the changes made to the store's initial state.
api/src/unraid-api/auth/authentication.guard.ts (2)

37-38: Class renamed from GraphqlAuthGuard to AuthenticationGuard

The guard has been renamed from GraphqlAuthGuard to AuthenticationGuard, which better reflects its general authentication purpose rather than being GraphQL-specific. The logger reference has been updated accordingly.

Also applies to: 41-41

36-102:
✅ Verification successful

Verify all references to GraphqlAuthGuard are updated

While the class name has been changed to AuthenticationGuard, ensure all references throughout the codebase have been updated to use the new name.

🏁 Script executed:
#!/bin/bash
# Search for any remaining references to GraphqlAuthGuard
echo "Searching for references to GraphqlAuthGuard..."
rg "GraphqlAuthGuard" --type ts

# Search for imports of authentication.guard.ts to check usage
echo "Checking imports of authentication.guard.ts..."
rg -A 1 "import.*authentication\.guard" --type ts
Length of output: 515
All references updated successfully

The search confirmed that no references to "GraphqlAuthGuard" remain and that "AuthenticationGuard" is properly imported in the code (e.g., in api/src/unraid-api/app/app.module.ts).

🧰 Tools

🪛 Biome (1.9.4)

[error] 42-44: This constructor is unnecessary.

Unsafe fix: Remove the unnecessary constructor.

(lint/complexity/noUselessConstructor)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.spec.ts (4)

4-7: Updated imports to support mocking

The imports have been updated to include both the necessary Vitest testing utilities and the newly referenced VmsService. This aligns with the change in approach from direct function imports to service-based architecture.

11-11: New VmsService variable for test setup

Added a variable to store the mocked service instance, which is good practice for test organization and readability.

15-23: Updated test module providers with VmsService mock

The test module now includes a mock implementation of VmsService with a stubbed getDomains method. This is an appropriate testing approach for isolating the resolver from its dependencies.

27-27: Retrieving the mocked service for test assertions

Properly retrieves the mocked service from the test module to allow for assertions on the mock in test cases.

.github/workflows/main.yml (2)

50-50: Added libvirt-daemon-system package to cached APT packages

The libvirt-daemon-system package has been added to the list of cached packages, which is necessary for the new libvirt setup step.

75-113: Added comprehensive libvirt setup step

This new step sets up the libvirt environment required for VM operations testing in the CI environment. The implementation is thorough, covering all necessary configurations:

Creates required groups (libvirt, kvm)

Sets up a libvirt user with appropriate group membership

Configures directories, permissions, and socket settings

Starts the libvirtd service

Verifies the service is running

The implementation follows best practices for libvirt configuration.

api/src/unraid-api/app/app.module.ts (2)

11-11: Updated auth guard import

The import has been updated to use the new AuthenticationGuard instead of the previous GraphQL-specific guard, which aligns with the move toward a more centralized authentication approach.

56-56: Replaced GraphqlAuthGuard with AuthenticationGuard

The application now uses AuthenticationGuard instead of the GraphQL-specific GraphqlAuthGuard. This change standardizes the authentication approach across the application, likely supporting multiple endpoints beyond just GraphQL.

api/src/unraid-api/auth/casbin/model.ts (1)

16-17: Enhanced authorization matcher logic

The Casbin matcher logic has been significantly improved:

Object matching now uses case-insensitive regex comparison with regexMatch(lower(r.obj), lower(p.obj)), improving flexibility

Action matching has been enhanced to support multiple patterns:

Exact matches

Wildcard matches with p.act == '*'

Pattern-based matches with regexMatch(lower(r.act), lower(concat(p.act, ':.*')))

This change provides more powerful authorization rules while maintaining backward compatibility with existing policies.
api/src/__test__/graphql/resolvers/subscription/network.test.ts (2)

269-274: Update includes new error handling for URLs without ports.

A new error message has been added to the urls.errors array for URLs without specified ports, which aligns with the VM controls feature being implemented.

194-268:
❓ Verification inconclusive

Snapshot update reflects VM-related changes.

The snapshot for the urls.urls array has been modified, but the pipeline failure suggests that the test is still failing with a snapshot mismatch. The snapshot needs to be updated using the current test output.

🏁 Script executed:
#!/bin/bash
# Verify the current test output matches the updated snapshot

# Run the specific test and capture the output
npx vitest run api/src/__test__/graphql/resolvers/subscription/network.test.ts -t "integration test, loading nginx ini and generating all URLs" --update
Length of output: 248
ACTION REQUIRED: Verify Snapshot Update Command Configuration

The snapshot update in api/src/__test__/graphql/resolvers/subscription/network.test.ts failed because the command couldn’t run due to a missing npx command. Please ensure your environment is set up correctly or adjust the command for your package manager.

Confirm that npx is installed, or if you’re using Yarn or PNPM, update the command accordingly (e.g., yarn vitest run ... --update).

Manually verify that after running the proper command, the updated snapshot matches the current test output.
🧰 Tools
🪛 GitHub Check: Test API

[failure] 194-194: src/test/graphql/resolvers/subscription/network.test.ts > integration test, loading nginx ini and generating all URLs
Error: Snapshot integration test, loading nginx ini and generating all URLs 1 mismatched

Expected

Received

@@ -24,10 +24,15 @@
"ipv4": "https://192-168-1-150.thisisfourtyrandomcharacters012345678900.myunraid.net:4443/",
"name": "FQDN LAN",
"type": "LAN",
},
{
"ipv4": "https://85-121-123-122.thisisfourtyrandomcharacters012345678900.myunraid.net:8443/",
"name": "FQDN WAN",
"type": "WAN",
},

{
"ipv4": "https://10-252-0-1.hash.myunraid.net:4443/",
"name": "FQDN WG 0",
"type": "WIREGUARD",
},
{
❯ src/test/graphql/resolvers/subscription/network.test.ts:194:23
🪛 GitHub Actions: CI - Main (API)

[error] 194-194: Snapshot integration test, loading nginx ini and generating all URLs 1 mismatched
api/codegen.ts (5)

5-5: Good addition of auth enum types import.

Importing getAuthEnumTypeDefs from the auth directive is necessary for including authentication enum types in the GraphQL schema.

35-42: Enhanced schema generation to include auth directives.

Added a new entry to generate a standalone GraphQL schema file that includes the authentication enum types and directives.

45-49: Consistent inclusion of auth enum types in API server types.

Auth enum types are now properly included in the schema for API server types, ensuring consistency across the codebase.

62-68: Consistent schema definition for API operations.

Auth enum types are now properly included in the schema for API operations, maintaining consistency with the other schema configurations.

85-104: Reorganized client generation configuration.

The mothership GraphQL client generation configuration has been moved but remains functionally identical.

api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (4)

9-22: Removal of jGrowl notifications is intentional.

The removal of jGrowl notifications aligns with previous architectural decisions to handle notifications on a separate page. Based on retrieved learnings, this is part of replacing the legacy notification system with a modern approach.

46-57: Consistent removal of jGrowl notification handling.

This change continues the removal of jGrowl notifications, maintaining consistency with the changes above.

60-67: Removed bell icon styling which is no longer needed.

With the jGrowl notifications removed, the bell icon styling and functionality is also no longer needed.

79-79: Added custom toaster component for notifications.

Added the <uui-toaster> component that will handle notifications, replacing the legacy jGrowl system. The position is correctly configured based on user preferences.

api/src/unraid-api/auth/api-key.service.ts (6)

81-82: Helpful comment on enum value retrieval.

The added comment clarifies that the actual enum value is being pulled from the GraphQL schema, which is helpful for future maintenance.

218-223: Improved role string normalization.

The update to parse JSON first and convert all role strings to uppercase ensures consistent case handling in the API key validation process, preventing issues with case-sensitive enum matching.

231-232: Improved error logging with extracted method.

Replaced direct error logging with a call to the new logApiKeyZodError method for better error handling.

251-252: Consistent error handling pattern.

Applied the same error handling pattern for ZodErrors when finding a key by ID.

276-279: Good extraction of error logging to a dedicated method.

Created a new private method logApiKeyZodError to centralize and standardize error logging for invalid API key structures, improving code maintainability.

312-313: Consistent application of error logging pattern.

Completed the refactoring by using the new error logging method during API key saving as well.

api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (3)

108-114: Nicely implemented subscriber monitoring functionality

Adding the monitor method to NchanSubscriber.prototype and creating an array to track subscribers is a good approach for managing multiple connection instances.

1239-1307: Well-implemented focus management for live updates

The implementation of nchanPaused, nchanFocusStart, and nchanFocusStop provides an efficient way to manage resources by pausing updates when the user isn't actively viewing the page. The logic for handling both window blur/focus events and the document visibility state is comprehensive.

A few observations:

The 30-second delay before stopping updates on blur is a good balance for user experience

The cleanup of blur timer when focus is regained prevents unnecessary pausing

The banner warning properly informs users that updates are paused

726-726: Good integration with existing reload mechanism

The modification to check nchanPaused before reloading ensures that the page won't unnecessarily reload when updates are paused, which is consistent with the overall focus management strategy.

api/src/unraid-api/auth/auth.service.spec.ts (3)

3-4: Appropriate import update for AuthActionVerb

The import statement has been correctly updated to include AuthActionVerb.

39-40: Good use of enum for action verbs

Replacing the hardcoded string with AuthActionVerb.READ.toUpperCase() is a good practice. Using enums instead of string literals improves type safety and makes the code more maintainable.

123-123: Consistent use of enum for roles

Using Role.GUEST instead of the string literal 'guest' is more consistent with the rest of the codebase and improves type safety.

api/src/unraid-api/auth/api-key.service.spec.ts (3)

12-12: Good import of AuthActionVerb from types

Importing AuthActionVerb from the generated types file is appropriate.

85-85: Improved type safety with enum

Using AuthActionVerb.READ instead of the string literal 'read' improves type safety and consistency.

604-610: Better error testing with inline snapshot

Using toThrowErrorMatchingInlineSnapshot instead of toThrow with a string message is a better practice for error testing. It ensures the exact error structure is validated, making tests more robust.

The simplification of the error logging expectation is also good - focusing on just the content string without unnecessary second arguments.
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (4)

7-7: Good addition of VmsService import

The import of VmsService aligns with the dependency injection pattern used in the rest of the codebase.

11-11: Constructor injection is a good practice

Using constructor injection for the VmsService dependency follows NestJS best practices and improves testability.

23-23: Improved domain retrieval implementation

Replacing dynamic imports with the injected service method improves maintainability and testability.

13-18:
❓ Verification inconclusive

Verify permission handling for vms query

The @UsePermissions decorator has been removed from this query method. Since this appears to be part of a broader refactoring of the authorization system, please verify that proper permission checks are still being applied through the new auth directive system.

🏁 Script executed:
#!/bin/bash
# Check if there's an @auth directive applied to the vms query in the schema
grep -r --include="*.graphql" "vms *{" api/src/graphql/schema/
grep -r --include="*.graphql" "@auth.*vms" api/src/graphql/schema/
Length of output: 133
Manual Verification Required: Confirm Auth Handling on vms Query

Our initial search in the GraphQL schema did not reveal any explicit @auth directive attached to the vms query. Since the @UsePermissions decorator was removed as part of the refactoring, please manually verify that the new authorization system (via auth directives or an equivalent global mechanism) is indeed protecting this query.

Confirm that permission checks for the vms query are implemented, either through an auth directive on the field or via a global resolver/middleware.

If the auth directive is expected but missing, update the schema accordingly.
api/src/graphql/schema/types/api-key/roles.graphql (1)

39-41: Standardized Role enum values to uppercase

Consistent with the Resource enum changes, the Role enum values are now uppercase. Verify that client code has been updated accordingly.
api/src/unraid-api/graph/graph.module.ts (4)

18-22: Good addition of auth directive imports

The imported auth-related functions will enable schema-level permission checks, which is a more maintainable approach.

38-49: Improved GraphQL configuration with auth enums

Adding auth enum type definitions to the schema and restructuring the resolvers object improves code organization.

64-68: Enhanced schema transformation with auth directives

The implementation of schema transformers for resolvers and directives centralizes permission checks at the schema level, which is a good architectural decision.

74-75:
✅ Verification successful

Verify impact of empty providers and module export

The providers array has been emptied and GraphQLModule is now exported. This suggests a refactoring of service provision. Verify that the removed providers are now provided elsewhere.

🏁 Script executed:
#!/bin/bash
# Check where the previously included providers might now be provided
echo "Checking for ConnectSettingsService provider in other modules:"
grep -r "ConnectSettingsService" --include="*.module.ts" api/src/
echo "Checking for ConnectResolver provider in other modules:"
grep -r "ConnectResolver" --include="*.module.ts" api/src/
echo "Checking for ConnectService provider in other modules:"
grep -r "ConnectService" --include="*.module.ts" api/src/
echo "Checking for NetworkResolver provider in other modules:"
grep -r "NetworkResolver" --include="*.module.ts" api/src/
echo "Checking for ServicesResolver provider in other modules:"
grep -r "ServicesResolver" --include="*.module.ts" api/src/
echo "Checking for SharesResolver provider in other modules:"
grep -r "SharesResolver" --include="*.module.ts" api/src/
Length of output: 2436
Service Providers Refactoring Verified

The removed providers in graph.module.ts have been successfully relocated. The verification shows that the following providers are now imported and provided within api/src/unraid-api/graph/resolvers/resolvers.module.ts:

ConnectSettingsService

ConnectResolver

ConnectService

NetworkResolver

ServicesResolver

SharesResolver

Please confirm that all functionality relying on these services is operating as expected with these changes.
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (3)

9-33: Good test setup with proper mocking

The test module setup correctly mocks the VmsService dependencies and initializes the resolver under test.

35-53: Comprehensive testing of startVm method

The tests for startVm properly verify both successful execution and error propagation.

55-73: Comprehensive testing of stopVm method

The tests for stopVm properly verify both successful execution and error propagation.

api/src/unraid-api/graph/directives/auth.directive.ts (2)

1-3: Imports are organized.
These lines cleanly pull in the necessary utilities from @graphql-tools and nest-authz without introducing unused imports.

114-143: Enum type definitions look good.
The getAuthEnumTypeDefs function systematically constructs SDL strings for authentication enums. Ensure robust testing with different enum values so that numeric and string enum overlap cases are handled consistently.

api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (1)

1222-1291: Graceful pausing and resuming of live updates on focus/blur.
The added logic with nchanFocusStart, nchanFocusStop, and the blurTimer elegantly manages resource usage when the window isn’t visible. One caution: if a user remains blurred for a short while, ensure clarity around re-activating updates, i.e., purposeful or accidental. This approach is beneficial for performance but confirm the UI feedback is clear if updates remain paused.

Would you like to verify that we handle multiple consecutive blur/focus events properly? I can generate a small script to simulate repeated focus/blur cycles and check for potential edge cases in resuming subscriptions.

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

5-7: New imports for Connect, Network, VM, and services.
Good addition of resolvers and services to handle connectivity, networking, and VM mutations. Verify carefully that you have no circular dependencies (for instance, between ConnectService and any similarly layered services).

Also applies to: 32-32, 34-36

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

110-119: Graceful shutdown fallback is good, but watch out for partial states.

When the domain is RUNNING or PAUSED, a graceful shutdown is attempted followed by a force destroy if needed. This approach is solid but be mindful of partially shut down states or hypervisor-specific edge cases. If the domain is stuck in a "SHUTDOWN" transitional phase, consider re-checking domain info before forcing a destroy.

api/src/graphql/schema/types/vms/domain.graphql (4)

3-4: Authorization directive for VM queries looks consistent.
Ensuring that only authorized users can query vms with @auth(action: READ, resource: VMS, possession: ANY) is logically correct for read access to virtual machines.

6-9: Confirm READ permission at the root of Mutation.
Using @auth(action: READ, resource: VMS, possession: ANY) on the top-level vms mutation field is slightly unusual if each nested VM operation requires UPDATE. Typically, you would also secure the parent field with the same or stricter permission.

Would you like to confirm that the parent vms field should indeed require only READ, or should it be UPDATE?

30-30: Consistent read authorization for VM domains.
Applying @auth(action: READ, resource: VMS, possession: ANY) to the domain field aligns neatly with the rest of your schema permissions.

34-34: Subscription layering is coherent.
Authorizing read access on the vms subscription ensures consistent permission checks across queries, mutations, and subscriptions for VM data.

web/composables/gql/graphql.ts (13)

197-203: ArrayDiskInput appears correct.
Your ArrayDiskInput type accepting an id plus an optional slot parameter is appropriately defined, offering flexibility for specifying an array disk location.

250-253: No issues for addDiskToArrayArgs.
Allowing an input of type ArrayDiskInput makes the arguments flexible.

255-258: clearArrayDiskStatisticsArgs is straightforward.
Requiring a single disk id is clear and easy to maintain.

260-263: mountArrayDiskArgs has no obvious problems.
This follows the same pattern as other disk operations, which keeps the API consistent.

265-268: removeDiskFromArrayArgs looks good.
Use of ArrayDiskInput again ensures consistent structure for disk-based operations.

270-273: setStateArgs references ArrayStateInput.
This design is clear, allowing users to specify a target array state.

275-278: unmountArrayDiskArgs is consistent.
Accepting an id clarifies which disk to unmount.

315-319: ArrayStateInput is straightforward.
Including desiredState: ArrayStateInputState is a concise approach to controlling the array’s operation mode.

320-326: ArrayStateInputState covers essential actions.
Having Start and Stop enumerations is sufficiently expressive for typical array lifecycles.

327-333: AuthActionVerb enumeration is standard.
The involvement of CREATE, DELETE, READ, and UPDATE is standard for role-based permission checks. No concerns here.

335-340: AuthPossession aligns with known patterns.
ANY, OWN, and OWN_ANY appear consistent for specifying possession-based rules.

1383-1411: Resource enum expansions look good.
Adding uppercase constants for ApiKey, Array, Cloud, etc. clarifies your resource set. However, if the front-end or other consumers rely on older values, ensure everything is updated to handle the new resource strings.

Could you verify that all references to these Resource enum values are updated in your codebase?

1416-1419: Role enum consistency.
Using uppercase strings for Admin, Connect, and Guest is consistent with the rest of the design.

api/generated-schema.graphql (26)

1-34: Authorization & Input Definitions Review

The new directive @auth (line 1) and the accompanying types—AccessUrl, AccessUrlInput, AddPermissionInput, AddRoleForApiKeyInput, AddRoleForUserInput, and AllowedOriginInput—are clearly defined and consistently named. The documentation attached to these inputs is helpful. Just ensure that any optional fields are validated according to your business rules when implemented in resolvers.

36-85: API Key & API Settings Types

The types ApiKey, ApiKeyResponse, ApiKeyWithSecret, and the input ApiSettingsInput are comprehensive and self-descriptive. The inline documentation (using triple quotes) in ApiSettingsInput clarifies field purposes well. Ensure that any validation for required combinations is enforced in the business logic.

87-325: Array-Related Definitions

The schema for arrays—including Array, ArrayCapacity, ArrayDisk (with its detailed documentation), related enums (like ArrayDiskFsColor, ArrayDiskStatus, ArrayDiskType), the ArrayMutations type, and the state inputs/enums—is very detailed. This provides a solid foundation for managing array configurations. Verify that computed fields (such as filesystem capacities) are properly resolved at runtime.

327-361: Hardware & Core Enums

The enums AuthActionVerb and AuthPossession and the types Baseboard, Capacity, and Case are straightforward and consistent. Their clarity supports both type-safety and developer understanding. Consider adding example values in the documentation if future consumers need guidance.

363-438: Cloud and Connection Configurations

The types Cloud, CloudResponse, Config, Connect, ConnectSettings, and ConnectSettingsValues (as well as the corresponding input types) are well structured and supported with descriptive comments. They should serve the configuration and connectivity aspects reliably. It would be beneficial to ensure that business logic later validates the interdependencies between these fields.

440-483: Container & API Key Input Configurations

The definitions for ContainerHostConfig, ContainerMount, ContainerPort (with its related enum ContainerPortType and ContainerState), along with CreateApiKeyInput and the scalar DateTime, are clear and adhere to naming conventions. Maintaining consistent validation practices for these will be key.

485-562: Disk, Devices, and Display Types

The Disk type (and its use of the Long scalar), associated enums for disk file system types, interfaces, and the Display type are written with excellent documentation. This level of detail should help ensure accurate client‐server interactions regarding disk and display information.

564-612: Docker System Types

The Docker, DockerContainer, DockerMutations, and DockerNetwork types are defined clearly. Their use of the JSON scalar for dynamic configurations is appropriate. Just double-check that the mutation operations have proper error handling in the resolvers.

614-645: Dynamic Remote Access, Flash, and GPU

The types DynamicRemoteAccessStatus, DynamicRemoteAccessType, and EnableDynamicRemoteAccessInput along with Flash and Gpu are introduced to handle specialized hardware and remote configurations. They look solid—ensure that boolean flags (and any potential nulls) are managed correctly in the actual implementation.

647-711: System Information Types

The Importance enum and the information-related types (Info, InfoApps, InfoCpu, InfoMemory) are well documented. The use of inline comments helps clarify their purpose. It will be important to ensure that any dynamic or JSON-resolved fields are properly validated upstream.

713-759: File and Scalar Definitions

The scalars and types for JSON, KeyFile, LogFile, LogFileContent, along with the Long scalar and Me type, are straightforward and beneficial for file operations and user identification. Consider implementing robust error boundaries where file access is performed.

761-783: Memory & Minigraph Types

The MemoryFormFactor, MemoryLayout, MemoryType and the MinigraphStatus with MinigraphqlResponse are defined to capture detailed memory and system graph data. Their documentation is clear—if some fields can be required, consider adding non-null constraints.

785-804: Mount-related Definitions

The Mount type (and its accompanying fields) is succinct and clear. If there is any numeric data (such as port numbers), make sure to enforce the correct formats in the resolvers.

806-866: Mutations Overview

The Mutation type aggregates a broad array of operations with helpful docstrings. This centralizes write operations (from adding permissions to VM control). It’s crucial to verify that all mutations are backed by resolvers that enforce both validation and authorization (possibly making good use of the new @auth directive).

868-887: Network & Node Interface

The Network type and the Node interface are defined with clarity. They are minimal yet effective. Just ensure that any implementing type fully conforms to the Node interface as intended.

889-935: Notification-Related Definitions

The types Notification, NotificationCounts, NotificationData, NotificationFilter, NotificationOverview, and the enum NotificationType are organized to support a robust notification system. Their structure supports both granular data and aggregate views well.

1035-1059: PCI, Permission, and Profile Types

The Pci, Permission, and ProfileModel definitions (plus the Port scalar) are succinct and clear. They adhere to naming conventions, making it easier for developers to work with these elements.

1061-1130: Query Type

The Query type exposes a diverse set of operations across the schema. It’s comprehensive and well documented. Consider performance implications on queries that return large datasets (such as logs or notifications) and add pagination or filtering as needed.

1132-1192: Registration Details

The Registration type and RegistrationState enum provide detailed statuses for licensing and registration. The inline documentation is clear—ensure that each status is handled appropriately in your resolver logic.

1194-1209: Relay and Remote Access Inputs

The RelayResponse, RemoteAccess, and RemoveRoleFromApiKeyInput types appear well defined. They should integrate smoothly with the rest of the mutation and query capabilities, provided null and error cases are well handled.

1212-1248: Resource and Role Enums

The enums Resource and Role cover a broad range of system elements and user roles, respectively. They provide a robust basis for authorization checks. Consistency in their usage throughout the schema is key.

1250-1266: Server Information

The Server type and its corresponding ServerStatus enum provide a clear summary of server identity and state. The fields seem appropriate for both local and remote views; ensure that IP addresses and URLs are validated.

1276-1313: Remote Access Setup & Share Definitions

The SetupRemoteAccessInput and Share types are quite detailed. The Share type, in particular, includes descriptions that clarify its many fields. Validate that fields like exclude and include behave as intended, especially if there are mutual exclusion rules.

1315-1348: Subscription Type

The Subscription type is broad, exposing many real-time updates. Its design appears robust; however, ensure that subscriptions are secure and potentially rate-limited to avoid misuse.

1350-1367: System & Basic Scalar Definitions

The System type along with enums like Temperature and Theme, and scalars such as URL and UUID are fundamental. Enforcing the correct formats and constraints (especially for URL and UUID values) will help maintain data integrity.

1445-1465: User & Account Definitions

The User type and UserAccount interface are straightforward. Notably, the use of a Boolean flag for password (indicating whether a password is set) is interesting—be sure that sensitive authentication details are handled securely elsewhere.

api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json

coderabbitai · 2025-03-31T20:32:33Z

web/codegen.ts

-          },
-        },
-      ],
+      schema: '../api/generated-schema.graphql',


💡 Verification agent

🧩 Analysis chain

Schema source changed from remote endpoint to local file

The GraphQL schema source has been changed from a remote HTTP endpoint to a local file at '../api/generated-schema.graphql'. This simplifies configuration and removes the need for headers, but ensure that the local schema file is properly generated and kept up to date.

🏁 Script executed:

#!/bin/bash # Check if the referenced schema file exists if [ ! -f "../api/generated-schema.graphql" ]; then echo "Error: Schema file '../api/generated-schema.graphql' does not exist" exit 1 fi # Check when the schema file was last modified echo "Schema file last modified on:" stat -c "%y" "../api/generated-schema.graphql" # Check the file size to ensure it's not empty echo "Schema file size (bytes):" stat -c "%s" "../api/generated-schema.graphql" # Look for schema generation scripts in package.json echo "Schema generation scripts in package.json:" cd ../api && rg "generate.*schema" package.json || echo "No schema generation script found"

Length of output: 208

Action Required: Generate the Local Schema File

The referenced file (../api/generated-schema.graphql) does not exist. Please ensure that the schema file is generated (via the schema generation script or your build process) and kept up to date before merging these changes.

Verify the schema generation process and update it if necessary.

Confirm that the generated file is committed or reliably produced during deployment.

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

api/src/graphql/schema/types/api-key/roles.graphql

api/src/unraid-api/graph/resolvers/resolvers.module.ts

api/src/unraid-api/graph/resolvers/vms/vms.service.ts

api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (10)

api/src/__test__/setup/config-setup.ts (2)
1-17: Script lacks error handling for the copy operation

While the script checks for the existence of the source file, it doesn't handle potential errors during the copy operation itself. If the destination directory doesn't exist or if there are permission issues, the script will fail without proper error reporting.

Consider adding try-catch for better error handling:
-copyFileSync(sourceFile, destFile);
+try {
+    copyFileSync(sourceFile, destFile);
+    console.log(`Successfully copied ${sourceFile} to ${destFile}`);
+} catch (error) {
+    console.error(`Error copying file: ${error.message}`);
+    process.exit(1);
+}
8-9: Hardcoded paths may cause issues in different environments

The hardcoded paths to specific locations under 'dev/Unraid.net/' could cause issues if the directory structure changes or in different deployment environments.

Consider making these paths configurable:
-const sourceFile = join(projectRoot, 'dev/Unraid.net/myservers.example.cfg');
-const destFile = join(projectRoot, 'dev/Unraid.net/myservers.cfg');
+const configDir = process.env.CONFIG_DIR || 'dev/Unraid.net';
+const sourceFile = join(projectRoot, configDir, 'myservers.example.cfg');
+const destFile = join(projectRoot, configDir, 'myservers.cfg');
api/src/unraid-api/graph/directives/auth.directive.spec.ts (4)
114-114: Use optional chaining for more concise code

The current null check could be simplified using optional chaining.
-if (queryFields.protectedField && queryFields.protectedField.resolve) {
+if (queryFields.protectedField?.resolve) {
🧰 Tools

🪛 Biome (1.9.4)

[error] 114-114: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

172-172: Use optional chaining for more concise code

Similar to the previous suggestion, use optional chaining here as well.
-if (queryFields.unprotectedField && queryFields.unprotectedField.resolve) {
+if (queryFields.unprotectedField?.resolve) {
🧰 Tools

🪛 Biome (1.9.4)

[error] 172-172: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

95-201: Consider refactoring duplicate test resolver code

Both tests for transformResolvers contain similar code for the testResolver function. Consider extracting this into a helper function to reduce duplication.
function createTestResolver(fieldName: string, schema: GraphQLSchema, mockSource = {}, mockArgs = {}, mockContext = {}) {
  return async () => {
    if (!schema.getQueryType()) return;

    // Get the schema fields
    const queryFields = schema.getQueryType()!.getFields();

    // Call the wrapped resolver
    if (queryFields[fieldName]?.resolve) {
      return await queryFields[fieldName].resolve(
        mockSource,
        mockArgs,
        mockContext,
        {
          fieldName,
          parentType: { name: 'Query' },
          schema,
          // These fields aren't used in the auth directive code
          fieldNodes: [] as any,
          returnType: {} as any,
          path: {} as any,
          fragments: {} as any,
          rootValue: null as any,
          operation: {} as any,
          variableValues: {} as any,
        } as unknown as GraphQLResolveInfo
      );
    }
  };
}

// Usage:
const testResolver = createTestResolver('protectedField', schema, mockSource, mockArgs, mockContext);
await testResolver();
🧰 Tools

🪛 Biome (1.9.4)

[error] 114-114: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 172-172: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

206-206: Consider using a more specific type instead of any

Using any loses type checking benefits. Consider defining a more specific type for the resolvers array to maintain type safety.
-] as any; // Type assertion to avoid complex IResolvers typing
+] as Array<Record<string, Record<string, () => string>>>; // More specific type
api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (4)

66-91: Consider consolidating cleanup logic.

cleanupDiskImage() and cleanupDomainXml() are largely similar helper functions, each removing a file if it exists. Unifying these into a single, parameterized cleanup method can reduce duplication and improve maintainability.

93-138: Potential domain cleanup race.

When cleanupDomain lists active and inactive domains, tests could collide if run in parallel, especially if domains are being created or destroyed simultaneously. Consider ensuring these tests run sequentially or adding synchronization checks to avoid conflicts in multi-tenant or parallel test environments.

177-200: Recommend restoring environment variable post-test.

Overriding LIBVIRT_URI is fine for integration tests, but it can affect other tests or modules if executed in parallel. Storing and restoring the original value in afterAll helps maintain a consistent environment for subsequent tests.

262-301: DRY up VM state checks.

The repeated while-loop and manual retry logic in the “start and stop the test VM” test could be extracted into a helper function (e.g., waitForDomainState). This reduces duplication and allows future expansions like more sophisticated backoff strategies or event-based checks.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between f3514b1 and 90dcdc3.

📒 Files selected for processing (7)

.gitignore (1 hunks)
api/dev/Unraid.net/myservers.example.cfg (1 hunks)
api/src/__test__/graphql/resolvers/subscription/network.test.ts (1 hunks)
api/src/__test__/setup/config-setup.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (1 hunks)
api/vite.config.ts (1 hunks)

✅ Files skipped from review due to trivial changes (1)

api/dev/Unraid.net/myservers.example.cfg

🚧 Files skipped from review as they are similar to previous changes (1)

api/src/test/graphql/resolvers/subscription/network.test.ts

🧰 Additional context used

🧬 Code Definitions (1)

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

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

domain (21-30)

🪛 Biome (1.9.4)

api/src/unraid-api/graph/directives/auth.directive.spec.ts

[error] 114-114: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 172-172: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: Build Web App
GitHub Check: Cloudflare Pages

🔇 Additional comments (10)

.gitignore (2)

108-109: Clarify the exclusion for activation-data.php.
The line for plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/data/activation-data.php is reintroduced here. Please confirm that re-adding this exclusion is intentional since previous changes indicated removing it from ignore.

110-111: Appropriately ignoring environment-specific configuration.
Adding api/dev/Unraid.net/myservers.cfg to the ignore list is a sensible move to prevent environment-specific changes from being tracked. Just verify that no critical configuration is inadvertently unversioned.

api/vite.config.ts (1)

172-172: Enhancement to test setup with config-setup.ts

The addition of config-setup.ts to the setupFiles array is appropriate for ensuring tests have the necessary configuration files available. This change ensures consistency in the test environment by copying the example configuration file to a usable location at runtime.

api/src/unraid-api/graph/directives/auth.directive.spec.ts (5)

1-25: Well-organized imports and mocking setup

The imports are properly organized, and the necessary dependencies are mocked correctly. The test file properly mocks the nest-authz library with the required AuthActionVerb, AuthPossession, and UsePermissions functions.

27-58: Good test setup pattern

The test suite follows good practices by:

Creating a fresh schema before each test

Applying transformations in the correct order

Clearing mocks between tests

This ensures test isolation and prevents test pollution.

60-93: Comprehensive tests for authSchemaTransformer

These tests thoroughly verify the core functionality of the auth schema transformer:

Adding permission information to field descriptions

Storing permission requirements in extensions

Not modifying fields without auth directives

The assertions are specific and validate the expected behaviors.

202-212: Good test for array handling

This test ensures that the transformer correctly handles an array of resolvers, which is important for real-world usage where resolvers might be split across multiple files or modules.

214-236: Thorough validation of enum type definitions

The test for getAuthEnumTypeDefs thoroughly verifies that the generated SDL includes all necessary enum types, values, and directive definitions. The dynamic approach to checking enum values is particularly robust, as it will automatically adapt if new enum values are added.
api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (2)

303-306: Updated error assertions look correct.

The test now properly asserts that starting/stopping an invalid VM UUID throws 'Failed to set VM state: Invalid UUID', aligning with the service’s error message. This addresses the mismatch highlighted in the past review.

54-63:
✅ Verification successful

Unused helper function.

verifyQemuInstallation() is never invoked in this test file, making it effectively dead code. Consider removing it or integrating it into the setup process if QEMU availability must be verified.

Use this script to confirm whether verifyQemuInstallation is referenced anywhere else:

🏁 Script executed:
#!/bin/bash
rg -A 3 'verifyQemuInstallation'
Length of output: 390
Action: Remove or Integrate the Unused Helper Function

The function verifyQemuInstallation is defined only in the test file (api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts) and isn't referenced anywhere. It's effectively dead code. Please remove it to reduce clutter, or integrate it into the test setup if QEMU validation is needed.

File: api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (lines 54-63)

Issue: Unused helper function

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

api/src/__test__/core/utils/array/array-is-running.test.ts (1)
7-14: Consider removing async from test functions

Since the tests no longer use dynamic imports or any other async operations, you could simplify by removing the async keyword from the test function declarations.
-test('Returns true if the array is started', async () => {
+test('Returns true if the array is started', () => {

-test('Returns false if the array is stopped', async () => {
+test('Returns false if the array is stopped', () => {
Also applies to: 16-22

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 90dcdc3 and 395a4f6.

📒 Files selected for processing (1)

api/src/__test__/core/utils/array/array-is-running.test.ts (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (3)

GitHub Check: Test API
GitHub Check: Build Web App
GitHub Check: Cloudflare Pages

🔇 Additional comments (1)

api/src/__test__/core/utils/array/array-is-running.test.ts (1)

4-4: Import method improved

Switching from dynamic import to static import is a good improvement. This simplifies the code structure and improves performance by resolving the import at load time instead of runtime within each test case.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (1)
114-114: Use optional chaining for cleaner checks.
Replacing && with optional chaining enhances readability and reduces complexity.
- if (queryFields.protectedField && queryFields.protectedField.resolve) {
+ if (queryFields.protectedField?.resolve) {

- if (queryFields.unprotectedField && queryFields.unprotectedField.resolve) {
+ if (queryFields.unprotectedField?.resolve) {
Also applies to: 173-173

🧰 Tools

🪛 Biome (1.9.4)

[error] 114-114: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 395a4f6 and ac47aa7.

📒 Files selected for processing (2)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/online/online.resolver.ts (1 hunks)

🧰 Additional context used

🪛 Biome (1.9.4)

api/src/unraid-api/graph/directives/auth.directive.spec.ts

[error] 114-114: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 173-173: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🪛 GitHub Check: Build API

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

[failure] 3-3:
Replace UsePermissions,·AuthPossession with AuthPossession,·UsePermissions

🪛 GitHub Check: Test API

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

[failure] 3-3:
Replace UsePermissions,·AuthPossession with AuthPossession,·UsePermissions

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: Build Web App
GitHub Check: Cloudflare Pages

🔇 Additional comments (7)

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

11-13: Verify type compatibility between AuthActionVerb and nest-authz.

Since AuthActionVerb is now imported from the generated types rather than directly from nest-authz, ensure there's no type mismatch. A previous solution used as any as a temporary fix.

Please confirm that the types are compatible by checking if the Typescript compiler raises any errors. If you're still experiencing type issues, you might need to add a type assertion:
    @UsePermissions({
        action: AuthActionVerb.READ,
        resource: Resource.ONLINE,
-       possession: AuthPossession.ANY,
+       possession: AuthPossession.ANY as any,
    })
api/src/unraid-api/graph/directives/auth.directive.spec.ts (6)

1-5: Imports look clean and well-structured.
No notable issues or redundancies identified here.

12-25: Mock definitions are consistent.
The approach to mock nest-authz appears sound, ensuring test isolation and clarity.

27-59: Well-organized test initialization.
Creating a fresh schema for each test ensures proper isolation and eliminates side effects.

60-93: Auth schema transformer tests are robust.
These checks confirm that the directive updates both field descriptions and extensions as intended.

204-214: Array resolver handling is validated.
Good job verifying that an array of resolvers is correctly supported by the transformation logic.

216-238: Auth enum SDL generation is comprehensive.
All enum values and directive definitions are covered, ensuring complete test coverage.

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

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (4)

api/src/unraid-api/graph/resolvers/vms/vms.service.ts (4)

77-136: Validate concurrency safety of class-level hypervisor reference.

When multiple requests invoke setVmState concurrently, the class-level hypervisor and its domain references may risk race conditions, especially if the hypervisor is reinitialized or disconnected. Consider clarifying concurrency guarantees or adding locking/queues around critical sections if simultaneous calls are expected.

210-237: Consider using in-library “reboot” method if available.

For graceful reboots, the libvirt API sometimes provides a dedicated method (e.g., domain.reboot) that handles both shutdown and startup internally. If it exists in your version of @unraid/libvirt, this approach may simplify code and reduce potential timing issues.

262-310: Minimize repeated domain info lookups.

This block fetches domain info repeatedly in the loop, mapping each to VmDomain. If performance becomes a concern (e.g., large domain counts), consider batching or caching. Even though current usage may be minimal, a single pass for info might be more efficient for large scale.

312-329: Adjust retry intervals for graceful shutdown.

The waitForDomainShutdown method loops up to 10 times, each waiting 1 second. For particularly slow responses or large memory VMs, you may need longer intervals or a configurable timeout. Evaluate real-world conditions to avoid prematurely forcing a destroy.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between ac47aa7 and e9ee14a.

📒 Files selected for processing (2)

api/src/unraid-api/graph/resolvers/online/online.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

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

⏰ Context from checks skipped due to timeout of 90000ms (4)

GitHub Check: Build Web App
GitHub Check: Test API
GitHub Check: Build API
GitHub Check: Cloudflare Pages

🔇 Additional comments (1)

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

77-136: Ensure errors match expected messages in tests.

This code re-throws errors as “Failed to set VM state,” whereas test cases or other callers might expect more specific messages (e.g., “Failed to start VM”) in certain scenarios. If there are existing tests expecting different error text, wrap the state change logic in a more targeted try/catch to yield a more specific message.

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (11)

api/src/unraid-api/auth/auth.interceptor.ts (1)
11-11: Avoid logging sensitive user information.
console.log('in auth interceptor', req.user) may expose user details in logs. Consider removing or sanitizing sensitive data to prevent potential PII leaks.
- console.log('in auth interceptor', req.user);
+ // console.debug('in auth interceptor', !!req.user);
api/src/unraid-api/graph/directives/auth.directive.spec.ts (2)
114-114: Use optional chaining for clarity and safety.
Enhance readability by converting the combined null check into optional chaining.
- if (queryFields.protectedField && queryFields.protectedField.resolve) {
+ if (queryFields.protectedField?.resolve) {
🧰 Tools

🪛 Biome (1.9.4)

[error] 114-114: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

173-173: Apply optional chaining similarly here.
Same reasoning as above: optional chaining can simplify conditional checks.
- if (queryFields.unprotectedField && queryFields.unprotectedField.resolve) {
+ if (queryFields.unprotectedField?.resolve) {
🧰 Tools

🪛 Biome (1.9.4)

[error] 173-173: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)
api/src/unraid-api/graph/graph.module.ts (3)

3-3: New imports introduced.

These newly added imports (e.g., UnauthorizedException, AuthModule, AuthZService, etc.) appear to support authentication and authorization features. Ensure each import is fully used or remove any that are not utilized in this module to avoid potential code clutter.

Also applies to: 6-6, 16-16, 20-20, 24-24

33-35: AuthZService injection.

You are injecting AuthZService but do not currently use it, as the corresponding transform logic is commented out. If this will be enabled soon, this is fine; otherwise consider removing AuthZService from the injection to keep the code clean.

52-58: Asynchronous context function.

Returning the raw req, connectionParams, and extra is sufficient for basic GraphQL operations. However, if you plan on verifying user tokens or setting additional properties (e.g., context.user), consider performing that here for centralized user context management.

api/src/unraid-api/graph/directives/auth.directive.ts (3)

13-112: transformResolvers function complexity.

This function is quite large and handles multiple permission checks, including the OWN_ANY logic. Consider splitting some of the inline checks into helper methods to keep the core logic readable and maintainable.

114-160: authSchemaTransformer logic.

Attaching permission information to field extensions provides a clean pattern for storing authorization metadata. The appended text to each field's description is helpful for documentation. If you want to avoid console warnings, use a dedicated logger or a more formal logging system to unify log management.

196-220: createGraphQLEnumFromTSEnum utility.

While currently not used for the Auth enums, this generic approach could be handy for other internal enums. If it remains unused, consider removing or documenting future usage to avoid confusion.

api/src/graphql/schema/types/vms/domain.graphql (1)

32-35: Vms type modifications.

Including an id and a domain array is helpful for identification and listing domain info. If each domain belongs to the broader “Vms” entity, confirm how “id” is used or generated. Uniqueness and naming consistency should be validated if used as a key in resolvers or DB queries.

Would you like me to propose a standard approach for generating unique IDs for these Vms entities?

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

17-20: Consider adding error handling or user feedback for VM operations.
Each VM operation returns only boolean. If the service fails to locate or operate on the VM, you might want to ensure the user receives meaningful error messages. One approach is to catch potential exceptions from vmsService and throw a more descriptive GraphQL error, or return additional status details.

Also applies to: 28-30, 38-40, 48-50, 58-60, 68-70, 78-80

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between e9ee14a and 9f6946e.

📒 Files selected for processing (10)

api/codegen.ts (4 hunks)
api/src/graphql/schema/types/vms/domain.graphql (1 hunks)
api/src/unraid-api/app/app.module.ts (2 hunks)
api/src/unraid-api/auth/auth.interceptor.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.spec.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.ts (1 hunks)
api/src/unraid-api/graph/graph.module.ts (3 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (2 hunks)
api/src/unraid-api/graph/utils/auth-enum.utils.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

api/codegen.ts
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts
api/src/unraid-api/app/app.module.ts

🧰 Additional context used

🧬 Code Definitions (3)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (2)

api/src/unraid-api/graph/directives/auth.directive.ts (3)

getAuthEnumTypeDefs (165-194)

transformResolvers (13-112)

authSchemaTransformer (114-160)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

api/src/unraid-api/graph/directives/auth.directive.ts (1)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

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

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

api/src/graphql/schema/loadTypesDefs.ts (1)

loadTypeDefs (5-28)

🪛 Biome (1.9.4)

api/src/unraid-api/graph/directives/auth.directive.spec.ts

[error] 114-114: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 173-173: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (3)

GitHub Check: Test API
GitHub Check: Build Web App
GitHub Check: Cloudflare Pages

🔇 Additional comments (16)

api/src/unraid-api/auth/auth.interceptor.ts (1)

1-4: Imports look good.
These imports cleanly bring in the required NestJS and RxJS modules without any apparent redundancies.

api/src/unraid-api/graph/utils/auth-enum.utils.ts (3)

1-2: Imports are straightforward.
Pulling in the relevant AuthPossession and AuthActionVerb from nest-authz is correct.

9-13: Helper function logic is sound.
Filtering numeric keys and returning the Enum keys suits GraphQL type generation well.

16-35: SDL generation is concise and effective.
The returned GraphQL definitions for actions, possessions, and the @auth directive are properly formed.

api/src/unraid-api/graph/directives/auth.directive.spec.ts (3)

12-25: Mocks look appropriate.
This mock allows testing logic without depending on the real authorization service.

27-27: Confirm the intentional use of describe.skip.
Skipping tests may prevent future regressions from being caught. Ensure they're deliberately deactivated or remove .skip.

208-214: Multiple resolvers test is well-structured.
Verifying that an array of resolvers can be transformed ensures broader coverage of your schema transformation logic.
api/src/unraid-api/graph/graph.module.ts (3)

37-47: Merged typeDefs and resolvers.

Combining plugins.typeDefs with your authentication enums (authEnumTypeDefs) is a neat approach to unify schema definitions. Ensure that any directive definitions in authEnumTypeDefs are recognized for all schema resolvers. Also confirm that plugin-based resolvers do not conflict with your built-in ones.

65-66: Registering typeDefs and resolvers.

Specifying typeDefs and resolvers in the returned config looks correct. Double-check that all scalar definitions and directive declarations are applied consistently across the entire schema (including nested modules).

77-78:
✅ Verification successful

Providers array emptied & GraphQLModule exported.

Removing providers suggests these services are no longer needed here. Exporting GraphQLModule is a clear approach for reusability. Just confirm that any removed providers are not required elsewhere for dependency injection.

🏁 Script executed:
#!/bin/bash
# Verify references to removed providers across the codebase
rg 'providers:\s*\[' -A 5
Length of output: 20880
Graph Module Review: Providers Removed Appropriately & GraphQLModule Exported

The change to clear the providers array in api/src/unraid-api/graph/graph.module.ts appears intentional. Exporting GraphQLModule now promotes reusability while ensuring that dependency injection still functions correctly. The shell script output shows that providers required for dependency injection are still properly declared in their respective modules and tests, with no indication that any necessary providers were inadvertently removed from the graph module.

File: api/src/unraid-api/graph/graph.module.ts (Lines 77-78)

Providers array is empty.

GraphQLModule is exported for reuse.

Verification across the codebase confirms that providers for related services are defined elsewhere, ensuring that dependency injection remains intact.
api/src/unraid-api/graph/directives/auth.directive.ts (2)

1-12: Imports and file documentation.

These imports set the stage for wrapping authorization checks around your resolvers. The doc comments at lines 9-12 note the challenge in applying permissions to every field. Confirm that references in commented code align with the new directive logic before finalizing.

162-194: Generating Auth enum-type definitions.

This helper function elegantly translates TypeScript enums to GraphQL SDL. Ensure the final schema references these enums exactly where needed (e.g., your @auth directive) so as not to break schema validation if anything is missing.
api/src/graphql/schema/types/vms/domain.graphql (1)

11-26:
❓ Verification inconclusive

VmMutations type.

These methods (start, stop, pause, resume, forceStop, reboot, reset) are clear for VM lifecycle control. Validate that your resolvers and underlying logic handle each operation’s asynchronous nature and potential system-level errors (e.g., what happens if a VM is already paused and you call resumeVm?).

🏁 Script executed:
#!/bin/bash
# Query for usages of these new mutations in the codebase to ensure coverage in resolvers & tests
rg 'Mutation.*(startVm|stopVm|pauseVm|resumeVm|forceStopVm|rebootVm|resetVm)'
Length of output: 5532
Review Verification – Ensure Asynchronous & Error Handling Are Thoroughly Validated

The GraphQL schema changes for VmMutations are well integrated, as shown by their consistent presence in both the generated types and operations. However, please ensure that the corresponding resolver implementations and business logic correctly handle asynchronous behavior and potential system-level errors. For example, verify the proper response when a VM is already paused and a resumeVm mutation is invoked.

Confirm that each mutation’s resolver manages asynchronous operations appropriately.

Review error handling within these resolvers, especially for state-related conflicts.

Ensure that thorough tests cover these edge cases.
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts (3)

1-4: Imports look good and aligned with project structure.
The chosen imports from @nestjs/graphql and nest-authz appear appropriate, and including explicit paths to local modules ensures clarity.

8-10: Constructor usage is straightforward.
Injecting the VmsService via the constructor is an idiomatic and testable approach.

12-16: Consistent permission checks across VM operations.
All mutation methods consistently apply the @UsePermissions decorator with AuthActionVerb.UPDATE and Resource.VMS. This is a clean and uniform approach to authorization.

Also applies to: 22-26, 32-36, 42-46, 52-56, 62-66, 72-76

api/src/unraid-api/auth/auth.interceptor.ts

api/src/graphql/schema/types/vms/domain.graphql

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (5)

api/src/core/modules/array/update-array.ts (2)
17-17: Dynamic import conversion introduces potential performance tradeoff

The change from static import to dynamic import for arrayIsRunning has implications:

Benefits: Lazy loading could improve initial load time, might resolve circular dependencies

Concerns: Each call to updateArray will trigger this import, which could be less efficient than a one-time static import if called frequently

Consider adding a comment explaining why this specific import needs to be dynamic while others remain static.

34-36: Verify imported function is available before usage

The dynamic import occurs at the beginning of the function, but there's no error handling if the import fails. The function is then used immediately in line 35.

Add error handling for the dynamic import:
try {
    const { arrayIsRunning } = await import('@app/core/utils/array/array-is-running.js');
    
    // Rest of function using arrayIsRunning
} catch (error) {
    locked = false; // Release lock if there was an error during import
    throw new AppError('Failed to load required dependencies: ' + error.message);
}
api/src/unraid-api/graph/directives/auth.directive.spec.ts (3)
28-28: Consider removing .skip from the test suite.
All the tests within this describe.skip block are currently ignored. If you want to run these tests and gain coverage, remove .skip.
-describe.skip('Auth Directive', () => {
+describe('Auth Directive', () => {
116-116: Use optional chaining for cleaner checks.
The static analysis hints suggest using optional chaining in place of && for safety and improved readability.
- if (queryFields.protectedField && queryFields.protectedField.resolve) {
+ if (queryFields.protectedField?.resolve) {

- if (queryFields.unprotectedField && queryFields.unprotectedField.resolve) {
+ if (queryFields.unprotectedField?.resolve) {
Also applies to: 175-175

🧰 Tools

🪛 Biome (1.9.4)

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

97-156: Consider adding negative tests for unauthorized scenarios.
While you verify that permissions are set, it would be helpful to test a scenario where permissions fail and confirm that an UnauthorizedException (or similar) is raised. This would ensure full verification of the failure path.

🧰 Tools

🪛 Biome (1.9.4)

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 9f6946e and 21c86bf.

📒 Files selected for processing (2)

api/src/core/modules/array/update-array.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.spec.ts (1 hunks)

🧰 Additional context used

🧬 Code Definitions (1)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (2)

api/src/unraid-api/graph/directives/auth.directive.ts (3)

getAuthEnumTypeDefs (165-194)

transformResolvers (13-112)

authSchemaTransformer (114-160)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

🪛 Biome (1.9.4)

api/src/unraid-api/graph/directives/auth.directive.spec.ts

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 175-175: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (4)

GitHub Check: Build Web App
GitHub Check: Test API
GitHub Check: Build API
GitHub Check: Cloudflare Pages

🔇 Additional comments (4)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (4)

13-26: Verify that UsePermissions is correctly invoked in all tests.
Great approach mocking UsePermissions here. Double-check that each test scenario either expects or explicitly ignores calls to UsePermissions, to ensure the final test coverage is complete.

62-95: Excellent coverage of authSchemaTransformer.
Your test coverage for adding and verifying permission metadata in field descriptions and extensions is thorough.

206-217: Array of resolvers handling looks good.
The test ensures that multiple resolvers can be processed, which is crucial for modular GraphQL design.

219-240: Coverage of getAuthEnumTypeDefs is robust.
Verifying the generated SDL includes the expected enums and directive definitions ensures that the schema is well-formed.

api/src/core/modules/array/update-array.ts

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 21c86bf and 1b5db8a.

📒 Files selected for processing (12)

api/src/__test__/core/modules/array/add-disk-to-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/get-array-data.test.ts (0 hunks)
api/src/__test__/core/modules/array/remove-disk-from-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/update-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/update-parity-check.test.ts (0 hunks)
api/src/core/modules/array/add-disk-to-array.ts (0 hunks)
api/src/core/modules/array/index.ts (0 hunks)
api/src/core/modules/array/remove-disk-from-array.ts (0 hunks)
api/src/core/modules/array/update-array.ts (0 hunks)
api/src/core/modules/array/update-parity-check.ts (0 hunks)
api/src/core/modules/index.ts (0 hunks)
api/src/unraid-api/graph/resolvers/array/array.service.ts (2 hunks)

💤 Files with no reviewable changes (11)

api/src/test/core/modules/array/update-parity-check.test.ts
api/src/test/core/modules/array/add-disk-to-array.test.ts
api/src/test/core/modules/array/remove-disk-from-array.test.ts
api/src/core/modules/index.ts
api/src/test/core/modules/array/update-array.test.ts
api/src/core/modules/array/index.ts
api/src/test/core/modules/array/get-array-data.test.ts
api/src/core/modules/array/update-parity-check.ts
api/src/core/modules/array/add-disk-to-array.ts
api/src/core/modules/array/remove-disk-from-array.ts
api/src/core/modules/array/update-array.ts

🧰 Additional context used

🧬 Code Definitions (1)

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

api/src/core/utils/clients/emcmd.ts (1)

emcmd (12-41)

⏰ Context from checks skipped due to timeout of 90000ms (4)

GitHub Check: Build Web App
GitHub Check: Test API
GitHub Check: Build API
GitHub Check: Cloudflare Pages

🔇 Additional comments (1)

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

4-4: New import for GraphQL error handling.

Good addition of GraphQLError for better error handling in the new method.

api/src/unraid-api/graph/resolvers/array/array.service.ts

coderabbitai

Actionable comments posted: 1

🔭 Outside diff range comments (1)

api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (1)
1325-1342: 🛠️ Refactor suggestion

Fix array splice operation in nchanFocusStop function

The array splice operation in the catch block uses incorrect parameters.
        try {
          e.stop();
        } catch(err) {
-          newsub.splice(newsub.indexOf(e,1));
+          let index = newsub.indexOf(e);
+          if (index !== -1) {
+            newsub.splice(index, 1);
+          }
        }

♻️ Duplicate comments (1)

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

147-186: 🛠️ Refactor suggestion

Method updateParityCheck lacks return value and error handling.

This is similar to previous feedback: most methods in this service return Promise<ArrayType> whereas updateParityCheck returns nothing, which may confuse its usage. Also consider wrapping emcmd in a try/catch for consistent error handling, and add JSDoc to clarify expected behavior.

Below is a sample diff reflecting these improvements:

 async updateParityCheck({ wantedState, correct }: { wantedState: string; correct: boolean }): Promise<ArrayType> {
   const { getters } = await import('@app/store/index.js');
   const running = getters.emhttp().var.mdResync !== 0;
   const states = {
     pause: { cmdNoCheck: 'Pause' },
     resume: { cmdCheck: 'Resume' },
     cancel: { cmdNoCheck: 'Cancel' },
     start: { cmdCheck: 'Check' },
   };

   let allowedStates = Object.keys(states);

   if (running) {
     allowedStates = allowedStates.filter((state) => state !== 'start');
   }

   if (!allowedStates.includes(wantedState)) {
     throw new GraphQLError(`Invalid parity check state: ${wantedState}`);
   }

   const writeCorrectionsToParity = wantedState === 'start' && correct;

-  await emcmd({
-      startState: 'STARTED',
-      ...states[wantedState],
-      ...(writeCorrectionsToParity ? { optionCorrect: 'correct' } : {}),
-  });
+  try {
+    await emcmd({
+      startState: 'STARTED',
+      ...states[wantedState],
+      ...(writeCorrectionsToParity ? { optionCorrect: 'correct' } : {}),
+    });
+  } catch (error: unknown) {
+    throw new GraphQLError(`Failed to perform parity check: ${error instanceof Error ? error.message : 'Unknown error'}`);
+  }
+
+  return getArrayData();
 }

🧹 Nitpick comments (26)

api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (2)
1291-1303: Missing else clause in visibilitychange event handler

The first condition branch is missing the corresponding else clause to match the structure in the second branch.
  <? if ( $display['liveUpdate'] == "no" ):?>
  if (document.hidden) {
    nchanFocusStop();
-  } 
+  } else {
+    nchanFocusStart();
+  }
<?else:?>
1305-1323: Enhance error handling in nchanFocusStart function

The function tries to call pageFocusFunction() without checking if it exists first.
    try {
-      pageFocusFunction();
+      if (typeof pageFocusFunction === 'function') {
+        pageFocusFunction();
+      }
    } catch(error) {}
.github/workflows/main.yml (3)

50-52: Libvirt-daemon-system Package Addition to APT Cache
The libvirt-daemon-system package has been added to the cache list. Verify that this package is required for your libvirt setup and ensure it doesn’t conflict with libvirt-dev.

75-112: Comprehensive Libvirt Setup Step
This new “Setup libvirt” step is thorough—it creates required groups, configures the libvirt user, sets up directories with the proper permissions, appends configuration with a heredoc, and starts the libvirt daemon. A couple of suggestions:

The /var/run/libvirt directory is created and configured twice (once in line 86 and again in line 100); consider consolidating these commands to improve readability and maintainability.

Using || true with sudo virsh list --all might mask issues if the libvirt daemon fails to start. A more explicit check could help surface startup problems earlier.

363-363: Minor Formatting Adjustment
This line change appears to be a minor formatting tweak. Just ensure that any trailing whitespace or unintended blank lines do not affect the workflow’s execution.
api/src/unraid-api/graph/directives/auth.directive.spec.ts (2)
28-28: describe.skip will prevent these tests from running.
If the tests are ready, consider removing .skip so they’re included in the test suite.
- describe.skip('Auth Directive', () => {
+ describe('Auth Directive', () => {
116-116: Use optional chaining to simplify conditions.
Instead of checking (queryFields.protectedField && queryFields.protectedField.resolve), you can use optional chaining for cleaner code.
- if (queryFields.protectedField && queryFields.protectedField.resolve) {
+ if (queryFields.protectedField?.resolve) {
- if (queryFields.unprotectedField && queryFields.unprotectedField.resolve) {
+ if (queryFields.unprotectedField?.resolve) {
Also applies to: 175-175

🧰 Tools

🪛 Biome (1.9.4)

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (2)

35-53: Consider expanding test coverage.

The success and error scenarios for startVm are well-tested. As an optional enhancement, you might also test edge cases like an empty or invalid vmId.

55-73: Edge case coverage suggestion.

Similarly, for stopVm, you could add a test verifying behavior when vmId is undefined or invalid to improve overall test robustness.

api/src/unraid-api/graph/directives/auth.directive.ts (2)

10-11: Keep the TODO in sync with usage.

The comment indicates that transformResolvers doesn't correctly apply permission to every field. Ensure you revisit this once the approach is clarified, so the TODO remains actionable.

40-46: Replace console.log with a proper logger.

Using console.log can create noisy output and is less flexible than a logging library. Consider using Nest's built-in logger or another structured logging solution.

api/src/unraid-api/graph/graph.module.ts (3)

3-3: Unused import check.

UnauthorizedException is imported here but does not appear to be used in this file. Removing unused imports helps keep the code clean.

6-6: Unused import check.

ApolloServerPlugin is also imported but not referenced below. Confirm if you intend to introduce a custom Apollo server plugin; otherwise, consider removing it.

68-71: Commented out transformResolvers.

You have a TODO indicating a future plan for transformResolvers. Ensure that once you address the function's behavior in auth.directive.ts, you reintroduce and test it here.

api/src/unraid-api/graph/resolvers/vms/vms.service.ts (5)

23-35: Consider enhancing the PID file check for robust error handling.

If the PID file is missing or access is denied, the code treats libvirt as not running. Consider logging more details or distinguishing permission issues from an actual shutdown to provide clearer diagnostics.

77-136: Refactor long state transition logic for maintainability.

setVmState effectively implements multiple VM operations in one method. For long-term maintainability, consider refactoring these state-specific behaviors into smaller private methods (e.g., startDomain(), shutdownDomain()) to clarify each operation's flow.

159-173: Validate transitions with negative tests.

isValidStateTransition covers a finite set of known transitions. Adding negative tests (e.g., transitions from CRASHED → RUNNING) can catch corner-case defects before they occur in production.

210-237: Handle paused or crashed domain before reboot.

When issuing a reboot, the code gracefully shuts down a domain that’s expected to be running. If a domain is paused or crashed, consider calling an unpause or forced shutdown path to ensure a consistent reboot flow.

312-328: Increase or make configurable the shutdown retry limit.

waitForDomainShutdown will attempt shutdown ten times. In some environments, VMs may take longer. Consider making this limit configurable or providing a backoff strategy to reduce false timeouts.

api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (3)

54-63: Mock QEMU for CI reliability.

Directly checking the QEMU installation can cause flaky CI if QEMU is unavailable. Consider mocking or skipping integration tests in CI while keeping local integration tests for developers.

202-207: Confirm cleanup exceptions are surfaced.

The afterAll hook calls cleanupDomain, which can fail silently. Ensure errors are not lost in logs if CI/CD requires a fail-fast approach, or provide a thrown exception to highlight domain cleanup failures.

262-301: Validate stability under high VM count.

The test systematically starts and stops the VM with a short retry. In production, many VMs may exist simultaneously. Consider adding or adjusting test coverage for concurrency or higher domain counts to ensure performance.

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

8-9: Consider a dedicated “Mutation” resolver class.

Using @Resolver('VmMutations') with multiple @ResolveField methods is valid but slightly unconventional. A dedicated “Mutation” type might be more idiomatic for grouping these VM mutation fields straightforwardly.

18-80: Validate error handling consistency across all mutation methods.

Each mutation delegates to the VmsService. When an error occurs, the service throws a GraphQLError with distinct messages. Confirm each GraphQL field returns consistent, user-friendly error texts and that logs capture lower-level errors for diagnosis.

api/generated-schema.graphql (2)

1381-1577: User, System Variables & Account Types
The UnassignedDevice, Uptime, Usb, and the user-related types (User, UserAccount) along with the extensive Vars type covering system variables are very detailed. Given the volume of configuration fields in Vars, consider refactoring if this section grows further, to improve maintainability.

1727-1770: User Inputs & Registration for VM Context
The final inputs—enums for WAN access and forwarding, the Welcome type, user creation and deletion inputs (addUserInput, deleteUserInput), and the enums mdState and registrationType together with usersInput—are clearly specified. A special note: handle passwords in addUserInput securely (e.g. proper hashing, secure transport).

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 1b5db8a and 4d35828.

⛔ Files ignored due to path filters (3)

api/src/graphql/generated/api/operations.ts is excluded by !**/generated/**
api/src/graphql/generated/api/types.ts is excluded by !**/generated/**
pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (63)

.github/workflows/main.yml (3 hunks)
.gitignore (1 hunks)
api/codegen.ts (4 hunks)
api/dev/Unraid.net/myservers.cfg (1 hunks)
api/dev/Unraid.net/myservers.example.cfg (1 hunks)
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json (1 hunks)
api/generated-schema.graphql (1 hunks)
api/package.json (1 hunks)
api/src/__test__/core/modules/array/add-disk-to-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/get-array-data.test.ts (0 hunks)
api/src/__test__/core/modules/array/remove-disk-from-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/update-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/update-parity-check.test.ts (0 hunks)
api/src/__test__/graphql/resolvers/subscription/network.test.ts (1 hunks)
api/src/__test__/setup/config-setup.ts (1 hunks)
api/src/__test__/store/modules/paths.test.ts (1 hunks)
api/src/core/modules/array/add-disk-to-array.ts (0 hunks)
api/src/core/modules/array/index.ts (0 hunks)
api/src/core/modules/array/remove-disk-from-array.ts (0 hunks)
api/src/core/modules/array/update-array.ts (0 hunks)
api/src/core/modules/array/update-parity-check.ts (0 hunks)
api/src/core/modules/index.ts (0 hunks)
api/src/core/modules/vms/get-domains.ts (0 hunks)
api/src/core/modules/vms/index.ts (0 hunks)
api/src/core/utils/vms/get-hypervisor.ts (0 hunks)
api/src/core/utils/vms/index.ts (0 hunks)
api/src/core/utils/vms/parse-domain.ts (0 hunks)
api/src/core/utils/vms/parse-domains.ts (0 hunks)
api/src/graphql/schema/types/api-key/roles.graphql (1 hunks)
api/src/graphql/schema/types/base.graphql (1 hunks)
api/src/graphql/schema/types/vms/domain.graphql (1 hunks)
api/src/store/modules/paths.ts (1 hunks)
api/src/unraid-api/app/app.module.ts (2 hunks)
api/src/unraid-api/auth/api-key.service.spec.ts (3 hunks)
api/src/unraid-api/auth/api-key.service.ts (5 hunks)
api/src/unraid-api/auth/auth.interceptor.ts (1 hunks)
api/src/unraid-api/auth/auth.service.spec.ts (3 hunks)
api/src/unraid-api/auth/authentication.guard.ts (1 hunks)
api/src/unraid-api/auth/casbin/model.ts (1 hunks)
api/src/unraid-api/auth/casbin/policy.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.spec.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.ts (1 hunks)
api/src/unraid-api/graph/graph.module.ts (3 hunks)
api/src/unraid-api/graph/resolvers/array/array.service.ts (2 hunks)
api/src/unraid-api/graph/resolvers/online/online.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/resolvers.module.ts (2 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (2 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1 hunks)
api/src/unraid-api/graph/utils/auth-enum.utils.ts (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (5 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (5 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (4 hunks)
api/vite.config.ts (1 hunks)
web/codegen.ts (1 hunks)
web/composables/gql/graphql.ts (12 hunks)

💤 Files with no reviewable changes (17)

api/src/test/core/modules/array/update-array.test.ts
api/src/test/core/modules/array/add-disk-to-array.test.ts
api/src/test/core/modules/array/remove-disk-from-array.test.ts
api/src/core/modules/vms/index.ts
api/src/test/core/modules/array/update-parity-check.test.ts
api/src/core/modules/index.ts
api/src/core/utils/vms/index.ts
api/src/core/modules/array/index.ts
api/src/core/utils/vms/get-hypervisor.ts
api/src/core/modules/vms/get-domains.ts
api/src/core/utils/vms/parse-domains.ts
api/src/core/modules/array/update-array.ts
api/src/core/modules/array/update-parity-check.ts
api/src/core/modules/array/remove-disk-from-array.ts
api/src/core/modules/array/add-disk-to-array.ts
api/src/core/utils/vms/parse-domain.ts
api/src/test/core/modules/array/get-array-data.test.ts

🚧 Files skipped from review as they are similar to previous changes (33)

api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/Notifications.page.last-download-time
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/.login.php.last-download-time
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/auth-request.php.last-download-time
api/vite.config.ts
.gitignore
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php.last-download-time
api/src/unraid-api/auth/casbin/model.ts
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch
api/src/graphql/schema/types/base.graphql
api/dev/Unraid.net/myservers.example.cfg
api/src/test/setup/config-setup.ts
api/src/unraid-api/graph/utils/auth-enum.utils.ts
api/package.json
api/dev/Unraid.net/myservers.cfg
api/src/unraid-api/graph/resolvers/vms/vms.resolver.spec.ts
api/src/unraid-api/app/app.module.ts
api/src/unraid-api/auth/authentication.guard.ts
api/src/test/store/modules/paths.test.ts
api/src/unraid-api/auth/api-key.service.ts
api/src/unraid-api/auth/auth.interceptor.ts
web/codegen.ts
api/src/unraid-api/graph/resolvers/online/online.resolver.ts
api/src/test/graphql/resolvers/subscription/network.test.ts
api/src/unraid-api/unraid-file-modifier/modifications/test/snapshots/DefaultPageLayout.php.modified.snapshot.php
api/src/unraid-api/auth/casbin/policy.ts
api/src/store/modules/paths.ts
api/src/graphql/schema/types/api-key/roles.graphql
api/src/unraid-api/auth/auth.service.spec.ts
api/src/unraid-api/graph/resolvers/resolvers.module.ts
api/src/graphql/schema/types/vms/domain.graphql
api/src/unraid-api/auth/api-key.service.spec.ts
api/codegen.ts

🧰 Additional context used

🧬 Code Definitions (8)

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

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

Resolver (8-81)

api/src/unraid-api/graph/directives/auth.directive.ts (1)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

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

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

domain (26-35)

api/src/unraid-api/graph/resolvers/vms/vms.service.ts (3)

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

domain (26-35)

web/composables/gql/graphql.ts (1)

VmDomain (1867-1874)

api/src/graphql/generated/api/types.ts (1)

VmDomain (1839-1846)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (2)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

api/src/unraid-api/graph/directives/auth.directive.ts (3)

getAuthEnumTypeDefs (165-194)

transformResolvers (13-112)

authSchemaTransformer (114-160)

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

api/src/core/utils/clients/emcmd.ts (1)

emcmd (12-41)

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

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

api/src/graphql/schema/loadTypesDefs.ts (1)

loadTypeDefs (5-28)

web/composables/gql/graphql.ts (1)

api/src/graphql/generated/api/types.ts (22)

ArrayDiskInput (194-199)

Scalars (14-26)

InputMaybe (6-6)

ArrayMutations (233-244)

Maybe (5-5)

ArrayType (112-131)

Disk (494-515)

ArrayMutationsaddDiskToArrayArgs (247-249)

ArrayMutationsclearArrayDiskStatisticsArgs (252-254)

ArrayMutationsmountArrayDiskArgs (257-259)

ArrayMutationsremoveDiskFromArrayArgs (262-264)

ArrayMutationssetStateArgs (267-269)

ArrayStateInput (312-315)

ArrayMutationsunmountArrayDiskArgs (272-274)

VmMutations (1848-1864)

VmMutationsforceStopVmArgs (1867-1869)

VmMutationspauseVmArgs (1872-1874)

VmMutationsrebootVmArgs (1877-1879)

VmMutationsresetVmArgs (1882-1884)

VmMutationsresumeVmArgs (1887-1889)

VmMutationsstartVmArgs (1892-1894)

VmMutationsstopVmArgs (1897-1899)

🪛 GitHub Actions: CI - Main (API)

api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts

[warning] 1-1: Domain shutdown timed out after 10 attempts.

🪛 Biome (1.9.4)

api/src/unraid-api/graph/directives/auth.directive.spec.ts

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 175-175: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (1)

GitHub Check: Cloudflare Pages

🔇 Additional comments (46)

api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (5)

108-109: New monitor method for NchanSubscriber looks good

This new method allows tracking all subscriber instances by adding them to a global array, making it possible to manage all subscribers collectively.

114-115: Good initialization of subscribers array

This array will store all the subscriber instances that are being monitored.

761-761: Smart addition of nchanPaused check for page reload

Integrating the new focus management by only reloading when updates aren't paused.

1273-1276: Good implementation of focus management variables

The initialization of nchanPaused and blurTimer variables provides the foundation for the focus management functionality.

1278-1289: Window focus/blur event handlers look good

Setting up the focus event and conditional blur event (based on liveUpdate setting) is a good approach for managing updates when the window changes state.

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

7-7: Import statement for VmsService looks correct.
No issues are found with the import path or naming.

11-12: Nice use of constructor injection for VmsService.
This approach follows NestJS best practices for dependency injection and keeps the resolver lean.

28-28: Good refactor to delegate domain retrieval to VmsService.
It's a clear separation of concerns, and the try/catch block ensures exceptions are handled properly.

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

4-4: GraphQLError import is fine.
Ensure consistent use of GraphQLError or a custom error class for clarity and maintainability.

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

9-11: Good structure for testing.

Declaring the resolver and service as variables inside the describe block is clear and accessible. Nicely done!

13-29: Effective use of NestJS testing module.

Your beforeEach block properly initializes the testing module and injects the mocked VmsService. Great job ensuring each test starts with a fresh setup.

api/src/unraid-api/graph/directives/auth.directive.ts (1)

55-82: Confirm the logic for OWN_ANY.

Currently, possession OWN_ANY sets required permissions for both ANY and OWN, but the code calls enforce only once with ANY. If the ANY check fails, we never check OWN. Verify if this approach is intended or if a second permission check is needed.

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

33-46: Integration of AuthModule and AuthZService is well done.

The approach of injecting AuthZService alongside PluginService appropriately sets the foundation for advanced permission checks. Good arrangement here!

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

304-306: Align error message checks with expected service text.

The test expects precise substrings for invalid UUID errors ("Failed to set VM state: Invalid UUID"). Ensure that the service consistently throws these to keep the test suite accurate and prevent brittle checks.

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

12-16: Verify permission checks in external integrations.

You use @UsePermissions for each VM operation. Ensure that external calls (like from a web UI or CLI) likewise follow or enforce the permission model so that unauthorized usage is comprehensively blocked.

web/composables/gql/graphql.ts (12)

197-202: Well-structured input type with clear field documentation.

The ArrayDiskInput type is well-defined with proper documentation for each field.

236-247: Good definition of array mutations with clear documentation.

The ArrayMutations type properly encapsulates all array-related operations. The documentation for each field is clear and helpful, especially the note about array state requirements.

250-277: Well-organized argument types for array mutations.

The argument types for array mutations follow a consistent pattern and are properly typed.

315-325: Clean enum representation for array state input.

The ArrayStateInput and ArrayStateInputState types are well-structured with clear documentation.

327-340: Good authorization enums for permission control.

The AuthActionVerb and AuthPossession enums provide a clear structure for authorization controls across the application.

576-576: Docker type enhanced with mutation capabilities.

Adding the mutations field to the Docker type provides a consistent API pattern for container management.

600-615: Docker mutations defined with appropriate arguments.

The Docker mutations for starting and stopping containers are well-defined with proper argument types.

876-885: Clear documentation of VM mutations with required permissions.

The vms field in the main Mutation type is properly documented with the required permissions, making the API more self-explanatory.

1384-1412: Consistent uppercase format for Resource enum values.

The Resource enum values have been standardized to use uppercase, improving consistency across the codebase.

1416-1419: Consistent uppercase format for Role enum values.

The Role enum values have been standardized to match the uppercase format used in other enums.

1876-1948: Comprehensive VM mutation operations with proper documentation.

The VmMutations type provides a complete set of VM control operations, each with clear documentation of required permissions.

1951-1984: Consistent argument structure for VM operations.

Each VM operation has a well-defined argument type that follows a consistent pattern, using the VM ID as the identifier.

api/generated-schema.graphql (19)

1-15: Authentication & Access URL Types Defined
The new @auth directive and the AccessUrl and AccessUrlInput types are well defined. They clearly specify required fields (using the custom scalar URL and enum URL_TYPE). Ensure that the resolvers later enforce these authentication directives consistently.

17-34: Permission and Role Input Types
The inputs for permissions (AddPermissionInput) and role assignment (AddRoleForApiKeyInput, AddRoleForUserInput) along with the allowed origins input (AllowedOriginInput) are structured clearly. Consider adding more descriptive comments on each field if further documentation is needed.

36-85: API Key & API Settings Definitions
The ApiKey, ApiKeyResponse, ApiKeyWithSecret types and the ApiSettingsInput input are comprehensive. The inline documentation in ApiSettingsInput aids clarity; just verify that validations (for example, ensuring a port is provided when using STATIC forwarding) are implemented in the resolvers.

87-325: Array Types & Mutations
The array-related types—Array, ArrayCapacity, ArrayDisk (with detailed field comments), enums like ArrayDiskStatus, ArrayDiskType, and the mutations under ArrayMutations—are very detailed. Please double-check that the enum values (and the note about parity or cache slot numbering) match business logic.

327-390: Authentication Enums and Hardware/Cloud Config Types
The enums AuthActionVerb and AuthPossession are concise and clear. Hardware types like Baseboard, Capacity, and Case as well as cloud-related types (Cloud, CloudResponse) and Config with its error enum are appropriately defined. Confirm that optional fields are handled correctly downstream.

392-438: Connect Types and User Sign-In Inputs
The Connect, ConnectSettings, and ConnectSettingsValues types—with accompanying inputs ConnectSignInInput and ConnectUserInfoInput—capture connection configuration well. Be sure to enforce proper validations on JSON fields in your resolvers.

440-470: Container-related Types
The definitions for ContainerHostConfig, ContainerMount, ContainerPort (plus its enum ContainerPortType) and the ContainerState enum are straightforward. Consider adding detailed documentation on expected formats or constraints (e.g. for networkMode and mount paths) if not already covered elsewhere.

472-483: API Key Creation Input & Scalar DateTime
The CreateApiKeyInput clearly exposes fields such as overwrite, permissions, and roles. The declaration of the DateTime scalar is standard. If custom parsing or validation is used, ensure that it is robust against invalid formats.

485-562: Disk, Devices, and Display Types
The Devices, Disk (with extensive field documentation), and Display types are meticulously detailed. Verify that numeric types (like Long and Int) correctly represent their intended values and that field descriptions accurately match backend data.

564-629: Docker & Dynamic Remote Access Types
The Docker, DockerContainer, DockerMutations, and DockerNetwork types, along with DynamicRemoteAccessStatus, its enum, and the EnableDynamicRemoteAccessInput are defined comprehensively. It might be useful to later add constraints or examples for JSON subfields (e.g. container labels) to aid client developers.

631-710: System Info & Hardware Components
The definitions for Flash, Gpu, and the Importance enum, followed by the rich system information types (Info, InfoApps, InfoCpu, InfoMemory), are detailed. Ensure that the data collected from underlying systems can be reliably mapped to these types.

713-804: Logging, User, Memory, & Minigraph Types
The scalar JSON, KeyFile, LogFile, and LogFileContent types support logging functionality well. The user type Me, along with memory-related enums (MemoryFormFactor, MemoryType) and the MemoryLayout type, plus minigraph types (MinigraphStatus, MinigraphqlResponse) and Mount—all are clearly declared.

806-883: Mutation & Network Definitions
The central Mutation type covers a wide array of operations, and the Network type is defined with multiple network-related fields. Given the breadth of mutations, ensure that each has proper resolver logic and security checks. Consider modularizing mutations if the list grows too long.

889-1034: Notification, OS, Owner & Partition Types
The notification-related types (Notification, NotificationCounts, NotificationData, NotificationFilter, NotificationOverview, and NotificationType) are comprehensive. Likewise, the Os, Owner, ParityCheck, and Partition types are defined in detail. Ensure that the Partition schema aligns with actual disk partition data.

1035-1192: PCI, Permission, ProfileModel, Query & Registration Types
The Pci and Permission types, along with the ProfileModel and an extensive Query type, provide a strong foundation for data retrieval. The Registration type and its corresponding state enum add useful detail. Confirm that each query and registration state is correctly handled in the backend.

1194-1248: Relay, RemoteAccess, Role Removal & Access Control
The RelayResponse, RemoteAccess, RemoveRoleFromApiKeyInput, and the enums Resource and Role are clearly defined. These will help ensure that permissions and resource accesses are strictly controlled.

1250-1274: Server & Service Types
The Server type (with its server status details) and the Service type are concise and clearly documented. Additional context on status transitions might be beneficial during the implementation phase.

1276-1379: Remote Access, Share, Subscription & System Settings
The input for setting up remote access, the Share type (with detailed disk inclusion/exclusion and size information), the Subscription type for real-time updates, and system-level types (System, Temperature, Theme, plus scalars for URL and UUID and the URL_TYPE enum) are all well constructed.

1649-1726: Versions & Virtual Machine Types
The Versions type that aggregates software versions and the VM types (VmDomain, VmMutations, VmState, and Vms) are defined with clarity. Ensure that virtual machine state transitions follow strict validation rules in the business logic.

api/src/unraid-api/graph/resolvers/vms/vms.service.ts

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

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

37-49: Consider surfacing errors more aggressively on initialization.
When onModuleInit encounters an error, the service logs a warning and sets isVmsAvailable = false but does not rethrow. If VM connectivity is critical, consider halting the application startup. Otherwise, confirm that all callers properly handle the “unavailable” state to avoid unexpected runtime errors.

311-327: Consider exponential backoff instead of fixed wait.
Using a simple loop and sleeping 1 second per iteration is fine for small-time usage. If domain shutdown times vary significantly, consider an exponential backoff or a more responsive callback model.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 4d35828 and 703dc0b.

📒 Files selected for processing (1)

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

🧰 Additional context used

🧬 Code Definitions (1)

api/src/unraid-api/graph/resolvers/vms/vms.service.ts (3)

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

domain (26-35)

web/composables/gql/graphql.ts (1)

VmDomain (1867-1874)

api/src/graphql/generated/api/types.ts (1)

VmDomain (1839-1846)

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: Build Web App
GitHub Check: Cloudflare Pages

🔇 Additional comments (11)

api/src/unraid-api/graph/resolvers/vms/vms.service.ts (11)

1-22: Looks good for imports and constructor.
No concerns here; the constructor and import statements are succinct and straightforward.

23-36: Confirm handling of missing libvirt-pid path.
If getters.paths()['libvirt-pid'] does not return a valid path (e.g., user environment not configured properly), this method will silently fall back to false. Ensure that the rest of the application gracefully handles the scenario when libvirt is not running or the PID file is missing.

51-74: Hypervisor initialization flow looks solid.
The step-by-step checks for libvirt availability and connectivity are logical. The error logging is thorough, which aids in diagnosing any connectivity issues.

76-135: Potential mismatch with test expectations.
As noted in past reviews, rethrowing an error as "Failed to set VM state" might not align with tests that specifically expect "Failed to start VM" or a state-specific message. If your test suite checks the error message for "Failed to start VM," consider catching the error and rethrowing a more specific one for better clarity.

137-156: Mapping domain states to VM states is correct.
All known libvirt states are handled, including edge cases like BLOCKED → IDLE and CRASHED.

158-172: Valid state transitions definition appears consistent.
The transition rules are clear and reduce the likelihood of invalid moves from one VM state to another.

174-188: Single-line methods delegate neatly to setVmState.
The approach of reusing setVmState ensures DRY logic for start, stop, pause, and resume.

190-207: Force-stop logic is acceptable.
Force stopping calls domain.destroy(), which aligns with the libvirt docs for forcibly terminating a domain.

209-236: Graceful reboot strategy is well-handled.
Shutting down and waiting for the domain to fully stop before re-creating it helps ensure a cleaner restart.

238-259: Reset logic is clear.
Stopping and starting the domain is a valid approach for “reset.” No issues detected here.

261-309: Domain retrieval is well-structured.
Logging domain names and mapping their states into VmDomain objects provides good visibility. Fallback to isVmsAvailable = false on error is consistent with the rest of the service.

coderabbitai

Actionable comments posted: 2

♻️ Duplicate comments (2)

api/src/unraid-api/graph/resolvers/array/array.service.ts (1)
146-186: 🛠️ Refactor suggestion

Consider aligning with previously suggested improvements for error handling and returning data.

We've recommended these changes before, so this is a . Overall, the method would benefit from:

Providing error handling for the emcmd call, wrapping it in a try-catch to throw a user-friendly error message if something fails.

Returning a Promise<ArrayType> (like other service methods here) to ensure consistency in the array update flow.

Documenting method parameters via JSDoc.

Using typed states (e.g., an enum) instead of raw string literals for wantedState.

Here's a partial diff illustrating these changes:
 async updateParityCheck({ wantedState, correct }: { wantedState: 'pause' | 'resume' | 'cancel' | 'start'; correct: boolean }): Promise<ArrayType> {
   const { getters } = await import('@app/store/index.js');
   const running = getters.emhttp().var.mdResync !== 0;
   const states = {
     /* existing code */
   };

   // Ensure we wrap emcmd in try/catch
-  await emcmd({
+  try {
+    await emcmd({
       startState: 'STARTED',
       ...states[wantedState],
       ...(wantedState === 'start' && correct ? { optionCorrect: 'correct' } : {}),
     });
+  } catch (err) {
+    throw new GraphQLError(`Failed to update parity check: ${err.message}`);
+  }

+  // Return updated array data to remain consistent
+  return getArrayData();
 }
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1)
23-49: 🛠️ Refactor suggestion

Ensure robust error handling on initialization.

When isLibvirtRunning fails, it sets isVmsAvailable to false, but the user might not discover the inoperability until they call a VM method. Consider re-throwing or storing a more descriptive error reason to help debug.
- this.isVmsAvailable = false;
+ this.isVmsAvailable = false;
+ this.libvirtErrorReason = error instanceof Error ? error.message : 'Unknown libvirt error';

🧹 Nitpick comments (19)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (3)
28-28: Tests are skipped—verify if intended.
The .skip on the entire test suite means these tests won't run. Remove .skip if you would like to include them in CI/CD or local test coverage.

116-116: Use optional chaining to simplify nested checks.
You can replace:
if (queryFields.protectedField && queryFields.protectedField.resolve) {
with:
-if (queryFields.protectedField && queryFields.protectedField.resolve) {
+if (queryFields.protectedField?.resolve) {
This is a minor improvement as flagged by the static analysis hints.

🧰 Tools

🪛 Biome (1.9.4)

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

175-175: Use optional chaining to simplify nested checks.
Similarly, replace:
if (queryFields.unprotectedField && queryFields.unprotectedField.resolve) {
with:
-if (queryFields.unprotectedField && queryFields.unprotectedField.resolve) {
+if (queryFields.unprotectedField?.resolve) {
to streamline your conditional checks.

🧰 Tools

🪛 Biome (1.9.4)

[error] 175-175: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)
api/src/unraid-api/graph/directives/auth.directive.ts (3)
13-16: Clarify function usage or remove comment blockers.

The comment indicates that transformResolvers might be partially incomplete or experimental. If it remains in production code, consider removing the @wip and @todo or clarify the plan to finalize it soon.

38-47: Avoid excessive console logs in production code.

Repeated console.log statements could clutter production logs. If the intention is to debug, consider using a logger with appropriate severity levels.
- console.log('resolving', info.fieldName, info.parentType.name, ...)
- console.log('user', context?.req?.user);
+ authLogger.debug(`Resolving: ${info.parentType.name}.${info.fieldName}`);
+ authLogger.debug(`User: ${JSON.stringify(context?.req?.user)}`);
200-219: Consider removing unused helper or demonstrating usage.

createGraphQLEnumFromTSEnum is not used for the auth enums. If it’s genuinely useful for other scenarios, demonstrate an example usage in code or remove it to avoid confusion.
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (3)

1-21: Provide clarity on environment defaults.

The constructor uses qemu:///system as a default URI. This is sensible, but make sure the environment variable LIBVIRT_URI is well-documented to avoid confusion in dev/test environments.

96-107: Handle unexpected domain states more gracefully.

When the domain is neither RUNNING nor PAUSED but the target is RUNNING, the code doesn't raise an immediate error if the current state is something else like IDLE or SHUTDOWN in certain edge cases. Consider adding a fallback or re-check domain states for safety.

311-327: Consider user-configurable wait times.

waitForDomainShutdown retries 10 times with a 1-second interval. For large or heavily loaded VMs, 10 seconds might be insufficient. Provide a config or environment variable to handle extended shutdown grace periods.

api/src/graphql/schema/types/vms/domain.graphql (1)

6-9: Consider a more expressive return type for vms mutations.

These fields return Boolean only, which might limit insight into any failure causes or updated VM state. If future expansions require additional data, you’ll have to perform a breaking change.
api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (3)
93-138: Consider centralizing the domain cleanup logic.

The block for finding, destroying, and undefining the domain is robust but verbose. Centralizing it into a helper method, or reusing the existing cleanupDomain in more scenarios, could help avoid repeating similar steps in the test setup, reducing complexity and improving readability.

233-246: Extract polling logic into a shared helper.

You’re using similar retry loops to wait for the VM to be found or to transition states. Extracting this into a small helper function (e.g., waitForDomainState(domainName, expectedState)) will make the tests more concise and readable. It also makes future expansions or modifications to the polling mechanism easier.
-            while (retries < maxRetries && !testVm) {
-                ...
-            }
+            await waitForDomainState(TEST_VM_NAME, 'SHUTOFF', maxRetries, 1000);
Also applies to: 270-281, 289-299

62-63: Add more descriptive error handling and logging.

Currently, certain steps (e.g., verifying QEMU and libvirt connection) throw or log general errors. Consider including additional details that can help pinpoint issues more quickly, such as environment variables, platform details, or resource availability.

Also applies to: 177-190
web/composables/gql/graphql.ts (1)
1388-1416: Consider avoiding double underscores in enum values.

The CONNECT__REMOTE_ACCESS resource name contains two underscores, which may be unconventional. A single underscore is typically sufficient for separation.

Here’s a potential change:
-  ConnectRemoteAccess = 'CONNECT__REMOTE_ACCESS',
+  ConnectRemoteAccess = 'CONNECT_REMOTE_ACCESS',
api/generated-schema.graphql (5)

1-1776: Overall Schema Quality and Coverage
This new GraphQL schema is impressively comprehensive. It introduces a wide range of types, inputs, enums, directives, queries, and mutations that cover key areas such as authentication (with the new @auth directive), API key management, array and disk configuration, Docker and system monitoring, and importantly, basic virtual machine controls. The field definitions generally include inline documentation which helps clarify intent. Please ensure that all input constraints and business rules implied by the schema (for example, in ApiSettingsInput or ArrayStateInput) are also enforced in the resolver logic.

1684-1715: VM Controls Definitions
The definitions for VmDomain, VmMutations, and VmState effectively capture the basic operations needed for controlling virtual machines (start, stop, pause, reboot, etc.). This aligns well with the PR objective of “basic vm controls.” Just be sure that the associated resolver implementations include appropriate validation and robust error handling to prevent unexpected behavior during state transitions.

60-88: API Settings Input Documentation
The ApiSettingsInput type is documented with multi-line comments that clearly explain the nuances of each field (e.g., the conditions under which certain fields are required). This clarity is valuable, but consider double-checking that any non-obvious constraints are also mirrored in the validation logic on the backend.

812-872: Mutation Type Composition
The Mutation type aggregates a large number of operations spanning different domains (permissions, notifications, array operations, VM controls, etc.). While it’s comprehensive, you might consider whether splitting some of these mutation fields into dedicated sub-types or modules would improve maintainability and clarity for future developers. Even if you choose to keep them together, adding brief inline descriptions or grouping related mutations with comments could be beneficial.

980-1039: Partition Type Complexity
The Partition type lists a very large number of fields, which makes it quite complex. Although this level of detail might be necessary for your use case, consider reviewing the naming conventions and whether some of the related fields could be grouped into nested input or object types. This refactor might help improve readability and maintainability in the long run.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between f34c6d3 and 9335a47.

⛔ Files ignored due to path filters (3)

api/src/graphql/generated/api/operations.ts is excluded by !**/generated/**
api/src/graphql/generated/api/types.ts is excluded by !**/generated/**
pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (63)

.github/workflows/main.yml (3 hunks)
.gitignore (1 hunks)
api/codegen.ts (4 hunks)
api/dev/Unraid.net/myservers.cfg (1 hunks)
api/dev/Unraid.net/myservers.example.cfg (1 hunks)
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json (1 hunks)
api/generated-schema.graphql (1 hunks)
api/package.json (1 hunks)
api/src/__test__/core/modules/array/add-disk-to-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/get-array-data.test.ts (0 hunks)
api/src/__test__/core/modules/array/remove-disk-from-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/update-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/update-parity-check.test.ts (0 hunks)
api/src/__test__/graphql/resolvers/subscription/network.test.ts (1 hunks)
api/src/__test__/setup/config-setup.ts (1 hunks)
api/src/__test__/store/modules/paths.test.ts (1 hunks)
api/src/core/modules/array/add-disk-to-array.ts (0 hunks)
api/src/core/modules/array/index.ts (0 hunks)
api/src/core/modules/array/remove-disk-from-array.ts (0 hunks)
api/src/core/modules/array/update-array.ts (0 hunks)
api/src/core/modules/array/update-parity-check.ts (0 hunks)
api/src/core/modules/index.ts (0 hunks)
api/src/core/modules/vms/get-domains.ts (0 hunks)
api/src/core/modules/vms/index.ts (0 hunks)
api/src/core/utils/vms/get-hypervisor.ts (0 hunks)
api/src/core/utils/vms/index.ts (0 hunks)
api/src/core/utils/vms/parse-domain.ts (0 hunks)
api/src/core/utils/vms/parse-domains.ts (0 hunks)
api/src/graphql/schema/types/api-key/roles.graphql (1 hunks)
api/src/graphql/schema/types/base.graphql (1 hunks)
api/src/graphql/schema/types/vms/domain.graphql (1 hunks)
api/src/store/modules/paths.ts (1 hunks)
api/src/unraid-api/app/app.module.ts (2 hunks)
api/src/unraid-api/auth/api-key.service.spec.ts (3 hunks)
api/src/unraid-api/auth/api-key.service.ts (5 hunks)
api/src/unraid-api/auth/auth.interceptor.ts (1 hunks)
api/src/unraid-api/auth/auth.service.spec.ts (3 hunks)
api/src/unraid-api/auth/authentication.guard.ts (1 hunks)
api/src/unraid-api/auth/casbin/model.ts (1 hunks)
api/src/unraid-api/auth/casbin/policy.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.spec.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.ts (1 hunks)
api/src/unraid-api/graph/graph.module.ts (3 hunks)
api/src/unraid-api/graph/resolvers/array/array.service.ts (2 hunks)
api/src/unraid-api/graph/resolvers/online/online.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/resolvers.module.ts (2 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (2 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1 hunks)
api/src/unraid-api/graph/utils/auth-enum.utils.ts (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (5 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (5 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (2 hunks)
api/vite.config.ts (1 hunks)
web/codegen.ts (1 hunks)
web/composables/gql/graphql.ts (7 hunks)

💤 Files with no reviewable changes (17)

api/src/test/core/modules/array/update-array.test.ts
api/src/test/core/modules/array/add-disk-to-array.test.ts
api/src/test/core/modules/array/remove-disk-from-array.test.ts
api/src/test/core/modules/array/update-parity-check.test.ts
api/src/core/modules/vms/index.ts
api/src/core/utils/vms/get-hypervisor.ts
api/src/core/utils/vms/index.ts
api/src/core/modules/array/index.ts
api/src/core/utils/vms/parse-domains.ts
api/src/core/modules/index.ts
api/src/core/modules/array/remove-disk-from-array.ts
api/src/test/core/modules/array/get-array-data.test.ts
api/src/core/utils/vms/parse-domain.ts
api/src/core/modules/array/update-parity-check.ts
api/src/core/modules/vms/get-domains.ts
api/src/core/modules/array/add-disk-to-array.ts
api/src/core/modules/array/update-array.ts

🚧 Files skipped from review as they are similar to previous changes (37)

api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php.last-download-time
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/Notifications.page.last-download-time
api/vite.config.ts
api/src/graphql/schema/types/base.graphql
.gitignore
api/dev/Unraid.net/myservers.cfg
api/package.json
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/auth-request.php.last-download-time
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/.login.php.last-download-time
api/src/store/modules/paths.ts
api/src/test/store/modules/paths.test.ts
web/codegen.ts
api/src/unraid-api/auth/auth.interceptor.ts
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json
api/dev/Unraid.net/myservers.example.cfg
api/src/unraid-api/auth/casbin/policy.ts
.github/workflows/main.yml
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch
api/src/unraid-api/graph/resolvers/vms/vms.resolver.spec.ts
api/src/unraid-api/graph/utils/auth-enum.utils.ts
api/src/unraid-api/app/app.module.ts
api/src/unraid-api/auth/api-key.service.ts
api/src/unraid-api/auth/casbin/model.ts
api/src/test/graphql/resolvers/subscription/network.test.ts
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php
api/src/unraid-api/auth/auth.service.spec.ts
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts
api/src/unraid-api/graph/resolvers/resolvers.module.ts
api/src/unraid-api/auth/authentication.guard.ts
api/src/graphql/schema/types/api-key/roles.graphql
api/src/unraid-api/auth/api-key.service.spec.ts
api/src/unraid-api/graph/resolvers/online/online.resolver.ts
api/src/test/setup/config-setup.ts
api/src/unraid-api/unraid-file-modifier/modifications/test/snapshots/DefaultPageLayout.php.modified.snapshot.php
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts
api/codegen.ts
api/src/unraid-api/graph/graph.module.ts

🧰 Additional context used

🧬 Code Definitions (6)

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

api/src/core/utils/clients/emcmd.ts (1)

emcmd (12-41)

api/src/unraid-api/graph/directives/auth.directive.ts (1)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

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

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

domain (26-35)

api/src/unraid-api/graph/resolvers/vms/vms.service.ts (3)

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

domain (26-35)

web/composables/gql/graphql.ts (1)

VmDomain (1871-1878)

api/src/graphql/generated/api/types.ts (1)

VmDomain (1843-1850)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (2)

api/src/unraid-api/graph/directives/auth.directive.ts (3)

getAuthEnumTypeDefs (165-194)

transformResolvers (13-112)

authSchemaTransformer (114-160)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

web/composables/gql/graphql.ts (1)

api/src/graphql/generated/api/types.ts (10)

Maybe (5-5)

VmMutations (1852-1868)

Scalars (14-26)

VmMutationsforceStopVmArgs (1871-1873)

VmMutationspauseVmArgs (1876-1878)

VmMutationsrebootVmArgs (1881-1883)

VmMutationsresetVmArgs (1886-1888)

VmMutationsresumeVmArgs (1891-1893)

VmMutationsstartVmArgs (1896-1898)

VmMutationsstopVmArgs (1901-1903)

🪛 Biome (1.9.4)

api/src/unraid-api/graph/directives/auth.directive.spec.ts

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 175-175: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (1)

GitHub Check: Cloudflare Pages

🔇 Additional comments (15)

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

4-4: No issues with the new import.
This import of GraphQLError seems valid and doesn't introduce any regressions.

api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (1)

1-75: Good test coverage for VM mutations.
These tests thoroughly validate startVm and stopVm methods, including error cases. This is clear and maintainable.
api/src/unraid-api/graph/directives/auth.directive.ts (2)

53-82: Check concurrency or repeated permission checks.

OWN_ANY triggers two distinct checks but only enforces once at line 70. If multiple user requests are processed in parallel, ensure there's no concurrency gap or race condition where the user context changes mid-execution.

1-7:
❓ Verification inconclusive

Confirm necessity of imports and potential circular references.

All imports are relevant to authentication and directive management. Ensure there's no unintentional circular reference involving @app/graphql/generated/api/types.js and nest-authz. This can happen if Resource or AuthActionVerb is redefined or extended somewhere else in the chain.

Use this script to confirm that no circular references or re-exports exist in the codebase:

🏁 Script executed:
#!/bin/bash
# Searching for re-exports of 'Resource' or 'AuthActionVerb' that might cause circular references.
rg 'export\s+(\*|{.*})\s+from\s+.*Resource|AuthActionVerb' -A 2
Length of output: 207

Request Manual Verification: Circular Imports Check via Corrected Regex

The initial regex command produced an error due to invalid syntax. Please re-run the following corrected script to search for any re-exports of Resource or AuthActionVerb that might cause circular references between @app/graphql/generated/api/types.js and nest-authz:

File: api/src/unraid-api/graph/directives/auth.directive.ts

Concern: Confirm that there are no inadvertent re-exports or circular references involving the authentication-related imports.

Please verify the output of the above script to ensure that no circular dependencies exist.

[request_verification, verify_review_comment]

🏁 Script executed:
#!/bin/bash
# Corrected: Search for re-exports of 'Resource' or 'AuthActionVerb'
rg "export\s+(?:\*|{[^}]*})\s+from\s+['\"][^'\"]*(Resource|AuthActionVerb)[^'\"]*['\"]" -A 2
Length of output: 393

Circular Reference Check: Manual Verification Required

The revised re-export search using a simpler regex is necessary to ensure no circular dependencies exist between @app/graphql/generated/api/types.js and nest-authz. Please re-run the following simplified script to verify that no re-exports (and thus no circular references) exist:

File: api/src/unraid-api/graph/directives/auth.directive.ts

Concern: Confirm that the imports related to authentication in the file do not inadvertently introduce circular references.

Once you've manually verified the output, confirm that no unintended re-exports are present.

[request_verification, verify_review_comment]

🏁 Script executed:
#!/bin/bash
# Using extended regex to search for any export statement mentioning Resource or AuthActionVerb
rg -E "export.*(Resource|AuthActionVerb)" -A 2 .
Length of output: 243
Circular Reference Check on Authentication Imports

Our automated re-export search using regex commands encountered errors and did not yield conclusive results. Although no evidence of circular references between @app/graphql/generated/api/types.js and nest-authz was discovered via these commands, it's advisable to manually verify that:

No re-export of Resource or AuthActionVerb is causing circular dependencies.

The authentication-related imports in api/src/unraid-api/graph/directives/auth.directive.ts are free from indirect circular dependencies.

Please perform a manual inspection to ensure these conditions are met.
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (2)

76-135: Validate domain UUID references.

setVmState looks up the domain by UUID. If the UUID is malformed or belongs to a domain that no longer exists, the code throws. Ensure there's a test that verifies the error path for a missing or invalid UUID.

190-207: Confirm domain destruction does not cause data loss.

forceStopVm calls await domain.destroy(), which forcibly stops a VM. Confirm that this approach doesn’t risk data corruption (e.g., workloads that need graceful shutdown).

api/src/graphql/schema/types/vms/domain.graphql (2)

11-25: Add resource-based authorization.

Mutations that start or stop VMs may need an @auth directive. Confirm that these schema fields align with your authentication rules.

32-35: Ensure id matches the domain’s usage in resolvers.

type Vms provides id: ID!, but it’s unclear how id is generated or used in the service. Verify whether this is needed or if you prefer domain calls by the uuid.

api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (2)

303-306: Great job updating the error checks for unavailable VMs.

These assertions now correctly anticipate the specific error message ("Failed to set VM state: Invalid UUID"), which aligns with the service’s thrown error. This helps maintain clarity and consistency between the service and the test.

151-301: Comprehensive integration test coverage for VM lifecycle.

Your test thoroughly covers the creation, retrieval, state changes (start/stop), and cleanup of the VM, giving confidence in the stability of the underlying service. The approach is sound for integration testing.

web/composables/gql/graphql.ts (5)

329-335: New enum definition looks consistent.

The enum AuthActionVerb uses clear names and aligns with standard naming conventions.

337-342: Good use of separate enum for possession types.

The AuthPossession enum is self-explanatory and readable. No issues found.

1880-1952: Well-structured VM mutations.

Each mutation is clearly documented with permission requirements. The naming and doc comments follow a consistent style.

1955-1987: Argument definitions look good.

All VM mutation args are simple and consistent. No immediate concerns with maintainability or readability.

2002-2010: Informative doc block for VM query permissions.

The documentation properly clarifies permission requirements for viewing VMs.

coderabbitai · 2025-04-03T14:03:05Z

api/src/unraid-api/graph/directives/auth.directive.ts

+/**
+ * Generates GraphQL SDL strings for the authentication enums.
+ */
+export function getAuthEnumTypeDefs(): string {
+    // Helper to generate enum values string part with descriptions
+    const getEnumValues = <T>(tsEnum: Record<string, T>): string => {
+        return Object.entries(tsEnum)
+            .filter(([key]) => isNaN(Number(key))) // Filter out numeric keys
+            .map(([key]) => `  ${key}`)
+            .join('\n');
+    };
+
+    return `"""
+Available authentication action verbs
+"""
+enum AuthActionVerb {
+${getEnumValues(AuthActionVerb)}
+}
+
+"""
+Available authentication possession types
+"""
+enum AuthPossession {
+${getEnumValues(AuthPossession)}
+}
+
+directive @auth(
+  action: AuthActionVerb!, 
+  resource: String!, 
+  possession: AuthPossession!
+) on FIELD_DEFINITION
+`;
+}


💡 Verification agent

🧩 Analysis chain

Avoid duplicated definitions of the @auth directive.

getAuthEnumTypeDefs also defines the @auth directive. Confirm that no other part of the schema redefines it, to prevent conflicts in the final schema.

🏁 Script executed:

#!/bin/bash # Searching for multiple @auth directive definitions in schema rg -A 2 '@auth('

Length of output: 81

Verification: Update Search for Duplicate @auth Directive Definitions

The original search command using a regex produced an error due to an unescaped parenthesis. To gather the necessary information, please run the updated command below that uses a fixed-string search (avoiding regex interpretation issues) to confirm that the @auth directive is defined only once in the schema.

File of Interest: api/src/unraid-api/graph/directives/auth.directive.ts

Once you verify that this command returns a single occurrence (confirming there are no duplicate definitions elsewhere), you can safely remove the concern regarding duplicated @auth directives.

🏁 Script executed:

#!/bin/bash # Re-run search for duplicates of the '@auth' directive using fixed-string search to avoid regex errors rg -F "directive @auth(" .

Length of output: 589

Action Required: Consolidate Duplicate @auth Directive Definitions

Our search indicates that the @auth directive is defined in multiple locations:

api/src/unraid-api/graph/directives/auth.directive.ts

api/src/unraid-api/graph/utils/auth-enum.utils.ts

api/src/graphql/schema/types/base.graphql

api/generated-schema.graphql (auto-generated)

Please refactor the schema so that @auth is defined in a single, centralized location to avoid possible conflicts in your final GraphQL schema.

coderabbitai · 2025-04-03T14:03:05Z

api/src/unraid-api/graph/resolvers/vms/vms.service.ts

+    public async getDomains(): Promise<Array<VmDomain>> {
+        if (!this.isVmsAvailable) {
+            throw new GraphQLError('VMs are not available');
+        }
+        if (!this.hypervisor) {
+            throw new GraphQLError('Libvirt is not initialized');
+        }
+
+        try {
+            const hypervisor = this.hypervisor;
+            libvirtLogger.info('Getting all domains...');
+            // Get both active and inactive domains
+            const domains = await hypervisor.connectListAllDomains(
+                ConnectListAllDomainsFlags.ACTIVE | ConnectListAllDomainsFlags.INACTIVE
+            );
+            libvirtLogger.info(`Found ${domains.length} domains`);
+
+            const resolvedDomains: Array<VmDomain> = await Promise.all(
+                domains.map(async (domain) => {
+                    const info = await domain.getInfo();
+                    const name = await domain.getName();
+                    const uuid = await domain.getUUIDString();
+                    libvirtLogger.info(
+                        `Found domain: ${name} (${uuid}) with state ${DomainState[info.state]}`
+                    );
+
+                    // Map DomainState to VmState using our existing function
+                    const state = this.mapDomainStateToVmState(info.state);
+
+                    return {
+                        name,
+                        uuid,
+                        state,
+                    };
+                })
+            );
+
+            return resolvedDomains;
+        } catch (error: unknown) {
+            // If we hit an error expect libvirt to be offline
+            this.isVmsAvailable = false;
+            libvirtLogger.error(
+                `Failed to get domains: ${error instanceof Error ? error.message : 'Unknown error'}`
+            );
+            throw new GraphQLError(
+                `Failed to get domains: ${error instanceof Error ? error.message : 'Unknown error'}`
+            );
+        }
+    }


🛠️ Refactor suggestion

Prevent silent fallback on repeated errors.

Within getDomains, setting this.isVmsAvailable = false upon error could obscure repeated issues if the system never tries to re-initialize. Provide a strategy to re-attempt hypervisor connection or at least log repeated attempts.

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (15)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (1)
28-28: Entire suite is being skipped.

describe.skip('Auth Directive', ...) prevents these tests from running. If this is unintended, switch to describe('Auth Directive', ...).
- describe.skip('Auth Directive', () => {
+ describe('Auth Directive', () => {
api/src/unraid-api/graph/directives/auth.directive.ts (2)
13-112: Add unit tests for transformResolvers function.
This function enforces permission checks, but we don’t see direct tests. Adding dedicated tests will help ensure correctness and catch any regressions.

38-46: Replace console logs with a proper logger for production readiness.
Excessive console.log usage can clutter logs. Consider using a logging library or NestJS logger for better maintenance.
- console.log(
-     'resolving',
-     info.fieldName,
-     ...
- );
+ this.logger.debug(
+     `Resolving ${info.parentType.name}.${info.fieldName}`,
+ );
api/src/unraid-api/graph/graph.module.ts (1)

66-71: transformResolvers commented out.
Looks intentional due to the @todo note. Remember to finalize once the approach is stable.

web/composables/gql/graphql.ts (1)

337-343: Consider clarifying the distinction of 'OWN_ANY'.
While the enum is consistent and self-explanatory, the difference between ANY and OWN_ANY might be subtle to newcomers. Consider adding a short comment or doc snippet clarifying use cases for OWN_ANY.

api/generated-schema.graphql (10)

90-315: Array Types and Enums:
The schema for arrays is comprehensive. Types such as Array, ArrayCapacity, ArrayDisk, and related inputs/enums (e.g. ArrayDiskFsColor, ArrayDiskStatus, ArrayDiskType, ArrayMutations, ArrayPendingState, ArrayState, ArrayStateInput) are well defined. Consider aligning enum value naming (e.g. use GREEN_OFF rather than green_off) for consistency with other enums.

395-444: Connect and User Info Types:
The Connect and ConnectSettings types, alongside the inputs ConnectSignInInput and ConnectUserInfoInput, are well designed.
Note: The field preferred_username in ConnectUserInfoInput could be renamed to preferredUsername to follow camelCase conventions consistently.

812-872: Mutation Type:
The Mutation type aggregates a wide range of operations (permissions, API key management, dynamic remote access, VM control, etc.). This extensive grouping is clear, though you might consider modularizing if complexity increases.

980-1039: Partition Type:
The Partition type is extremely detailed with many identifiers. Consider reviewing whether all these fields are necessary to avoid potential over-fetching.

1067-1136: Query Type:
The Query type covers a wide array of operations. Given its breadth, consider breaking it into logical groups or splitting the schema into multiple files to enhance maintainability.

1217-1247: Resource Enum:
The Resource enum is very comprehensive.
Note: The value CONNECT__REMOTE_ACCESS uses a double underscore; consider using a single underscore to maintain consistency with other enum values.

1321-1341: Subscription Type:
The subscription definitions efficiently cover change notifications. It may be useful to consider whether some fields require additional authorization handling.

1387-1441: UnassignedDevice Type:
This type provides extensive disk information. As with the Partition type, ensure that including all these fields is necessary for your queries.

1473-1653: Vars Type:
The Vars type is highly detailed and covers many configurable parameters. As the schema grows, consider modularizing or splitting this type if it becomes too unwieldy.

1694-1715: VM Mutations:
The VM mutation operations (force stop, pause, reboot, reset, resume, start, stop) are well outlined. Make sure that appropriate authorization checks are implemented in the resolvers.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 9335a47 and a193307.

⛔ Files ignored due to path filters (3)

api/src/graphql/generated/api/operations.ts is excluded by !**/generated/**
api/src/graphql/generated/api/types.ts is excluded by !**/generated/**
pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (63)

.github/workflows/main.yml (2 hunks)
.gitignore (1 hunks)
api/codegen.ts (4 hunks)
api/dev/Unraid.net/myservers.cfg (1 hunks)
api/dev/Unraid.net/myservers.example.cfg (1 hunks)
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json (1 hunks)
api/generated-schema.graphql (1 hunks)
api/package.json (1 hunks)
api/src/__test__/core/modules/array/add-disk-to-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/get-array-data.test.ts (0 hunks)
api/src/__test__/core/modules/array/remove-disk-from-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/update-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/update-parity-check.test.ts (0 hunks)
api/src/__test__/graphql/resolvers/subscription/network.test.ts (1 hunks)
api/src/__test__/setup/config-setup.ts (1 hunks)
api/src/__test__/store/modules/paths.test.ts (1 hunks)
api/src/core/modules/array/add-disk-to-array.ts (0 hunks)
api/src/core/modules/array/index.ts (0 hunks)
api/src/core/modules/array/remove-disk-from-array.ts (0 hunks)
api/src/core/modules/array/update-array.ts (0 hunks)
api/src/core/modules/array/update-parity-check.ts (0 hunks)
api/src/core/modules/index.ts (0 hunks)
api/src/core/modules/vms/get-domains.ts (0 hunks)
api/src/core/modules/vms/index.ts (0 hunks)
api/src/core/utils/vms/get-hypervisor.ts (0 hunks)
api/src/core/utils/vms/index.ts (0 hunks)
api/src/core/utils/vms/parse-domain.ts (0 hunks)
api/src/core/utils/vms/parse-domains.ts (0 hunks)
api/src/graphql/schema/types/api-key/roles.graphql (1 hunks)
api/src/graphql/schema/types/base.graphql (1 hunks)
api/src/graphql/schema/types/vms/domain.graphql (1 hunks)
api/src/store/modules/paths.ts (1 hunks)
api/src/unraid-api/app/app.module.ts (2 hunks)
api/src/unraid-api/auth/api-key.service.spec.ts (3 hunks)
api/src/unraid-api/auth/api-key.service.ts (5 hunks)
api/src/unraid-api/auth/auth.interceptor.ts (1 hunks)
api/src/unraid-api/auth/auth.service.spec.ts (3 hunks)
api/src/unraid-api/auth/authentication.guard.ts (1 hunks)
api/src/unraid-api/auth/casbin/model.ts (1 hunks)
api/src/unraid-api/auth/casbin/policy.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.spec.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.ts (1 hunks)
api/src/unraid-api/graph/graph.module.ts (3 hunks)
api/src/unraid-api/graph/resolvers/array/array.service.ts (2 hunks)
api/src/unraid-api/graph/resolvers/online/online.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/resolvers.module.ts (2 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (2 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1 hunks)
api/src/unraid-api/graph/utils/auth-enum.utils.ts (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (5 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (5 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (2 hunks)
api/vite.config.ts (1 hunks)
web/codegen.ts (1 hunks)
web/composables/gql/graphql.ts (7 hunks)

💤 Files with no reviewable changes (17)

api/src/test/core/modules/array/update-array.test.ts
api/src/core/modules/vms/index.ts
api/src/test/core/modules/array/remove-disk-from-array.test.ts
api/src/test/core/modules/array/update-parity-check.test.ts
api/src/core/modules/index.ts
api/src/core/modules/array/index.ts
api/src/core/utils/vms/parse-domains.ts
api/src/core/modules/array/update-parity-check.ts
api/src/core/utils/vms/get-hypervisor.ts
api/src/core/modules/vms/get-domains.ts
api/src/core/utils/vms/index.ts
api/src/core/modules/array/add-disk-to-array.ts
api/src/core/utils/vms/parse-domain.ts
api/src/test/core/modules/array/get-array-data.test.ts
api/src/test/core/modules/array/add-disk-to-array.test.ts
api/src/core/modules/array/remove-disk-from-array.ts
api/src/core/modules/array/update-array.ts

🚧 Files skipped from review as they are similar to previous changes (35)

api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php.last-download-time
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/.login.php.last-download-time
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/Notifications.page.last-download-time
api/vite.config.ts
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/auth-request.php.last-download-time
api/dev/Unraid.net/myservers.cfg
api/src/unraid-api/auth/authentication.guard.ts
web/codegen.ts
api/src/unraid-api/auth/auth.interceptor.ts
api/src/store/modules/paths.ts
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json
api/dev/Unraid.net/myservers.example.cfg
api/src/test/setup/config-setup.ts
api/src/unraid-api/graph/utils/auth-enum.utils.ts
api/package.json
api/src/unraid-api/auth/casbin/policy.ts
api/src/unraid-api/graph/resolvers/vms/vms.resolver.spec.ts
.gitignore
api/src/unraid-api/auth/casbin/model.ts
api/src/test/store/modules/paths.test.ts
api/src/unraid-api/auth/api-key.service.ts
api/src/unraid-api/app/app.module.ts
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch
api/src/graphql/schema/types/base.graphql
api/src/test/graphql/resolvers/subscription/network.test.ts
api/src/unraid-api/graph/resolvers/online/online.resolver.ts
api/src/unraid-api/auth/api-key.service.spec.ts
api/src/unraid-api/auth/auth.service.spec.ts
.github/workflows/main.yml
api/src/unraid-api/graph/resolvers/array/array.service.ts
api/src/graphql/schema/types/api-key/roles.graphql
api/src/unraid-api/unraid-file-modifier/modifications/test/snapshots/DefaultPageLayout.php.modified.snapshot.php
api/src/unraid-api/graph/resolvers/resolvers.module.ts
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts

🧰 Additional context used

🧬 Code Definitions (7)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (2)

api/src/unraid-api/graph/directives/auth.directive.ts (3)

getAuthEnumTypeDefs (165-194)

transformResolvers (13-112)

authSchemaTransformer (114-160)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

api/codegen.ts (1)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

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

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

api/src/graphql/schema/loadTypesDefs.ts (1)

loadTypeDefs (5-28)

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

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

domain (26-35)

web/composables/gql/graphql.ts (1)

api/src/graphql/generated/api/types.ts (10)

Maybe (5-5)

VmMutations (1852-1868)

Scalars (14-26)

VmMutationsforceStopVmArgs (1871-1873)

VmMutationspauseVmArgs (1876-1878)

VmMutationsrebootVmArgs (1881-1883)

VmMutationsresetVmArgs (1886-1888)

VmMutationsresumeVmArgs (1891-1893)

VmMutationsstartVmArgs (1896-1898)

VmMutationsstopVmArgs (1901-1903)

api/src/unraid-api/graph/resolvers/vms/vms.service.ts (3)

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

domain (26-35)

web/composables/gql/graphql.ts (1)

VmDomain (1871-1878)

api/src/graphql/generated/api/types.ts (1)

VmDomain (1843-1850)

api/src/unraid-api/graph/directives/auth.directive.ts (1)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

🪛 Biome (1.9.4)

api/src/unraid-api/graph/directives/auth.directive.spec.ts

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 175-175: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (1)

GitHub Check: Cloudflare Pages

🔇 Additional comments (93)

api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (6)

109-109: No visible logic change.

This empty line predominantly serves as a spacing adjustment and has no functional or maintainability impact.

115-115: No visible logic change.

Similarly, this empty line for spacing does not affect functionality or introduce new maintainability considerations.

759-759: Conditional reload logic looks fine.

The check for $myPage['Load'] > 0 before setting up a timed reload is clear. Ensure that edge cases (e.g., missing or non-numeric Load) are handled in upstream code.

1295-1295: Closing brace for focus logic block.

No functional modification; it simply closes the if block.

1314-1314: No visible logic change.

A blank line after resetting nchanPaused.

1318-1318: No visible logic change.

The additional blank line here does not alter behavior.

api/src/unraid-api/graph/directives/auth.directive.spec.ts (3)

1-12: Imports & initial setup look good.

Imports from Vitest and local modules are straightforward. This block correctly references the tested utilities (authSchemaTransformer, getAuthEnumTypeDefs, transformResolvers).

13-27: Mocking nest-authz.

Using vi.mock to override the nest-authz exports is a clean approach for isolating tests. Consider verifying each mock covers all needed methods (e.g., authZService usage) if complexity grows.

29-241: Overall test coverage is thorough.

Confirms authSchemaTransformer properly appends permission information to field descriptions.

Validates transformResolvers applies permission checks.

Tests are well structured for success and error paths.

No critical issues found.

🧰 Tools

🪛 Biome (1.9.4)

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 175-175: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (4)

1-12: Test module setup is clear.

The TestingModule approach with mocks for VmsService is straightforward, ensuring each test remains isolated.

13-34: Basic instantiation checks.

Validates the resolver is defined, a useful sanity test. This helps catch broken DI configurations early.

35-53: startVm test group.

Verifies it calls startVm with the correct ID.

Properly handles and propagates errors.
Implementation is appropriately tested.

55-73: stopVm test group.

Similar thoroughness for stopVm, confirming correct call and error propagation.
No issues found.

api/src/unraid-api/graph/directives/auth.directive.ts (4)

1-2: No major concerns for these imports.
Everything looks correct in terms of dependency references.

114-158: Schema transformer logic appears solid.
The approach of appending permission info to fieldConfig.description and storing data in fieldConfig.extensions is straightforward and clear.

186-191: Duplicate @auth directive definition still remains.
A prior review advised consolidating the directive to a single location. Please remove or unify other definitions to avoid schema conflicts.

194-217: No concerns with createGraphQLEnumFromTSEnum.
The conversion logic is concise and correct for typical use cases.

api/codegen.ts (2)

5-5: Importing getAuthEnumTypeDefs is correct for the new auth enums.
No issues observed.

35-41: Consistent usage of getAuthEnumTypeDefs() across multiple outputs.
Embedding the auth enums into multiple schema targets looks appropriate.

Also applies to: 45-49, 63-67

api/src/unraid-api/graph/graph.module.ts (5)

3-6: Revised imports align with the new authentication model.
Adding UnauthorizedException, AuthZService, and AuthModule is appropriate given the updated authorization logic.

Also applies to: 15-15, 20-20, 24-24

33-35: New injection of AuthZService for factory function.
This ensures that the authorization logic is centrally accessible.

37-47: Appending auth enum SDL to typeDefs.
Merging plugins.typeDefs with authEnumTypeDefs ensures a unified schema. Nicely done.

52-58: Asynchronous context function usage.
Providing req, connectionParams, and extra supports robust authentication checks. No issues found.

77-78: Empty providers array & exported GraphQLModule.
This minimal approach is fine given the module refactor.

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

76-135: Robust and clear VM state transition logic.

Your approach for gracefully shutting down and resuming domains is well-structured. The transition checks, fallback to forced destroy, and consistent error reporting look good.

261-309: Prevent silent fallback on repeated errors.

This mirrors a previous suggestion to avoid permanently disabling VM availability after a single failure. Reattempting a connection or logging repeated issues may improve resiliency.

api/src/graphql/schema/types/vms/domain.graphql (2)

6-25: Return type may limit future enhancements.

Returning Boolean! can constrain conveying more detailed status or errors. A richer return type (e.g., a VM object or union type) could be more flexible.

32-35: No immediate concerns with this addition.

Defining id: ID! and domain: [VmDomain!] within Vms is straightforward and consistent with existing naming.

api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (3)

54-63: Effective QEMU installation check.

Verifying that QEMU exists at the expected path helps avoid test setup failures and improves the reliability of the VM integration tests.

93-138: Comprehensive domain cleanup procedure.

Your cleanup logic thoroughly handles active and inactive domains, preventing test residue. The added waits after destroy and undefine help ensure the domain’s final state.

304-306: Appropriate error handling tests.

Verifying that the service throws the expected error message for an invalid UUID helps ensure consistency between service logic and consumer expectations.

web/composables/gql/graphql.ts (7)

329-335: New enum for action verbs looks coherent.
The defined constants align with standard CRUD operations, making them clear and easy to integrate.

880-889: Check permission action in the doc comment.
The doc block states that READ permission is required, yet vms references mutations dealing with VM state changes. Verify whether UPDATE or READ is correct here to maintain consistency between the schema and documentation.

1388-1415: Resource enum naming is now consistent.
Switching to uppercase string representations improves consistency across the schema. Great job.

1420-1422: Role enum values look consistent.
Renaming them to uppercase is in line with the rest of the schema convention.

1880-1952: Validate mismatch between doc comments and required permissions.
Each new VM mutation is documented with UPDATE as the required permission. However, the vms field above references READ permissions. Confirm that these references to required permissions align with the actual intended behavior.

1955-1957: Argument definitions look solid.
Each mutation uses the same ID argument type, which ensures consistent usage patterns. No issues detected.

Also applies to: 1960-1962, 1965-1967, 1970-1972, 1975-1977, 1980-1982, 1985-1987

2002-2010: Query documentation is consistent with READ access.
The annotation here correctly indicates read permissions for retrieving VM info. Nicely spelled out.

api/generated-schema.graphql (55)

1-15: Directive and URL Types:
The new @auth directive and the URL-related types (AccessUrl and AccessUrlInput) are clearly defined. Ensure that your custom URL scalar is implemented consistently in resolvers.

17-34: Permission and Role Inputs:
The inputs for permissions and role assignments—AddPermissionInput, AddRoleForApiKeyInput, AddRoleForUserInput, and AllowedOriginInput—are straightforward and well documented.

36-58: API Key Types:
The types ApiKey, ApiKeyResponse, and ApiKeyWithSecret provide a solid structure for API key management, including metadata and permissions.

60-88: API Settings Documentation and Input:
The documentation enclosed in triple quotes is clear and informative. The ApiSettingsInput input type appropriately details various API configuration options.

330-343: Authentication Enums:
The enums AuthActionVerb and AuthPossession are clearly defined, laying a good foundation for granular access control. Ensure that these are reliably enforced in resolvers.

345-393: Hardware & Cloud Configuration:
The new types for system hardware (Baseboard, Capacity, Case) and cloud configuration (Cloud, CloudResponse, Config, ConfigErrorState) are structured with clear descriptions.

446-476: Container Configuration:
The types ContainerHostConfig, ContainerMount, and ContainerPort (with ContainerPortType and ContainerState) are clearly documented and organized.

478-487: API Key Creation Input:
CreateApiKeyInput is concise and clear, with helpful documentation regarding key replacement.

489-568: Device and Disk Types:
The scalar DateTime and types like Devices, Disk, and the associated enums (DiskFsType, DiskInterfaceType, DiskPartition, DiskSmartStatus) are well detailed.

546-595: Display and Docker Containers:
The Display type and Docker-related types (Docker, DockerContainer, DockerMutations, DockerNetwork) are detailed with helpful field descriptions.

620-630: Dynamic Remote Access:
The types DynamicRemoteAccessStatus, DynamicRemoteAccessType, and EnableDynamicRemoteAccessInput neatly cover remote access configuration.

637-651: Flash and GPU:
The definitions for Flash and Gpu are succinct and capture the essential attributes.

653-675: Importance and Info Types:
The Importance enum along with the Info type (and its subtypes InfoApps, InfoCpu, InfoMemory) are comprehensive and well documented.

719-748: Log File Types and Scalars:
The scalars (JSON, Long) and the log-related types (KeyFile, LogFile, LogFileContent) are clearly declared, aiding in log management.

758-765: User Account (Me) Type:
The Me type implementing UserAccount is simple and effective in capturing user details.

767-789: Memory Specifications:
The enum MemoryFormFactor and types MemoryLayout and MemoryType are detailed and support in-depth memory reporting.

791-803: Minigraph and Mount Types:
The MinigraphStatus enum and MinigraphqlResponse type provide valuable system status information, and Mount is appropriately minimal.

874-893: Network and Node Foundation:
The Network type and the Node interface are straightforward and serve as robust foundations for related types.

895-916: Notification Types:
The Notification and NotificationCounts types include clear descriptions for timestamps and importance levels.

918-936: Notification Inputs:
The NotificationData and NotificationFilter inputs, along with the NotificationOverview type, are well structured and descriptive.

938-941: Notification Enum:
The NotificationType enum is defined simply and fits the overall design.

943-949: Notifications Container:
The Notifications type, which includes filtering and an overview, is clear and effectively designed.

951-970: OS and Owner Types:
The types Os and Owner offer essential system data with concise documentation.

972-978: Parity Check:
The ParityCheck type provides necessary detail for monitoring parity checks.

1041-1051: PCI Type:
The Pci type is clear and includes the essential device identification attributes.

1053-1057: Permission Type:
The Permission type is concise and effectively defines the actions and resource association.

1058-1065: Scalar Port and ProfileModel:
These are brief and to the point.

1138-1198: Registration and Related States:
The registration types and RegistrationState enum are thoroughly defined. It would help to have external documentation explaining the various error and state codes.

1200-1204: Relay Response:
A succinct definition that meets its purpose well.

1206-1210: Remote Access:
The RemoteAccess type is clearly designed with the necessary fields.

1212-1215: Role Removal Input:
RemoveRoleFromApiKeyInput is self-explanatory and straightforward.

1249-1255: Role Enum:
The Role enum is simple and clear.

1256-1273: Server Types:
The Server type and ServerStatus enum are well structured and include all necessary fields.

1274-1280: Service Type:
The Service type is minimal yet sufficient. Additional descriptive fields could be added later if needed.

1282-1286: Setup Remote Access Input:
This input type is clear and concise.

1288-1319: Share Type:
The Share type covers numerous parameters for disk allocation and configuration. Verify that the optionality of these fields matches backend expectations.

1356-1364: System Type:
Provides basic system information in a clear and concise manner.

1365-1372: Temperature and Theme Enums:
These enums are simple; they can be expanded in the future if necessary.

1374-1383: URL Scalar and URL_TYPE Enum:
The URL scalar and corresponding URL_TYPE enum are defined clearly. Ensure that scalar validation is robust.

1385-1386: UUID Scalar:
A straightforward definition that fits well into the schema.

1442-1445: Uptime Type:
A minimal and clear definition for reporting uptime.

1446-1450: USB Type:
The Usb type is defined simply and clearly.

1451-1463: User Type:
The User type is straightforward. Review the inclusion of sensitive flags (like the password boolean) to ensure proper security handling in your resolvers.

1465-1471: UserAccount Interface:
This interface is consistent with the User type and clearly states the required fields for a user account.

1655-1682: Versions Type:
Provides clear version information for various system components.

1684-1693: VM Domain Type:
A minimal yet essential definition for virtual machines, capturing the key fields.

1717-1726: VM State Enum:
A comprehensive set of states for virtual machines that should cover most scenarios.

1728-1732: Vms Wrapper Type:
Provides a simple grouping of VM domains with an identifier.

1733-1742: WAN Access and Forward Enums:
The enums for WAN access (WAN_ACCESS_TYPE) and port forwarding (WAN_FORWARD_TYPE) are clear and should help set proper behavior for remote access features.

1744-1747: Welcome Type:
A friendly and straightforward type designed for introductory messaging.

1748-1753: Add User Input:
The addUserInput input type is clear for creating new users. Consider adding further validation at the resolver level if needed.

1754-1756: Delete User Input:
A straightforward input type for user deletion.

1758-1762: MD State Enum:
Simple and effective definitions for disk states.

1763-1772: Registration Type Enum:
A thorough enum for registration types. Ensure that the semantics for each value are clearly documented externally for developers.

1774-1776: Users Input:
A minimal input type; consider expanding with additional filters as the application’s user management grows.

coderabbitai · 2025-04-03T16:55:50Z

api/codegen.ts

+        // Generate Types for Mothership GraphQL Client
+        'src/graphql/generated/client/': {
+            documents: './src/graphql/mothership/*.ts',
+            schema: {
+                [process.env.MOTHERSHIP_GRAPHQL_LINK as string]: {
+                    headers: {
+                        origin: 'https://forums.unraid.net',
+                    },
+                },
+            },
+            preset: 'client',
+            presetConfig: {
+                gqlTagName: 'graphql',
+            },
+            config: {
+                useTypeImports: true,
+                withObjectType: true,
+            },
+            plugins: [{ add: { content: '/* eslint-disable */' } }],
+        },


💡 Verification agent

🧩 Analysis chain

Validate environment variable for the Mothership GraphQL link.
Ensure process.env.MOTHERSHIP_GRAPHQL_LINK is set in all relevant environments for successful client generation.

🏁 Script executed:

#!/bin/bash # Check if MOTHERSHIP_GRAPHQL_LINK is set [ -z "$MOTHERSHIP_GRAPHQL_LINK" ] && echo "MOTHERSHIP_GRAPHQL_LINK is not set" || echo "MOTHERSHIP_GRAPHQL_LINK is set to: $MOTHERSHIP_GRAPHQL_LINK"

Length of output: 94

Action Required: Define MOTHERSHIP_GRAPHQL_LINK Environment Variable
The GraphQL client configuration in api/codegen.ts (lines 85-104) relies on process.env.MOTHERSHIP_GRAPHQL_LINK. The verification script indicates that this variable is currently not set, which may lead to client generation failures. Please ensure that the environment variable is defined across all relevant environments (development, CI, production) or implement a fallback mechanism.

coderabbitai · 2025-04-03T16:55:50Z

api/src/unraid-api/graph/resolvers/vms/vms.service.ts

+    async onModuleInit() {
+        try {
+            libvirtLogger.info(`Initializing VMs service with URI: ${this.uri}`);
+            await this.initializeHypervisor();
+            this.isVmsAvailable = true;
+            libvirtLogger.info(`VMs service initialized successfully with URI: ${this.uri}`);
+        } catch (error) {
+            this.isVmsAvailable = false;
+            libvirtLogger.warn(
+                `VMs are not available: ${error instanceof Error ? error.message : 'Unknown error'}`
+            );
+        }
+    }


🛠️ Refactor suggestion

Consider re-initialization or error recovery pathways.

Currently, if initializeHypervisor() fails, the service sets isVmsAvailable = false but doesn’t retry later. This may leave the service in a permanently unavailable state if the hypervisor becomes available afterward. Consider implementing a re-initialization or error notification strategy.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

api/src/__test__/common/allowed-origins.test.ts (2)
1-1: Import changes improve test capabilities

The import updates appropriately reflect the new test requirements. Adding getExtraOrigins and getServerIps imports allows for testing the expanded functionality, while including beforeEach supports the new test setup pattern.

The static analysis suggests alphabetically ordering imports from vitest. Consider reordering:
-import { expect, test, vi, beforeEach } from 'vitest';
+import { beforeEach, expect, test, vi } from 'vitest';
Also applies to: 5-5, 9-9

11-41: Well-structured mocks for deterministic testing

The mock setup for both network functionality and store state is well-implemented. This approach ensures predictable test behavior by controlling external dependencies.

Two minor formatting issues to fix according to static analysis:

Indentation inconsistencies in the mock functions

Missing spaces at the beginning of some lines
vi.mock('@app/graphql/resolvers/subscription/network.js', () => ({
-  getServerIps: vi.fn(),
-  getUrlForField: vi.fn(({ url, port, portSsl }) => {
-    if (port) return `http://${url}:${port}`;
-    if (portSsl) return `https://${url}:${portSsl}`;
-    return `https://${url}`;
-  }),
+    getServerIps: vi.fn(),
+    getUrlForField: vi.fn(({ url, port, portSsl }) => {
+        if (port) return `http://${url}:${port}`;
+        if (portSsl) return `https://${url}:${portSsl}`;
+        return `https://${url}`;
+    }),
}));

vi.mock('@app/store/index.js', () => ({
-  store: {
+    store: {
🧰 Tools

🪛 GitHub Check: Test API

[failure] 13-13:
Replace ·· with ····

[failure] 14-14:
Insert ··

[failure] 15-15:
Insert ····

[failure] 16-16:
Insert ····

[failure] 17-17:
Replace ···· with ········

[failure] 18-18:
Insert ··

[failure] 22-22:
Insert ··

🪛 GitHub Check: Build API

[failure] 13-13:
Replace ·· with ····

[failure] 14-14:
Insert ··

[failure] 15-15:
Insert ····

[failure] 16-16:
Insert ····

[failure] 17-17:
Replace ···· with ········

[failure] 18-18:
Insert ··

[failure] 22-22:
Insert ··

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between a193307 and d3a76e5.

📒 Files selected for processing (2)

api/dev/states/myservers.cfg (1 hunks)
api/src/__test__/common/allowed-origins.test.ts (1 hunks)

✅ Files skipped from review due to trivial changes (1)

api/dev/states/myservers.cfg

🧰 Additional context used

🪛 GitHub Check: Test API

api/src/__test__/common/allowed-origins.test.ts

[failure] 2-2:
Insert getServerIps·}·from·'@app/graphql/resolvers/subscription/network.js';⏎import·{·

[failure] 5-5:
Delete import·{·getServerIps·}·from·'@app/graphql/resolvers/subscription/network.js';⏎

[failure] 9-9:
Replace expect,·test,·vi,·beforeEach with beforeEach,·expect,·test,·vi

[failure] 13-13:
Replace ·· with ····

[failure] 14-14:
Insert ··

[failure] 15-15:
Insert ····

[failure] 16-16:
Insert ····

[failure] 17-17:
Replace ···· with ········

[failure] 18-18:
Insert ··

[failure] 22-22:
Insert ··

🪛 GitHub Check: Build API

api/src/__test__/common/allowed-origins.test.ts

[failure] 2-2:
Insert getServerIps·}·from·'@app/graphql/resolvers/subscription/network.js';⏎import·{·

[failure] 5-5:
Delete import·{·getServerIps·}·from·'@app/graphql/resolvers/subscription/network.js';⏎

[failure] 9-9:
Replace expect,·test,·vi,·beforeEach with beforeEach,·expect,·test,·vi

[failure] 13-13:
Replace ·· with ····

[failure] 14-14:
Insert ··

[failure] 15-15:
Insert ····

[failure] 16-16:
Insert ····

[failure] 17-17:
Replace ···· with ········

[failure] 18-18:
Insert ··

[failure] 22-22:
Insert ··

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: Build Web App
GitHub Check: Cloudflare Pages

🔇 Additional comments (2)

api/src/__test__/common/allowed-origins.test.ts (2)

43-64: Effective test setup with beforeEach

Good practice using beforeEach to reset mocks and establish a consistent test environment. The mock data for getServerIps covers a wide range of URL formats, which helps ensure robust testing.

The explicit comment "// Mock getServerIps to return a consistent set of URLs" clearly explains the purpose of this setup.

66-100: Improved test assertions with specific expectations

The switch from snapshot testing to explicit assertions is a significant improvement. This approach:

Makes the test's expectations clearer

Provides more detailed feedback when tests fail

Tests for specific important values rather than the entire output structure

Validates that there are no duplicates in the results

The test now properly verifies socket paths, local URLs, remote URLs, and extra origins from the configuration.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

api/src/__test__/common/allowed-origins.test.ts (1)
74-101: Detailed assertions covering all expected URL types

The test has been significantly improved with explicit assertions that verify each type of URL that should be included in the allowed origins. The organization by URL type (socket paths, local URLs, connect URLs, extra origins, remote URLs) makes the test more readable and easier to maintain.

The final check for no duplicates is a good validation of the uniq functionality in the implementation.

Consider adding a test to verify that URLs with trailing slashes are properly handled, as this is mentioned in the implementation of getAllowedOrigins:
// Suggestion: Add test for trailing slash handling
const urlWithTrailingSlash = 'https://example.com/';
expect(allowedOrigins).toContain('https://example.com');
expect(allowedOrigins).not.toContain(urlWithTrailingSlash);

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between d3a76e5 and d14f5e6.

📒 Files selected for processing (1)

api/src/__test__/common/allowed-origins.test.ts (1 hunks)

🧰 Additional context used

🧬 Code Definitions (1)

api/src/__test__/common/allowed-origins.test.ts (3)

api/src/store/modules/emhttp.ts (1)

loadStateFiles (133-152)

api/src/store/modules/config.ts (1)

loadConfigFile (121-166)

api/src/common/allowed-origins.ts (1)

getAllowedOrigins (90-98)

⏰ Context from checks skipped due to timeout of 90000ms (4)

GitHub Check: Build API
GitHub Check: Build Web App
GitHub Check: Test API
GitHub Check: Cloudflare Pages

🔇 Additional comments (6)

api/src/__test__/common/allowed-origins.test.ts (6)

1-2: Updated imports to include new functionality

The imports have been updated to include getExtraOrigins alongside getAllowedOrigins, and to import getServerIps which will be mocked in the tests. This aligns with the enhanced testing approach.

9-10: Updated testing imports

Clean import of testing utilities from Vitest with proper spacing.

11-19: Good mock implementation for network functions

The mock implementation for the network module is well-structured and handles the various URL formats correctly based on port parameters.

21-41: Well-defined store mock with necessary configuration

The store mock provides all the required state structure and configuration for testing, including the extraOrigins setting that will be used by the new getExtraOrigins function.

43-64: Comprehensive test setup in beforeEach

The beforeEach block properly clears mocks and sets up consistent test data for the getServerIps function, providing a variety of URL formats that cover different use cases.

72-73: Simple function call with clear variable naming

The function under test is called with a clear variable name for its result.

coderabbitai

Actionable comments posted: 4

🧹 Nitpick comments (4)

api/src/unraid-api/graph/resolvers/array/array.service.ts (3)
154-156: Consider importing getters at the module level instead of dynamically.

Dynamic imports can impact performance and make the code harder to test. Since this import doesn't seem to be conditional based on runtime factors, consider importing it at the top of the file.
+ import { getters } from '@app/store/index.js';

// ...
async updateParityCheck({ ... }) {
-    const { getters } = await import('@app/store/index.js');
     const running = getters.emhttp().var.mdResync !== 0;
179-182: Use consistent error handling approach.

Other methods in this class throw BadRequestException with an AppError inside, but this method throws a GraphQLError directly. Consider adopting a consistent approach to error handling.
- throw new GraphQLError(`Invalid parity check state: ${wantedState}`);
+ throw new BadRequestException(new AppError(`Invalid parity check state: ${wantedState}`));
196-196: Remove extra whitespace on this line.

There's unnecessary whitespace at the beginning of this line that's causing CI failures.
-        
+
🧰 Tools

🪛 GitHub Check: Build API

[failure] 196-196:
Delete ········

🪛 GitHub Check: Test API

[failure] 196-196:
Delete ········
api/src/unraid-api/auth/auth.interceptor.ts (1)
11-11: Consider removing or replacing console.log statement.

This console.log statement appears to be debugging code. In production code, consider removing it or replacing it with proper logging using a NestJS logger.
- console.log('in auth interceptor', req.user);
+ // Consider using NestJS Logger service instead
🧰 Tools

🪛 Biome (1.9.4)

[error] 11-12: Illegal use of an import declaration not at the top level

move this declaration to the top level

(parse)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between d14f5e6 and b2ace2f.

📒 Files selected for processing (2)

api/src/unraid-api/auth/auth.interceptor.ts (1 hunks)
api/src/unraid-api/graph/resolvers/array/array.service.ts (2 hunks)

🧰 Additional context used

🧬 Code Definitions (1)

api/src/unraid-api/graph/resolvers/array/array.service.ts (3)

api/src/graphql/generated/api/types.ts (1)

ArrayType (114-133)

api/src/core/utils/clients/emcmd.ts (1)

emcmd (12-41)

api/src/core/modules/array/get-array-data.ts (1)

getArrayData (9-61)

🪛 GitHub Check: Build API

api/src/unraid-api/graph/resolvers/array/array.service.ts

[failure] 153-153:
Replace ·wantedState,·correct·}:·{·wantedState:·'pause'·|·'resume'·|·'cancel'·|·'start';·correct:·boolean with ⏎········wantedState,⏎········correct,⏎····}:·{⏎········wantedState:·'pause'·|·'resume'·|·'cancel'·|·'start';⏎········correct:·boolean;⏎···

[failure] 196-196:
Delete ········

🪛 GitHub Check: Test API

api/src/unraid-api/graph/resolvers/array/array.service.ts

[failure] 153-153:
Replace ·wantedState,·correct·}:·{·wantedState:·'pause'·|·'resume'·|·'cancel'·|·'start';·correct:·boolean with ⏎········wantedState,⏎········correct,⏎····}:·{⏎········wantedState:·'pause'·|·'resume'·|·'cancel'·|·'start';⏎········correct:·boolean;⏎···

[failure] 196-196:
Delete ········

🪛 GitHub Actions: CI - Main (API)

api/src/unraid-api/graph/resolvers/array/array.service.ts

[error] 153-153: Replace ·wantedState,·correct·}:·{·wantedState:·'pause'·|·'resume'·|·'cancel'·|·'start';·correct:·boolean with ⏎········wantedState,⏎········correct,⏎····}:·{⏎········wantedState:·'pause'·|·'resume'·|·'cancel'·|·'start';⏎········correct:·boolean;⏎··· prettier/prettier

api/src/unraid-api/auth/auth.interceptor.ts

[error] 13-13: Parsing error: Declaration or statement expected

🪛 Biome (1.9.4)

api/src/unraid-api/auth/auth.interceptor.ts

[error] 11-12: Illegal use of an import declaration not at the top level

move this declaration to the top level

(parse)

[error] 12-13: Expected an expression but instead found '...'.

Expected an expression here.

(parse)

[error] 15-15: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)

[error] 16-16: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)

[error] 15-16: This code is unreachable

... because this statement will throw an exception beforehand

(lint/correctness/noUnreachable)

⏰ Context from checks skipped due to timeout of 90000ms (1)

GitHub Check: Cloudflare Pages

🔇 Additional comments (4)

api/src/unraid-api/graph/resolvers/array/array.service.ts (3)

147-152: Good job implementing the JSDoc documentation!

The documentation clearly explains the purpose, parameters, and return value of the method, which is excellent for maintainability.

187-195: Good implementation of error handling for emcmd!

You've properly implemented try/catch handling for the emcmd call, which addresses a potential issue where emcmd could throw exceptions based on its implementation.

197-197: Good job returning the updated array data.

The method correctly returns the updated array data, which is consistent with other methods in this class.

api/src/unraid-api/auth/auth.interceptor.ts (1)

1-21: Use NestJS-provided exception for unauthorized access.

This implementation correctly follows the recommendation to use UnauthorizedException instead of a generic Error, but has syntax issues that need to be fixed.

🧰 Tools

🪛 Biome (1.9.4)

[error] 11-12: Illegal use of an import declaration not at the top level

move this declaration to the top level

(parse)

[error] 12-13: Expected an expression but instead found '...'.

Expected an expression here.

(parse)

[error] 15-15: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)

[error] 16-16: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)

[error] 15-16: This code is unreachable

... because this statement will throw an exception beforehand

(lint/correctness/noUnreachable)

🪛 GitHub Actions: CI - Main (API)

[error] 13-13: Parsing error: Declaration or statement expected

api/src/unraid-api/graph/resolvers/array/array.service.ts

api/src/unraid-api/auth/auth.interceptor.ts

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (22)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (2)
116-116: Use optional chaining for cleaner logic (line 116).

You can simplify the conditional by switching from a logical AND to optional chaining.

Here's an example diff:
- if (queryFields.protectedField && queryFields.protectedField.resolve) {
+ if (queryFields.protectedField?.resolve) {
🧰 Tools

🪛 Biome (1.9.4)

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

175-175: Use optional chaining for cleaner logic (line 175).

Similarly, consider simplifying with optional chaining for unprotectedField.
- if (queryFields.unprotectedField && queryFields.unprotectedField.resolve) {
+ if (queryFields.unprotectedField?.resolve) {
🧰 Tools

🪛 Biome (1.9.4)

[error] 175-175: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)
api/src/unraid-api/graph/directives/auth.directive.ts (2)

9-11: Address the pending TODO.

You have a warning in the doc comment that transformResolvers is not fully operational. Consider breaking down the issues and finalizing this function soon, as it’s a key part of applying permissions to each field.

196-219: Remove or use the createGraphQLEnumFromTSEnum function.

This helper function is currently unused. Either remove it to keep the file lean or consider employing it for other enums you plan to automatically convert from TypeScript to GraphQL.

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

70-70: Revisit transformResolvers usage.

The transform function remains commented out. If you plan to integrate field-level authorization checks, remember to re-enable and confirm that it properly wraps resolver logic.

api/src/unraid-api/graph/resolvers/vms/vms.service.ts (5)

16-16: Conditional availability flag.

isVmsAvailable is a helpful indicator, but certain errors set it to false without retry. Consider either periodic re-initialization or a manual approach to recover from transient failures.

29-35: Graceful handling of libvirt process checks.

The check for libvirt-pid is straightforward. For debug or production stability, ensure logs or additional monitoring exist to quickly detect ephemeral states (e.g., if the process restarts).

96-127: Validate or log unsupported transitions.

You handle valid transitions well, but consider more detailed logging or distinct error messages for invalid transitions to ease debugging.

190-207: Confirm force stop scenarios.

Forcing domain shutdown can lead to data corruption in certain scenarios. If possible, add a warning or user confirmation before forcibly stopping a running VM.

311-327: Shutdown polling logic.

The retry loop for shutting down the domain is clear, but for large or stalled VMs, 10 seconds may be insufficient. Consider a configurable timeout or more checks for safety.

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

151-307: Consider refactoring repeated waits.

Your tests use a loop with await new Promise((resolve) => setTimeout(resolve, 1000)); at multiple points. Creating a helper utility (e.g., await waitForVmState(...)) could improve readability and consistency, especially if you plan to add more VM lifecycle tests.

api/generated-schema.graphql (11)

3-15: Access URL Types:
The AccessUrl and AccessUrlInput types correctly capture the URL details. To further improve usability, consider adding descriptions for individual fields (e.g. clarifying the expected format for IPv4/IPv6).

36-59: API Key Types:
The definitions for ApiKey, ApiKeyResponse, and ApiKeyWithSecret are clear. Adding field-specific documentation could further assist API consumers in understanding each field’s purpose.

252-328: Array Mutation and State Types:
The ArrayMutations type and related state enums (ArrayPendingState, ArrayState, ArrayStateInput, and ArrayStateInputState) comprehensively cover array operations. Consider adding detailed error responses in mutation definitions to improve client feedback.

659-717: System Info Types:
The Info, InfoApps, InfoCpu, and InfoMemory types provide a comprehensive snapshot of system metrics. Consider documenting the expected units (e.g. for memory sizes or CPU speeds) to prevent interpretation errors.

812-872: Mutation Operations:
The Mutation type aggregates a broad set of operations—from permission and API key management to array actions and VM controls. Given its complexity, consider ensuring that each resolver is as modular as possible to maintain clarity and testability.

1067-1136: Query Type:
The Query type exposes diverse system data including API keys, arrays, logs, and virtual machines. For lists and logs, enforce pagination and filtering to safeguard performance in large data sets.

1138-1287: System Operation and Registration Types:
Types related to user registration, remote access (e.g., Registration, RegistrationState, RelayResponse, RemoteAccess), and resource management (Resource, Role, Server, Service, SetupRemoteAccessInput) are comprehensively defined. Note that input types like addUserInput and deleteUserInput would benefit from renaming to PascalCase for consistency.

1288-1319: Share Type Details:
The Share type is detailed with fields for inclusion, exclusion, capacity, and user comments. Consider adding validation hints or constraints for numerical fields such as free, total, and used space.

1374-1440: URL and Unassigned Device Types:
The scalar definitions for URL and UUID along with the UnassignedDevice type provide essential identifiers. Verify that external URLs are validated and that the data exposed does not reveal sensitive information.

1655-1682: Versions Type:
The Versions type compiles version details for various software components effectively. Adding short descriptions per field could further clarify the context for each version number.

1748-1776: User Input Naming Consistency:
The input types addUserInput, deleteUserInput, and usersInput are functional but should be renamed to follow PascalCase (e.g. AddUserInput, DeleteUserInput, UsersInput) to maintain consistency with the rest of the schema.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 2dc79c9 and 4efb8e7.

⛔ Files ignored due to path filters (3)

api/src/graphql/generated/api/operations.ts is excluded by !**/generated/**
api/src/graphql/generated/api/types.ts is excluded by !**/generated/**
pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (65)

.github/workflows/main.yml (2 hunks)
.gitignore (1 hunks)
api/codegen.ts (4 hunks)
api/dev/Unraid.net/myservers.cfg (1 hunks)
api/dev/Unraid.net/myservers.example.cfg (1 hunks)
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json (1 hunks)
api/dev/states/myservers.cfg (1 hunks)
api/generated-schema.graphql (1 hunks)
api/package.json (1 hunks)
api/src/__test__/common/allowed-origins.test.ts (1 hunks)
api/src/__test__/core/modules/array/add-disk-to-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/get-array-data.test.ts (0 hunks)
api/src/__test__/core/modules/array/remove-disk-from-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/update-array.test.ts (0 hunks)
api/src/__test__/core/modules/array/update-parity-check.test.ts (0 hunks)
api/src/__test__/graphql/resolvers/subscription/network.test.ts (1 hunks)
api/src/__test__/setup/config-setup.ts (1 hunks)
api/src/__test__/store/modules/paths.test.ts (1 hunks)
api/src/core/modules/array/add-disk-to-array.ts (0 hunks)
api/src/core/modules/array/index.ts (0 hunks)
api/src/core/modules/array/remove-disk-from-array.ts (0 hunks)
api/src/core/modules/array/update-array.ts (0 hunks)
api/src/core/modules/array/update-parity-check.ts (0 hunks)
api/src/core/modules/index.ts (0 hunks)
api/src/core/modules/vms/get-domains.ts (0 hunks)
api/src/core/modules/vms/index.ts (0 hunks)
api/src/core/utils/vms/get-hypervisor.ts (0 hunks)
api/src/core/utils/vms/index.ts (0 hunks)
api/src/core/utils/vms/parse-domain.ts (0 hunks)
api/src/core/utils/vms/parse-domains.ts (0 hunks)
api/src/graphql/schema/types/api-key/roles.graphql (1 hunks)
api/src/graphql/schema/types/base.graphql (1 hunks)
api/src/graphql/schema/types/vms/domain.graphql (1 hunks)
api/src/store/modules/paths.ts (1 hunks)
api/src/unraid-api/app/app.module.ts (2 hunks)
api/src/unraid-api/auth/api-key.service.spec.ts (3 hunks)
api/src/unraid-api/auth/api-key.service.ts (5 hunks)
api/src/unraid-api/auth/auth.interceptor.ts (1 hunks)
api/src/unraid-api/auth/auth.service.spec.ts (3 hunks)
api/src/unraid-api/auth/authentication.guard.ts (1 hunks)
api/src/unraid-api/auth/casbin/model.ts (1 hunks)
api/src/unraid-api/auth/casbin/policy.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.spec.ts (1 hunks)
api/src/unraid-api/graph/directives/auth.directive.ts (1 hunks)
api/src/unraid-api/graph/graph.module.ts (3 hunks)
api/src/unraid-api/graph/resolvers/array/array.service.ts (2 hunks)
api/src/unraid-api/graph/resolvers/online/online.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/resolvers.module.ts (2 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (2 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (1 hunks)
api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1 hunks)
api/src/unraid-api/graph/utils/auth-enum.utils.ts (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (5 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time (1 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (5 hunks)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (2 hunks)
api/vite.config.ts (1 hunks)
web/codegen.ts (1 hunks)
web/composables/gql/graphql.ts (7 hunks)

💤 Files with no reviewable changes (17)

api/src/test/core/modules/array/remove-disk-from-array.test.ts
api/src/test/core/modules/array/update-parity-check.test.ts
api/src/core/modules/vms/index.ts
api/src/test/core/modules/array/add-disk-to-array.test.ts
api/src/test/core/modules/array/update-array.test.ts
api/src/core/modules/index.ts
api/src/core/utils/vms/index.ts
api/src/core/modules/array/remove-disk-from-array.ts
api/src/core/modules/array/index.ts
api/src/core/utils/vms/parse-domains.ts
api/src/core/utils/vms/get-hypervisor.ts
api/src/core/modules/vms/get-domains.ts
api/src/core/utils/vms/parse-domain.ts
api/src/test/core/modules/array/get-array-data.test.ts
api/src/core/modules/array/update-array.ts
api/src/core/modules/array/add-disk-to-array.ts
api/src/core/modules/array/update-parity-check.ts

🚧 Files skipped from review as they are similar to previous changes (37)

api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/auth-request.php.last-download-time
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/.login.php.last-download-time
api/src/unraid-api/auth/casbin/policy.ts
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/Notifications.page.last-download-time
api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php.last-download-time
api/dev/Unraid.net/myservers.cfg
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch
api/src/test/store/modules/paths.test.ts
api/src/unraid-api/graph/utils/auth-enum.utils.ts
api/src/graphql/schema/types/base.graphql
api/src/test/setup/config-setup.ts
.github/workflows/main.yml
api/src/unraid-api/auth/casbin/model.ts
web/codegen.ts
api/src/unraid-api/app/app.module.ts
api/src/store/modules/paths.ts
.gitignore
api/src/unraid-api/auth/authentication.guard.ts
api/dev/Unraid.net/myservers.example.cfg
api/src/unraid-api/graph/resolvers/vms/vms.resolver.spec.ts
api/src/test/graphql/resolvers/subscription/network.test.ts
api/src/unraid-api/auth/auth.interceptor.ts
api/src/unraid-api/graph/resolvers/online/online.resolver.ts
api/src/unraid-api/auth/api-key.service.ts
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts
api/src/unraid-api/auth/auth.service.spec.ts
api/codegen.ts
api/src/unraid-api/auth/api-key.service.spec.ts
api/dev/states/myservers.cfg
api/src/unraid-api/unraid-file-modifier/modifications/test/snapshots/DefaultPageLayout.php.modified.snapshot.php
api/vite.config.ts
api/src/unraid-api/graph/resolvers/resolvers.module.ts
api/src/graphql/schema/types/api-key/roles.graphql
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts
api/package.json
api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php

🧰 Additional context used

🧬 Code Definitions (7)

api/src/unraid-api/graph/directives/auth.directive.spec.ts (2)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

api/src/unraid-api/graph/directives/auth.directive.ts (3)

getAuthEnumTypeDefs (165-194)

transformResolvers (13-112)

authSchemaTransformer (114-160)

api/src/unraid-api/graph/resolvers/array/array.service.ts (3)

api/src/graphql/generated/api/types.ts (1)

ArrayType (114-133)

api/src/core/utils/clients/emcmd.ts (1)

emcmd (12-41)

api/src/core/modules/array/get-array-data.ts (1)

getArrayData (9-61)

api/src/unraid-api/graph/directives/auth.directive.ts (1)

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

api/src/unraid-api/graph/resolvers/vms/vms.service.ts (3)

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

domain (26-35)

web/composables/gql/graphql.ts (1)

VmDomain (1871-1878)

api/src/graphql/generated/api/types.ts (1)

VmDomain (1843-1850)

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

api/src/unraid-api/graph/utils/auth-enum.utils.ts (1)

getAuthEnumTypeDefs (7-36)

api/src/graphql/schema/loadTypesDefs.ts (1)

loadTypeDefs (5-28)

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

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

domain (26-35)

web/composables/gql/graphql.ts (1)

api/src/graphql/generated/api/types.ts (10)

Maybe (5-5)

VmMutations (1852-1868)

Scalars (14-26)

VmMutationsforceStopVmArgs (1871-1873)

VmMutationspauseVmArgs (1876-1878)

VmMutationsrebootVmArgs (1881-1883)

VmMutationsresetVmArgs (1886-1888)

VmMutationsresumeVmArgs (1891-1893)

VmMutationsstartVmArgs (1896-1898)

VmMutationsstopVmArgs (1901-1903)

🪛 Biome (1.9.4)

api/src/unraid-api/graph/directives/auth.directive.spec.ts

[error] 116-116: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[error] 175-175: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

⏰ Context from checks skipped due to timeout of 90000ms (1)

GitHub Check: Cloudflare Pages

🔇 Additional comments (45)

api/src/__test__/common/allowed-origins.test.ts (4)

1-2: Updated imports to support enhanced testing.

The addition of getExtraOrigins import and the new getServerIps import are necessary for the enhanced test coverage. This aligns with the API summary indicating getExtraOrigins was added to the allowed-origins module.

11-42: Well-structured mocks for consistent testing behavior.

The mock implementations for network services and store state provide a controlled environment for testing. The explicit configuration of extraOrigins in the mock store will help validate that external origins are properly included.

43-64: Good test setup with consistent test data.

The beforeEach hook properly clears mocks and sets up consistent test data. The comprehensive array of different URL formats provides thorough coverage for testing the origin handling logic.

74-100: Improved test assertions with specific expectations.

The test now uses explicit expectations instead of snapshots, which makes the test more resilient to minor changes. The checks for different types of origins (socket paths, local URLs, connect URLs, extra origins, remote URLs) provide comprehensive coverage.

The duplicate check on line 100 is particularly valuable to ensure the uniqueness of origins.

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

4-4: Import is good to have.

No concerns with this newly added import.

146-206: Consider restricting pause/resume states when no check is running.

Currently, if no parity check is running, the code still allows pause or resume as valid states. This may not make sense functionally, as pausing or resuming a non-existent check might be confusing or error-prone. Consider filtering out pause and resume from allowedStates when no check is running, or confirm that this behavior is intended.

api/src/unraid-api/graph/directives/auth.directive.spec.ts (1)

28-28: Confirm necessity of 'describe.skip'.

All tests in this suite are skipped. Verify whether this is intentional, as it prevents test coverage and may hide potential regressions.

api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.spec.ts (1)

1-75: Tests look effective and well-structured.

The unit tests thoroughly verify the resolver’s interactions with the service, including error propagation.

api/src/unraid-api/graph/directives/auth.directive.ts (1)

165-194: Eliminate duplicate implementations.

A similar definition of the @auth directive and enum type generation exists in auth-enum.utils.ts. Consolidate them in a single file or remove one to avoid conflicts and confusion.

api/src/unraid-api/graph/graph.module.ts (4)

33-35: Dependency injection for AuthZService.

Injecting AuthZService indicates plans to enforce authorization. Ensure all logic referencing it is fully tested, especially if you re-enable transformResolvers or transformSchema.

37-48: Verify the presence of authEnumTypeDefs.

Merging the generated auth enum definitions with plugin-based typeDefs is beneficial. Confirm that no conflicting or duplicated directives are introduced when combining them.

52-58: Async context setup looks good.

The code properly returns an object containing the request, connection parameters, and extra metadata within an async context function. This approach is clean and clear.

77-78: Exporting GraphQLModule.

Exporting GraphQLModule ensures it’s accessible to other modules. Make sure the rest of your application modules import this correctly if they need immediate access to GraphQL features.

api/src/graphql/schema/types/vms/domain.graphql (3)

6-9: Returning Boolean! might limit flexibility [duplicate feedback].

If you find yourself needing to return additional data (e.g., status messages, updated fields, error info), consider a more detailed type beyond Boolean!. This mirrors a past recommendation regarding the same pattern.

11-25: Each VM mutation returns only Boolean! [duplicate feedback].

Similar to the existing mutation field under type Mutation, you might benefit from returning a more descriptive type if you anticipate expanded responses (such as partial VM data or error details).

32-35: Looks good!

Defining type Vms with an id and a domain field is straightforward and consistent with GraphQL’s schema design best practices.

api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (3)

20-51: Well-structured architecture detection logic.

The getArchConfig() function reliably handles multiple combinations of platform and architecture. This approach clarifies the extended branches for x86 vs. ARM, and macOS vs. other Unix-like environments.

53-63: Straightforward QEMU presence check.

Verifying QEMU installation via existsSync in combination with your multi-arch config is concise. No issues detected.

65-149: Robust cleanup utilities.

The disk image and domain XML cleanup, as well as cleanupDomain() for stopping and undefining domains, are well-structured. This thorough teardown approach helps maintain a clean test environment.

web/composables/gql/graphql.ts (6)

329-342: Well-structured enum definitions for authentication.

The new AuthActionVerb and AuthPossession enums provide clear categorization of authentication actions and possession types, which will be useful for role-based access control throughout the application.

880-889: Good implementation of VM mutations with proper permission documentation.

The addition of the vms field to the Mutation type properly follows the existing pattern in the schema. The permission documentation is thorough and clearly specifies the required action, resource, and possession type.

1262-1270: Consistent permission documentation pattern.

The permission documentation for the vms field in the Query type matches the style used in the Mutation type, ensuring consistency throughout the schema.

1388-1416: Good standardization of Resource enum values.

Converting the Resource enum values to uppercase improves consistency across the schema and follows naming conventions for enum constants.

1420-1423: Consistent format for Role enum values.

Converting the Role enum values to uppercase maintains consistency with the updated Resource enum and follows best practices for enum constants.

1880-1987: Comprehensive VM mutation operations with proper typing.

The VmMutations type includes a full set of VM control operations (start, stop, pause, resume, etc.) with:

Proper return types using Scalars['Boolean']['output']

Well-defined argument types requiring VM ID

Detailed permission documentation for each operation

Consistent naming and documentation patterns

This implementation will enable proper frontend control of virtual machines through the GraphQL API.

api/generated-schema.graphql (20)

1-1: Authorization Directive Defined:
The new @auth directive is clearly specified with non-nullable arguments on FIELD_DEFINITION.

330-343: Auth Enums Consistency:
The AuthActionVerb and AuthPossession enums are defined correctly and complement the directive. Ensure that their values remain in sync with the resolver logic.

17-34: Permission and Role Inputs:
Input types such as AddPermissionInput, AddRoleForApiKeyInput, AddRoleForUserInput, and AllowedOriginInput are well structured. They provide a clear contract for permission/role operations.

60-88: API Settings Input:
The ApiSettingsInput type is well documented with inline comments explaining constraints and field interactions. Ensure that the resolver logic enforces the described field relationships.

90-187: Array and Disk Types:
The Array, ArrayCapacity, and ArrayDisk types are detailed and include useful documentation. Verify that numeric fields (e.g. disk percentages and sizes) are validated and transformed appropriately in the backend.

189-250: Array Disk Enums and Input:
The enums (ArrayDiskFsColor, ArrayDiskStatus, ArrayDiskType) along with the ArrayDiskInput are well defined. Confirm that each enum value accurately reflects the underlying system status.

345-379: System Hardware and Cloud Types:
Types like Baseboard, Capacity, Case, Cloud, and CloudResponse are defined clearly. While they are functional, adding consistent field-level descriptions might further enhance clarity.

395-476: Connect and Container Types:
The types for remote access and container configuration (including Connect, ConnectSettings, ConnectSettingsValues, ConnectSignInInput, ConnectUserInfoInput, ContainerHostConfig, ContainerMount, ContainerPort, ContainerPortType, and ContainerState) are organized and clear. Ensure that optional fields are handled gracefully during resolution.

478-489: Create API Key and DateTime Scalar:
The CreateApiKeyInput and scalar DateTime are defined appropriately. Be sure that input validation logic confirms that all constraints (such as key uniqueness) are respected in the implementation.

491-539: Devices and Disk Partitioning:
The Devices, Disk, and DiskPartition types offer an extensive view of the underlying hardware details. Confirm that the backend supplies all required fields, and verify that values such as sizes and status indicators match the hardware’s state accurately.

546-618: Display and Docker Types:
Definitions for display configurations and Docker-related types (Display, Docker, DockerContainer, DockerMutations, and DockerNetwork) are well constructed. Make sure that the JSON scalar fields are properly sanitized prior to use.

620-657: Remote Access, Flash, and GPU Types:
The DynamicRemoteAccessStatus, Flash, and Gpu types are succinct and clear. Double-check that numerical and Boolean fields reliably reflect the underlying service state.

721-748: File and Log Types:
The KeyFile, LogFile, and LogFileContent types are effectively designed and include helpful inline comments. Their clarity will aid in proper log handling in the API.

756-783: Scalar and Memory Types:
The definition of scalar Long along with the Me, MemoryFormFactor, and MemoryLayout types adhere to expected conventions. Consistency in unit representation (e.g. bytes, MHz) should be verified within the resolvers.

791-810: Minigraphql and Mount Types:
The MinigraphqlResponse and Mount types add valuable metadata. Their fields are self-explanatory, which should facilitate easy integration with clients.

1321-1354: Subscription Type:
Subscriptions are set up to deliver real-time updates (e.g., log file changes and notifications). Ensure that subscription resolvers are rigorously secured and efficiently implemented.

1356-1372: System and Theme Types:
The System type and enums for Temperature and Theme are straightforward. It would be beneficial to document the scale or expected values for temperature measurements for clarity.

1473-1653: Vars Type Modularization:
The Vars type is very large and incorporates a broad range of configuration parameters. As noted in previous reviews, consider splitting this type into smaller, modular components to improve readability and maintainability.

1684-1732: Virtual Machine Types:
The new VM-related types (VmDomain, VmMutations, VmState, and Vms) align well with the basic VM controls introduced in this PR. Ensure that state transitions are validated appropriately in the backend logic.

1733-1747: WAN Access and Welcome Types:
The WAN_ACCESS_TYPE, WAN_FORWARD_TYPE enums and the Welcome type are defined in a conventional and clear manner.

coderabbitai · 2025-04-03T19:58:35Z

api/generated-schema.graphql

+"""A local user account"""
+type User implements UserAccount {
+  description: String!
+  id: ID!
+
+  """A unique name for the user"""
+  name: String!
+
+  """If the account has a password set"""
+  password: Boolean
+  permissions: [Permission!]
+  roles: [Role!]!
+}
+
+interface UserAccount {
+  description: String!
+  id: ID!
+  name: String!
+  permissions: [Permission!]
+  roles: [Role!]!
+}


⚠️ Potential issue

User and Account Types:
The User type and the UserAccount interface correctly encapsulate user data. However, the password field (being a Boolean) should be handled carefully to ensure it does not leak sensitive state information.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

api/src/__test__/graphql/resolvers/subscription/network.test.ts (2)
1-1: Unused import detected

The vi import from 'vitest' is included but doesn't appear to be used in this file. Consider removing it if not needed.
-import { expect, test, vi } from 'vitest';
+import { expect, test } from 'vitest';
194-226: Improved test robustness with dynamic validation

Good improvement on the test approach - moving from static snapshots to dynamic structure validation makes the test more resilient to minor changes while still ensuring correct functionality.

Consider these additional enhancements:

The URL pattern validation is repeated with similar regex for each type. This could be refactored to reduce duplication:
+// Define expected URL patterns by type
+const URL_PATTERNS = {
+  [URL_TYPE.DEFAULT]: /^https:\/\/.*:\d+\/$/,
+  [URL_TYPE.LAN]: /^https:\/\/.*:\d+\/$/,
+  [URL_TYPE.MDNS]: /^https:\/\/.*:\d+\/$/,
+  [URL_TYPE.WIREGUARD]: /^https:\/\/.*:\d+\/$/,
+};

 // Verify that the URL matches the expected pattern based on its type
-if (url.type === URL_TYPE.DEFAULT) {
-    expect(url.ipv4?.toString()).toMatch(/^https:\/\/.*:\d+\/$/);
-    expect(url.ipv6?.toString()).toMatch(/^https:\/\/.*:\d+\/$/);
-} else if (url.type === URL_TYPE.LAN) {
-    expect(url.ipv4?.toString()).toMatch(/^https:\/\/.*:\d+\/$/);
-} else if (url.type === URL_TYPE.MDNS) {
-    expect(url.ipv4?.toString()).toMatch(/^https:\/\/.*:\d+\/$/);
-} else if (url.type === URL_TYPE.WIREGUARD) {
-    expect(url.ipv4?.toString()).toMatch(/^https:\/\/.*:\d+\/$/);
-}
+expect(url.ipv4?.toString()).toMatch(URL_PATTERNS[url.type]);
+if (url.type === URL_TYPE.DEFAULT && url.ipv6) {
+    expect(url.ipv6.toString()).toMatch(URL_PATTERNS[url.type]);
+}
Consider validating the name property values as well, not just their existence.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 4efb8e7 and d2c27e1.

📒 Files selected for processing (1)

api/src/__test__/graphql/resolvers/subscription/network.test.ts (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (4)

GitHub Check: Build Web App
GitHub Check: Build API
GitHub Check: Test API
GitHub Check: Cloudflare Pages

github-actions · 2025-04-03T20:11:28Z

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/1293/dynamix.unraid.net.plg

🤖 I have created a release *beep* *boop* --- ## [4.7.0](v4.6.6...v4.7.0) (2025-04-24) ### Features * add basic docker network listing ([#1317](#1317)) ([c4fdff8](c4fdff8)) * add permission documentation by using a custom decorator ([#1355](#1355)) ([45ecab6](45ecab6)) * basic vm controls ([#1293](#1293)) ([bc3ca92](bc3ca92)) * code first graphql ([#1347](#1347)) ([f5724ab](f5724ab)) ### Bug Fixes * container names always null ([#1335](#1335)) ([8a5b238](8a5b238)) * **deps:** update all non-major dependencies ([#1337](#1337)) ([2345732](2345732)) * hide reboot notice for patch releases ([#1341](#1341)) ([4b57439](4b57439)) * move docker mutations to the mutations resolver ([#1333](#1333)) ([1bbe7d2](1bbe7d2)) * PR build issue ([457d338](457d338)) * remove some unused fields from the report object ([#1342](#1342)) ([cd323ac](cd323ac)) * sso unreliable if API outputs more than raw json ([#1353](#1353)) ([e65775f](e65775f)) * vms now can detect starting of libvirt and start local hypervisor ([#1356](#1356)) ([ad0f4c8](ad0f4c8)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

elibosley requested review from mdatelle, pujitm and zackspear as code owners March 28, 2025 20:35

mdatelle approved these changes Mar 31, 2025

View reviewed changes

elibosley changed the title ~~Feat/basic-vm-controls~~ feat: basic vm controls Mar 31, 2025

elibosley force-pushed the feat/basic-docker-controls branch 2 times, most recently from 044e2e6 to 3804367 Compare March 31, 2025 19:52

elibosley force-pushed the feat/basic-vm-controls branch 2 times, most recently from 331d4ed to 639f342 Compare March 31, 2025 20:20

Base automatically changed from feat/basic-docker-controls to main March 31, 2025 20:21

elibosley force-pushed the feat/basic-vm-controls branch from 639f342 to f3514b1 Compare March 31, 2025 20:23

coderabbitai bot reviewed Mar 31, 2025

View reviewed changes

api/src/unraid-api/graph/resolvers/online/online.resolver.ts Outdated Show resolved Hide resolved

coderabbitai bot reviewed Apr 1, 2025

View reviewed changes

api/src/unraid-api/auth/auth.interceptor.ts Outdated Show resolved Hide resolved

api/src/graphql/schema/types/vms/domain.graphql Show resolved Hide resolved

coderabbitai bot reviewed Apr 1, 2025

View reviewed changes

api/src/core/modules/array/update-array.ts Outdated Show resolved Hide resolved

coderabbitai bot reviewed Apr 1, 2025

View reviewed changes

api/src/unraid-api/graph/resolvers/array/array.service.ts Outdated Show resolved Hide resolved

elibosley force-pushed the feat/basic-vm-controls branch from 1b5db8a to 4d35828 Compare April 2, 2025 18:56

coderabbitai bot reviewed Apr 2, 2025

View reviewed changes

api/src/unraid-api/graph/resolvers/vms/vms.service.ts Show resolved Hide resolved

coderabbitai bot reviewed Apr 2, 2025

View reviewed changes

elibosley force-pushed the feat/basic-vm-controls branch from f34c6d3 to 9335a47 Compare April 3, 2025 13:53

coderabbitai bot reviewed Apr 3, 2025

View reviewed changes

elibosley force-pushed the feat/basic-vm-controls branch from 9335a47 to a193307 Compare April 3, 2025 16:48

coderabbitai bot reviewed Apr 3, 2025

View reviewed changes

elibosley and others added 17 commits April 3, 2025 15:52

chore: add unit test around auth schema

15e6f20

chore: fix tests

1d80119

chore: fix return

108ac5f

chore: fix a couple of lint errors

3f47764

fix: disable decorator and fix unit tests

698ebb3

chore: fix type-check

a45850b

chore: remove unused array logic and fix flaky test

a36bdba

chore: fix log on vm URI

48ab2a2

fix: update patch file

a7cc7d3

chore: update generated schema

1a32fb6

chore: allowed origins test fixed

c0a0803

chore: lint

784c27c

Update api/src/unraid-api/auth/auth.interceptor.ts

0118144

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Update api/src/unraid-api/graph/resolvers/array/array.service.ts

6c491e1

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

chore: lint

730c1d8

chore: fix type error

b18d625

chore: lint

4efb8e7

elibosley force-pushed the feat/basic-vm-controls branch from 2dc79c9 to 4efb8e7 Compare April 3, 2025 19:52

coderabbitai bot reviewed Apr 3, 2025

View reviewed changes

chore: resolve issues with network test

d2c27e1

coderabbitai bot reviewed Apr 3, 2025

View reviewed changes

elibosley merged commit bc3ca92 into main Apr 4, 2025
9 checks passed

elibosley deleted the feat/basic-vm-controls branch April 4, 2025 13:52

github-actions bot mentioned this pull request Apr 4, 2025

chore(main): release 4.7.0 #1332

Merged

coderabbitai bot mentioned this pull request Apr 29, 2025

chore: add a prefix scalar instead of prefix plugin #1361

Merged

coderabbitai bot mentioned this pull request Jun 3, 2025

chore: extract connect to an API plugin #1367

Merged

coderabbitai bot mentioned this pull request Aug 23, 2025

feat: generated UI API key management + OAuth-like API Key Flows #1609

Merged

github-actions bot mentioned this pull request Oct 27, 2025

chore(main): release 4.26.0 #1744

Open

Uh oh!

feat: basic vm controls #1293

feat: basic vm controls #1293

Uh oh!

Conversation

elibosley commented Mar 28, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Possibly related PRs

Poem

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

elibosley commented Mar 28, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 28, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)