packages/unraid-shared/src/util/data.ts (2)

19-27: Minor: make opts optional and destructure defaults to avoid creating a new object each call

This keeps behavior identical while avoiding allocating { noEmpty: true } on every call and makes the signature a bit friendlier.

-export function csvStringToArray(
-  csvString?: string | null,
-  opts: { noEmpty?: boolean } = { noEmpty: true }
-): string[] {
-  if (!csvString) return [];
-  const result = csvString.split(",").map((item) => item.trim());
-  if (opts.noEmpty) {
-    return result.filter((item) => item !== "");
-  }
-  return result;
-}
+export function csvStringToArray(
+  csvString?: string | null,
+  opts?: { noEmpty?: boolean }
+): string[] {
+  if (!csvString) return [];
+  const { noEmpty = true } = opts ?? {};
+  const result = csvString.split(",").map((item) => item.trim());
+  if (noEmpty) {
+    return result.filter((item) => item !== "");
+  }
+  return result;
+}

---

53-63: Harden numeric coercion against Infinity while keeping NaN fallback semantics

Consider treating non-finite numbers the same as NaN. This is a safe enhancement for defensive parsing and does not affect typical Var inputs.

 export function toNumberAlways(value: unknown, defaultValue = 0): number {
   const num = Number(value);
-  return Number.isNaN(num) ? defaultValue : num;
+  return Number.isFinite(num) ? num : defaultValue;
 }

If desired, we can add a short note to the JSDoc clarifying that empty strings and null coerce to 0 (by design).

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

1-7: Enum registration looks good; verify there's no import-order coupling

Registering ParityCheckStatus here is fine, but UnraidArray in array.model.ts consumes the enum too. Today this file is imported by ParityService, so registration will happen before schema generation, but that coupling is implicit. Consider centralizing enum registrations in a small GraphQL-layer enums module that’s imported unconditionally by the GraphQL module to avoid order pitfalls.

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

97-100: Add a guard and GraphQL-friendly error if emhttp.var is absent

Aligns with prior guidance to surface read errors as GraphQLError at the service level and avoids possible runtime undefined access in edge environments.

-    async getParityCheckStatus(): Promise<ParityCheckStatus> {
-        const { getters } = await import('@app/store/index.js');
-        return getParityCheckStatus(getters.emhttp().var);
-    }
+    async getParityCheckStatus(): Promise<ParityCheckStatus> {
+        const { getters } = await import('@app/store/index.js');
+        const emhttp = getters.emhttp?.();
+        if (!emhttp?.var) {
+            throw new GraphQLError('emhttp.var not available');
+        }
+        return getParityCheckStatus(emhttp.var);
+    }

api/src/core/modules/array/parity-check-status.ts (1)

19-21: Coerce mdResyncPos for robustness and consistency.

You already normalize mdResyncDt and sbSyncExit via toNumberAlways. mdResyncPos can also arrive as a string depending on source; coercing it avoids subtle truthiness/coercion pitfalls and keeps logic uniform.

