feat: basic array controls #1291
Conversation
WalkthroughThe changes include updates to configuration files, code cleanups, enhanced error handling, and new GraphQL definitions. The version number in the configuration files is updated, and a connection status field is modified. An unused import is removed, and error handling in GET requests is refined. New GraphQL enumerations, input types, and mutations are introduced. Additionally, new resolver classes and a service for managing arrays are added, along with updates to the dependency injection module. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as GraphQL Client
participant MR as MutationResolver
participant AMR as ArrayMutationsResolver
participant AS as ArrayService
Client->>MR: Request array mutations
MR-->>Client: { __typename: 'ArrayMutations' }
Client->>AMR: setState(input)
AMR->>AS: updateArrayState(input)
AS-->>AMR: Updated Array data
AMR-->>Client: Array mutation result
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
api/src/core/utils/clients/emcmd.ts (1)
5-5: Improved error handling for Unix socket connectivity.The changes enhance error handling by providing a specific error message when the socket is unavailable. The addition of
enableUnixSockets: trueis necessary for Unix socket support with the 'got' library.However, there are formatting issues flagged by static analysis:
- .get(url, { + .get(url, {- searchParams: { ...commands, csrf_token: csrfToken } + searchParams: { ...commands, csrf_token: csrfToken },Also applies to: 31-40
api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (5)
1-8: Consider adhering to multi-line import style and absolute paths.
Static analysis indicates that line 5 should be multi-line and line 7 should use an absolute path. These are minor style fixes.-import { ArrayDiskInput, ArrayStateInput, Resource, ArrayMutations } from '@app/graphql/generated/api/types.js'; +import { + ArrayDiskInput, + ArrayMutations, + ArrayStateInput, + Resource, +} from '@app/graphql/generated/api/types.js';🧰 Tools
🪛 GitHub Check: Test API
[failure] 5-5:
Replace·ArrayDiskInput,·ArrayStateInput,·Resource,·ArrayMutations·with⏎····ArrayDiskInput,⏎····ArrayMutations,⏎····ArrayStateInput,⏎····Resource,⏎
[failure] 7-7:
import statements should have an absolute path🪛 GitHub Check: Build API
[failure] 5-5:
Replace·ArrayDiskInput,·ArrayStateInput,·Resource,·ArrayMutations·with⏎····ArrayDiskInput,⏎····ArrayMutations,⏎····ArrayStateInput,⏎····Resource,⏎
[failure] 7-7:
import statements should have an absolute path
23-32:addDiskToArrayis straightforward.
Checks for a running array are appropriate. Consider a helpful error message if the disk ID is invalid.
43-52:mountArrayDiskplaceholder.
Since it’s unimplemented, consider adding a detailed TODO comment if you intend to handle special errors or checks.
53-62:unmountArrayDiskplaceholder.
Similar tomountArrayDisk, it’s a placeholder. Consider specifying intended logic to avoid confusion.
63-72:clearArrayDiskStatisticsplaceholder.
Explicitly document if this will reset counters or clear logs, so it’s clear for team members.api/src/unraid-api/graph/resolvers/array/array.service.ts (5)
1-2: Check import reordering.
Static analysis suggests reordering or removing certain imports. Confirm if they’re all needed and follow your style guide.Also applies to: 10-14
31-68:updateArrayStateconcurrency handling.
Settingthis.pendingStatehelps avoid multiple triggers. Consider a more robust lock if other requests can mutate array state concurrently (e.g. distributed lock).🧰 Tools
🪛 GitHub Check: Test API
[failure] 40-40:
Replacenew·AppError(Array·state·is·still·being·updated.·Changing·to·${pendingState})with⏎················new·AppError(Array·state·is·still·being·updated.·Changing·to·${pendingState})⏎············🪛 GitHub Check: Build API
[failure] 40-40:
Replacenew·AppError(Array·state·is·still·being·updated.·Changing·to·${pendingState})with⏎················new·AppError(Array·state·is·still·being·updated.·Changing·to·${pendingState})⏎············
70-85:addDiskToArraylogic is concise.
Preventing disk addition while running is a nice safeguard. Optionally validatediskIdor slot availability before applying changes.
104-112:mountArrayDiskandunmountArrayDiskare placeholders.
Document your future approach (e.g., do you plan system calls, logging, or checks?).
114-118:clearArrayDiskStatisticsplaceholder.
Clarify expected behavior (e.g., reset counters to zero?). A short note can help future development.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (10)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/dev/states/myservers.cfg(2 hunks)api/src/core/modules/array/get-array-data.ts(0 hunks)api/src/core/utils/clients/emcmd.ts(2 hunks)api/src/graphql/schema/types/array/array.graphql(1 hunks)api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(3 hunks)
💤 Files with no reviewable changes (1)
- api/src/core/modules/array/get-array-data.ts
🧰 Additional context used
🧬 Code Definitions (2)
api/src/unraid-api/graph/resolvers/array/array.service.ts (3)
api/src/graphql/generated/api/types.ts (3)
ArrayStateInput(312-315)ArrayType(112-131)ArrayDiskInput(194-199)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/resolvers/array/array.mutations.resolver.ts (3)
api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1)
Resolver(11-34)api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts (1)
Resolver(3-11)api/src/graphql/generated/api/types.ts (3)
Resolver(1886-1886)ArrayStateInput(312-315)ArrayDiskInput(194-199)
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts
[failure] 1-1:
Replace r,·ResolveField with Field,·Resolver
api/src/core/utils/clients/emcmd.ts
[failure] 31-31:
Delete ·
[failure] 33-33:
Replace · with ,
api/src/unraid-api/graph/resolvers/array/array.service.ts
[failure] 10-10:
Insert DiskInput,⏎····Array
[failure] 14-14:
Delete ,⏎····ArrayDiskInput
[failure] 40-40:
Replace new·AppError(Array·state·is·still·being·updated.·Changing·to·${pendingState}) with ⏎················new·AppError(Array·state·is·still·being·updated.·Changing·to·${pendingState})⏎············
api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts
[failure] 5-5:
Replace ·ArrayDiskInput,·ArrayStateInput,·Resource,·ArrayMutations· with ⏎····ArrayDiskInput,⏎····ArrayMutations,⏎····ArrayStateInput,⏎····Resource,⏎
[failure] 7-7:
import statements should have an absolute path
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts
[failure] 1-1:
Replace r,·ResolveField with Field,·Resolver
api/src/core/utils/clients/emcmd.ts
[failure] 31-31:
Delete ·
[failure] 33-33:
Replace · with ,
api/src/unraid-api/graph/resolvers/array/array.service.ts
[failure] 10-10:
Insert DiskInput,⏎····Array
[failure] 14-14:
Delete ,⏎····ArrayDiskInput
[failure] 40-40:
Replace new·AppError(Array·state·is·still·being·updated.·Changing·to·${pendingState}) with ⏎················new·AppError(Array·state·is·still·being·updated.·Changing·to·${pendingState})⏎············
api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts
[failure] 5-5:
Replace ·ArrayDiskInput,·ArrayStateInput,·Resource,·ArrayMutations· with ⏎····ArrayDiskInput,⏎····ArrayMutations,⏎····ArrayStateInput,⏎····Resource,⏎
[failure] 7-7:
import statements should have an absolute path
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (14)
api/dev/Unraid.net/myservers.cfg (1)
2-2: Version number updatedThe API version has been updated from 4.1.3 to 4.4.1. This change is consistent with updates in other configuration files.
api/dev/states/myservers.cfg (2)
2-2: Version number updatedThe API version has been updated from 4.1.3 to 4.4.1, maintaining consistency with other configuration files.
23-23: Connection status value changedThe minigraph connection status has been changed from "PRE_INIT" to "ERROR_RETRYING". This change likely reflects an update in the connection status handling logic related to the resolver modifications mentioned in the PR title.
api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1)
9-9: Appropriate use of dependency injection for ArrayService.The implementation correctly injects the ArrayService using NestJS dependency injection pattern. The service is properly marked as private and readonly, following best practices.
Also applies to: 13-14
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
6-6: Module correctly updated with new providers.The resolvers module has been properly updated to include the new resolver and service components. The organization is logical, with array-related providers grouped together.
Also applies to: 8-8, 19-19, 33-34, 42-42
api/src/graphql/schema/types/array/array.graphql (5)
6-11: Enum usage looks good.
By addingSTARTandSTOP, you've clarified array state transitions.
13-16: Clear input type definition.
DefiningArrayStateInputwith a mandatorydesiredStateis straightforward and consistent with the enum.
18-20: Well-structured mutation fields.
IntroducingArrayMutationswithsetState,addDiskToArray, andremoveDiskFromArrayis a sensible approach for grouping array-related operations.Also applies to: 23-23, 25-25
33-35: Good schema extensibility for mutations.
Addingarray: ArrayMutationstoMutationkeeps your schema modular and scalable.
41-41: Renamed input type is consistent.
Renaming and usingArrayDiskInputaligns with the new mutation signatures.api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (2)
13-22: Well-definedsetStatemutation.
Logic is clean. Ensure the associated service method handles concurrent updates consistently if multiple clients request state changes simultaneously.
33-42:removeDiskFromArraylogic is consistent.
MirrorsaddDiskToArray. Same note on validating disk presence.api/src/unraid-api/graph/resolvers/array/array.service.ts (2)
26-29:arrayIsRunningmethod is clean.
Clear usage ofgetters.emhttp().var.mdState. Ensure this remains synchronized with actual array status.
87-102:removeDiskFromArrayapproach matchesaddDiskToArray.
Keeping them symmetrical is good practice. Similar checks on valid disk ID or slot can be added.
| import { Resolver, ResolveField } from '@nestjs/graphql'; | ||
|
|
||
| @Resolver('Mutation') | ||
| export class MutationResolver { | ||
| @ResolveField() | ||
| public async array() { | ||
| return { | ||
| __typename: 'ArrayMutations' | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix import format according to static analysis.
The implementation follows good GraphQL patterns by using the "delegate to type" pattern for organizing mutations into namespaces. However, static analysis tools suggest replacing the imports:
-import { Resolver, ResolveField } from '@nestjs/graphql';
+import { Field, Resolver } from '@nestjs/graphql';Also, ensure that this pattern aligns with how other resolvers are structured in the codebase.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Resolver, ResolveField } from '@nestjs/graphql'; | |
| @Resolver('Mutation') | |
| export class MutationResolver { | |
| @ResolveField() | |
| public async array() { | |
| return { | |
| __typename: 'ArrayMutations' | |
| }; | |
| } | |
| } | |
| import { Field, Resolver } from '@nestjs/graphql'; | |
| @Resolver('Mutation') | |
| export class MutationResolver { | |
| @ResolveField() | |
| public async array() { | |
| return { | |
| __typename: 'ArrayMutations' | |
| }; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: Test API
[failure] 1-1:
Replace r,·ResolveField with Field,·Resolver
🪛 GitHub Check: Build API
[failure] 1-1:
Replace r,·ResolveField with Field,·Resolver
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (3)
13-16: Consider switching to a typed approach.
Using@Resolver(() => ArrayMutations)instead of@Resolver('ArrayMutations')can provide stronger type checks and clearer reflection data. This is optional but can enhance readability.
57-65: Potential runtime error for unimplemented unmount functionality.
Same situation as mount—currently throws an error if called. Let me know if you need help implementing.
67-75: clearArrayDiskStatistics not yet implemented.
Calling this will throw a "Not implemented" error. Confirm if this is an intended placeholder.api/src/unraid-api/graph/resolvers/array/array.service.ts (4)
31-70: Concurrency approach is acceptable but might need more robust locking.
You're usingpendingStateto block repeated starts/stops, which is good as a minimal approach. Consider a more advanced mechanism if there's a risk of multiple concurrent calls.
106-109: mountArrayDisk not implemented.
Currently throws an error. Let me know if you’d like assistance implementing a mount command.
111-114: unmountArrayDisk not implemented.
Same approach as mount; verifying if this is intentional.
116-119: clearArrayDiskStatistics is also yet to be implemented.
Consistent placeholder approach, but confirm next steps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
api/src/core/utils/clients/emcmd.ts(2 hunks)api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts
- api/src/core/utils/clients/emcmd.ts
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (3)
api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts (1)
Resolver(3-11)api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1)
Resolver(11-34)api/src/graphql/generated/api/types.ts (3)
Resolver(1886-1886)ArrayStateInput(312-315)ArrayDiskInput(194-199)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (6)
1-4: Imports look solid.
No issues found with the NestJS and nest-authz imports.
5-12: GraphQL type imports and service injection are coherent.
The references toArrayDiskInput,ArrayMutations, andArrayStateInputalign nicely with the corresponding type definitions.
17-25: setState resolver logic looks valid.
Properly calls the service method and enforcesUPDATEpermissions onResource.ARRAY.
27-35: addDiskToArray is straightforward.
Correctly delegates to the service, ensuring the same permission constraints.
37-45: removeDiskFromArray is well-defined.
Mirrors the behavior of addDiskToArray and also looks good.
47-55: Potential runtime error for unimplemented mount functionality.
SincemountArrayDiskis not yet implemented in the service, calling this resolver will throw an error. Verify whether this is a placeholder or needs immediate implementation.api/src/unraid-api/graph/resolvers/array/array.service.ts (7)
1-2: Imports from NestJS and common libraries look good.
No immediate concerns.
3-4: Case conversion imports recognized.
capitalCaseandconstantCaseare used appropriately in later code.
5-18: Error and utility modules are well-structured.
Integrating these error classes and functions helps maintain a clean architecture.
19-22: Injectable service with pendingState tracking.
Allows for concurrency management, though it might require more robust locking under high load.
23-29: arrayIsRunning helper is concise.
Returns a boolean based onmdState. Straightforward approach.
72-87: addDiskToArray method checks array state before modifying.
This pattern helps prevent invalid operations. Looks good.
89-104: removeDiskFromArray mirror logic is correct.
Matches the approach in addDiskToArray and returns updated data.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
api/src/unraid-api/graph/resolvers/array/array.service.spec.ts (1)
171-190: Apply formatting fixes for thrown errors.
The pipeline suggests each string be on a separate line for style compliance. Consider updating these to meet the required formatting. For instance:- await expect(service.addDiskToArray(input)).rejects.toThrow('Array needs to be stopped before any changes can occur.'); + await expect(service.addDiskToArray(input)).rejects.toThrow( + 'Array needs to be stopped before any changes can occur.' + );Make similar changes for lines 186 and 190 regarding multiline or indentation style to satisfy the pipeline’s checks.
🧰 Tools
🪛 GitHub Check: Build API
[failure] 171-171:
Replace'Array·needs·to·be·stopped·before·any·changes·can·occur.'with⏎············'Array·needs·to·be·stopped·before·any·changes·can·occur.'⏎········
[failure] 186-186:
Replace'Array·needs·to·be·stopped·before·any·changes·can·occur.'with⏎············'Array·needs·to·be·stopped·before·any·changes·can·occur.'⏎········
[failure] 190-190:
Replace'Array·must·be·running·to·mount·disks'with⏎············'Array·must·be·running·to·mount·disks'⏎········🪛 GitHub Check: Test API
[failure] 171-171:
Replace'Array·needs·to·be·stopped·before·any·changes·can·occur.'with⏎············'Array·needs·to·be·stopped·before·any·changes·can·occur.'⏎········
[failure] 186-186:
Replace'Array·needs·to·be·stopped·before·any·changes·can·occur.'with⏎············'Array·needs·to·be·stopped·before·any·changes·can·occur.'⏎········
[failure] 190-190:
Replace'Array·must·be·running·to·mount·disks'with⏎············'Array·must·be·running·to·mount·disks'⏎········api/src/unraid-api/graph/resolvers/array/array.service.ts (3)
104-116: Return a more precise type instead ofPromise<any>.
Currently,mountArrayDiskis declared to returnPromise<any>. For stronger type safety, replace it withPromise<ArrayType>or the appropriate interface that reflects the shape of the returned data.- async mountArrayDisk(id: string): Promise<any> { + async mountArrayDisk(id: string): Promise<ArrayType> {
118-130: Return type forunmountArrayDiskshould be explicit.
Similarly, avoidanyin favor of a dedicated type such asArrayType.
132-144: Refine return type forclearArrayDiskStatistics.
Again, specify the method’s return type to ensure consistency among all disk operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/src/unraid-api/graph/resolvers/array/array.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/resolvers/array/array.service.ts (4)
api/src/graphql/generated/api/types.ts (3)
ArrayStateInput(312-315)ArrayType(112-131)ArrayDiskInput(194-199)api/src/core/utils/clients/emcmd.ts (1)
emcmd(12-41)api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1)
array(21-23)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.resolver.spec.ts
[failure] 6-6:
import statements should have an absolute path
[failure] 7-7:
import statements should have an absolute path
api/src/unraid-api/graph/resolvers/array/array.service.spec.ts
[failure] 6-6:
import statements should have an absolute path
[failure] 6-6:
Delete {·ArrayService·}·from·'./array.service.js';⏎import·
[failure] 8-8:
Replace ArrayState,·ArrayStateInputState·}·from·'@app/graphql/generated/api/types with getArrayData·}·from·'@app/core/modules/array/get-array-data
[failure] 10-10:
Replace getters·}·from·'@app/store/index with ArrayState,·ArrayStateInputState·}·from·'@app/graphql/generated/api/types
[failure] 11-11:
Replace ArrayData·}·from·'@app/core/modules/array/get-array-data with ters·}·from·'@app/store/index.js';⏎⏎import·{·ArrayService·}·from·'./array.service
[failure] 171-171:
Replace 'Array·needs·to·be·stopped·before·any·changes·can·occur.' with ⏎············'Array·needs·to·be·stopped·before·any·changes·can·occur.'⏎········
[failure] 186-186:
Replace 'Array·needs·to·be·stopped·before·any·changes·can·occur.' with ⏎············'Array·needs·to·be·stopped·before·any·changes·can·occur.'⏎········
[failure] 190-190:
Replace 'Array·must·be·running·to·mount·disks' with ⏎············'Array·must·be·running·to·mount·disks'⏎········
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/array/array.resolver.spec.ts
[failure] 6-6:
import statements should have an absolute path
[failure] 7-7:
import statements should have an absolute path
api/src/unraid-api/graph/resolvers/array/array.service.spec.ts
[failure] 6-6:
import statements should have an absolute path
[failure] 6-6:
Delete {·ArrayService·}·from·'./array.service.js';⏎import·
[failure] 8-8:
Replace ArrayState,·ArrayStateInputState·}·from·'@app/graphql/generated/api/types with getArrayData·}·from·'@app/core/modules/array/get-array-data
[failure] 10-10:
Replace getters·}·from·'@app/store/index with ArrayState,·ArrayStateInputState·}·from·'@app/graphql/generated/api/types
[failure] 11-11:
Replace ArrayData·}·from·'@app/core/modules/array/get-array-data with ters·}·from·'@app/store/index.js';⏎⏎import·{·ArrayService·}·from·'./array.service
[failure] 171-171:
Replace 'Array·needs·to·be·stopped·before·any·changes·can·occur.' with ⏎············'Array·needs·to·be·stopped·before·any·changes·can·occur.'⏎········
[failure] 186-186:
Replace 'Array·needs·to·be·stopped·before·any·changes·can·occur.' with ⏎············'Array·needs·to·be·stopped·before·any·changes·can·occur.'⏎········
[failure] 190-190:
Replace 'Array·must·be·running·to·mount·disks' with ⏎············'Array·must·be·running·to·mount·disks'⏎········
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
api/src/unraid-api/graph/resolvers/array/array.resolver.spec.ts (4)
4-4: Good use of vitest imports.
No issues are detected here; this setup aligns with typical vitest usage.
11-11: Variable declaration looks good.
DeclaringarrayServiceimproves clarity by providing direct access to the mocked methods.
15-28: Well-structured mock for ArrayService.
Providing mock implementations for each service method ensures that tests remain isolated and predictable.
32-32: Correct service retrieval.
FetchingArrayServicefrom the testing module is aligned with NestJS best practices.api/src/unraid-api/graph/resolvers/array/array.service.ts (2)
1-2: Import statements appear consistent.
Using@nestjs/commonis a standard approach and does not raise any issues.
5-16: 🛠️ Refactor suggestionConsider switching all service imports to absolute paths.
For consistency with the pipeline rules and other imports, use@app/style paths rather than relative paths.- import { getArrayData } from '@app/core/modules/array/get-array-data.js'; - import { emcmd } from '@app/core/utils/clients/emcmd.js'; - // ... - import { ArrayService } from './array.service.js'; // if it were relevant + import { getArrayData } from '@app/core/modules/array/get-array-data.js'; + import { emcmd } from '@app/core/utils/clients/emcmd.js'; + // ... + // and so forth, using @app/unraid-api/graph/resolvers/array/... if neededLikely an incorrect or invalid review comment.
api/src/unraid-api/graph/resolvers/array/array.resolver.spec.ts
Outdated
Show resolved
Hide resolved
elibosley
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
api/src/unraid-api/graph/resolvers/array/array.service.ts (6)
15-15: Consider removing unused import.
It appears thatgettersfrom'@app/store/index.js'is not referenced within the file. Removing unused imports can help keep the code clean and clear.🧰 Tools
🪛 GitHub Check: Build API
[failure] 15-15:
Delete/index.js';⏎import·{·arrayIsRunning·as·arrayIsRunningUtil·}·from·'@app/core/utils🪛 GitHub Check: Test API
[failure] 15-15:
Delete/index.js';⏎import·{·arrayIsRunning·as·arrayIsRunningUtil·}·from·'@app/core/utils
18-29: Address the TODO regardingarrayIsRunningutility.
There’s a@todonote suggesting inlining or simplifying the logic ofarrayIsRunningUtilinto this service. Implementing that refactoring could improve cohesion.
30-69: Review concurrency approach withpendingState.
Using a single class-level lock variable is useful for preventing parallel updates, but it may not cover multi-instance or multi-process scenarios. Consider a more robust approach if needed for distributed environments.
105-117: Refine return type formountArrayDisk.
This method returnsgetArrayData(), which is presumably anArrayType. Consider usingPromise<ArrayType>instead ofPromise<any>for better clarity.
119-131: Refine return type forunmountArrayDisk.
Similar tomountArrayDisk, this function also appears to return anArrayType. Consider declaring it asPromise<ArrayType>.
133-145: Refine return type forclearArrayDiskStatistics.
This method likewise returnsgetArrayData(), so switching toPromise<ArrayType>can enhance type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/unraid-api/graph/resolvers/array/array.service.ts(1 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 (3)
ArrayStateInput(312-315)ArrayType(112-131)ArrayDiskInput(194-199)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] 10-10:
Insert ·arrayIsRunning·as·arrayIsRunningUtil·}·from·'@app/core/utils/index.js';⏎import·{
[failure] 15-15:
Delete /index.js';⏎import·{·arrayIsRunning·as·arrayIsRunningUtil·}·from·'@app/core/utils
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/array/array.service.ts
[failure] 10-10:
Insert ·arrayIsRunning·as·arrayIsRunningUtil·}·from·'@app/core/utils/index.js';⏎import·{
[failure] 15-15:
Delete /index.js';⏎import·{·arrayIsRunning·as·arrayIsRunningUtil·}·from·'@app/core/utils
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
api/src/unraid-api/graph/resolvers/array/array.service.ts (3)
1-4: Imports look good overall.
71-86: Disk addition logic.
Implementation is straightforward: disallows adding disks while the array is running, then applies the change. This looks correct.
88-103: Disk removal logic.
Similar to adding a disk, preventing changes while the array is running ensures consistency. This is well-structured.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/array/array.service.ts (1)
21-24: Address the@todofor in-service utility logic.
Currently,arrayIsRunningis just delegating toarrayIsRunningUtil. Consider renaming or integrating the utility code directly into the service for consistency and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/unraid-api/graph/resolvers/array/array.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/resolvers/array/array.service.ts (4)
api/src/graphql/generated/api/types.ts (3)
ArrayStateInput(312-315)ArrayType(112-131)ArrayDiskInput(194-199)api/src/core/utils/clients/emcmd.ts (1)
emcmd(12-41)api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1)
array(21-23)api/src/core/modules/array/get-array-data.ts (1)
getArrayData(9-61)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
api/src/unraid-api/graph/resolvers/array/array.service.ts (2)
53-56: Validate dynamic property names.
[cmd${capitalCase(desiredState)}]relies oncapitalCase. If future states contain spaces or special characters, this might yield invalid property names. Consider sanitizing or validating thedesiredStatestring.
70-102: Verify offline requirement before adding or removing disks.
The logic prevents adding or removing disks if the array is running. Confirm this restriction aligns with the underlying system constraints. If partial updates while running are permissible, consider relaxing or customizing this check.
| // Prevent this running multiple times at once | ||
| if (this.pendingState) { | ||
| throw new BadRequestException( | ||
| new AppError(`Array state is still being updated. Changing to ${pendingState}`) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance concurrency handling around this.pendingState.
The guard clause will block a second invocation only if this.pendingState is still set. However, multiple concurrent calls could slip in before it's set, causing a race. Consider a more robust locking mechanism or a queue to safely enqueue requests.
elibosley
force-pushed
the
feat/array-controls
branch
from
3416667 to
994fb49
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/src/unraid-api/graph/resolvers/array/array.resolver.spec.ts (2)
11-11: New variable for ArrayService injection.
Consider adding tests that directly verify calls to the mocked service methods for more thorough coverage.
15-28: Injected ArrayService mock with multiple methods.
Looks good. You might want to assert calls (e.g.,toHaveBeenCalledWith) in your tests to confirm the resolver interacts with the mocked service as intended.api/src/unraid-api/graph/resolvers/array/array.service.spec.ts (1)
31-67: Setup forArrayServicetesting module and default mocks.
Good approach. Consider extracting repeated mocking objects (e.g.,mockArrayData) into a helper file or function to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (12)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/dev/states/myservers.cfg(2 hunks)api/src/core/modules/array/get-array-data.ts(0 hunks)api/src/core/utils/clients/emcmd.ts(2 hunks)api/src/graphql/schema/types/array/array.graphql(1 hunks)api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/array/array.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(3 hunks)
💤 Files with no reviewable changes (1)
- api/src/core/modules/array/get-array-data.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- api/dev/Unraid.net/myservers.cfg
- api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts
- api/src/unraid-api/graph/resolvers/array/array.resolver.ts
- api/dev/states/myservers.cfg
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
- api/src/graphql/schema/types/array/array.graphql
- api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/resolvers/array/array.service.ts (4)
api/src/graphql/generated/api/types.ts (3)
ArrayStateInput(312-315)ArrayType(112-131)ArrayDiskInput(194-199)api/src/core/utils/clients/emcmd.ts (1)
emcmd(12-41)api/src/unraid-api/graph/resolvers/array/array.resolver.ts (1)
array(21-23)api/src/core/modules/array/get-array-data.ts (1)
getArrayData(9-61)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Deploy Plugin
- GitHub Check: Cloudflare Pages
🔇 Additional comments (12)
api/src/unraid-api/graph/resolvers/array/array.resolver.spec.ts (3)
4-4: Import of testing utilities.
No issues spotted.
7-7: Absolute import path usage.
This aligns well with pipeline checks and best practices.
32-32: Retrieving the injected ArrayService instance.
No concerns here.api/src/core/utils/clients/emcmd.ts (3)
3-3: UsingAppErrorfor improved error semantics.
This maintains consistency with the app’s custom error structure.
31-33: Switched to usingsearchParamsand enabled Unix sockets.
This modernization of the request call is clear and aligns withgotbest practices.
35-40: Refined error handling forENOENT.
Great enhancement to detect and throw a meaningful error if the socket is unavailable.api/src/unraid-api/graph/resolvers/array/array.service.spec.ts (3)
13-25: Mocking core utilities and store getters.
This provides excellent isolation, ensuring the external dependencies are controlled during testing. No issues found here.
69-126: VerifyingupdateArrayState, disk additions, and removals.
All tests appear solid. You might add dedicated tests for STOP scenarios or partial states if relevant.
159-210: Error-handling tests for running vs. stopped array states.
Thorough negative test coverage. Great job ensuring that each method enforces the correct preconditions.api/src/unraid-api/graph/resolvers/array/array.service.ts (3)
36-41: [Duplicate Concurrency Concern]
Your guard setsthis.pendingStatebut does not protect fully against simultaneous calls arriving before it’s set. A more robust locking or queue approach could prevent race conditions.
70-102: Controlled add/remove disk logic.
This ensures an error is thrown if the array is running, which is consistent with typical stopping-first procedures. Logical flow is straightforward and correct.
104-144: Mount/Unmount methods with checks for running array.
This is consistent with the system’s requirements, and error messages are appropriately descriptive.
🤖 I have created a release *beep* *boop* --- ## [4.5.0](v4.4.1...v4.5.0) (2025-04-02) ### Features * add webgui theme switcher component ([#1304](#1304)) ([e2d00dc](e2d00dc)) * api plugin system & offline versioned dependency vendoring ([#1252](#1252)) ([9f492bf](9f492bf)) * **api:** add `unraid-api --delete` command ([#1289](#1289)) ([2f09445](2f09445)) * basic array controls ([#1291](#1291)) ([61fe696](61fe696)) * basic docker controls ([#1292](#1292)) ([12eddf8](12eddf8)) * copy to webgui repo script docs + wc build options ([#1285](#1285)) ([e54f189](e54f189)) ### Bug Fixes * additional url fixes ([4b2763c](4b2763c)) * **api:** redirect benign pnpm postinstall warning to log file ([#1290](#1290)) ([7fb7849](7fb7849)) * **deps:** update dependency chalk to v5 ([#1296](#1296)) ([6bed638](6bed638)) * **deps:** update dependency diff to v7 ([#1297](#1297)) ([3c6683c](3c6683c)) * disable all config watchers ([#1306](#1306)) ([5c1b435](5c1b435)) * extract callbacks to library ([#1280](#1280)) ([2266139](2266139)) * OEM plugin issues ([#1288](#1288)) ([d5a3d0d](d5a3d0d)) * replace files lost during pruning ([d0d2ff6](d0d2ff6)) --- 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>
…other functions
Summary by CodeRabbit
New Features
Chores
Refactor