Apply this diff:

 export function getParityCheckStatus(varData: Var): ParityCheckStatus {
-    const { mdResyncPos, mdResyncDt, sbSyncExit, sbSynced, sbSynced2 } = varData;
+    const { mdResyncPos, mdResyncDt, sbSyncExit, sbSynced, sbSynced2 } = varData;
     const mdResyncDtNumber = toNumberAlways(mdResyncDt, 0);
     const sbSyncExitNumber = toNumberAlways(sbSyncExit, 0);
+    const mdResyncPosNumber = toNumberAlways(mdResyncPos as any, 0);
 
-    if (mdResyncPos > 0) {
+    if (mdResyncPosNumber > 0) {
         return mdResyncDtNumber > 0 ? ParityCheckStatus.RUNNING : ParityCheckStatus.PAUSED;
     }

api/src/core/types/states/var.ts (1)

71-76: Docstring for mdResync is misleading; it suggests a running-state indicator.

Running/paused detection is implemented via mdResyncPos and mdResyncDt. mdResync typically represents total size of the operation.

Apply this doc-only tweak:

-    /**
-     * Serves a dual purpose depending on context:
-     * - Total size of the operation (in sectors/blocks)
-     * - Running state indicator (0 = paused, >0 = running)
-     */
+    /**
+     * Total size of the resync/parity operation (in sectors/blocks).
+     * Use mdResyncPos (> 0) and mdResyncDt (> 0) to determine running/paused state.
+     */

api/src/core/modules/array/parity-check-status.test.ts (2)

422-431: Test name doesn’t match the assertion (Infinity case).

Title says “PAUSED” but the expectation is RUNNING (and the comment explains why).

Apply this rename:

-        it('should return PAUSED when mdResyncDt is Infinity string', () => {
+        it('should return RUNNING when mdResyncDt is Infinity string', () => {

---

46-86: Optional: add a case for string mdResyncPos to lock in robustness if we coerce it.

This guards against regressions if mdResyncPos arrives as a string from emhttp.var.

Add:

+        it('should handle string mdResyncPos by treating as RUNNING when > 0', () => {
+            const varData = createMockVarData({
+                mdResyncPos: '100' as any,
+                mdResyncDt: '1',
+            });
+            expect(getParityCheckStatus(varData)).toBe(ParityCheckStatus.RUNNING);
+        });

Note: This will pass if the mdResyncPos coercion suggested in parity-check-status.ts is applied or if ingestion already coerces it.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

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

You can enable these sources in your CodeRabbit configuration.

Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.

Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.

packages/unraid-shared/src/util/data.ts (1)

44-51: LGTM: readability improvements without behavior change

The getNestedValue formatting cleanup retains the original semantics and reads better.

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

9-9: LGTM: correct ESM import path

Using the .js extension follows our ESM guideline for TS source.

---

86-86: Good: mock includes parityCheckStatus to satisfy non-null GraphQL field

This keeps the test data aligned with the updated schema and model.

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

8-9: LGTM: ESM import path for enum

Importing ParityCheckStatus with a .js specifier complies with our TS/ESM rule.

---

147-149: LGTM: new parityCheckStatus field on UnraidArray

Non-null GraphQL enum is appropriate here. Ensure get-array-data always resolves a value (the core helper’s default to NEVER_RUN should cover this).

api/src/core/modules/array/get-array-data.ts (2)

4-4: ESM import looks correct.

Good use of .js extension per our ESM guideline for TS sources.

---

65-66: Surface parityCheckStatus on UnraidArray — LGTM.

Field is computed from emhttp.var and returned with the rest of the array payload. Preconditions already ensure var is loaded, so this won’t throw at runtime.

api/generated-schema.graphql (2)

159-161: Schema surface addition matches PR intent.

Adding parityCheckStatus: ParityCheckStatus! to UnraidArray aligns with the objectives. Non-null is appropriate given we always compute a value.

Please confirm that all code paths resolving UnraidArray (queries and subscriptions) populate parityCheckStatus to avoid non-null violations. The tests mention mocks updated in array.service.spec.ts; ensure all mocks include this field.

---

183-190: ParityCheckStatus enum registration — double-check enum wiring.

The GraphQL enum uses UPPER_CASE values while the TS enum uses lower-case string values (e.g., 'never_run'). This is fine as long as registerEnumType(ParityCheckStatus, { name: 'ParityCheckStatus' }) is used so GraphQL values map to the TS enum string values.

If not already present, ensure api/src/unraid-api/graph/resolvers/array/parity.model.ts registers the enum and that resolvers return ParityCheckStatus members (not raw strings).

api/src/core/modules/array/parity-check-status.ts (1)

23-39: Clarify fallback semantics (NEVER_RUN) when a prior start timestamp exists but no completion/exit codes.

Current fallback returns NEVER_RUN even if sbSynced > 0, sbSynced2 === 0, and sbSyncExitNumber === 0 (i.e., “started sometime in the past, not running now, and no exit code”). That state may not literally mean “never run.”

• Is this state representable with real emhttp.var data, and if so, should it be exposed as NEVER_RUN or is PAUSED/FAILED more accurate?
• If NEVER_RUN is the intended UX, consider documenting this explicitly in the function’s JSDoc and enum description to avoid confusion.

I can adjust logic and tests if product semantics dictate a different outcome.

api/src/core/types/states/var.ts (1)

89-91: mdResyncPos is already normalized at the ingestion boundary; no changes needed

A review of api/src/store/state-parsers/var.ts confirms that the raw iniFile.mdResyncPos (string) is passed through toNumber when building the Var object (see line 237). Since Var.mdResyncPos is thus always a number by the time it reaches core logic (e.g. getParityCheckStatus), downstream comparisons like > 0 are safe.
– No additional coercion needed in the loader.
– Core functions and tests already assume a numeric mdResyncPos, and all pass.

api/src/core/modules/array/parity-check-status.ts (3)

15-20: Guard speed calculation against negatives and non-finite values

Handle negative/infinite inputs defensively and keep speed non-negative.

 function calculateParitySpeed(deltaTime: number, deltaBlocks: number) {
-    if (deltaTime === 0 || deltaBlocks === 0) return 0;
+    if (deltaTime <= 0 || deltaBlocks <= 0) return 0;
     const deltaBytes = deltaBlocks * 1024;
-    const speedMBps = deltaBytes / deltaTime / 1024 / 1024;
-    return Math.round(speedMBps);
+    const speedMBps = deltaBytes / deltaTime / 1024 / 1024;
+    if (!Number.isFinite(speedMBps) || speedMBps < 0) return 0;
+    return Math.round(speedMBps);
 }

---

43-58: Minor tidy-up: compute status once and reuse it; improves readability

Compute status once before building the object.

-export function getParityCheckStatus(varData: Var): ParityCheck {
+export function getParityCheckStatus(varData: Var): ParityCheck {
     const { sbSynced, sbSynced2, mdResyncDt, mdResyncDb, mdResyncPos, mdResyncSize } = varData;
     const deltaTime = toNumberAlways(mdResyncDt, 0);
     const deltaBlocks = toNumberAlways(mdResyncDb, 0);
 
     // seconds since epoch (unix timestamp)
     const now = sbSynced2 > 0 ? sbSynced2 : Date.now() / 1000;
+    const status = getStatusFromVarData(varData);
     return {
-        status: getStatusFromVarData(varData),
+        status,
         speed: String(calculateParitySpeed(deltaTime, deltaBlocks)),
         // percentage as integer, clamped to [0, 100]
         progress:
             mdResyncSize > 0
                 ? Math.round(
                       Math.min(100, Math.max(0, (mdResyncPos / mdResyncSize) * 100))
                   )
                 : 0,
-        date: sbSynced > 0 ? new Date(sbSynced * 1000) : undefined,
-        duration: Math.round(now - sbSynced),
+        date: sbSynced > 0 ? new Date(sbSynced * 1000) : undefined,
+        duration: sbSynced > 0 ? Math.round(now - sbSynced) : undefined,
     };
 }

---

3-5: Layering: core module imports a GraphQL model type — consider decoupling

Even though it’s a type-only import, core depending on @app/unraid-api/graph/... ties layers together and risks circular references in refactors. Prefer returning a core-level plain TypeScript type (e.g., ParityCheckSnapshot) and mapping it to the GraphQL model at the API layer.

If you’d like, I can draft a minimal core type and an adapter in unraid-api to keep boundaries clean.

api/src/core/modules/array/parity-check-status.test.ts (2)

424-433: Fix test title to match the expectation (Infinity -> RUNNING)

The description says “PAUSED” but the assertion is RUNNING.

-it('should return PAUSED when mdResyncDt is Infinity string', () => {
+it('should return RUNNING when mdResyncDt is Infinity string', () => {

---

1-681: Optional: add assertions for progress rounding/clamping and undefined date/duration for NEVER_RUN

Given the schema uses Int for progress and date/duration are nullable, it’s worth asserting these behaviors explicitly.

Proposed additions:

it('returns integer progress clamped to [0,100]', () => {
  const varData = createMockVarData({
    mdResyncPos: 150,
    mdResyncSize: 100, // 150%
    mdResyncDt: '10',
  });
  const { progress } = getParityCheckStatus(varData);
  expect(Number.isInteger(progress)).toBe(true);
  expect(progress).toBe(100);
});

it('leaves date and duration undefined when NEVER_RUN', () => {
  const varData = createMockVarData({
    sbSynced: 0,
    sbSynced2: 0,
    mdResyncPos: 0,
    mdResyncDt: '0',
  });
  const { status, date, duration } = getParityCheckStatus(varData);
  expect(status).toBe(ParityCheckStatus.NEVER_RUN);
  expect(date).toBeUndefined();
  expect(duration).toBeUndefined();
});

I can open a follow-up PR to add these tests after the producer changes land.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

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

You can enable these sources in your CodeRabbit configuration.

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

4-4: LGTM on ESM import specifier

Uses .js extension per repo guidelines; import path aligns with shared util placement.

---

7-7: Enum import is correct

Importing the enum from core keeps history mapping consistent with live status computation.

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

86-92: LGTM: parityCheckStatus shape matches the new GraphQL/enum contract

Test data mirrors the new enum and field schema. Keeping date undefined is appropriate for NEVER_RUN.

api/src/unraid-api/graph/resolvers/array/parity.model.ts (2)

1-7: Enum registration looks good

registerEnumType called once with a stable name. Make sure this module isn’t imported multiple times in a hot-reload context during tests to avoid “type already exists” noise, but with Vitest + Nest this is typically fine.

If you’ve seen duplicate-type warnings previously, we can centralize enum registrations in a single bootstrap file.

---

20-21: Ensure ParityCheck.progress is an integer per GraphQL Int

• In api/src/unraid-api/graph/resolvers/array/parity-check-status.ts, confirm that getParityCheckStatus now:
– Rounds the raw percentage to the nearest whole number
– Clamps the result between 0 and 100
– Returns a JavaScript number without fractional part for the progress field

• The GraphQL model in api/src/unraid-api/graph/resolvers/array/parity.model.ts should still declare @Field(() => Int) for progress. No change to the schema is needed.

• Add or update a unit test in your array resolvers (e.g. in parity-check-status.spec.ts) to verify:
– An input that would yield a fractional progress (e.g. 12.7) returns 13
– Negative or >100 inputs are clamped to 0 and 100 respectively

• Manually exercise the GraphQL sandbox (e.g. run a query like
{ array { status { progress } } }) and confirm the returned progress value is an integer.

Please verify these changes and ensure that all existing clients relying on the Int type continue to work as expected.

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

43-49: Simplify and harden statusCodeToStatusEnum (avoid re-coercion and NaN→COMPLETED)

• statusCode is already numeric; reapplying toNumberAlways can convert NaN to 0 and mislabel as COMPLETED.
• Use direct equality checks.

Apply this diff:

-    statusCodeToStatusEnum(statusCode: number): ParityCheckStatus {
-        return statusCode === -4
-            ? ParityCheckStatus.CANCELLED
-            : toNumberAlways(statusCode, 0) === 0
-              ? ParityCheckStatus.COMPLETED
-              : ParityCheckStatus.FAILED;
-    }
+    statusCodeToStatusEnum(statusCode: number): ParityCheckStatus {
+        if (statusCode === -4) return ParityCheckStatus.CANCELLED;
+        if (statusCode === 0) return ParityCheckStatus.COMPLETED;
+        return ParityCheckStatus.FAILED;
+    }

---

19-21: Prefer GraphQLError for user-facing resolver failures

For consistency with the rest of this resolver and cleaner client errors, wrap the “history file not found” case in GraphQLError.

Apply this diff:

-        const history = await readFile(historyFilePath).catch(() => {
-            throw new Error(`Parity history file not found: ${historyFilePath}`);
-        });
+        const history = await readFile(historyFilePath).catch(() => {
+            throw new GraphQLError(`Parity history file not found: ${historyFilePath}`);
+        });

api/src/core/modules/array/parity-check-status.ts (1)

15-20: Consider clamping negative speeds to 0 MB/s

If mdResyncDt becomes negative due to bad input, speed can be negative, which is confusing. Optional: clamp to 0 to reflect “no transfer”.

Apply this diff:

-function calculateParitySpeed(deltaTime: number, deltaBlocks: number) {
-    if (deltaTime === 0 || deltaBlocks === 0) return 0;
+function calculateParitySpeed(deltaTime: number, deltaBlocks: number) {
+    if (deltaTime <= 0 || deltaBlocks <= 0) return 0;
     const deltaBytes = deltaBlocks * 1024;
     const speedMBps = deltaBytes / deltaTime / 1024 / 1024;
     return Math.round(speedMBps);
 }

api/src/core/modules/array/parity-check-status.test.ts (3)

424-433: Fix misleading test name (expects RUNNING, title says PAUSED)

The assertion expects RUNNING for mdResyncDt = 'Infinity' (correct), but the test title says PAUSED. Rename for clarity.

Apply this diff:

-        it('should return PAUSED when mdResyncDt is Infinity string', () => {
+        it('should return RUNNING when mdResyncDt is "Infinity" string', () => {

---

6-46: Reduce maintenance burden of Var-heavy test fixtures

You only need a subset of Var for getParityCheckStatus. Consider deriving the param type and mocking only those fields to keep tests future-proof.

Example change:

-const createMockVarData = (overrides: Partial<Var> = {}): Var =>
-    ({
-        mdResyncPos: 0,
-        mdResyncDt: '0',
-        mdResyncDb: '0',
-        mdResyncSize: 100000,
-        sbSyncExit: '0',
-        sbSynced: 0,
-        sbSynced2: 0,
-        // many unrelated defaults...
-        ...overrides,
-    }) as Var;
+type Input = Parameters<typeof getParityCheckStatus>[0];
+const createMockVarData = (overrides: Partial<Input> = {}): Input => ({
+    mdResyncPos: 0,
+    mdResyncDt: '0',
+    mdResyncDb: '0',
+    mdResyncSize: 100000,
+    sbSyncExit: '0',
+    sbSynced: 0,
+    sbSynced2: 0,
+    ...overrides,
+});

---

681-799: Speed tests are thorough; consider aligning with any “no negative speed” decision

If you adopt clamping in calculateParitySpeed, update the negative-deltaTime test to expect '0'. Otherwise, the current expectation of '-1' is fine and demonstrates the behavior explicitly.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

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

You can enable these sources in your CodeRabbit configuration.